Classify RPC errors and recover quietly from Durable Object resets by ndisidore · Pull Request #58 · cloudflare/cloudflare-os

ndisidore · 2026-08-07T01:52:29Z

Stacked on #56.

Addresses this class of error

Routine DO resets (storage timeouts, overload aborts i.e. the stuff we saw in the logs) were surfacing in the frontend as scary terminal errors. The reject frames actually arrive with structured flags (durableObjectReset, overloaded, etc., thanks to enhanced_error_serialization), but nothing ever read them: every call site was just catch(console.error).

Two details made it worse than it needed to be:

A user-DO reset rejects the in-flight RPC but the WebSocket stays healthy, so nothing re-fetched. Model list, sidebar, onboarding check just stayed broken until you reloaded the page.
The API session cached its user-DO stub for its whole lifetime. A stub is bound to one incarnation of the object and is permanently broken after a reset (see the DO error handling docs), so a retry through the cached one could never succeed anyway.

What changed

New RPC error classifier. Flags first; message matching only as a fallback for errors that lose them in transit.
Idempotent reads and subscribes retry once (~1.5s, jittered) after a reset. Never writes. The retry lives at one chokepoint: a withReadRetries proxy installed where useAuth creates the API stub, keyed by a method-level allowlist — idempotency is a property of the method, not the call site, so every consumer (present and future) gets the policy without per-call-site wrapping.
The backend keeps one user-DO stub per session — e-order (in-order delivery) is guaranteed per stub, so this preserves request ordering (see the review discussion). A Proxy around the stub observes rejections: flagged errors (durableObjectReset/retryable) drop the cached stub and propagate — the call may have executed, so the frontend owns recovery — while a flagless dead-capability rejection proves the call never reached the object and is transparently re-issued exactly once on a fresh stub, which is safe even for writes. Net effect: an idle reset costs the user nothing at all.
Transient failures (ones a retry or reconnect will cure) log at debug instead of toasting, and logRpcFailure reports do-resets on action paths (chat send/new, workspace create) to the client-errors endpoint so they stay visible in telemetry. State fallbacks unchanged.
Failed chat sends show an inline "may not have been sent — check the thread" hint instead of the old toast. Deliberately not auto-retried: if the reset lands after the write commits, a retry double-sends.
Auth errors get stable codes (same pattern as the workspace-open ones, both now built by one codedErrorFamily helper). Canary tests pin the capnweb message strings we match against and round-trip the flags through capnweb's real serializer; the workerd dead-capability message lives once in workshop-shared, referenced by both the backend matcher and the frontend classifier, so the canary guards both.

Intentionally Deferred

There remains a gap in this work around "Established subscriptions remain dead after a User DO reset." - subscriptions present a slightly more challenging workflow so that work is intentionally left out here.

Testing

Integration tests (abortAllDurableObjects(), the non-graceful teardown): the same session recovers transparently on the first call after a reset (this also canaries the dead-capability message — if workerd's string drifts, the never-sent retry stops firing and the test fails loudly), and the stub cache re-arms instead of churning or staying poisoned.
Unit tests pin the chokepoint contract (listed reads retry once, writes never) alongside the existing classifier/retry/canary suites. 279 backend + 142 frontend tests green.
Manual fault-injection re-run (chrome-devtools against a local dev stack, temporary debugAbort() injector): a single idle reset followed by page navigation, and a 20-abort storm at 150ms intervals across five SPA navigations, both completed with every page functional and zero application console output. Server-side spans confirmed exactly the injected aborts as exceptions, the only casualties being long-lived subscriptions (the deferred gap above); every user-facing read and write in the window succeeded, including writes issued after resets. One in-flight write during a burst failed loudly and succeeded on manual retry — the intended contract.

ndisidore · 2026-08-07T13:57:47Z

 @validateRpc()
 class AuthenticatedApiImpl extends RpcTarget implements AuthenticatedApi {
  constructor(private ctx: ExecutionContext, private env: Env,
-      private user: DurableObjectStub<UserDurableObject>,


This is likely a controversial decision.
Its driven by https://developers.cloudflare.com/durable-objects/best-practices/error-handling/ specifically the block

Many exceptions leave the DurableObjectStub in a "broken" state, such that all attempts to send additional requests will just fail immediately with the original exception. To avoid this, you should avoid reusing a DurableObjectStub after it throws an exception. You should instead create a new one for any subsequent requests.

When the the user DO resets e.g. storage timeout, overloaded abort (which is exactly what we saw in the logs) that stub is permanently poisoned. Even if we retry it will fail.

This is not super obvious because the premise does hold for the workspace path: Overseer stubs get re-resolved through the namespace on each open, so a retried openGadget genuinely reaches the restarted object. The UserDO path is the only one where a stub is cached across calls.

This should be cheap and safe: namespace.get(id) is not a network call. Stub creation is local and lazy.

Why not just force a re-load? a reload doesn't avoid the retry; it is the retry, multiplied by everything else and makes blast radius wildly disproportionate. It give a worse UX as well as the entire page resets (as opposed to trying to recover silently where possible)

i don't think it should be a problem, but do different stubs mean we lose request ordering to the userDO? i couldn't find any instances were that would be a big problem though, so it seems like a worthwhile tradeoff.

haha when writing this I told claude "have some pre-canned responses ready for the inevitable push back"
and it had one for e-order! the concern is valid. but practically nothing in our code relies on cross-call ordering through the user stub

preserving ordering while recovering poisoned stubs would require caching and centrally invalidating the stub after native RPC failures across every UserDO operation, adding substantial kernel complexity

ndisidore · 2026-08-07T14:06:20Z

+// through the closure-captured dead stub could never succeed anyway. Deliberately retries even
+// when `overloaded` is set alongside the reset — the reset destroyed the queue that was
+// overloaded, and one jittered attempt is not a retry loop. Never use for writes.
+export async function withDoResetRetry<T>(fn: () => Promise<T>, delayMs = 1500): Promise<T> {


This comment goes more in depth here, but I want get ahead of any guttural reactions: Retry is confined to idempotent reads, bounded to exactly 1 attempt.

Reads the structured flags enhanced_error_serialization already delivers to the browser (durableObjectReset, retryable, overloaded, durableObjectId; semantics per workerd jsg/util.c++ — see MR 238), with message matching as fallback. Nothing consumed these before: every call site treated a transient DO reset like a terminal error.

A reset DO rejects in-flight RPCs while the WebSocket stays healthy, and nothing ever re-fetched — the sidebar, model list, onboarding check, vendor branding, and connected-account subscriptions stayed broken until reload. The object restarts on its next request, so withDoResetRetry retries once after a jittered delay. Reads only; writes are never retried (a reset after commit would double-apply).

Transient failures (DO reset, connection loss) were logged as scary console errors with dead-end toasts at every load site. Route them through logRpcFailure — debug-level for transient, loud otherwise — and skip the toasts, since a reconnect or retry is expected to cure them. Failed chat sends now show an inline composer hint instead of a toast; the wording is hedged because a reset after commit means the message may have landed. DO resets on the send path report through reportDoResetError for telemetry.

Per the DO error-handling docs, flags are the supported contract: - auth strings move to a shared AUTH_ERROR_MESSAGES constant thrown by the backend and imported by the classifier, so they cannot drift - a canary test pins the flagless capnweb transport messages to the installed build, so an upgrade fails in CI rather than in the UX - the workerd reset strings are documented as re-wrap fallback only - withDoResetRetry documents why one jittered retry is safe despite the overloaded flag accompanying reset errors

ask-bonk

Review — Classify RPC errors and recover quietly from DO resets

I reviewed PR #58 independently from a clean install, isolating this PR's own 4 commits (f7a4a14..7d776df) from the #56 stack it's built on, and re-ran every check myself rather than relying on the prior ask-bonk passes.

Verification I ran (fresh `pnpm install`)

vitest run src/rpcErrors.test.ts → 20/20 pass (both canaries); src/ResourcePicker.test.tsx + src/homePromptFlow.test.tsx → 3/3 pass.
types:check on workshop-backend + workshop-shared + workshop-frontend → all green.
lint:check → exit 0; the only warnings (server.ts:487/517 no-shadow) predate this PR.
The load-bearing capnweb assumption, at the source: round-tripped an Error carrying durableObjectReset/overloaded/durableObjectId/code/retryable through the installed capnweb's serialize/deserialize — all five survive and the result stays instanceof Error. So "flags/codes authoritative, messages fallback" genuinely holds over the wire, and the round-trip canary (rpcErrors.test.ts:208) pins it so a dependency upgrade fails CI instead of silently demoting every classification to message-matching.

The design holds up

Flags-first classifier, precedence correct. do-reset (rpcErrors.ts:59) precedes connection (:61) and auth (:66), so a reset frame also carrying overloaded/retryable still resolves to do-reset (pinned at rpcErrors.test.ts:48).
Backend get user() is the reachability fix, and safe. Re-resolving the stub per call (server.ts:93) is what lets a retried read reach the restarted incarnation; a session-cached stub stays poisoned per the DO error-handling docs. this.users is constructor-assigned (server.ts:83) and userId is a constructor param — the getter is never invoked during construction, so this is not a field-init regression. .id.name/.id.toString() are synchronous native-DurableObjectStub reads, so a fresh stub per access is harmless micro-churn, not a disposal leak.
Integration test reproduces the real failure. open-gadget-rpc.test.ts:145 uses abortAllDurableObjects() (non-graceful) on the same socket/same AuthenticatedApiImpl — exactly what a cached stub could never recover from; the comment correctly rules out graceful evictDurableObject.
Retries confined to idempotent reads/subscribes. withDoResetRetry refuses flagless local transport errors (rpcErrors.ts:101), so it never fires through a closure-captured dead stub — the connection manager keeps that recovery. Exactly one jittered attempt, never a loop. I confirmed the workspace-create path (routes/index.tsx:121) is a write and is correctly NOT wrapped in retry — it only classifies/quiets/rethrows.
Writes correctly not auto-retried. Chat-send keeps the inline sendHiccup hint; submitMessage captures submittedChatKey and only sets the hint if the composer is still on that chat (ChatInterface.tsx:2467). Sound double-send reasoning (a reset landing after the write commits).
Auth drift closed. Both authenticate-path throws migrated to createAuthError; the non-Base64 token path (user.ts:302) now classifies as auth instead of leaking the decoder's SyntaxError. I grepped — the only remaining raw auth-ish strings (server.ts:715/718/741/744) are deployment-config messages surfaced verbatim from explicit login/createAccount submissions, correctly outside the session-token classification path. All shared AUTH_ERROR_* exports are doc-commented and isAuthErrorCode mirrors the existing isOpenGadgetErrorCode guard — kernel bar met.
"Loud on purpose" carve-outs (Connections.tsx:73, ObserverConfigModal.tsx:154) correctly keep console.error + toast where there's no retry path.
A latent leak fixed in passing: ResourcePicker.tsx adds a cancelled guard so a subscription resolving after unmount is disposed rather than stored unconditionally — and it's pinned by the new ResourcePicker.test.tsx. Good.

Non-blocking observations (all previously raised, none worth changing)

ObserverConfigModal.tsx:166 wraps Promise.all([...]) in the retry, so a reset on the second call re-runs both idempotent reads — harmless.
providers.tsx:149 wraps listModels/getQuickModel individually but leaves getAiConfig unwrapped in the same Promise.all — that one read just won't retry; slightly arbitrary.
chatKey={selectedChatId} stays null across two different not-yet-created "new chat" conversations, so a hiccup hint could persist between them — cosmetic, and handleSend clears it on the next send.
Chat-send is the one transient surface that still fires reportDoResetError to the external Reporter on every reset, whereas the retried reads don't. Defensible asymmetry; worth a one-line note so a future reader doesn't "fix" it in either direction.

Verdict

Solid, well-scoped, mergeable. The kernel change is minimal and correct, the retries are confined to idempotent reads with a fresh-stub-per-attempt getter that makes them reachable, the load-bearing wire assumption is verified and pinned, and the commit split keeps workshop-backend/workshop-shared reviewable apart from the UI. LGTM.

ask-bonk · 2026-08-07T23:14:28Z

Review posted to PR #58 at 23:14:12Z.

Summary

I independently reviewed PR #58 (RPC error classification + quiet DO-reset recovery) from a fresh pnpm install, isolating its own 4 commits (f7a4a14..7d776df) from the #56 stack, and verified its load-bearing assumptions at the source rather than relying on the prior reviews.

What I verified:

vitest run src/rpcErrors.test.ts → 20/20 pass (both canaries); the two other added/modified frontend tests → 3/3 pass.
types:check on workshop-backend + workshop-shared + workshop-frontend → all green; lint:check → exit 0 (only pre-existing warnings).
The load-bearing capnweb assumption, at the source: round-tripped an Error with all five custom props through the installed capnweb's serialize/deserialize — all survive and it stays instanceof Error, so the flag-first classifier genuinely works over the wire and the round-trip canary pins it against dependency drift.
The backend get user() getter is the correct reachability fix and safe (not a field-init regression, native-stub reads mean no disposal leak); the integration test uses the correct non-graceful abortAllDurableObjects harness; retries are confined to idempotent reads that refuse flagless local errors; the workspace-create write path is correctly not auto-retried; auth drift is closed with the only remaining raw strings correctly outside the classification path.

Verdict: Solid, well-scoped, mergeable — LGTM. I raised four non-blocking observations (all previously surfaced), none worth changing.

github run

Kieran-Hulsman

i'm a bit out of my depth here, but i spent an hour thinking through this with an agent, and everything seems sensible to me!

Kieran-Hulsman · 2026-08-07T23:24:06Z

        {draftUpdateBanner}
+        {sendHiccup && (
+          <div className="px-4 pt-2 text-xs text-kumo-warning">
+            Connection hiccup — your message may not have been sent. Check the thread, then try again.


@ndisidore what happens in this case?

The user tries to send a message to chat

The workspace reads the user’s profile and model configuration through its cached User DO connection.

The User DO has reset, so that connection fails.

The message is not written to the Overseer DO.

it looks like the message's body & attachments aren't thrown away, but retrying won't get a fresh User DO stub yet. so when the user tries to resend, will it work?

…tion E-order is guaranteed per stub, so the per-access getter from the previous commit traded ordering for reset recovery. Cache one stub per session instead, and intercept rejections through a Proxy in the getter: flagged errors (durableObjectReset/retryable) drop the cache and rethrow — the call may have executed, so the frontend owns that recovery — while a flagless dead-capability rejection proves the call never reached the DO and is re-issued exactly once on a fresh stub, safe even for writes. Two JSRPC subtleties the tests caught: `.apply` on a stub method proxy is an RPC path segment (use Reflect.apply), and JsRpcPromise.then rejects a non-function first argument. The retry goes through the raw target (via a symbol escape hatch) so cascading resets cannot retry unboundedly. The reset-recovery integration test doubles as the canary for workerd's dead-capability message; a new case pins that the cache re-arms after recovery instead of churning or staying poisoned.

Architectural: the idempotent-read retry moves from 19 per-call-site withDoResetRetry wraps (which had already drifted — providers.tsx left getAiConfig unwrapped while OnboardingWizard wrapped it) to a single withReadRetries proxy installed where useAuth creates the stub, keyed by a method-level allowlist — idempotency is a property of the method, not the call site. Future reads get the policy for free; writes and unlisted methods pass through untouched. Mechanical: the backend Proxy's RAW_USER_STUB symbol escape hatch is replaced by caching the raw stub in a second field — no symbol, no double casts, and the thrash guard compares against the trap's own target. logRpcFailure now owns do-reset telemetry via a reportSite option, collapsing the classify-report-log ritual at three action sites. The workerd dead-capability message now lives once, in workshop-shared, referenced by the backend matcher and the frontend classifier, so the frontend canary guards both. Block/line: the two coded-error families in api.ts share one codedErrorFamily factory (membership derived from the message record instead of hand-enumerated); the composer's send-hiccup hint is one state value scoped by its render condition instead of a boolean, a ref mirrored during render, and a comparison at set time; fake-timer try/finally scaffolding in rpcErrors.test.ts becomes afterEach, twin retry tests merge into it.each, and error fixtures share an rpcError helper. New tests pin the chokepoint: listed reads retry once, writes never do.

ask-bonk

Review — Classify RPC errors and recover quietly from DO resets

Heads-up for anyone reading the prior ask-bonk LGTMs: the design changed materially after them. Two new commits — b2e88a9 (Restore e-order by caching the user-DO stub, with reset-safe invalidation) and 7998195 (Simplification pass) — reversed the per-call get user() getter that every earlier review signed off on, and reworked the frontend retry from 19 per-call-site wraps into a single chokepoint. So I re-reviewed the current HEAD (7998195) from scratch rather than trusting the earlier passes, which no longer describe the code.

Verification I ran (fresh `pnpm install`)

vitest run rpcErrors.test.ts + ResourcePicker.test.tsx + homePromptFlow.test.tsx → 25/25 pass.
Integration: open-gadget-rpc.test.ts via the integration config → 2 reset-recovery tests pass (4 network-dependent auth cases skipped).
types:check on workshop-backend + workshop-shared + workshop-frontend → all green.
lint:check → exit 0 (only pre-existing warnings; server.ts:545/575 no-shadow predate this PR).

The new kernel design — backend stub caching with reset-safe invalidation — holds up

This is the substantive change and it's a genuine improvement over both prior approaches (session-cached-forever and per-call getter). The #wrapUserStub Proxy (server.ts:119) keeps e-order by caching one stub, and splits recovery by what the rejection proves:

Flagged (durableObjectReset/retryable) → the call may have executed, so it drops the cache and rethrows; the frontend owns recovery. Correct — never silently re-runs a possibly-committed write.
Flagless dead-capability (#DEAD_CAPABILITY_MESSAGES) → proves the call never reached the DO, so it re-issues once on a fresh stub, safe even for writes.

I traced the two hard cases:

Concurrent failures on the same dead target: the thrash guard if (this.rawUserStub === target) (:138) lets only the first failure swap the cache; a second concurrent neverSent failure skips the swap but still retries on the current rawUserStub — so both land on the same fresh stub, neither clobbers it. The re-arms … instead of churning integration test (:164) pins exactly this.
Cascading reset (fresh stub also dead): the retry goes through Reflect.apply on the raw target (:144), not the proxy, so a second reset can't re-enter the wrapper — bounded to one retry. The comment at :140 earns its place.

Two JSRPC subtleties are handled correctly and documented: Reflect.apply instead of value.apply (.apply is an RPC path segment on a JSRPC method proxy, :125), and .then(v=>v, onRej) because JsRpcPromise.then rejects a non-function first arg (:129). Both were caught by tests, per the commit message — good.

Pipelining is preserved where it matters. The .then(v=>v) demotes the returned JsRpcPromise to a plain Promise, which loses server-internal pipelining on the return value — but every stub-returning this.user.* method (subscribeConnectedAccounts, startResourceConfigurator, …) is just returned straight back over Cap'n Web, so nothing internal pipelines through it. .id reads take the non-function branch (:123) and aren't wrapped. Confirmed no correctness loss.

The frontend `withReadRetries` chokepoint is the right call

Moving retry from 19 call sites to one method-level allowlist installed at stub creation (useAuth.ts:59/85) makes idempotency a property of the method, not the call site — and it demonstrably fixes real drift (providers.tsx had left getAiConfig unwrapped while OnboardingWizard wrapped it). I checked the allowlist (rpcErrors.ts:126): all 8 entries exist on AuthenticatedApi and all are genuine reads; no write leaked in. subscribeConnectedAccounts is safe in the set — every call site .then(stub => …)s it (never pipelines through it), so demoting its promise is harmless, and a reset destroys the incarnation holding the subscription so a retry can't leak a dangling one.

The Proxy's passthrough branch (return (...args) => target[prop](...args), :149) preserves pipelining for the un-listed stub-returning methods (openGadget/newGadget/startResourceConfigurator), which several call sites rely on (SidebarWorkspaces.tsx:143 "pipelining"). Symbol.dispose is a symbol, so RETRYING_READS.has() is false and dispose passes through cleanly. Good.

Also correct

Classifier flag-first, do-reset → connection → auth precedence; the round-trip canary (rpcErrors.test.ts:217) pins custom-prop survival through capnweb's real serialize/deserialize, so a dependency regression fails CI instead of silently demoting to message-matching.
WORKERD_DEAD_CAPABILITY_MESSAGE now lives once in workshop-shared, referenced by both the backend never-sent matcher and the frontend classifier — one source, one canary.
codedErrorFamily factory derives membership from the message record instead of hand-enumerated guards; kernel bar met (all shared exports doc-commented).
Writes stay inline-hint-not-auto-retry with correct double-send reasoning; workspace.create is correctly not retried.

Non-blocking observations

The production dead-capability string isn't exercised end-to-end. The integration test triggers abortAllDurableObjects(), whose message is a separate literal in #DEAD_CAPABILITY_MESSAGES; the production WORKERD_DEAD_CAPABILITY_MESSAGE is only string-pinned in the frontend canary against our own classifier, not against workerd. So if workerd changes that production message, the never-sent retry silently stops firing there and no test catches it (the abort path still passes). Inherent to the harness — worth a one-line note near the constant so a future reader knows the real string rides on the doc-comment's "workerd probe," not a test.
routes/index.tsx:69 now toasts a retry-exhausted do-reset on the model list (only suppresses connection), while sibling reads just logRpcFailure. Defensible (load-bearing surface) and the inline comment explains it — just an intentional asymmetry.
The already-noted cosmetic items (the null new-chat send-hint key coalescing) still stand, none worth changing.

Verdict

Solid, well-scoped, mergeable — and notably better than the version the earlier reviews saw: the stub-caching-with-reset-safe-invalidation recovers e-order without giving up reset recovery, and the frontend chokepoint kills a whole class of per-call-site drift. The kernel diff is small, the subtle bits are tested and commented, and the commit split keeps workshop-backend/workshop-shared reviewable apart from the UI. LGTM.

ask-bonk · 2026-08-08T15:45:54Z

Review posted to PR #58.

Summary

I reviewed PR #58 and posted a fresh review — importantly, the design changed materially after all the prior ask-bonk LGTMs, so I did not rely on them.

The key finding: two new commits (b2e88a9, 7998195) that landed after the last review reversed the per-call get user() getter every earlier review had approved, replacing it with:

Backend: a Proxy-based stub cache with reset-safe invalidation that restores e-order (which the per-call getter had traded away — a concern Kieran-Hulsman raised) while still recovering from resets. Flagged rejections rethrow (call may have committed → frontend owns recovery); flagless dead-capability rejections re-issue once on a fresh stub (proven never-sent, safe even for writes).
Frontend: the 19 per-call-site withDoResetRetry wraps collapsed into a single withReadRetries chokepoint keyed by a method-level allowlist, fixing real drift (getAiConfig was unwrapped in one place, wrapped in another).

What I verified independently:

Tests: 25/25 frontend + 2 integration reset-recovery tests pass; types green; lint exit 0.
Traced the two hard concurrency cases in the backend Proxy (concurrent failures on a dead target; cascading reset) — thrash guard and raw-target retry correctly bound recovery to one attempt.
Confirmed pipelining is preserved for the stub-returning passthrough methods and that no write leaked into the retrying-reads allowlist.

Verdict: LGTM — and genuinely better than the version prior reviews saw. Raised 3 non-blocking observations (the production dead-capability string isn't exercised end-to-end; an intentional toast asymmetry on the home model list; a cosmetic send-hint edge).

github run