Skip to content

fix(server): settle lifespan cleanup after startup failures - #1223

Merged
bokelley merged 2 commits into
mainfrom
fix/server-hook-cleanup
Sep 26, 2026
Merged

bokelley merged 2 commits into
mainfrom
fix/server-hook-cleanup

Conversation

@bokelley

@bokelley bokelley commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

A later on_startup failure 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 with CancelledError; 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, and make typecheck-all pass. 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.

Comment thread tests/test_serve_lifespan_hooks.py
Comment thread tests/test_serve_lifespan_hooks.py
Comment thread src/adcp/server/serve.py
Comment thread src/adcp/server/serve.py
Comment thread tests/test_serve_lifespan_hooks.py
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 25, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 26, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 26, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

I rebased the server-hook fix onto current main after #1219 merged. The prior CI timeout-only commit was superseded by main’s 30-minute core PG matrix setting and was dropped. Fresh CI is running on a1355c5; please review the current head when it is green.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bokelley
bokelley merged commit 7c5a123 into main Sep 26, 2026
51 checks passed
@bokelley
bokelley deleted the fix/server-hook-cleanup branch September 26, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant