fix: absorb user-DO resets and wedges on the Overseer's chat hot path - #121
fix: absorb user-DO resets and wedges on the Overseer's chat hot path#121Maximo-Guk wants to merge 3 commits into
Conversation
Routine user-DO resets (storage timeouts, overload aborts — the errors seen in production logs) were surfacing in the browser as terminal failures, and the DO's in-memory subscriber registrations died silently with each reset. Recovery now lives in the Worker, one same-colo hop from the object: - Every user-DO RPC goes through the #userRead / #userWrite / #userCall choke points — a naked this.#user.x() is a review defect. Fresh stub per call: a stub is bound to one incarnation and permanently broken once it resets (DO error-handling docs), so a post-reset call simply restarts the object. Writes additionally serialize on a per-session chain: per-call stubs forfeit cross-stub e-order, and overlapping optimistic writes (rapid pin toggles) must arrive in issue order. - Idempotent reads retry once (150-400ms jittered) via do-retry.ts, whose predicate reads only the structured flags workerd attaches natively (durableObjectReset/retryable, never bare overloaded) — no message matching. Writes are never retried: a reset can land after the commit but before the response, and the client must see that ambiguity. Reads that provision vendor accounts (listProvidedAccounts callers) and subscription registration are deliberately not retried either — neither is retry-safe. - Subscriptions self-heal: the DO exposes a per-incarnation id, returned atomically from subscribe; when the session observes drift — via reset flags, a throttled check after successful calls, or an unexpected incarnation on a new subscribe — it marks the replay boundary with the subscriber's beginReplay() (fire-and-forget, ordered by the FIFO socket) and re-registers the browser's own dup()'d subscriber stub (ownership transfers per send; RPC result envelopes are disposed with the retained disposer dup()'d). Recovery passes serialize on a promise chain, no-op when the incarnation hasn't drifted, and skip entries already at the target incarnation, so over-triggering is harmless. A Worker-minted disposer survives resets so dispose always works. Accepted, pre-existing cost made explicit: retained stubs pin a subscribed user DO resident, so resets — not idle eviction — are what this machinery recovers from. TODO(stopgap): all drift detection deletes when workerd native RPC grows an onRpcBroken equivalent (capnweb already has one browser-side). - Central #onUserDoReset logging keeps reset volume visible in telemetry (user_do.reset.recovered / subscription.recovered / recover_failed) now that users stop seeing it. - authenticate() retries token verification on a fresh stub; auth failures (including corrupt base64 tokens) use the coded error family. Integration tests pin that local vitest-pool-workers aborts reject FLAGLESS (so the flag paths are unit-tested with production-shaped synthetic errors, and a pool upgrade that adds real flags fails loudly), and cover: same- session recovery across abortAllDurableObjects(), incarnation drift, the post-recovery beginReplay + catch-up replay reaching existing subscribers (and skipping already-current ones), and disposal of registrations that died with a reset.
The subscribe promise can resolve after the component unmounts; the stub was stored into a ref nothing would ever read again, leaking the DO-side registration for the rest of the session. Track cancellation and dispose the stub on late resolution; subscription failures also route through logRpcFailure so transient ones stay quiet.
The Worker-side choke points (#userRead et al.) stopped user-DO resets at the session boundary, but the Overseer kept its own naked user-DO RPCs: OverseerClientInterface minted one clientUser stub at open() and reused it for the session's lifetime. A stub is bound to one incarnation and permanently broken once it resets, so after a routine user-DO reset every getChatContext-carrying call on the still-open session — newChat, sendChatMessage, retryAgent, mergeChanges, attachment uploads — failed until the WebSocket reconnected. Worse than the reset is the wedge: an object stuck behind a closed input gate (a hung storage op) rejects nothing at all. The call queues forever, newChat never settles, and the browser hangs on send with no error and no reconnect — the exact frame trace that motivated this change (pull for newChat never resolved while the pipelined getMetadata on the same workspace stub answered, proving the Overseer itself was healthy and parked on its one outbound await). All nine getChatContext sites now go through #chatContext, which mirrors the Worker-side pattern: a fresh stub per attempt and one jittered retry via retryOnDoReset, plus a deadline (withDoCallTimeout, new in do-retry.ts) that converts the silent-wedge case into a retriable rejection. getChatContext is a pure config read, so both the retry and the timeout are safe; the timed-out call is orphaned (workerd RPC has no cancellation) with its late settlement swallowed. Reset observations log through the same user_do.reset.retrying/surfaced events as the session Worker so the absorption thesis stays checkable in telemetry. The integration test pins the asymmetric reset the retained-stub design could not absorb — the user DO dying while the workspace DO keeps running — by injecting state.abort() into just the user DO via runInDurableObject (abortAllDurableObjects kills the Overseer too, and local aborts complete, so the first post-reset fresh-stub call succeeds without needing the production-only reset flags). The test fails against the previous overseer.ts with the poisoned-stub rejection and passes with this change. Known remaining naked call, deliberately out of scope: the best-effort last-active bump (#bumpLastActiveImpl) still uses a retained stub; its failure is caught and logged, never user-facing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: absorb user-DO resets and wedges on the Overseer's chat hot path
Solid, well-scoped kernel fix. The #chatContext() helper faithfully mirrors the Worker-side #userRead choke point, the timeout precisely targets the wedged-input-gate failure mode that reset flags can't see, and the read-only/idempotent contract is respected throughout. Verified locally: types:check green, do-retry.test.ts 17/17 pass, oxlint clean on the changed lines (only pre-existing warnings elsewhere in overseer.ts). The broader unit run has unrelated failures from @gadgets/typed-storage not being prebuilt in my environment — not caused by this PR (214 tests still pass, all failures are Vite entry-resolution for that package).
Correctness — looks right
- Fresh stub per attempt.
this.impl.users.get(this.clientUser.id)mints a new stub each call, so the retry genuinely reaches the restarted incarnation rather than replaying the poisoned one. This is the crux of the asymmetric-reset bug and it's handled correctly. - Read-only safety.
getChatContext(user.ts:678) is a pure storage read — no writes — so both the jittered retry and the deadline are safe, exactly as the doc-comment anddo-retry.tsguardrails require. Good that the comment onwithDoCallTimeoutexplicitly forbids wrapping writes. - Orphan swallowing.
void call.catch(() => {})on the timeout path avoids the unhandled-rejection crash, and the unit test pins that behavior.DoCallTimeoutErroris deliberately excluded fromisDoResetErrorand matched by class inretryOnDoReset— clean separation. - Telemetry contract. Reuses
user_do.reset.retrying/user_do.reset.surfacedwithdurableObjectIdset to the user DO id (distinct from the workspace'sgadgetIdcarried byimpl.logger), so absorption stays checkable and the two DOs stay distinguishable in logs. - Scoping is accurate. The five other
getChatContextsites (agent resume, action association, hook callbacks, naming quick-model, binding creation) already mint fresh stubs per call and are off the synchronous browser-send path — correctly left alone. The retained-stub follow-ups (whoami/updatePinned/observer/sharing) are the real remaining work and are noted.
Minor observations (non-blocking)
-
withDoCallTimeoutleaves a live timer on the happy path. When the call wins the race,scheduler.wait(timeoutMs)keeps a 10s timer pending (its.thenresult is just discarded). This matches the existing pattern atoverseer.ts:5503, and the timer can't outlive an open request, so there's no hibernation/leak concern — but with nine chat calls per session each potentially arming a 10s timer, it's worth a one-line note that this is intentional and bounded. Not worth changing. -
PR-body nit on
#bumpLastActiveImpl. The body says it "still uses a retained user-DO stub"; it actually mints a fresh stub (this.users.get(...), overseer.ts:3026) but is a write (setGadgetLastActive), so it correctly must not retry and is caught + logged best-effort. The out-of-scope conclusion is right; only the "retained stub" wording is imprecise. -
Integration test relies on a workerd-abort side effect.
runInDurableObject(userStub, (_i, state) => state.abort(...))aborting the very call that delivers the closure — so the injected abort surfaces as therunInDurableObjectrejection you assert viarejection(...). This is clever and the comment explains it, but it's a subtle dependency on abort timing; if a future pool upgrade changes whenstate.abort()tears down the in-flight call, this test could silently stop exercising the reset. Theuser-DO reset flagstest above it is the tripwire for pool-behavior drift, so acceptable.
Verification I'd want confirmed in CI (can't fully run here)
- The integration suite green claim (
open-gadget-rpc.test.ts) — my env can't run the workers pool for the full integration run. The PR states it was verified to fail against the pre-fixoverseer.tswith the poisoned-stub rejection and pass with the fix; that failing-first evidence is the important part and is documented.
LGTM. The design is careful about the write/read distinction that makes this class of retry safe, the choke-point mirroring keeps the two recovery paths consistent, and the follow-ups are honestly enumerated.
|
Review posted to PR #121. SummaryI reviewed PR #121 ( Verification done:
Verdict: LGTM. The fix is well-scoped and careful about the write/read distinction that makes this retry class safe. Minor non-blocking notes I raised:
One thing I flagged that CI should confirm: the full integration suite couldn't run in my environment (workers pool + an unrelated |
58866bc to
1b25391
Compare
2 participants
WIP