fix(client): quarantine timed-out operator suspend - #2212
Conversation
yuezengwu
left a comment
There was a problem hiding this comment.
Approved.
This change fixes the post-sleep/manual-suspend deadlock by bounding operator suspend settlement at 30 seconds and permanently quarantining the exact timed-out handler generation. The timeout path establishes the inbox custody boundary before releasing suspension, so only the provider-entered prefix is ACKed and the unentered tail remains recovery debt. Ordinary routing can then install a fresh handler without joining the lost callback, while an in-flight start/resume timeout keeps provider admission closed and Reset consistently fails with a restart-required result.
The narrower permanent-quarantine model also removes the late-materialization convergence race from #2205: a late completion matching the quarantined handler/generation is ignored instead of becoming ordinary teardown debt that later route admission or manager shutdown could join.
Static diff review only; I did not rerun tests or QA.
|
Blocking review finding on [P1 / R4 —
Please transfer The quarantine shape itself is substantially narrower than #2205 and the normal inbox-recovery, late-generation fence, restart-required Reset, and bounded manager-shutdown paths otherwise look consistent. Static diff review only; I did not rerun tests or QA. CI is currently green. |
yuezengwu
left a comment
There was a problem hiding this comment.
Requesting changes; this supersedes my earlier approval after confirming the newly identified P1 on the same head.
The quarantine design and the normal custody/recovery paths remain sound, but the timeout branch can consume a terminal provider failure without its required durable chat notice. pendingRuntimeFailureNotice is transferred to the inbox ledger only after waitForHandlerSuspend() resolves. If suspend() emits the structured terminal/retry-exhausted event and then never settles, the 30-second timeout skips that transfer and continues to prepareOperatorSuspend(), which can promote and ACK the processing prefix. The inbox row is then permanently consumed even though the session event is only a live trace and the durable notice was never posted.
Please move the pending-notice transfer into the continuation shared by successful settlement and timeout, before invalidating the generation and before prepareOperatorSuspend(). Add the proposed regression proving that a terminal event emitted by a never-settling suspend() keeps the row unACKed and that recovery/redelivery posts the notice before its only ACK.
Static diff review only; I did not rerun tests or QA.
|
Addressed the P1 notice-debt finding in 81b3e61.
Validation: focused regression passed; all 148 session-manager edge tests passed; full client suite passed (2422 passed, 7 skipped); repo typecheck passed; touched files pass Biome. Please re-review when convenient. |
yuezengwu
left a comment
There was a problem hiding this comment.
Approved on 81b3e616c9bc6fd1276e7e51182bf9f9a029a74f.
The goal remains to bound a lost manual-suspend callback without losing inbox custody or letting the quarantined handler/generation block ordinary routing and manager shutdown. The new commit resolves the outstanding P1: terminal provider-failure notice debt is now transferred on both successful settlement and timeout, before generation invalidation and before prepareOperatorSuspend() can promote the processing prefix.
The added regression covers the exact failure sequence: suspend() emits provider_failure_terminal and never resolves; the entry remains unACKed with recovery debt after timeout; redelivery persists the durable notice before the single ACK; and the old handler is not re-entered. I found no remaining blocker in the incremental diff.
This PR still introduces the core in-memory quarantinedSessions lifecycle state and has no database migration. Static diff review only; I did not rerun tests or QA.
|
Re-reviewed The P1 is resolved at the correct boundary: The regression covers the missing branch end to end: Static incremental-diff review only; I did not rerun tests or QA. |
Summary
prepareOperatorSuspend()before releasing the boundary so only the provider-entered prefix is ACKed and the unentered tail becomes recovery debt before a fresh handler routeapplied:false) while quarantine exists, and keep provider admission fail closed when the timeout caught an unfinished start/resume transitionScope
This intentionally does not provide same-daemon Reset convergence after provider teardown becomes unobservable. Restarting the agent daemon clears the in-memory quarantine and lets bind-reset recover unACKed inbox work before Reset is retried.
This replacement supersedes #2205 and keeps the two required parts of its review: the inbox custody boundary and never joining the lost provider callback again. The reject-then-succeed Reset retry and late-materialization Reset race paths are removed from this scope.
Testing
pnpm exec vitest run src/__tests__/session-manager-edge-coverage.test.ts --reporter=dot --testTimeout=10000 --hookTimeout=10000(147 passed)pnpm --filter @first-tree/client test(2421 passed, 7 skipped)pnpm --filter @first-tree/client typecheckpnpm check && pnpm typecheck(passed; repository-baseline warnings only)Closes #2144