harden(listen): release held finalization leases on shutdown cancel (follow-up to #10066)

[aryanorastar]

Summary

Follow-up to #10066, which landed the core #9995 fix (process-scope ownership of claimed finalization via start_background_task, best-effort result callback). This PR ships the residual hardening on top of that mechanism:

• Shutdown lease release: except asyncio.CancelledError in _process_conversation_task converts the only cancellation that can still reach a claimed job — the process-shutdown drain (drain_background_tasks cancels tracked tasks) — into an explicit mark_finalization_retryable(..., 'worker_shutdown') under asyncio.shield, so a restart requeues the held lease instead of waiting out lease expiry. Lease-epoch fencing keeps the release a no-op after completion.
• In-flight visibility: PUSHER_DETACHED_FINALIZATION_TASKS gauge (inc/dec around the task body) — the observability Git-on-my-level's earlier review asked for, surviving fix(listen): preserve finalization after source close #10066's mechanism change.
• Result-send pre-check: send_result returns early when the socket is not CONNECTED, complementing fix(listen): preserve finalization after source close #10066's narrowed exception handling; the regression test asserts the frame is dropped, not attempted.
• FC-session-scoped-durable-work registration (separate commit): the class fix(backend): make listen finalization durable #9648/fix(backend): give inline listen finalization a terminal failure state #9704/fix(listen): preserve finalization after source close #10066 were all instances of, so the next one is declared instead of rediscovered.
• Route-level regression coverage that fix(listen): preserve finalization after source close #10066 did not add: a close-after-104 session through the real _websocket_util_trigger proving the claimed job converges to completed with no later live session, plus a mid-lease cancel proving the worker_shutdown requeue.

Relates to #9995 (core fix landed in #10066; this completes its hardening tail).

Failure class

Failure-Class: new

FC-session-scoped-durable-work: durable ledger-owned work, once claimed, must not inherit the lifetime of the connection or session that dispatched it. #9648 (durable job ledger), #9704 (inline terminal state), and #10066 (session-close cancellation) were all retail fixes at this boundary; the definition names the class so recurrence is declared, not rediscovered.

Durable guard (as of current head)

• Spawn ownership is main's: opcode-104 dispatch uses start_background_task (process scope, tracked, drained at shutdown) — this PR does not carry a private detached registry; the earlier detach implementation was superseded by fix(listen): preserve finalization after source close #10066 and dropped in the merge.
• The shutdown drain cancels tracked tasks; without this PR that CancelledError bypasses the retry/dead-letter handlers (it is a BaseException) and strands the claimed job leased until expiry. The new handler is the missing half of fix(listen): preserve finalization after source close #10066's ownership move.
• The gauge bounds the "unbounded set" observation from the earlier review: any future inline fan-out is visible before it matters.

Product invariants affected

none (verified with scripts/pr-preflight --suggest)

Validation

• Old-code check (this branch's pusher.py/metrics.py replaced with origin/main's, tests kept): pytest tests/unit/test_listen_finalization_cloud_tasks.py -k "survives or requeues_the_held_lease" → 4 failed (lease never released on cancel; gauge/no-send assertions unmet). Restored branch code → same selection passes.
• Full file via the repo runner (file-isolated, as CI executes): tests/unit/test_listen_finalization_cloud_tasks.py → 72 passed (includes fix(listen): preserve finalization after source close #10066's own tests — test_pusher_keeps_a_completed_job_terminal_when_source_result_delivery_fails still green with the pre-check in place).
• Branch is merged with current origin/main (95ca0fa); post-merge diff vs main is exactly the residual surface: backend/routers/pusher.py +31/−1, gauge in utils/metrics.py, FC definition, +127 test lines.
• Pre-push bounded gate passed on the pushed head.

CI checks on this head await maintainer workflow-run approval (fork PR) — nothing reports until then.

Not exercised live: a real listen→pusher deployment restarting mid-finalization (no live services here); the mid-lease cancel test encodes that timing hermetically.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Git-on-my-level]

Verified the fix end-to-end against the close-after-104 regression from #9995. Strong work — the ownership model here is the right shape.

What I confirmed locally (cloned head 3a6870d, origin/main base):

• pytest tests/unit/test_listen_finalization_cloud_tasks.py → 69 passed (both new tests green under asyncio and trio).
• Broader sweep across the 8 finalization/pusher suites cited in the PR body → 225 passed.
• scripts/failure-class validate --pr-body-file … --base origin/main → ok: true, has_fix_commit: true, declaration: new.

Static review notes (all clean):

• Detached ownership is correctly scoped: the new module-level _detached_finalization_tasks set is only touched by _spawn_detached_finalization and the on_done callback (sync add/discard, no awaits in between), so it's safe under the single asyncio loop. Session cleanup's drain_tasks(...) now cancels only bg_main_tasks, so a claimed finalization job survives a close-after-104.
• except asyncio.CancelledError sits ahead of the ConversationFinalizationError/Exception handlers and re-raises after best-effort mark_finalization_retryable('worker_shutdown') under asyncio.shield(...). Since the only cancellation that can reach a detached task is process shutdown, converting it to an explicit lease release (guarded by lease_epoch) is the right call — a restart no longer waits out lease expiry, and the epoch fence makes it a no-op post-completion.
• send_result is now state-guarded (client_state/application_state != CONNECTED → return) and wraps send_bytes in try/except, so a closed origin socket can't turn a completed job into a spurious worker_failed retry. The regression test asserts websocket.sent == [] for exactly this case.
• BYOK invariant holds: keys live in contextvars (backend/utils/byok.py), and asyncio.create_task copies the current context, so the detached continuation keeps the live-presented keys without any durable credential storage. The blocked_byok contract is untouched.

The failure-class definition (FC-session-scoped-durable-work) is well-formed, references the right prior PRs (#9648, #9704), and the scope hints point at the real blast radius.

Flagging for a human maintainer sign-off before merge rather than approving myself: this PR changes a durable-state ownership boundary (claimed Firestore job ledger ↔ pusher session lifetime) on a production finalization path, and the lease-release-on-shutdown behavior is the kind of correctness call that benefits from a maintainer's eyes even though the tests are persuasive. Not a request for changes — the implementation and validation look sound.

One minor (non-blocking) observation for whoever lands it: the _detached_finalization_tasks set is unbounded in principle. In practice finalization jobs are Firestore-claim-bounded per conversation, so this is fine today, but if inline dispatch ever fans out you'd want a cap or a gauge.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[aryanorastar]

Thanks for the thorough verification @Git-on-my-level.

Addressed the one observation: pushed harden(listen): gauge in-flight detached finalization tasks — the detached registry size is now exported as pusher_detached_finalization_tasks (mirrors the existing pusher_active_ws_connections gauge), and the close-after-104 regression test now also asserts the gauge at 1 while the job is in flight and back to 0 after completion. Agreed a hard cap is unwarranted while claims bound the registry; the gauge makes any future inline fan-out visible before that changes.

Branch is also rebased onto current main (picked up the new FirestoreContentionExhausted admission test in the same file — 70 passed locally). make preflight still 19/19 with this PR body.

[undivisible]

Request changes

Core #9995 fix already shipped in #10066. This PR body still describes that work.

Please

1. Rewrite title/body as follow-up after #10066 (cancel→requeue + gauge + FC). Drop “Fixes #9995”.
2. Match the durable-guard section to the actual residual diff.
3. Get CI green on this head.

[aryanorastar]

@undivisible Updated as requested:

• Title + body rewritten as a follow-up to fix(listen): preserve finalization after source close #10066 — the detach-from-session narrative and Fixes #9995 are gone; the body now describes only what this head actually ships: the shutdown-cancel lease requeue, the PUSHER_DETACHED_FINALIZATION_TASKS gauge, the FC-session-scoped-durable-work registration, the send_result CONNECTED pre-check, and the route-level close-after-104 / mid-lease-cancel regression coverage fix(listen): preserve finalization after source close #10066 didn't add.
• Durable-guard section aligned with the actual diff: spawn ownership is main's start_background_task; this PR carries no private detached registry. The branch is merged with current main (95ca0fa) and the post-merge diff vs main is exactly that residual surface (+179/−1 across 4 files).
• Verification on this head: with pusher.py/metrics.py swapped to origin/main's versions the two new tests fail 4/4 (lease stranded on cancel); with the branch code the full file passes 72/72 via the repo runner. scripts/failure-class validate → ok, declaration new.
• CI: checks report nothing because the fork workflow runs are awaiting maintainer approval — once approved this head should go green; happy to fix anything CI surfaces.

Ready for re-review.