fix(server): settle lifespan cleanup after startup failures - #1223
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — lifespan hook fix with no blocking or medium findings.
The PR moves on_startup/on_shutdown into a single retained lifecycle task so that partial-startup failures still run every cleanup hook exactly once. The reviewer traced the full _user_lifespan_hooks state machine against the diff tests: error precedence, repeated-cancellation settling via the shielded join, and secret-free cleanup logging are all correct. The fix(server) patch-semver signal is appropriate — no public surface change.
high_risk is true only because src/adcp/server/serve.py is a (modified) match on src/adcp/server/**; per the decision rules a modification with no medium-or-higher findings is presumed safe. gated_paths is false. No no-auto-approve team match. No prior decision.
No rows 1–8 fired (no critical/high/medium findings, no deletions, no gate, no team match), so this falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass, clean.
This is a follow-up push to PR #1223 (prior decision: approve). The delta is one 8-line block in _user_lifespan_hooks (src/adcp/server/serve.py:123-130) that re-raises a recorded startup failure when a CancelledError wins the shield(ready) race. The reviewer verified: startup_error is non-None only on a true startup-phase failure, there is no double-raise when it is already the primary error, ready.result() cannot raise, and the path is covered by test_startup_failure_settles_cleanup_despite_repeated_cancellation. Public/wire surface is unchanged.
No critical/high/medium findings in this run and no prior blockers to re-surface.
Decision-table walk: no critical/high findings (row 1 n/a); gated_paths is false (row 2 n/a); high_risk is true but the only high-risk reason is serve.py (modified) with no medium finding, so rows 3/5 do not fire; no medium data-loss/schema/infra findings (row 4 n/a); prior decision was approve, not escalate (row 6 n/a); no no-auto-approve team match (row 7 n/a); zero medium findings (row 8 n/a). Falls through to row 9 — approve.
2d76606 to
d336ecd
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean diff, no blocking or medium findings.
Checked:
- serve.py transport="both" lifespan reworked into a single retained hook task (_user_lifespan_hooks/_settle_lifespan_task); cancellation-settling, error-precedence, ended-early lifecycle failure, and credential-safe secondary logging all covered by ~469 lines of added tests — all paths hold.
- Correct
fix(server):semver signal; no public-surface break, import layering intact, no credentials in ctx_metadata. - CI Postgres lane bump to 25 minutes and the polling de-flake of the late-account reporting conformance test are both legitimate; no test disabled, no gate weakened.
Decision-table walk: no critical/high (row 1 no); gated_paths false (row 2 no); high_risk true but reason is (modified) with zero medium findings, so rows 3/5 don't fire; no medium data-loss/schema/infra (row 4 no); prior decision was approve, not escalate (row 6 no); no no-auto-approve team match (row 7 no); fewer than three medium findings (row 8 no). Falls through to row 9 → approve.
Note: review_decision is REVIEW_REQUIRED but gated_paths is false, so the row-2 hard gate does not apply.
d336ecd to
a1355c5
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass on PR #1223 (head a1355c5).
The retained-task lifespan rewrite and startup-error-through-cancellation fix are correct and thoroughly tested: cleanup settles before transport teardown, secondary-error logging omits adopter exception text and callable reprs, and partial-startup cleanup fires each hook once. No public-surface, import-layering, generated-code, CI-gate, or ctx_metadata credential contracts are touched.
Decision-table walk:
- Row 1 (critical/high): no such findings.
- Row 2 (gated_paths): gated_paths is false — does not fire, despite review_decision=REVIEW_REQUIRED.
- Row 3 (deleted high-risk): no deletions; serve.py is (modified).
- Row 4/5 (medium findings): none.
- Row 6 (sticky escalate): prior decision was approve, not escalate.
- Row 7 (no-auto-approve team): no team match.
- Row 8 (≥3 medium): zero medium findings.
- Row 9: approve.
high_risk is true only because src/adcp/server/serve.py (modified) matches src/adcp/server/**, but the reviewer found no medium-or-higher concerns on that modified file, so the modification is presumed safe per the high-risk heuristic guidance. Consistent with the prior approve on the earlier head.
A later
on_startupfailure previously skipped adopter cleanup hooks. A cancelled shutdown callback could also skip later cleanup, and secondary-error logging formatted adopter exception bodies and callable representations.The synchronous
serve(..., transport="both")runner now retains one lifecycle task for startup and shutdown callbacks. Paired ContextVar tokens and AnyIO scopes stay valid, partial startup invokes all registered cleanup callbacks, and repeated cancellation waits for cleanup before MCP/A2A teardown. Registration order and primary-error precedence are preserved. Cleanup logs contain only the hook index. Unexpected termination of the runner-owned task ends the server lifespan. Python 3.13 exposed a race where cancellation could replace a recorded startup failure withCancelledError; after cleanup settles, the runner now surfaces the recorded failure.The task copies incoming context, so startup ContextVar mutations remain local to the hook lifecycle. Hooks must tolerate partial startup and arrange dependent cleanup in registration order. A hook that never settles can hold shutdown indefinitely; the SDK imposes no cleanup timeout. The public signature and transport restrictions are unchanged.
This branch is rebased on current main. Its former CI timeout follow-up was superseded by main's 30-minute core Postgres matrix setting, leaving only the server source and lifecycle tests in this PR. On the current head, 23 focused lifecycle tests,
make lint, andmake typecheck-allpass. The pre-rebase head passed the required CI matrix; fresh CI and independent review are pending for this head.Addresses the runner portion of PY-SVC-002 in the reliable-reporting service audit. Service resource ownership, health/readiness, and general worker supervision remain in their separate implementation lane.