Harden the workbench SSE stream: unhandled rejections, dead streams, ordering - #496
Conversation
Covers the six workbench-events defects: unhandled rejections from a rejecting authorize(), a dead stream after a failed write, the bare catch swallowing the platform-subscribe failure, out-of-order deliveries under a slow authorize(), the presence-snapshot/delta race, and the periodic keepalive.
…own, add keepalive
Six defects lived in bridgeWorkbenchStream's delivery path: authorize()
sat outside the try block and deliveries were floating `void deliver()`
calls, so a transient resolver error became an unhandled rejection per
event; the write-failure teardown never closed the stream, so the
client's EventSource never saw an error and never reconnected, and the
route itself parked on a promise that never resolved; a bare `catch {}`
around the platform subscribe swallowed failures with no reportError;
deliveries weren't sequenced, so two events could land out of
publication order depending on when their authorize() calls resolved;
and the presence snapshot fired as a floating promise after both
subscriptions were installed, letting a racing event overwrite it on
the client.
Every write (snapshot, event, keepalive) now funnels through a single
chained-promise queue so writes land in enqueue order regardless of
authorize() timing, bounded by MAX_QUEUED_DELIVERIES to close a stream
that has fallen too far behind rather than buffer it unboundedly. The
presence snapshot is enqueued before either subscription installs, so
nothing can write ahead of it. authorize() and every write path report
through reportError and close the stream on failure. bridgeWorkbenchStream
now returns { teardown, closed }; the route awaits closed instead of a
promise that never resolves. A periodic keepalive keeps idle
connections alive behind proxies.
…eSSE Hono's StreamingApi.write swallows writer errors internally and never rejects, so writeSSE can never throw in production; the write-failure branch in deliverEvent only fires for a stream implementation that does throw (tests today, conceivably a future Hono release). The disconnect signal that actually fires for a real client is stream.onAbort, wired by the route. Correct the comments and the test name that implied write-failure detection was the live mechanism catching a disconnected client.
|
Caveat worth flagging for review: Hono's StreamingApi.write() (which writeSSE calls) wraps the writer.write() call in a bare try/catch and never rethrows (verified against the pinned hono@4.13.3 source). So writeSSE can never reject in production, and the write-failure branch in deliverEvent (unsubscribe + close + report) is dead code against the real dependency today — it only fires for a stream implementation that does throw (the unit tests). The actual disconnect signal that fires for a real client remains stream.onAbort, wired unchanged in routes.ts. This doesn't regress anything (the pre-existing code had the same limitation) and the other five defects (unhandled rejections from authorize(), the route parking forever, the bare catch{} around platform subscribe, unserialized delivery ordering, and the presence-snapshot race) are all fixed independent of this. Comments and one test name were corrected to describe this accurately rather than implying write-failure detection is what catches a disconnected client in practice. |
1 participant
Summary
Six defects in
bridgeWorkbenchStream's delivery path (packages/chat/src/workbench-events.ts, wired frompackages/chat/src/routes.ts):authorize()sat outside thetryblock and both subscriptions invoked delivery as a floatingvoid deliver(event)— a transient resolver error became an unhandled rejection per delivered event.stream.close(), so the client'sEventSourcenever saw an error and never reconnected; the route itself parked on a promise that never resolved.catch {}around the platform subscribe swallowed failures with noreportError.authorize()calls resolved.Changes
authorize()timing, bounded byMAX_QUEUED_DELIVERIESto close a stream that's fallen too far behind rather than buffer it unboundedly.authorize()and every write path report throughreportError(withoperation/roomIdcontext) and close the stream on failure.bridgeWorkbenchStreamnow returns{ teardown, closed }; the route awaitsclosedinstead of a promise that never resolves, and wiresstream.onAbort(teardown).Scope note
packages/presence/src/routes.tshas an identical-shaped leak; that's CL-7212 and is untouched here.Test plan
cd packages/chat && bun run typecheck— cleanbun test— 727 pass, 0 failbunx prettier --checkon touched files — cleanhttps://linear.app/abklabs/issue/CL-7197/harden-the-workbench-sse-stream-unhandled-rejections-dead-streams