Fix code that aborts the WebSocket when an overseer DO dies.

[kentonv]

b2a215e had switched the overseer death handling from a simple call to ctx.abort() (an experimental API) to a more elaborate mechanism that closes the WebSocket.

However, the code closed the wrong end of the WebSocket, and so it didn't work.

This change introduces a cleaner approach that actually shuts down the RPC session by disposing the main stub. However, this requried cloning some functions from Cap'n Web. Arguably, this feature should be folded back into Cap'n Web. However, at the same time, ctx.abort() will become generally available within a few days, and once it does we might as well switch back to it.

This PR also includes a couple of UI tweaks that make reconnecting more pleasant.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[kentonv]

I have read the CLA Document and I hereby sign the CLA

[ask-bonk]

Review

I verified the core claim and the fix holds up. Overall this is a clean improvement over b2a215e.

Backend: the bug and the fix are correct

The old code did resp?.webSocket?.close(). In newWorkersRpcResponse, resp.webSocket is pair[1] — the client end that gets handed back in the 101 response. Closing that handle from the Worker side does not tear down the server-side RPC session, so abortSession() was effectively a no-op. Confirmed.

The new approach disposes the server-side main stub returned by newWebSocketRpcSession(server, ...). Disposing it tears down the RpcSession, which drives WebSocketTransport.abort() → server.close(3000, message) — the server end the Worker actually holds. That's the right end.

I diffed the clones of newWorkersRpcResponse / newWorkersWebSocketRpcResponse against capnweb v0.8.0 (src/index.ts, src/websocket.ts) and they are faithful copies apart from:

• threading options through to newHttpBatchRpcResponse / newWorkersWebSocketRpcResponse (upstream drops it), and
• the clearly-fenced -- ADDED FOR GADGETS -- abort-signal → stub[Symbol.dispose]() block.

The aborted this-tick race the old code guarded against is now handled inside the clone: it checks abortSignal.aborted synchronously before attaching the listener, so an abort that fires before/around session creation still disposes. Good.

Two minor nits (non-blocking):

• newWorkersWebSocketRpcResponse is exported but only referenced by the local newWorkersRpcResponse in this same module — nothing imports it. The upstream is exported so mirroring makes sense, but the export here is dead; could drop it.
• The abortSignal.addEventListener("abort", ...) listener is never removed. Fine in practice — the closure, stub, and controller are all scoped to the connection lifetime and GC together — worth a one-line note if you want to be explicit.

Frontend

Connections.tsx: re-keying the load effect from [overseer, chatId] to [gadget, chatId] is the right call and the comment explains the intermediate-render hazard well. loadGatekeepers does use overseer.listHooks(), but since gadget is derived from the new overseer, by the render where the fresh gadget arrives the fresh overseer already exists, so listHooks() won't hit a dead stub. Reasoning is sound.

ReconnectingChip extraction + moving from the full-width ConnectionLostBanner to an inline chip: the reflow-jolt justification is convincing, and the dedup across GadgetEditor and AppShell is a nice cleanup. __root.tsx still uses connectionLost for the loading-spinner copy, and the removed AuthenticatedShell prop was threaded out cleanly — no dangling refs or unused imports.

Verdict

LGTM. The direction (dispose the main stub) is the correct mechanism, the clones are faithful and well-fenced, and the TODO to revert to ctx.abort() once it's non-experimental is the right long-term plan.

[ask-bonk]

Review posted to PR #135.

Summary

I reviewed PR #135 and verified the core claims against the upstream Cap'n Web v0.8.0 source.

The bug and fix are correct. The old code's resp?.webSocket?.close() was closing pair[1] — the client end returned in the 101 response — which doesn't tear down the server-side RPC session, so abortSession() was a no-op. The new code disposes the server-side main stub, which drives WebSocketTransport.abort() → server.close(3000, ...) on the correct (server) end.

The clones are faithful. I diffed the cloned newWorkersRpcResponse/newWorkersWebSocketRpcResponse against capnweb v0.8.0 src/index.ts and src/websocket.ts — identical except for threading options through and the clearly-fenced -- ADDED FOR GADGETS -- abort block, which also handles the already-aborted case synchronously.

Frontend changes are sound. The Connections.tsx re-keying from overseer to gadget correctly avoids the stale-intermediate-render, the ReconnectingChip extraction is well-justified, and __root.tsx cleanup left no dangling refs.

Two minor non-blocking nits: the export on newWorkersWebSocketRpcResponse is dead (nothing imports it), and the abort listener is never removed (harmless — GC'd with the connection).

Verdict: LGTM.

github run

[ndisidore]

smart play patching in the abortSignal 👍

[ask-bonk]

Review

I re-verified the backend fix against upstream Cap'n Web v0.8.0 (src/index.ts, src/websocket.ts) and read the frontend changes end-to-end. This is a solid, well-reasoned change.

Backend — the bug and the fix are correct

The old resp?.webSocket?.close() closed pair[1], the client end returned to the runtime in the 101 response. The RPC session runs on server = pair[0], so closing the client handle from the Worker never tore down the server-side session — abortSession() was a no-op. Confirmed against upstream.

The new code disposes the stub from newWebSocketRpcSession(server, ...). Disposal tears down the RpcSession, driving WebSocketTransport.abort() → this.#webSocket.close(3000, message) on server — the server end the Worker actually holds. Right end, right mechanism.

The clones of newWorkersRpcResponse / newWorkersWebSocketRpcResponse are faithful to v0.8.0, differing only by:

• threading options through to newHttpBatchRpcResponse / the websocket helper (upstream drops it), and
• the fenced -- ADDED FOR GADGETS -- abort block.

The abort block correctly handles the already-aborted-before-attach case synchronously (abortSignal.aborted → dispose immediately), which subsumes the aborted this-tick race the old code guarded with a post-await re-check. Passing ExtendedRpcSessionOptions (extra abortSignal) where upstream expects RpcSessionOptions is structurally compatible, so the HTTP-batch path is unaffected.

Frontend

• Connections.tsx: re-keying the load effect from [overseer, chatId] to [gadget, chatId] is correct. gadget.stub is derived from overseer.stub.getGadget(...) in GadgetEditor.tsx:1127-1135 (keyed on [overseer, selectedGadgetId]), so a replacement overseer yields a new gadget stub one render later. Keying on the derived stub skips the intermediate render where overseer is fresh but gadget still points into the dead session — exactly the spurious-failure window the comment describes. Reconnects remain covered because a new overseer always produces a new gadget stub.
• ReconnectingChip extraction dedups the identical pill across GadgetEditor and AppShell; the inline-chip-vs-full-width-banner reflow rationale is convincing. role="status" is a nice touch.
• __root.tsx: ConnectionLostBanner removed cleanly. connectionLost is still legitimately used for the loading-spinner copy (__root.tsx:58) and the AuthenticatedShell prop was threaded out with no dangling refs. The AppShell chip uses ml-auto deliberately (documented) so it doesn't get parked left when the hamburger is hidden on desktop.

Notes (non-blocking)

• Re: an earlier review's nit that newWorkersWebSocketRpcResponse is a dead export — it is not exported in the current head (nor in f8d12c9); it's declared function newWorkersWebSocketRpcResponse. No action needed.
• ExtendedRpcSessionOptions.abortSignal is declared required, yet newWorkersWebSocketRpcResponse guards options?.abortSignal. Harmless (optionality lives on options, and the callsite always supplies it); the guard is just belt-and-suspenders. Could relax to abortSignal?: AbortSignal for accuracy, but not worth churn.
• The addEventListener("abort", ...) listener is never removed — fine, since the signal, stub, and connection are all connection-scoped and GC together.

Verdict

LGTM. Correct mechanism, faithful and clearly-fenced clones, and the TODO to revert to ctx.abort() once it's non-experimental is the right long-term plan.

[ask-bonk]

Review posted to PR #135.

Summary

I independently verified the PR against upstream Cap'n Web v0.8.0 and read all six changed files.

Backend fix is correct. The old resp?.webSocket?.close() closed pair[1] (the client end handed to the runtime in the 101 response), which never tore down the server-side RPC session running on server = pair[0] — so abortSession() was a no-op. The new code disposes the stub from newWebSocketRpcSession(server, ...), which drives WebSocketTransport.abort() → server.close(3000, ...) on the correct (server) end.

Clones are faithful. newWorkersRpcResponse / newWorkersWebSocketRpcResponse match v0.8.0 apart from threading options through and the fenced -- ADDED FOR GADGETS -- abort block, which correctly handles the already-aborted case synchronously.

Frontend is sound. Confirmed gadget.stub is derived from overseer in GadgetEditor.tsx:1127-1135, validating the Connections.tsx re-keying rationale. ReconnectingChip extraction and __root.tsx cleanup are clean, with connectionLost still legitimately used at the loading spinner.

Correction of a prior review nit: newWorkersWebSocketRpcResponse is not exported in the current head (nor in f8d12c9) — that earlier nit was incorrect. My only new observation is a harmless type-declaration inconsistency (abortSignal declared required but guarded with ?.).

Verdict: LGTM.

github run