a810f7e16c
* fix(serve): make session restore timeouts safe Co-authored-by: Qwen-Coder <[email protected]> * fix(cli): restore missing core mock exports in the ACP worktree suite The restore-tracing change added `extractDaemonTraceContext` and `withDaemonSpan` to `acpAgent.ts`, but `acpAgent.worktree.test.ts` replaces `@qwen-code/qwen-code-core` with a full mock factory that never listed them. `loadSession` then failed on an undefined export, taking all three cases down and producing teardown rejections from the half-built agent. The sibling suite was updated; this one was missed. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(serve): bound and disambiguate the abandoned restore lifecycle Four follow-ups from review of the restore timeout work. A startup budget may now raise the restore budget but never lower it. Taking an explicitly configured `initializeTimeoutMs` as the restore fallback meant a deployment that tightened its child-initialize check still inherited a sub-default restore deadline — exactly the failure this change exists to remove. An explicit `sessionRestoreTimeoutMs` still wins outright, including below the default, for deployments that want restore to fail fast. Validation now names the field actually at fault. A restore fenced behind a timed-out predecessor is no longer reported as an ordinary in-flight restore. It carries `reason: awaiting_abandoned_cleanup` and a retry hint of one restore budget (capped at 120s) instead of the ordinary 5 seconds, because the fence cannot clear until the non-cancellable ACP request settles and a 5-second cadence just spins the caller against a 409 it cannot resolve. Whether a channel is condemned is now derived rather than sticky. A timeout recorded `emptyReapPending` permanently, so any channel that had ever seen one was guaranteed to be reaped once its remaining work drained, forcing a cold respawn even when the late restore had landed and closed cleanly. The reap condition is now computed from an outstanding `unsettledAbandonedRestores` set, quarantine, or an ordinary pending empty reap; real settlement clears the entry and hands the channel back to the configured idle policy. Abandonment no longer retains ownership without bound. One further restore budget after the deadline, a still-unsettled restore marks the channel `restoreSettlementOverdue`: existing sessions and workspace control keep working, but fresh session work is refused so the channel can drain, since closing the transport is the only lever that releases a permanently hung request. Releasing capacity while hidden work runs would allow unbounded oversubscription, and force-killing a channel with live siblings would reintroduce the failure this work removes, so neither is done. Fresh-admission blocking is now scanned across alive channels rather than tracked in a single reference, so a second condemned channel cannot silently displace the first. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(serve): keep the abandoned restore lifecycle off ids it no longer owns Two correctness gaps in the abandoned-restore machinery introduced by this PR, both reported by automated review and both confirmed by mutation testing (each new test fails when its fix is reverted). A caller-supplied `sessionId` is used verbatim by the agent, but `spawnOrAttach` never consulted `inFlightRestores`. A fresh spawn could therefore take an id that a restore still owns, in either lifecycle phase. The consequences were silent: `abandonedRestoreIds` suppresses session updates, guardrail events, and child notifications, so the new session would have registered successfully and then emitted nothing; and a late `settleAbandonedRestore` would have closed and tombstoned it out from under its owner. Such a spawn is now rejected with the same `RestoreInProgressError` and reason the restore path uses, so the caller gets the correct retry hint for whichever phase is holding the id. The cleanup path is guarded independently, because the request-level check only covers the id the caller asked for and a session registers under the id the child returns. An abandoned restore never reaches `createSessionEntry` — the deadline rejects before registration — so any live entry under that id belongs to someone else. Cleanup now detects that and returns without closing or tombstoning, releasing its own bookkeeping instead. The notification fence has no TTL and was only cleared by `markRestoreInFlight`, which covers a subsequent restore and nothing else. `createSessionEntry` now clears it for every registration route, so a legitimate owner of the id is never handed a session that silently drops everything the child sends it. Also tightens two tests that could not observe the values they pin. The SDK default restore timeout admitted any value in (30s, 70s]; it is now split at the exact boundary, so collapsing the default onto the 60s server budget — which would make the client abort race the daemon's own deadline and cost the caller its structured 504 — fails. And the advertised-budget propagation from capabilities through to the SDK call had no live-path assertion; dropping the capabilities argument at the real call site left every existing test green. The `as never` casts are replaced with typed `DaemonCapabilities` values so a field rename fails typecheck. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(serve): let a condemned channel drain without its wedged child Merging main's active-work close protocol (#8588) into this PR's abandoned restore bound produced a deadlock that neither side has on its own, and the conflict resolution was committed without running tests. `maybeCloseIdleSession` now routes through `confirmChildUnheld`, which asks the child whether it still holds work before closing a session nobody is attached to. That is right in general and wrong for a channel this PR has already condemned. `restoreSettlementOverdue` and quarantine exist precisely because the child stopped being answerable, and their whole premise is that visible work drains so the channel can be reaped — closing the transport is the only thing that can release a restore we cannot cancel. Making that drain depend on a round trip to the wedged child inverts it: a child stuck in a non-cancellable restore is exactly the one that cannot reply inside `ACTIVE_WORK_CLOSE_TIMEOUT_MS`, so the sessions never close, the channel never drains, the reap never fires, and the bound never takes effect. A channel condemned by the restore lifecycle now skips the round trip and proceeds to local teardown. Nothing is attached to the session by then — `maybeCloseIdleSession` gates on that — and the sibling-safety invariant is untouched: this closes sessions whose clients have already left, it does not force-kill a channel that still has live ones. The regression test drives an overdue channel whose child never answers the close-if-unheld probe and asserts the detach still reaps it. Reverting the guard reproduces the deadlock as a test timeout. Co-Authored-By: Claude Opus 5 <[email protected]> * test(serve): pin the restore-timeout contract the review found unasserted Automated review identified eleven places where the restore-timeout work's behavior was correct but unpinned — each with a mutation that ships green. Every fix below was verified the same way: apply the mutation, watch the new assertion fail, revert, watch it pass. The timeout path's telemetry had no coverage at all, which is the sharpest gap given that observability is what this work exists to deliver. A shared recorder now asserts the public timeout result and its kill_empty-vs- fence_shared signal, the late arrival, and the cleanup outcome for both the closed and quarantined cases. The deadline timer's cancellation on a successful restore was likewise unpinned: deleting both `clearTimeout` calls kept the whole suite green, while in production the stale timer fires one budget after a successful restore and abandons a live session — fencing its frames, closing its event bus, and emitting a spurious timeout. A success-path test now advances past the deadline and asserts no second public result. Three more bridge assertions proved less than they claimed: the concurrent- restore case never checked that the abandoned restore settles, the workspace-control case never checked that the deferred reap eventually fires, and the resolver never pinned the accepting side of the MAX boundary (a `>` to `>=` mutation rejects the largest legal delay at boot). The workspace-control case also needed a positive channel idle budget, since with the default zero the idle-timer kill substitutes for the reap junction under test; its assertions are rewritten around the derived reap semantics rather than the sticky flag they predate. Outside the bridge: the scheduled-task timeout wiring had no test, so deleting the arguments silently fell back to the helpers' own defaults; the cold restore path never asserted that `live_restore_ms` is absent; the SDK's per-request validation and its over-ceiling clamp were untested; the WebUI watchdog test jumped straight to its own value, staying green for any watchdog at or below it, including the 30s attach value that would recreate the original symptom in the browser; and the two new known error types were unexercised, so dropping either would relabel every restore-timeout and quarantine error as unknown. Two review items are deliberately not taken here and are recorded in the design doc's non-goals instead: transcript materialization is still not separately attributable from `config_setup`, which needs instrumentation inside the core session loader that P1/P2 restructures anyway, and sibling event-loop latency during a large restore remains unmeasured. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(serve): bound the condemned-channel close and complete the fence contract Second automated review round, on the code the first round produced. One Critical and twelve suggestions; all verified by mutation before and after. **The Critical is a regression I introduced.** Letting a condemned channel skip the bounded hold probe routed it into `closeSessionImpl`, whose agent close is unbounded when it throws on failure — so the fix traded a bounded wait on a wedged child for an unbounded one. A settlement-overdue channel with an unresponsive child would hang `detachClient` forever, strand the session in `closing`, never drain, never reap, and 503 every new session until restart: strictly worse than before. `CloseSessionOpts` now carries an `agentCloseTimeoutMs` that the condemned path sets, so a hang lands in the existing unknown-outcome recovery, which kills the channel — the teardown the drain was waiting for. The earlier test missed this because its fake child still answered the plain close; it now answers nothing at all, and asserts the detach itself returns. **The fence was invisible on the transports clients actually use.** `toRpcError` had no `RestoreInProgressError` case, so over acp-http and acp-ws — which SDK negotiation prefers over REST — the fence degraded to an opaque internal 500 with no code, reason, or hint, and the backoff contract this work documents was impossible to honor. **Two retry hints still advertised five seconds for states that outlive a budget.** The restore 504 creates the fence, and quarantine lasts until the channel drains; a fresh-id caller never reaches the 409 that carries the real hint, so its header was the only signal it got. Both now derive from the budget through one shared clamp helper, which also replaces the formula that was inlined in the bridge and gives the documented 5-120s bounds a test. **A spawn collision reported an operation the caller never issued**, naming the restore owner's action as both the active and the requested one and telling the caller to retry an endpoint it never called. The rest: five places still described the initialize-timeout fallback as a plain chain rather than raise-only, contradicting sibling docs shipped in this same PR; the design doc omitted the retry-hint clamp; the protocol reference omitted the new spawn emission site; the error taxonomy omitted `restore_settlement_overdue`, which matters because its audience is monitoring. Test-only gaps: the dynamic 409 had no HTTP-layer coverage, the 120-second cap was unpinned, and the SDK's precedence of an explicit global timeout over the advertised budget was pinned only branch-by-branch. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(serve): preserve restore session ownership handoff Co-authored-by: Qwen-Coder <[email protected]> --------- Co-authored-by: Qwen-Coder <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]> Co-authored-by: qwen-code-dev-bot <[email protected]>
13 KiB
Safe and observable large-session restore timeouts
Context
Issue #8678 exposed that session restore reconstructs the persisted JSONL transcript before it can return an ACP loadSession or unstable_resumeSession response. Large transcripts can therefore exceed the bridge's former shared 10-second initialization timeout. The bridge cannot cancel the underlying ACP request because the current SDK does not expose its JSON-RPC request id or a cancellation operation.
The old timeout path stopped waiting while the child continued restoring. Channel cleanup could then kill a multiplexed child that still owned unrelated live sessions, or a late restore could create a session and writer lease after the caller had already received an error. The resulting error was also classified as a generic initialization timeout or plain 500, which hid the affected session and restore phase.
The 8 MiB live journal and 4 MiB compacted replay limits do not bound this work. They constrain client replay after reconstruction; they do not cap the on-disk transcript read.
Goals and non-goals
This change gives restore a dedicated configurable deadline, makes timeout errors structured, keeps non-cancellable late work fenced until settlement, and provides phase-level traces across the daemon and ACP child.
It does not stream the JSONL parser, add a transcript index or snapshot, guarantee that every large transcript finishes within 60 seconds, or change the WebUI's detach-before-load transaction. Those are follow-up changes.
Two deliberate gaps are worth stating rather than leaving to be rediscovered.
Transcript materialization is not separately attributable. The child records config_setup as one stage, and the full JSONL read plus active-chain reconstruction both happen inside it. The JSONL read runs in loadCliConfig through argv.resume, which both load and resume handlers set; when chat recording and the writer lease are enabled, config.initialize() additionally performs an authoritative reload. So a restore that again exceeds its budget shows config_setup_ms ≈ budget without telling an operator whether the cost was the transcript or a regressed runtime setup, and the P1/P2 work cannot baseline the phase it exists to optimize from these traces alone. Issue #8678's P0 asks for transcript indexing/read and active-chain reconstruction as distinct phases; this change delivers the coarser split it claims (transcript/config work versus authentication, registration, and replay) and not that finer one. Closing it means instrumenting inside the core session loader — the same code P1/P2 restructures — so it is sequenced with that work. The contained version, if it is wanted sooner, is for the loader to record its own read duration on the resumed session data and for the child profiler to report it as a transcript_read stage under the existing attribute prefix.
Sibling latency during a large restore is unmeasured. Siblings keep their sessions and stay logically usable, which is asserted; nothing establishes that they stay responsive while a multi-hundred-megabyte transcript is reconstructed on the shared child's event loop. The instrument already exists — the child runs an event-loop lag monitor whose snapshot carries mean, p50, p99, and max — so a fixture slow enough to cross the deadline would close this and the untested empty-channel budget increase in one run.
Timeout contract
The server restore budget resolves in this order:
sessionRestoreTimeoutMs/--session-restore-timeout-ms, which wins outright — including values below the default, for deployments that deliberately want restore to fail fast;- otherwise 60,000 ms, raised to an explicitly supplied
initializeTimeoutMs/--initialize-timeout-mswhen that value is larger.
A startup budget may raise the restore budget but never lower it. The two measure different work, and a deployment that tightened its child-initialize check must not silently inherit the sub-default restore deadline this change exists to remove.
Server values must be positive integers no greater than 2^31-1. The daemon publishes the effective value as optional v1 field limits.sessionRestoreTimeoutMs in both bootstrap and runtime capabilities.
The TypeScript SDK chooses a restore timeout in this order: per-request timeoutMs, explicit global fetchTimeoutMs, cached server capability plus 10 seconds, then 70 seconds. Per-request zero disables the client timer and is not serialized. Calling capabilities() refreshes the cached static budget; a restore does not issue an implicit capability request.
The WebUI passes server budget plus 10 seconds to SDK load/resume and uses server budget plus 15 seconds for its own watchdog. Missing capability values fall back to 70 and 75 seconds. Attach retains its existing 30-second watchdog. A derived delay above the JavaScript timer ceiling disables that client-side timer so Node cannot compress it to approximately one millisecond.
Restore ownership and terminal arbitration
inFlightRestores owns two promises: a public promise returned to callers and a settlement promise representing the real ACP request plus cleanup. It also owns the session-id fence and accounts for restore capacity. On normal success, registration transfers capacity to the live session and may release admission; after abandonment, the fence, admission, and in-flight capacity remain until real settlement.
An explicit lifecycle flag arbitrates the deadline and ACP result. Whichever observes active first changes the public terminal state. This avoids relying on Promise.race callback ordering at an equal-time boundary.
| State | Meaning and allowed transition |
|---|---|
active |
Same-action, same-shape requests coalesce. ACP success may register the session; failure may return normally. |
abandoned |
The deadline won. The public caller receives a structured timeout. The id is removed from pending restore events and fenced against late child notifications. Same-id retries receive restore_in_progress with reason: awaiting_abandoned_cleanup and a retry hint derived from the restore budget — clamped to at least 5 seconds and at most 120 — rather than a fixed 5 seconds. |
| overdue | One further restore budget passed with no settlement. Existing sessions and workspace control remain usable, but fresh spawn/load/resume/branch work is rejected so the channel can drain — closing the transport is the only lever that releases a permanently hung request. The channel is never force-killed while live siblings remain. |
| cleanup | A late ACP success or failure triggers exactly one child qwen/control/session/close. Resource-not-found is already clean. |
| quarantined | Cleanup outcome is uncertain. Existing sessions and workspace control remain usable, but fresh spawn/load/resume/branch work is rejected until the channel drains. |
| settled | Close succeeded, the resource was absent, or the transport closed. Restore admission, the capacity slot, and the in-flight entry are released. The notification fence remains until a new registration attempt for that id takes ownership or the channel exits. |
Abandonment retains ownership, but not indefinitely. overdue bounds the window in which a hung restore can hold an admission slot, an in-flight entry, and a session-id fence that nothing else can release: after the grace period the channel stops taking new work, and the ordinary drain path then recycles the child. Releasing capacity while the hidden work is still running would allow unbounded oversubscription, and force-killing a channel that still has live siblings would reintroduce exactly the failure this change removes — so neither is done.
The abandoned notification fence has no TTL and remains until the channel exits or a new registration attempt for the id explicitly takes ownership. Caller-supplied spawns and restores synchronously reserve that ownership before either path can await channel setup, so opposite-direction races cannot run two ACP registrations for the same id. The ownership transfer happens before the ACP registration call so legitimate startup notifications can be buffered; a failed attempt purges those frames and restores the ordinary 60-second closed-session tombstone.
Channel safety
At timeout, the bridge removes the id from pendingRestoreIds, clears its early events, and records the id in the channel's unsettledAbandonedRestores. If the channel has no live sessions, pending spawn, other restore, or workspace-control operation, it is synchronously marked dying and its child is killed asynchronously; the 504 response does not wait for TERM/KILL escalation. The outer qwen serve process continues running.
If any sibling work exists, the child remains alive. A late cleanup failure sets a distinct quarantine bit and does not set isDying, so sibling attach, prompt, close, and workspace control continue. Fresh work is rejected before capacity checks so the stable error is acp_channel_unavailable rather than a coincidental session-limit response. Once visible work drains, the child is recycled and a subsequent request can create a clean channel.
Whether a channel is condemned is derived, not sticky. It is reaped once visible work drains while any of these still hold: an ordinary pending empty reap, a non-empty unsettledAbandonedRestores, or quarantine. Real settlement removes the id from that set, so a channel whose late restore landed and closed cleanly returns to the configured idle-channel policy rather than being forced into a cold respawn by a timeout it already recovered from. While a restore genuinely remains unresolved, draining is what closes the transport — the only lever that breaks a permanently hung request.
Shutdown waits each real settlement promise, but every ACP request is raced with channel.exited. Killing the channel therefore releases shutdown even if the SDK request promise itself never settles. All abandoned-result handlers are rejection-safe.
Error contract
SessionRestoreTimeoutError subclasses BridgeTimeoutError but is classified first as restore_timeout. REST returns HTTP 504 with a budget-derived Retry-After clamped to 5–120 seconds and the fields code, errorKind, retryable, sessionId, action, and timeoutMs. ACP HTTP and WebSocket return JSON-RPC -32603 with the same data plus httpStatus: 504 and retryAfterSeconds.
A channel closed to fresh work rejects it with HTTP/ACP status 503 and code and error kind acp_channel_unavailable. reason distinguishes the two causes: restore_cleanup_failed (cleanup of a timed-out restore failed, so the child's state is unknown) and restore_settlement_overdue (an abandoned restore has still not settled one restore budget after its deadline).
Observability
The bridge creates a restore span and always injects its trace context into ACP load/resume metadata. It records action, channel id, session id, configured budget, public result, late result, and cleanup or quarantine result without transcript content.
The child continues that context in qwen-code.daemon.session_restore and records durations for settings load, live restore or persistence existence check, config setup, authentication, filesystem setup, session registration/replay, and response construction. The first failing stage is recorded as failed_stage.
Verification
Unit tests use deterministic delayed children and fake timers for empty-channel reap, sibling survival, same-id fencing, exactly-once late close, quarantine drain and recovery, capacity retention, transport-close arbitration, and hanging-request shutdown. REST and ACP mappings, capability propagation, SDK transport abort behavior, WebUI timer derivation, trace propagation, and failed-stage attribution are asserted independently.
The E2E plan uses a delayed child to validate the public deadline and process survival. An approximately 80 MiB transcript is a manual benchmark and trace fixture only; CI does not assert a machine-dependent latency.