Classify RPC errors and recover quietly from Durable Object resets

[ndisidore]

Stacked on #56.

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 one backend change: the authenticated API re-resolves its user-DO stub per call instead of caching a poisoned one, which is what makes the retry actually reach the restarted object.
• Transient failures (ones a retry or reconnect will cure) log at debug instead of toasting. 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), and a canary test pins the capnweb message strings we match against, so a dependency upgrade fails CI instead of quietly breaking classification. A second canary round-trips the flags through capnweb's real serializer since the whole design leans on them surviving the wire.

Testing

Frontend suite + tsc green, backend too. The classifier tests use a reject frame captured from prod.

[ask-bonk]

Review — Classify RPC errors and recover quietly from DO resets

I read the whole diff against the PR base (c637d9f), verified the capnweb error-serialization semantics the classifier depends on, and ran the frontend suite + backend/shared/frontend builds. Solid, well-scoped work — the commit split, the doc-comments, and the "loud on purpose" annotations make the intent easy to follow.

Verification I ran

• vitest run src/rpcErrors.test.ts → 17 passed, including the capnweb transport-string canary.
• pnpm --filter workshop-backend --filter workshop-shared build and --filter workshop-frontend build → all green (covers the AuthenticatedApiImpl stub refactor and the shared AUTH_ERROR_* additions).

The load-bearing assumption checks out

The whole design rests on custom Error props (durableObjectReset, overloaded, retryable, durableObjectId, and the new code) surviving the WebSocket round-trip. I confirmed this against the installed capnweb 0.8.0:

• Serialize walks Object.keys(e) and captures every own-enumerable prop except name/message/stack (dist/index.cjs:1515).
• Deserialize restores them onto a real Error subclass instance (dist/index.cjs:1697, :1709), so both flag(err, …) and err instanceof Error in messageOf behave as intended.

So "flags/codes are authoritative, messages are fallback" is genuinely true over this transport — good.

The backend stub fix is the right call

Turning the cached user stub into a per-call getter (server.ts get user()) is what actually makes the frontend retries reachable, and the doc-comment explaining "poisoned incarnation → fresh stub per attempt" is exactly the kind of kernel comment that belongs here. Migrating the two raw auth throws to createAuthError(...) and confirming there are no other stringly-typed auth throws left (I grepped — only the message table and tests reference the literals) closes the drift loop cleanly.

Suggestions (non-blocking)

1. The canary guards messages but not the property round-trip. The classifier's primary path is flag-based, yet the only capnweb-pinned behavior is CONNECTION_MESSAGES. The reject-frame test uses a synthetic Object.assign(new Error(...), {...}), so a hypothetical capnweb regression that stopped serializing custom Error props would pass CI green while silently reverting every flag-based classification to the message fallback (and dropping code entirely → auth errors misclassified). Consider one integration-style assertion that an Error with a custom prop survives an actual capnweb devaluate/evaluate round-trip, so the assumption I verified by hand is pinned the same way the strings are.

2. get user() now mints a fresh stub on every access, including repeated accesses within one method. e.g. server.ts:215-216 reads this.user.id.toString() then this.user.id.name! back-to-back, and #isAdmin() is called alongside other this.user reads in the same request. The comment correctly notes stub creation is local/lazy, so this is a micro-cost, not a correctness issue — but a couple of hot methods that touch this.user several times could bind it to a const locally to avoid the churn. Purely optional.

3. 'auth' currently has no consumer beyond making isTransientRpcError return false. That's the intended safety property (never quiet/retry auth failures), so this is fine — just flagging that the whole AUTH_ERROR_* shared surface exists to encode a negative right now. Worth a one-line comment at the classifyRpcError auth branch noting the class is deliberately terminal, so a future reader doesn't assume there's a missing "re-auth" handler.

Nothing here blocks merge. The retries are correctly confined to idempotent reads/subscribes, writes keep the inline-hint-not-auto-retry behavior (the double-send reasoning in the chat-send path is right), and the ObserverConfigModal/Connections "loud on purpose" carve-outs show the quieting was applied thoughtfully rather than blanket.

[ask-bonk]

Review posted to PR #58.

Summary

I reviewed PR #58 (RPC error classification + quiet DO-reset recovery), verified its core assumptions, and posted a review.

What I checked:

• Read the full diff against the PR base (c637d9f) — all 18 files.
• Ran vitest src/rpcErrors.test.ts → 17/17 pass, including the capnweb canary.
• Built workshop-backend, workshop-shared, and workshop-frontend → all green.
• Independently verified the load-bearing assumption in capnweb 0.8.0 source: custom Error properties (durableObjectReset, overloaded, retryable, durableObjectId, code) do survive the wire round-trip (dist/index.cjs:1515 serialize, :1697/:1709 deserialize into a real Error subclass), so "flags/codes authoritative, messages fallback" holds over WebSocket.
• Grepped for stray raw auth-error strings — confirmed only the message table + tests remain after the createAuthError migration.

Verdict: Solid, well-scoped, mergeable. The backend per-call stub getter is what makes the retries reachable and is correctly the elegant kernel fix. Retries are confined to idempotent reads/subscribes; writes keep the inline-hint (not auto-retry) behavior with correct double-send reasoning.

Non-blocking suggestions raised:

1. The canary pins the transport strings but not the custom-prop round-trip the flag path depends on — a capnweb regression could silently revert everything to message fallback while CI stays green. Suggested pinning it with a real devaluate/evaluate assertion.
2. get user() mints a fresh stub on each access, including repeated accesses in one method — micro-cost, optionally bind to a local const in hot methods.
3. The 'auth' class is deliberately terminal (only makes isTransientRpcError false) — worth a one-line comment so a future reader doesn't assume a missing re-auth handler.

github run

[ask-bonk]

Review — Classify RPC errors and recover quietly from DO resets

I re-read the full diff against the base (5d8cc25), ran the classifier suite myself, and independently checked the load-bearing assumptions. This is solid, well-scoped work and the earlier non-blocking feedback has been folded in cleanly.

Verification I ran

• vitest run src/rpcErrors.test.ts → 20 passed (up from 17 — the new round-trip test is included).
• Re-read rpcErrors.ts, the server.ts/user.ts stub + auth-code changes, the AUTH_ERROR_* additions in workshop-shared, and every frontend call site (ChatInterface, routes/index, __root, gatekeepers, SidebarWorkspaces, ObserverConfigModal, Connections, …).

Prior suggestions are addressed

1. Property round-trip is now pinned. rpcErrors.test.ts:208-218 runs a real deserialize(serialize(...)) and asserts durableObjectReset/overloaded/durableObjectId/code all survive and that the result still classifies as do-reset. This is exactly the gap I'd have flagged: the flag path is primary, and now a capnweb serialization regression fails CI instead of silently demoting everything to message-matching. Good.
2. Terminal auth is documented. rpcErrors.ts:64-65 states the class is deliberately terminal with no missing re-auth handler — a future reader won't mistake it for an omission.
3. The get user() micro-cost was explicitly optional; leaving it is fine since the getter is a local, lazy this.users.get(id), not a network call.

The design holds up

• Classifier precedence is correct. do-reset (:59) is checked before connection (:61) and auth (:66), so a reset frame that also carries overloaded/retryable still resolves to do-reset — matched by the test at :48.
• Retries are correctly confined to idempotent reads/subscribes. withDoResetRetry refuses flagless local transport errors (:101), so a retry never fires through a closure-captured dead stub; connection-manager recovery keeps ownership of that path. The chat-send path stays inline-hint-not-auto-retry, and the double-send reasoning (a reset after commit) is right.
• The backend stub fix is the reachability fix. server.ts get user() re-resolving per call is what lets a retried read hit a fresh incarnation; a cached stub would stay poisoned. This is the correct kernel change and the doc-comment earns its place.
• Auth drift is closed. Both raw throws migrated to createAuthError, and the non-Base64 token path in user.ts now classifies as an auth failure rather than leaking the decoder's SyntaxError — a nice catch.
• The "loud on purpose" carve-outs in Connections/ObserverConfigModal (no retry path → don't quiet) show the quieting was applied per-surface, not blanket.

Non-blocking nits

• ObserverConfigModal.tsx:131 wraps Promise.all([...]) in withDoResetRetry. If the first call resolves but the second rejects with a reset, the retry re-runs both reads. Both are idempotent gatekeeper-metadata reads so it's harmless, just slightly more work than necessary — not worth changing.
• withDoResetRetry around subscribeConnectedAccounts is safe (the failing attempt returns no stub, and the .then disposes on cancelled), but it's worth keeping in mind that any future non-idempotent subscribe with side effects on open would need a different wrapper. The doc-comment's "never use for writes" covers the intent.

Nothing blocks merge. LGTM.

[ask-bonk]

Review posted to PR #58.

Summary

I independently re-reviewed PR #58 rather than rubber-stamping the prior review, verified its core assumptions myself, and posted a fresh review.

What I checked:

• Read the full diff against the base (5d8cc25) — the classifier, backend stub/auth-code changes, shared AUTH_ERROR_* additions, and all frontend call sites.
• Ran vitest run src/rpcErrors.test.ts → 20/20 pass (the suite grew from 17 to 20).
• Verified classifier precedence, retry confinement, and stub-disposal safety by reading the code.

Key finding — the prior review's suggestions are now implemented:

1. The capnweb custom-property round-trip is pinned (rpcErrors.test.ts:208-218): a real serialize/deserialize asserts the flags + code survive. This closes the gap where a serialization regression could have silently reverted the flag path to message-matching while CI stayed green.
2. The terminal auth class now carries an explanatory comment (rpcErrors.ts:64-65).
3. The optional get user() micro-cost was correctly left as-is.

Verdict: Solid, well-scoped, mergeable. The backend per-call get user() getter is the correct kernel fix that makes retries reachable; retries are confined to idempotent reads/subscribes and refuse flagless local transport errors; the chat-send path keeps inline-hint-not-auto-retry with correct double-send reasoning. I raised two non-blocking nits (the Promise.all retry re-runs both reads on partial failure; a note about subscribe wrappers and future non-idempotent opens), neither worth changing. LGTM.

github run

[ask-bonk]

Review — Classify RPC errors and recover quietly from DO resets

I re-reviewed the full diff against the base (5d8cc25) independently, ran the classifier suite, and typechecked the kernel changes rather than relying on the two prior reviews.

Verification I ran

• vitest run src/rpcErrors.test.ts → 20 passed, including the capnweb transport-string canary and the new serialize/deserialize custom-property round-trip (rpcErrors.test.ts:208-218).
• types:check on workshop-backend + workshop-shared → green (covers the get user() refactor and the shared AUTH_ERROR_* surface).

The design holds up

• Flags-first classifier, precedence is correct. do-reset (rpcErrors.ts:59) is checked before connection (:61) and auth (:66), so a reset frame that also carries overloaded/retryable still resolves to do-reset (pinned by the test at :48). The round-trip test now closes the one gap I'd have flagged: a capnweb regression that dropped custom Error props would fail CI instead of silently demoting everything to message-matching.
• Backend stub fix is the reachability fix. Turning the cached stub into a per-call get user() (server.ts:93) is what lets a retried read hit a fresh incarnation; the doc-comment ("poisoned incarnation → fresh stub per attempt") earns its place in the kernel. I checked the subscription path: subscribeConnectedAccounts (server.ts:332) returns a stub bound to the resolved incarnation, so a reset that fails the first attempt leaves nothing registered on a live incarnation and the retry can't leak a dangling subscription.
• Retries confined to idempotent reads/subscribes. withDoResetRetry refuses flagless local transport errors (rpcErrors.ts:101), so a retry never fires through a closure-captured dead stub — the connection manager keeps ownership of that recovery. Writes keep inline-hint-not-auto-retry; the double-send reasoning (a reset after commit) is right.
• Auth drift closed. Both raw throws migrated to createAuthError, and the non-Base64 token path in user.ts:302 now classifies as an auth failure instead of leaking the decoder's SyntaxError — a good catch.

One observation on the auth message fallback (non-blocking)

AUTH_MESSAGES is matched with message.includes(m) (rpcErrors.ts:66), and one value — "invalid session token" — is a short, generic phrase. A future unrelated error whose message merely contains that substring would misclassify as auth. That's harmless here because auth is the terminal class (never quieted, never retried), so the misclassification can only ever make the UX more conservative, never wrongly quiet/retry a real failure — and AUTH_ERROR_MESSAGES is documented as a fallback for older deployments where the code didn't survive. Worth keeping in mind if the fallback list ever grows with genuinely ambiguous phrases; not worth changing now.

The "loud on purpose" carve-outs are right

Connections.tsx:73 and ObserverConfigModal.tsx:120 deliberately keep console.error + toast because those surfaces have no retry path — a quieted transient there would strand the user on a permanent loader / empty panel. Applying the quieting per-surface rather than blanket is the correct call.

Nothing blocks merge. Solid, well-scoped, and the commit split keeps the kernel changes easy to read apart from the UI. LGTM.

[ask-bonk]

Review posted to PR #58.

Summary

I independently re-reviewed PR #58 by reading the full diff against the base (5d8cc25), running the classifier suite (20/20 pass), and typechecking the backend + shared kernel changes — not relying on the two prior ask-bonk reviews.

What I verified myself:

• Classifier precedence (do-reset → connection → auth) is correct and flag-first; the new serialize/deserialize round-trip test (rpcErrors.test.ts:208-218) pins the load-bearing custom-property survival so a capnweb regression fails CI rather than silently reverting to message-matching.
• The backend get user() per-call getter (server.ts:93) is the correct kernel change that makes retries reachable; I confirmed the subscribe path can't leak a dangling subscription on a reset-failed first attempt.
• Retries are confined to idempotent reads/subscribes and refuse flagless local transport errors (rpcErrors.ts:101); writes correctly keep inline-hint-not-auto-retry with sound double-send reasoning.
• Auth drift is closed (both raw throws migrated to createAuthError; the non-Base64 token path now classifies as auth instead of leaking a SyntaxError).

One new observation the prior reviews didn't surface: the auth message fallback matches "invalid session token" as a substring, which is a generic phrase — but since auth is the terminal class, any misclassification can only be more conservative (never wrongly quiets/retries). Non-blocking, just worth noting if that fallback list grows.

Verdict: Solid, well-scoped, mergeable. LGTM.

github run

[ask-bonk]

Review — Classify RPC errors and recover quietly from DO resets

I reviewed the full diff against the base (010dd53) independently — reading the classifier, the backend get user()/auth-code changes, the shared AUTH_ERROR_* surface, and every frontend call site — and re-ran the checks myself rather than relying on the prior reviews.

Verification I ran (from a clean install)

• vitest run src/rpcErrors.test.ts → 20/20 pass, including both canaries (the transport-string pin and the serialize/deserialize custom-property round-trip at rpcErrors.test.ts:208).
• types:check on workshop-backend + workshop-shared + workshop-frontend → all green.
• lint:check → exit 0 (no new warnings from any file this PR touches).

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. The round-trip canary closes the one real gap: a capnweb regression dropping custom Error props would fail CI instead of silently demoting everything to message-matching.
• Backend stub fix is the reachability fix, and not a regression. Turning the cached stub into a per-call get user() (server.ts:93) is what lets a retried read hit a fresh incarnation. I checked the field-init order: this.users is constructor-assigned exactly as before (only the user param moved to a getter), and the getter is never invoked during construction — so the refactor is safe. The doc-comment earns its place in the kernel.
• Subscribe retry can't leak a subscription. I traced subscribeConnectedAccounts (user.ts:1342): a reset destroys the incarnation that would hold the subscription, so a retry through the fresh stub re-subscribes to the new incarnation with nothing dangling on a live one. Safe.
• Retries confined to idempotent reads/subscribes. withDoResetRetry refuses flagless local transport errors (rpcErrors.ts:101), so a retry never fires through a closure-captured dead stub; the connection manager keeps that recovery. Writes keep inline-hint-not-auto-retry, and the double-send reasoning (a reset after commit) is right.
• Auth drift closed. Both raw throws migrated to createAuthError, and the non-Base64 token path (user.ts:302) now classifies as auth instead of leaking the decoder's SyntaxError.
• "Loud on purpose" carve-outs (Connections.tsx, ObserverConfigModal.tsx subscribe) correctly keep console.error + toast where there's no retry path.

One new non-blocking observation

ChatInput clears the send-hiccup hint on chatKey change (useEffect(() => setSendHiccup(false), [chatKey])), and chatKey={selectedChatId} is number | null. When the composer is on the "new chat" surface, selectedChatId stays null across two different not-yet-created conversations, so a hiccup hint from a first failed new-chat send would persist into a second new-chat attempt rather than clearing. It's a cosmetic edge (a stale but accurate "may not have been sent — check the thread" warning on the same empty surface), and handleSend resets it on the next send anyway (setSendHiccup(false) at the top), so nothing misfires. Not worth changing; just noting the null-key coalescing since the prior reviews didn't.

Verdict: Solid, well-scoped, and the commit split keeps the kernel changes reviewable apart from the UI. LGTM.

[ask-bonk]

Review posted to PR #58.

Summary

I independently re-reviewed PR #58 from a clean checkout rather than relying on the three prior ask-bonk reviews — reading the full diff against the base (010dd53) and re-running every check myself.

What I verified:

• Tests: vitest run src/rpcErrors.test.ts → 20/20 pass (both canaries included).
• Typecheck: workshop-backend + workshop-shared + workshop-frontend all green.
• Lint: lint:check exit 0, no new warnings from any touched file.
• Kernel get user() fix: confirmed it's the reachability fix and not a field-init regression — this.users is constructor-assigned exactly as before; only the user param became a lazy getter, never invoked during construction.
• Subscribe retry safety: traced subscribeConnectedAccounts — a reset destroys the incarnation holding the subscription, so retrying through a fresh stub can't leak a dangling subscription on a live incarnation.
• Classifier precedence, retry confinement, auth-code migration, double-send reasoning: all check out against the code.

One new non-blocking observation the prior reviews missed: ChatInput clears the send-hiccup hint on chatKey change, but chatKey={selectedChatId} stays null across two different not-yet-created "new chat" conversations, so a hiccup hint could persist across them. Purely cosmetic (the warning stays accurate, and handleSend resets it on next send) — not worth changing.

Verdict: Solid, well-scoped, mergeable. LGTM.

github run

[ask-bonk]

Review — Classify RPC errors and recover quietly from DO resets

I reviewed PR #58 from a fresh checkout, reading the full diff against the base and re-running every check myself rather than relying on the prior ask-bonk reviews.

Verification I ran

• Installed deps with a frozen lockfile, then vitest run src/rpcErrors.test.ts → 20/20 pass (both canaries: the transport-string pin and the serialize/deserialize custom-property round-trip at rpcErrors.test.ts:208).
• build on workshop-backend + workshop-shared + workshop-frontend → all green (covers the get user() refactor, the shared AUTH_ERROR_* surface, and every touched frontend call site).
• lint:check → exit 0 (no new warnings from any file this PR touches; the two no-shadow warnings in server.ts:487/517 predate this PR).
• Grepped for stray raw auth strings → only the AUTH_ERROR_MESSAGES table remains; both raw throws migrated to createAuthError.

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. The round-trip canary closes the one real gap: a capnweb regression dropping custom Error props fails CI instead of silently demoting everything to message-matching.
• Backend stub fix is the reachability fix, and safe. Turning the cached stub into a per-call get user() (server.ts:93) is what lets a retried read hit a fresh incarnation. The getter reads the constructor param userId and this.users (both assigned before any method runs) and is never invoked during construction, so it's not a field-init regression. .id.name/.id.toString() are synchronous local reads on a native DurableObjectStub, not Cap'n Web stubs, so the fresh-stub-per-access getter has no disposal-leak consequence — only the micro-churn the prior review noted, correctly left alone.
• Retries confined to idempotent reads/subscribes. withDoResetRetry refuses flagless local transport errors (rpcErrors.ts:101), so a retry never fires through a closure-captured dead stub; the connection manager keeps that recovery. Writes keep inline-hint-not-auto-retry, and the double-send reasoning (a reset after commit) is right.
• Auth drift closed. The non-Base64 token path (user.ts:302) now classifies as auth instead of leaking the decoder's SyntaxError. The shared additions are all doc-commented per the kernel bar.

New non-blocking observation

The chat-send path (ChatInterface.tsx:5384) fires reportDoResetError("chat.send", …) on every genuine reset, before logRpcFailure decides whether to quiet. So a reset there is simultaneously (a) sent to the external Reporter as a warning, (b) logged at console.debug, and (c) surfaced as the inline sendHiccup hint via the rethrow. That's intentional and fine — but note it makes chat-send the one transient surface that still reports to the external Reporter, whereas the read sites (routes/index, SidebarWorkspaces, __root) deliberately do not report their DO resets (they retry and only logRpcFailure). The asymmetry is defensible (a failed send is user-visible and non-retried, so worth a warning-level signal) — just worth a one-line note so a future reader doesn't "fix" the inconsistency by either dropping the send report or adding reports to the reads. Not worth changing.

Verdict

Solid, well-scoped, mergeable. The kernel change is minimal and correct, retries are correctly confined, and the commit split keeps workshop-backend/workshop-shared reviewable apart from the UI. LGTM.

[ask-bonk]

Review posted to PR #58.

Summary

I independently reviewed PR #58 (RPC error classification + quiet DO-reset recovery) from a fresh checkout, verified its assumptions myself, and posted a review.

What I verified:

• Installed deps and ran vitest src/rpcErrors.test.ts → 20/20 pass (both canaries included).
• Built workshop-backend + workshop-shared + workshop-frontend → all green.
• lint:check → exit 0, no new warnings from touched files.
• Confirmed the get user() getter is safe: reads constructor-assigned userId/this.users, never invoked during construction, and .id reads are synchronous native-DO-stub accesses (no Cap'n Web disposal leak).
• Confirmed retries are confined to idempotent reads/subscribes and refuse flagless local transport errors, and that both raw auth throws migrated to createAuthError.

Verdict: Solid, well-scoped, mergeable. The kernel change is minimal and correct.

One new non-blocking observation the prior reviews missed: chat-send is the one transient surface that still fires reportDoResetError to the external Reporter on every reset, whereas the read sites deliberately don't report their (retried) resets. The asymmetry is defensible but worth a one-line note so a future reader doesn't "fix" it in either direction. Not worth changing.

LGTM.

github run

[ndisidore]

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)

[ndisidore]

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.

[ask-bonk]

Review — Classify RPC errors and recover quietly from DO resets

I reviewed PR #58 independently against the base (origin/chore/handle-do-resets, the #56 stack) rather than relying on the prior ask-bonk passes — reading the classifier, the backend get user()/auth-code changes, the shared AUTH_ERROR_* surface, and every touched frontend call site, then re-running the checks myself.

Verification I ran (fresh pnpm install)

• vitest run src/rpcErrors.test.ts → 20/20 pass, including both canaries: the transport-string pin (rpcErrors.test.ts:194) and the serialize/deserialize custom-property round-trip (:208).
• types:check on workshop-backend + workshop-shared + workshop-frontend → all green (covers the AuthenticatedApiImpl getter refactor, the shared additions, and every call site).

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. The round-trip canary closes the one real gap: a capnweb regression dropping custom Error props fails CI instead of silently demoting every classification to message-matching.
• Backend stub fix is the reachability fix, and safe. The per-call get user() (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. I checked field-init order: this.users is constructor-assigned (server.ts:83) and userId is a constructor param, and the getter is never invoked during construction, so this is not a field-init regression. All 40+ this.user reads are native DurableObjectStub accesses — .id.name/.id.toString() are synchronous local reads and native stubs need no disposal — so minting a fresh stub per access is harmless micro-churn, not a leak.
• abortAllDurableObjects is the right test harness. The new integration test (open-gadget-rpc.test.ts:145) reproduces the exact failure: a non-graceful reset on the same socket/same AuthenticatedApiImpl, which a cached stub could never recover from. The comment correctly rules out evictDurableObject (graceful, never breaks a stub).
• Retries confined to idempotent reads/subscribes. withDoResetRetry refuses flagless local transport errors (rpcErrors.ts:101), so a retry never fires through a closure-captured dead stub — the connection manager keeps that recovery. Exactly one jittered attempt, never a loop. The subscribe wrapper in ObserverConfigModal is leak-safe: the failing attempt returns no stub, the retry's stub is captured (or disposed on cancelled), and a reset destroys the incarnation that would hold the subscription.
• Writes correctly not auto-retried. Chat-send keeps the inline sendHiccup hint (ChatInterface.tsx:3062) with sound double-send reasoning (a reset landing after the write commits). The composer only sets hint state; the onSend handlers own the logging.
• Auth drift closed. Both raw throws migrated to createAuthError, and the non-Base64 token path (user.ts:300) now classifies as auth instead of leaking the decoder's SyntaxError. The shared additions are all doc-commented per the kernel bar.
• "Loud on purpose" carve-outs (Connections.tsx:73, ObserverConfigModal.tsx:119) correctly keep console.error + toast where there's no retry path, so a quieted transient can't strand the user on a permanent loader.

Non-blocking observations

1. ObserverConfigModal.tsx:131 wraps Promise.all([listGatekeeperVendors(), listAddableGatekeepers()]) in the retry, so a reset on the second call re-runs both. Both are idempotent metadata reads — harmless, just slightly more work than a per-call wrap. Not worth changing.
2. ChatInput send-hint key coalescing (ChatInterface.tsx:1845): chatKey={selectedChatId} stays null across two different not-yet-created "new chat" conversations, so a hiccup hint could persist between them. Cosmetic — the warning stays accurate and handleSend clears it on the next send.
3. Chat-send report asymmetry: reportDoResetError("chat.send", …) fires to the external Reporter on every genuine reset (ChatInterface.tsx:5384), whereas the retried read sites deliberately don't report. Defensible (a non-retried, user-visible failure is worth a warning-level signal), but worth a one-line note so a future reader doesn't "fix" the inconsistency in either direction.

Verdict

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

[ask-bonk]

Review posted to PR #58.

Summary

I independently reviewed PR #58 (RPC error classification + quiet DO-reset recovery) from a fresh pnpm install, verified its load-bearing assumptions myself, and posted a review.

What I verified:

• Read the full diff against the base (origin/chore/handle-do-resets, the #56 stack) — the classifier, the backend get user()/auth-code changes, the shared AUTH_ERROR_* surface, the new integration test, and all frontend call sites.
• vitest run src/rpcErrors.test.ts → 20/20 pass (both canaries).
• types:check on workshop-backend + workshop-shared + workshop-frontend → all green.
• Confirmed the get user() getter is safe: reads constructor-assigned this.users and param userId, never invoked during construction, and the 40+ this.user reads are native DO-stub accesses (synchronous .id, no disposal leak) — only micro-churn.
• Confirmed retries are confined to idempotent reads/subscribes and refuse flagless local transport errors; the subscribe wrapper is leak-safe; writes keep inline-hint-not-auto-retry with correct double-send reasoning.
• Confirmed abortAllDurableObjects is the correct harness for the reset failure (vs. graceful evictDurableObject).

Verdict: Solid, well-scoped, mergeable. The kernel change is minimal and correct — the per-call stub getter is what makes retries reachable. I raised three non-blocking observations (Promise.all retry re-runs both reads on partial failure; the null new-chat send-hint key coalescing; the chat-send external-Reporter asymmetry), none worth changing. LGTM.

github run