fix(client): recover from stuck operator suspend - #2205
Conversation
yuezengwu
left a comment
There was a problem hiding this comment.
Requesting changes for two blocking recovery gaps:
-
The timeout branch returns before establishing the inbox settlement/recovery boundary. At
suspendSession()lines 4207-4218, an established session with a timed-out suspend skipsprepareOperatorSuspend(). ItsSessionEntryremainssuspended(notevicted) and the coordinator has norecoveryDebt, so bothdispatch()andrecoverDebtBeforeResume()allow the next inbox row to enter the fresh handler immediately. If the ledger still contains an older provider-entered or queued row, that later row executes before the prefix is resolved; only its eventualfinishTurn()notices the non-terminal prefix gap and requests recovery, after execution, making the later row eligible for redelivery/replay. Please establish the correct coordinator boundary before any fresh provider re-entry (resolve only the provable consumed prefix and retain the rest as recovery debt), and extend the regression test with real ledger custody to prove recovery happens beforefreshHandler.resume(). -
A permanently stuck provider callback can still fence the chat again after the first rescue. The timeout path marks the old handler retired, but
handlerForRouteTransition()registers its shutdown as ordinarypendingTeardownsonly after the currentsettleTeardownDebtBeforeRoute()call has passed. This permits exactly one fresh route. If the oldshutdown()also never settles—which is the relevant case for handlers such as Claude whereshutdown()delegates to the samesuspend()wait—the pending raw shutdown remains forever. After the fresh handler later suspends, the next resume joins that old promise insettleTeardownDebtBeforeRoute()and hangs indefinitely (and manager shutdown waits on it too). The abandoned timed-out generation needs a bounded/non-route-blocking teardown policy that still preserves late-output fencing and Reset safety. Please add a regression with both oldsuspend()andshutdown()never settling, then suspend the fresh handler and prove a second resume can proceed.
The generation fence itself is the right containment mechanism, but the current test only covers a handler with immediately successful shutdown and no inbox ledger state, so it misses both production failure modes above.
|
Addressed the blocking recovery gaps in
The regression coverage now:
Validation: full |
yuezengwu
left a comment
There was a problem hiding this comment.
The two previously reported routing/custody blockers are addressed, but the new abandoned-debt path introduces one blocking Reset convergence issue.
abandonTimedOutSuspendHandler() clears abandonedSuspendHandlers only when its first best-effort shutdown() resolves. If that shutdown rejects, the rejection branch deliberately leaves both the marker and pendingTeardowns entry in place. Every later session:terminate then sees the marker at lines 1146-1150 / 1191-1195 and throws before attempting shutdownHandler() again. Since the completed rejected shutdown record is otherwise retryable, this turns a transient teardown failure into permanent Reset failure: even if the next shutdown attempt would succeed, no Reset retry can ever make it.
That conflicts with the existing Reset contract and with the nearby teardown comments that a failed apply remains retryable and teardown retry converges. Please let a later Reset perform a strict retry for an abandoned handler once the original shutdown promise has settled (while still refusing to join a genuinely pending lost callback), and add a regression where the timed-out handler's first shutdown rejects, the next shutdown succeeds, and a subsequent Reset completes. The currently added test covers only a shutdown promise that never settles, so it cannot catch this branch.
|
Addressed the Reset convergence blocker in The abandoned marker now distinguishes the two states using the existing coalesced shutdown record:
This applies to both an abandoned handler still attached to the current session and one detached into pending teardown debt. Ordinary route admission continues to skip abandoned debt, so this does not reintroduce the route hang. The new regression times out Validation:
Please take another look. |
yuezengwu
left a comment
There was a problem hiding this comment.
Approved. The latest change resolves the remaining Reset convergence blocker: a still-pending abandoned shutdown is not joined, while a settled rejection can be retried strictly; success clears both the teardown debt and abandoned marker, and another rejection remains retryable. The regression covers the reject-then-succeed path, and the earlier inbox-custody, repeated-resume, late-generation fencing, and manager-shutdown concerns remain addressed.
Static diff review only; I did not rerun tests or QA.
|
Addressed the late-materialization Reset race in
The post-quiesce abandoned-producer check remains as a defensive boundary for the rest of terminate. The new regression uses a real
Validation:
Please take another look. |
yuezengwu
left a comment
There was a problem hiding this comment.
Requesting changes for one blocking late-materialization path on 60cdde8d18d1772c36ccae4e8d332b36b38b92e0.
abandonTimedOutSuspendHandler() and detachHandlerWithPendingTeardown() clear abandonedSuspendHandlers as soon as a best-effort shutdown succeeds (lines 2350-2354 and 2369-2373), even if the handler still has an abandoned route producer. That success is not final stop proof: discardStaleRouteTransition() explicitly documents that a pre-materialization shutdown may be a no-op, and starts a new after-prior shutdown when the producer returns (lines 2413-2427).
If the first shutdown succeeds while the producer is still pending, then the producer materializes late and the new after-prior shutdown never settles, its debt no longer has the abandoned marker. settleTeardownDebtBeforeRoute() therefore joins it as ordinary route-blocking debt, and manager shutdown joins it too. The chat is permanently fenced again even though the late generation remains safely retired. The new early Reset check does not cover this: once the producer settles it disappears from routeProducers, leaving only the now-ordinary pending shutdown.
Please retain/propagate the abandoned status until every abandoned producer for that handler has settled and its post-materialization stop is confirmed. Add a regression where the first pre-materialization shutdown resolves, the producer returns late, and the second after-prior shutdown never settles; prove ordinary later routing and manager shutdown remain available while Reset still fails closed.
The newly added test covers only an initial shutdown rejection, which keeps the marker present and misses this branch. Static diff review only; Client & Web CI passed, while the current Server job failed in two unchanged Reset tests.
|
Reviewed head A successful pre-materialization shutdown currently clears the abandoned-handler marker before its abandoned producer settles. If that producer later materializes and the required after-prior shutdown never settles, the debt becomes ordinary again, so both later route admission and manager shutdown join it forever. The fix should retain or propagate abandonment until producer settlement plus post-materialization stop is confirmed, with a regression for “first shutdown succeeds, late producer returns, second shutdown never settles.” Formal review: #2205 (review) Static diff review only. Client & Web CI passed; the current Server job failed in two unchanged Reset tests. |
Summary
Why this shape
A laptop sleep and wake commonly continues the same daemon process, so startup-only cleanup does not run in the reported failure mode. A true daemon restart already drops the in-memory suspend barrier and uses normal inbox recovery. This keeps the timeout local to manual pause while preserving the durable recovery and Reset safety boundaries.
Testing
pnpm --filter @first-tree/client test(2415 passed, 7 skipped)pnpm check && pnpm typecheckpnpm testwas also attempted on the earlier head; full parallel execution hit unrelated existing Web and CLI hook timeouts. Both failed files passed when rerun independently.Fixes #2144