Reopen the workspace after a Durable Object reset - #124
Conversation
Rewrites the connection layer as a testable state machine (connectionManager.ts) wired to a singleton in connection.ts. A reconnect attempt only replaces the current stub after a probe RPC round-trips. Previously every backoff cycle published an unproven stub, so all ~30 authenticatedApi-keyed effects re-fired against a connecting socket and failed en masse, once per cycle. Subscribers now hear exactly twice per outage: lost, restored. This also retires markConnectionRestored(), which cleared the banner optimistically off a pipelined stub.
Passive close detection misses sockets killed during sleep or background throttling, so a backgrounded tab woke to a zombie connection that only failed once the user acted. On visibilitychange/ online, probe the current connection if it hasn't proven itself recently; on failure, reconnect immediately (no backoff — the network likely just returned). A wake during an ongoing reconnect skips the remainder of the backoff sleep.
The connection manager now records outage bookkeeping — trigger, attempt count, duration, first failure reason, and any DO-reset flags and durableObjectIds seen across attempts — and delivers a single summary at recovery. connection.ts forwards it to the client-errors endpoint via reportIssue, so prod finally answers which DOs, how often, and how long recovery takes. One POST per outage per client; a no-op unless frontend error reporting is configured.
A workspace-DO reset rejects in-flight RPCs while the WebSocket stays healthy, so nothing re-ran the open effect: the workspace sat broken until a full reconnect (today forced server-side by the notifyClosed abort). useWorkspaceOpen now exposes notifyWorkspaceRpcError, which classifies reset errors, reports them, and coalesces bursts into one debounced reloadNonce bump — reopening just this workspace over the healthy connection. ChatInterface routes its subscribe/send failures through it, and open attempts that fail transiently show the existing "Reconnecting…" state instead of a terminal error page. This is the client-side alternative to MR 238: per the review discussion there, a failed open() fails its pipelined capability with it, so recovery belongs in the caller — not in tearing down the whole session.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Reopen the workspace after a Durable Object reset
Read every changed line. This is a well-considered PR: the central idea — folding recovery into logRpcFailure so "quieting an error" and "recovering from it" are literally the same code path — is elegant and eliminates the per-call-site wiring that a parallel notify-API would have required. The connection manager extraction is a clear improvement over the main.tsx globals, and the prove-before-publish contract is the right invariant. The comments carry real reasoning (the workerd onRpcBroken probe finding, the eager-reset-defeats-backoff explanation, the flapping-DO settlement guard) rather than restating the code. Tests are strong: coalescing, backoff growth, the 30s health reset, the flag-only retryable path, the never-settles guard, the two-publishes-per-outage invariant, and the gadget-pane auto-retry are all covered.
A few observations, all minor — nothing blocking.
connectionManager.ts — lastProvenAt can be set on a stale stub after a wake-probe race
In onWakeSignal, if the probe succeeds (line 181) but a real break swapped snapshot.stub while it was in flight, lastProvenAt = now() (182) records a proof for a stub that is no longer current. The catch branch already guards this exact race (snapshot.stub !== suspect, line 185), but the success branch does not. Consequence is benign — lastProvenAt only gates the next wake probe's idle threshold, so at worst one future wake probe is skipped — but the asymmetry with the catch branch is a small latent trap. Consider guarding the success write the same way:
await withTimeout(suspect.getServerConfig(), WAKE_PROBE_TIMEOUT_MS)
if (snapshot.stub === suspect) lastProvenAt = now()useWorkspaceOpen.ts — the stalled overseer stub stays published to the UI until the reopen lands
When raceOpenSettlement returns null (stall), the effect has already published the stalled stub via setOverseer({ stub: overseerStub }) at line 217, and it returns without disposing it (disposal waits for the reopen effect's cleanup). So for the duration of the coalescing/backoff delay, downstream subscribers (ChatInterface, GadgetEditor's workpieces/console-logs subscriptions) keep issuing calls against a wedged stub. Those calls fail transient and are quieted, and the "Reconnecting…" pill is showing, so the UX is correct — but it's worth confirming this is intended: the pill is the only signal, and any in-flight user action against the stalled stub is silently absorbed until the swap. If that's the accepted behavior it reads fine; a one-line comment at 227 noting "the stalled stub stays published; its calls fail transient and are quieted until the reopen swaps it" would save the next reader the trace.
rpcErrors.ts — reportSite-less do-reset recovery is silent by design; confirm that's the intent for useActions
useActions.ts and GadgetCodeInterface.tsx call logRpcFailure without a reportSite, so a do-reset there schedules the reopen but emits no telemetry. The comments say this is deliberate (the always-mounted subscriptions in GadgetEditor carry the reporting), which is reasonable. Just flagging that a reset which only ever manifests through one of these unreported sites would recover invisibly — acceptable given the aggregated connection.outage report and the tagged sites, but worth a conscious sign-off.
Nits
connectionManager.ts:96—reason: String(err).slice(0, 300)will render a bareErroras"Error: <message>"(fine) but a plain rejected object as"[object Object]". Given the backend runsenhanced_error_serialization, real cases are Errors, so this is only a theoretical loss of the reason string. Not worth changing unless you've seen non-Error rejections in the wild.- The idle-reset gap is documented thoroughly in the description and code, and I agree a heartbeat is the right follow-up rather than part of this PR. Leaving it out keeps the moving-parts count honest.
Overall this is solid, carefully-reasoned work with the recovery invariants pinned down by tests. The observations above are refinements, not corrections. I could not run pnpm test in this environment (dependencies not installed), so I'm relying on the described 149-green run for the suite result.
|
Review posted to PR #124. Summary of what I found: Verdict: Solid, well-reasoned PR. The core idea — making Observations raised (all minor, none blocking):
I noted I couldn't run |
1 participant
Sibling of #104 (the user-DO half) building on #110. This PR is the workspace half, plus a rework of the socket-reconnect path it leans on.
The problem. When a workspace's Overseer DO resets (storage timeouts, overload, ... clusters #56 measured), the long-lived
open()call dies along with every capability pipelined on it, but the WebSocket underneath stays perfectly healthy. Our only break signal,onRpcBroken, is session-level (no way to signal "one capability died") so the client hears nothing. The workspace looks fine, but every chat send fails with "Failed to start conversation" until a full page reload. Reproduced on a pre-PR baseline. (The server already tries to convert DO-loss into a socket close (thenotifyClosedhack inserver.ts) but it demonstrably doesn't fire for these resets, and its own TODO asks for exactly this client-side fix.)Solve
flowchart TD X([something breaks]) --> D{what died?} D -- "the WebSocket —<br/>onRpcBroken fires" --> C["connection manager:<br/>jittered backoff, probe round-trip,<br/>proven before publish"] C --> N["new session: app rebuilds everything,<br/>one aggregated lost/restored report"] D -- "the workspace DO —<br/>socket healthy, no signal at all" --> Q{was a call in flight?} Q -- "yes: it rejects with a<br/>reset-classified error" --> L["logRpcFailure — quieting the error<br/>and scheduling recovery are<br/>the same code path"] Q -. "no (idle): silent until the next<br/>failing call — the accepted gap" .-> L L --> H["coalesced, jittered reopen:<br/>500ms → 5s → … 60s cap"] H --> O["fresh openGadget()<br/>over the same socket"] O -- "new overseer stub" --> Z["chat / workpieces / console logs /<br/>actions / gadget pane all resubscribe"] O -. "open hangs >15s: settlement guard →<br/>'Reconnecting…' pill, retry" .-> HWorkspace reopen.
logRpcFailurenow doubles as the recovery entry point: any error it classifies as a DO reset schedules a reopen of the open workspace over the still-healthy socket.ke automatically when the stub is replaced, so it heals with the workspace instead of sticking on an error banner.Connection manager. The socket-reconnect logic moved out of
main.tsxglobals into a testedconnectionManager.ts. Reconnects are proven via a real round-trip (getServerConfig, which touches no DO) before the new stub is published, so the app never sees a half-alive connection. A tab-wake probe catches sockets that died while the tab slept, and each outage logs one aggregated lost/restored summary instead of a line per retry.Accepted gap (deliberate)
No noise when a worspace is idle - Because there's no ping/health, recovery waits until the next call. Because recovery rides
logRpcFailure, any failing call in the workspace is now enough (send, approve, hook toggle, subscribe…), but the wait is real: in fault injection, a workspace stayed dormant for minutes after a sustained abort loop until the next user action, which then healed it. A liveness heartbeat would close this; we left it out on purpose to keep the moving parts down.