Skip to content

Retry pure user-DO reads once across a reset - #263

Open
ndisidore wants to merge 1 commit into
mainfrom
nathan/chore-user-do-retry
Open

Retry pure user-DO reads once across a reset#263
ndisidore wants to merge 1 commit into
mainfrom
nathan/chore-user-do-retry

Conversation

@ndisidore

@ndisidore ndisidore commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

#133 and #162 made user-DO resets survivable by minting a fresh stub per call, but deliberately left retries out until telemetry showed they were worth adding. The user_do.reset.surfaced volume has now made that case: the one call in flight at the moment the DO resets still fails all the way to the browser.

This change retries that call once. A reset can't tell the caller whether the failed call was applied or not, so only calls that are safe to replay may retry. It requires an explicit retryOnDoReset(() => this.#user.x()) wrapper around strictly pure reads. The thunk re-evaluates the fresh-stub getter, so the second attempt reaches the new incarnation; errors are never re-wrapped, so the workerd flags the browser's classifier reads stay intact.

Successful retries log user_do.reset.recovered, which correlated against user_do.reset.surfaced gives the retry win rate and tells us whether more sites are worth wrapping.


Open in Devin Review

@github-actions github-actions Bot added the kernel Changes to the Workshop kernel label Aug 18, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@ndisidore
ndisidore force-pushed the nathan/chore-user-do-retry branch from f172c11 to c356bec Compare August 18, 2026 23:27
user_do.reset.surfaced volume (from #133/#162, which deliberately deferred
retries) showed the one call in flight at reset time is worth recovering
instead of surfacing to the browser.

do-telemetry.ts becomes do-retry.ts and gains retryOnDoReset(thunk): exactly
one retry, full jitter (a mass reset fails every in-flight call at once), and
identity rethrow so the workerd flags the frontend classifier reads survive.
The retry predicate is narrower than isDoResetError: durableObjectReset
retries even when flagged overloaded (the incarnation is dead, so the queue
that overloaded it died with it -- this is the shape production
storage-timeout resets arrive in, {remote, overloaded, durableObjectReset}),
while bare retryable retries only if the object isn't shedding load.

The replay-safety judgment lives at each call site as an explicit, greppable
wrapper -- no method-name allowlist. A reset can't distinguish "never applied"
from "applied, response lost", so only strictly pure reads are wrapped: the 14
read delegations in server.ts and the read sites on the fresh-stub session
getters in overseer.ts (whoami, getChatContext, listModels, plus the two
OverseerImpl-internal fresh-stub reads). The thunk re-evaluates the fresh-stub
getter, so the second attempt reaches the new incarnation for free. Reads
with hidden writes (listOutputs, listProvidedAccounts) and every write keep
today's behavior; getChatContext's doc now pins that it must stay write-free.

Successful retries log user_do.reset.recovered; correlated against
user_do.reset.surfaced (which still fires per attempt, with operation
attribution) this gives the retry win rate and the signal for wrapping more
sites later.
@ndisidore
ndisidore force-pushed the nathan/chore-user-do-retry branch from c356bec to 2440a15 Compare August 18, 2026 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel Changes to the Workshop kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant