Absorb user-DO resets in the Worker by ndisidore · Pull Request #133 · cloudflare/cloudflare-os

ndisidore · 2026-08-10T21:20:58Z

Routine Durable Object resets — storage timeouts, overload aborts; ~3k over a recent 4-day window in production logs — were wedging sessions: a stub is permanently broken once its incarnation resets, so a cached stub turned one reset into every subsequent call failing until reload, surfacing as scary terminal errors.

What changed

Fresh stub per call. The session re-resolves its user-DO stub on every access, so a post-reset call simply restarts the object

Ordering is now a client-side contract. Per-call stubs forfeit workerd's per-stub e-order, so cross-call delivery order is no longer guaranteeded arrival would silently invert the durable state — now guard in-flight (clicks are ignored until the RPC settles).

Deliberately no retries. With fresh stubs, the only thing a reset can still break is the call in flight at that moment; workerd's structured reset flags reach the client, which #110 already classifies and quiets.

Deliberately deferred / out of scope

Subscription self-heal: the DO holds subscriber registrations in memory only, so a reset still silently kills them while the browser's WebSocket stays health
Workspace (Overseer) DO resets keep their pre-existing session-abort handling;

Testing

Integration (abortAllDurableObjects(), the non-graceful teardown): the same session recovers transparently after a reset via fresh stubs, and the local-aborts-reject-flagless shape is pinned so the flag path can graduate from synthetic unit tests to real-reset integration tests if a future pool upgrade attaches the production flags. (The suite also caught a real wrapper subtlety: native RPC method handles are proxies, so the wrapper must invoke through the stub rather than .apply on an extracted handle.)
Unit: the reset-flag predicate against production-shaped synthetic errors. 284 backend unit + integration + 135 frontend green.
Manual fault injection (chrome-devtools against a local dev stack, temporary debugAbort() injector, composed with the Classify RPC errors client-side and quiet recoverable failures #110 client): run on the pre-split branch — 72 aborts across idle, reload-race, and SPA-navigation-storm scenarios with every page functional and zero application console output; a chat send racing a workspace-DO abort showed the inline hint (Classify RPC errors client-side and quiet recoverable failures #110's UI), preserved the draft, and did not double-send.

Maximo-Guk · 2026-08-10T22:33:36Z

GPT comments:

1. Medium: do-retry.ts:19 retries errors with both overloaded and reset flags. Cloudflare explicitly says overloaded errors must not be retried; this can amplify an overload. Check overloaded before retrying.
2. Medium: server.ts:125-152 allows queries to overtake earlier commands because each uses a fresh stub and queries bypass #commandChain. This regresses RPC e-order/pipelining. Queries should await a snapshot of prior commands.
3. Low: server.ts:811,861 retries authentication resets without emitting the new retry/surfaced telemetry, undercounting resets.

ask-bonk · 2026-08-11T13:51:07Z

Both guards are present and consistent:

BlueprintList.handleTogglePin uses a Set<string> ref (per-item, since pins are keyed by blueprint id).
providers.handleSetQuick uses a boolean ref (correct, since quickModel is a single scalar, not per-row).

Both ignore clicks while in flight, revert optimistic state on error, and clear the guard in finally. The design contract is documented in server.ts:106-110.

The code is complete and correct. My conclusion:

Summary

The one blocking item from the reviews — the unguarded optimistic toggle in providers.tsx handleSetQuick — is already fixed and committed on the branch (commit 360182b, "guard optimistic toggles client-side"). The working tree is clean and matches origin. There is no outstanding code change to make.

What happened in the comment history: after ask-bonk's substantive review (11:39) flagged handleSetQuick, the fix was landed by the author in 360182b. The later "Fixed:" comment (13:26) from a Bonk run tried to re-apply an already-present change and then failed on git push with a 403 (Permission to cloudflare/cloudflare-os.git denied to github-actions[bot]) — an infrastructure permission problem, not a code problem. Those 403 comments are noise.

Current state of PR #133

All review feedback resolved:

handleSetQuick guarded via quickInFlight boolean ref (providers.tsx:190-207), mirroring pinsInFlight in BlueprintList.tsx:206-224. A boolean is the right shape since quickModel is a single scalar, not per-row.
The three GPT items were correctly assessed as stale against the superseded Absorb user-DO resets in the Worker and self-heal subscriptions #104 retry design — no retry layer exists here; do-reset.ts is a telemetry-only classifier that already excludes overloaded-alone (do-reset.ts:11-18).
delayMs: undefined nit resolved — #onUserDoReset (server.ts:159-166) emits only event/operation/durableObjectId/error.

Design is sound at HEAD:

Fresh stub per call (#userStub, server.ts:112) with the sole #userCall chokepoint (server.ts:126); no naked stub calls remain.
Client-side ordering contract documented at server.ts:106-110, and both live optimistic toggles now guard in-flight.

No further code changes are warranted. The branch is ready to merge pending the push-permission issue being resolved on the infrastructure side (which the GitHub Action handles automatically).

github run

Routine user-DO resets (storage timeouts, overload aborts — the errors seen in production logs) were wedging sessions: a stub is bound to one incarnation and permanently broken once it resets (DO error-handling docs), so a cached stub turned one reset into every subsequent call failing until reload. - Fresh stub per call: #user re-resolves on every access, so a post-reset call simply restarts the object and the session self-heals structurally. - Every user-DO RPC goes through the #userCall / #userCommand choke points — a naked this.#user.x() is a review defect. Typed #query/#command Proxies route the ~35 plain delegation sites through them while deriving the telemetry operation name from the DO method. The split is about ordering: commands are effectful and serialize on a per-session chain (per-call stubs forfeit cross-stub e-order, and overlapping optimistic writes like rapid pin toggles must arrive in issue order); queries stay concurrent. - Deliberately NO retries: fresh stubs already fix the wedged-session mode, what remains is only the call in flight at the reset moment, workerd's structured reset flags reach the client (which classifies and quiets them, #110), and the DO platform is moving toward transparent recovery. do-reset.ts keeps the flag predicate (durableObjectReset/retryable, never bare overloaded) purely to classify surfaced resets for telemetry (user_do.reset.surfaced) — the volume check on this design's thesis. - authenticate() rejects corrupt base64 tokens as coded auth failures instead of leaking the decoder's SyntaxError. Integration tests pin that local vitest-pool-workers aborts reject FLAGLESS (so the flag predicate is unit-tested with production-shaped synthetic errors, and a pool upgrade that adds real flags fails loudly) and cover same-session recovery across abortAllDurableObjects(). Deliberately deferred: self-healing of connected-accounts subscriptions across resets (the DO's in-memory registrations die with the incarnation, so a subscribed browser silently stops receiving updates until it re-subscribes or the socket reconnects). A TODO(deferred) at the subscribe chokepoint marks the gap; the implementation is split out to the follow-up branch feat/do-reset-subscription-self-heal.

Review consensus: cross-stub write ordering isn't worth a Worker-side scheduler. With retries already gone, the command chain was the last piece of call machinery, and its live beneficiaries were the two optimistic toggles — BlueprintList's pin and the providers page's quick-model row (the very example the original chain comment cited): two unawaited writes to the same key could land reversed on separate per-call stubs and silently invert the durable state (both succeed, so nothing reverts the UI). Move the protection to where the overlap originates: both toggles now ignore clicks while their RPC is in flight, and the stub-lifetime comment documents the client-side contract — UI that can fire overlapping writes to the same state must guard in-flight. #userCommand and #commandChain are deleted, and the #query/#command proxies collapse into a single #user proxy, so delegation sites read like main's `this.user.x()` again (the raw fresh-stub getter becomes #userStub). #userCall remains the sole chokepoint, for reset telemetry and the no-naked-stub discipline.

Per kentonv's review: the #userCall chokepoint + method-proxy machinery was more complexity than the job needed. It collapses into one function, wrapDoStubForTelemetry(stub) — a transparent Proxy that logs DO-reset rejections (user_do.reset.surfaced, operation = method name) and rethrows unchanged. The #user getter applies it, so every delegation site is just this.#user.x() and the bespoke closure sites become plain calls; helpers receive the wrapped stub directly. isDoResetError moves into the same file (do-reset.ts was too small to live alone) with the suggested comment, and the stub getter takes the suggested comment verbatim. Wrapper subtlety worth keeping: invoke through the stub (target[prop](...)) rather than .apply on the extracted handle — native RPC method handles are themselves proxies, and touching .apply on one is interpreted as a nested RPC property access (the DO rejects a call to "apply"; caught by the integration suite). Also from review: drop the subscribe TODO (the deferred self-heal belongs client-side — detect the subscriber callback's disposal and re-subscribe — so there is nothing to mark here) and shrink the toggle-guard comments.

Co-authored-by: Kenton Varda <kenton@cloudflare.com>

ask-bonk · 2026-08-11T17:18:47Z

All naked this.users.get() calls are in PublicApiImpl (pre-auth: authenticate/authenticateFromCfAccess/login/createAccount) — a single call each, no session to recover, consistent with the design. Correct.

The code is clean and well-reasoned. Let me do a final assessment of correctness on the observability field and the test consistency, then form my conclusion.

Let me verify the do-telemetry.test.ts matches the actual predicate and check the observability.ts durableObjectId field is the one used.

The unit test covers the predicate thoroughly, the integration test pins the local-flagless invariant, and durableObjectId was added to the observability field vocabulary and used in wrapDoStubForTelemetry. All consistent.

Now let me confirm whether the do-telemetry file's docstring reference to itself and comments are accurate. The file comment references do-telemetry.ts (self). Good.

I've reviewed all 9 changed files thoroughly. Let me summarize my findings.

Review complete

I read every line of the diff at HEAD (80c40ff) across all 9 changed files, plus the surrounding chokepoint call sites and both test files. This is a clean, well-scoped, well-documented kernel change. I found no actionable code defects introduced by this change.

What I independently verified:

Fresh-stub-per-call is correct. get #user (server.ts:96) mints a new stub per access so a poisoned incarnation is never reused; the integration test pins the crucial permanently-broken (not fail-once) invariant the design rests on (open-gadget-rpc.test.ts).
No naked user-DO calls bypass telemetry. Every authenticated method routes through #user; the only naked this.users.get() calls (server.ts:682/700/727/754) are the pre-auth PublicApiImpl methods, each a single call with no session to recover — consistent with the design.
getGatekeeperApp correctly reuses one stub across its two dependent calls (server.ts:577), avoiding a torn read across incarnations.
wrapDoStubForTelemetry proxy is transparent: non-functions pass through, symbol keys (incl. Symbol.dispose) are handled (do-telemetry.ts:36), and the "invoke through the stub, not .apply on the extracted handle" subtlety is correctly documented and implemented.
isDoResetError matches its comment and tests. overloaded-alone is excluded; the production {remote, overloaded, durableObjectReset} shape still matches (via durableObjectReset). Unit tests cover all shapes.
Corrupt-token classification (user.ts:300-306) converts the base64 SyntaxError into a coded invalidSessionToken. Correct.
The prior GPT comments (retry layer / query-command split / auth-retry telemetry) are all stale against HEAD — there is no retry layer or command chain in this branch; do-telemetry.ts is telemetry-only.

One non-blocking observation on PR state (not a code defect): kentonv's 14:38 review asked to drop the pinsInFlight/quickInFlight guards in BlueprintList.tsx and providers.tsx as "pedantic." Those guards are still present at HEAD — the two ask-bonk comments claiming to have removed them both 403'd on push, so the removal never landed. Since kentonv said "I don't care that much since it's UI code," this doesn't block, but the requested change remains unapplied on the branch. Not raising it as an inline finding since it's a maintainer preference already voiced, not a defect in the change.

LGTM!

github run