Skip to content

Reconnect SSE stream after fatal EventSource errors - #921

Merged
selfcontained merged 4 commits into
mainfrom
agt_8dac8d0b7908/build-sse-eventsource-reconnect
Aug 10, 2026
Merged

Reconnect SSE stream after fatal EventSource errors#921
selfcontained merged 4 commits into
mainfrom
agt_8dac8d0b7908/build-sse-eventsource-reconnect

Conversation

@selfcontained

@selfcontainedselfcontained commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Problem

EventSource only auto-reconnects after transient failures. A fatal one — a non-200 response or a wrong content-type, e.g. loading the page while the server is mid-restart — puts it in CLOSED permanently, per spec.

use-sse.ts didn't handle that case:

  • onerror only called recordSSEReconnect().
  • The dead instance stayed in eventSourceRef, so openSSE() early-returned forever.
  • Only a tab hide/show or an authState change cleared the ref — a visible desktop tab never self-healed.

Impact: silent loss of all realtime updates (agent status, terminal-state banner, media/review refreshes, injection-hold badge) while the app looks perfectly healthy.

Fix

In onerror, branch on readyState:

  • CONNECTING — the browser is retrying on its own, leave it alone.
  • CLOSED — the instance is dead: drop it from the ref and reopen on a capped backoff (1s, doubling to 30s).

The backoff resets when the tab is foregrounded, and when the stream delivers an event on a connection that has already lasted 10s (STABLE_CONNECTION_MS).

That stability gate matters more than it looks. The server writes the snapshot unconditionally the moment it accepts a connection (events-routes.ts:42-48), so a delivered event only proves the connect succeeded — not that the stream is healthy. Resetting on it made a no-backoff loop reachable by the very scenario this PR targets: the browser's own transient retry lands on a half-started server that accepts and sends a snapshot, the stream dies, and the next retry hits the 502 window → CLOSED, but the delay was already back at the 1s floor. A server restarting in a loop would pin every open tab near the floor, so the cap never engaged in the case it exists for — and each cycle carried a full agent-list re-render plus the invalidation burst.

The reset stays keyed on a delivered event rather than a bare timer, so a hung proxy that holds the socket open without sending anything still can't clear the backoff. Tradeoff: a connection that stays healthy but silent for its whole lifetime fails with an elevated delay (capped at 30s, and cleared on foreground). Documented in a comment.

No heartbeat watchdog. The idea suggested one as optional; it isn't implementable client-side here. The server's keepalives are SSE comments (: keepalive), which EventSource never surfaces to onmessage — a liveness timer couldn't tell a hung proxy from a genuinely idle stream. That would need a real data-event heartbeat from the server; out of scope.

No "realtime is down" indicator. Deliberate for this PR — silent self-healing beats "stale until you reload", and a one-second blip during a routine restart shouldn't paint anything. Filed as a follow-up, with the sharper argument that the settings health dots currently read ok in exactly this scenario (the API is up; only the stream is dead).

Validation

Unit — 8 tests in use-sse.test.ts drive a fake EventSource with fake timers: fatal error reopens, transient error doesn't, backoff doubles, a stable connection's delivery resets it, a flapping server's connect-time snapshot does not, hidden-tab with a pending retry doesn't reconnect behind the user's back and recovers exactly once on foreground, and unmount stops retrying. Verified non-vacuous: 3 fail against the pre-fix hook, and the flapping test fails if the stability gate is set to 0.

Live, in a browser — against an isolated dev stack:

  • Fatal path (/api/v1/events → 500 + text/html): retry deltas 1.1s / 2.2s / 4.1s / 8.1s / 16.0s, the capped doubling, with no tab switch.
  • Flapping path (alternating 200 + snapshot and 502): our backoff escalated 1.1 → 2.0 → 4.0 → 8.0 → 16.2 → 30.1 → 30.1s to the cap, cycle time growing 4.2s → 33.3s. Pre-fix this stayed pinned near the floor.
  • End-to-end: renamed an agent server-side while the stream was dead → UI did not update (the bug's symptom, reproduced); un-blocked the endpoint → UI picked up the new name 6.6s later, no reload, no tab switch.

Test harness fix — this vitest config sets neither globals: true nor setupFiles, so React Testing Library's auto-cleanup never registered and every rendered hook stayed mounted for the rest of the file. Without the explicit cleanup(), the new hidden-tab test sees 7 connections instead of 2 — six leaked hooks answering the same visibilitychange.

Checkspnpm run check 0 errors · finalize:web builds · web unit 712 passed (55 files) · E2E 178 passed / 12 skipped (terminal-live, needs tmux) · CI green.

Structure

The connection and its backoff are one state machine, so they share one lifetime: source, reconnectDelayMs, reconnectTimer, and connectStartedAt are all effect-scoped locals torn down together. Nothing here is read during render, so none of it needs a ref — the EventSource was demoted from one.

The backoff reset lives with the rest of the backoff mutation (noteStreamActivity, next to cancelReconnect/scheduleReconnect) and is wrapped at the onmessage assignment site, so handleSSEMessage stays pure event dispatch and depends only on queryClient/jotaiStore. That keeps the dispatch half free to move out later.

connectionAliveSince is stamped on open — which fires on every establishment, including the browser's own internal retries after a transient drop. Those never re-run openSSE, so stamping only there would measure the age of the EventSource instance rather than of the connection, and a connect-time snapshot delivered by an internal retry would clear the backoff. The openSSE stamp is kept as the pre-open fallback: generous by the establishment latency, but never 0 — a zero would make the staleness check trivially true.

Review

Four persona reviews (frontend-ux, backend-security, and two architecture passes on different models); 13 items, all resolved.

  • frontend-ux and backend-security independently identified the flapping-reset hole and converged on the same gate. Backend-security separately verified there's no post-logout reconnect window and no rate limiter or lockout on /api/v1/events, so 401 retries are benign, and judged jitter not worth adding at this scale. Frontend-ux found the RTL cleanup leak and the missing hidden-tab test.
  • architecture confirmed no restructure belongs in this PR and took the remaining items as placement/naming: the reset's location, the split lifetimes, the connectStartedAt name, and the constant's rationale. The second architecture pass found the connection-age gap above, plus a defensive asymmetry in onerror where the identity check guarded the null-out but not scheduleReconnect(). Two things recorded rather than built — splitting event dispatch out of useSSE (brain idea split-use-sse-event-dispatch), and an explicit don't on a shared backoff primitive, since the three reconnect implementations here are different policies rather than instances of one (use-terminal is linear/ref-keyed/UI-driving on a WebSocket; use-release-stream doesn't reconnect at all).

Note on PR #918

This is a fresh re-land of #918, which was opened and closed without merging on 2026-08-09. That PR had no review comments or objections — Brad confirmed it was simply stopped, not rejected. Re-validated from scratch against current main.

🤖 Generated with Claude Code

selfcontainedand others added 4 commits August 9, 2026 16:38
EventSource only auto-reconnects after transient failures. A fatal one —
a non-200 response or wrong content-type, e.g. loading the page while the
server is mid-restart — puts it in CLOSED permanently, per spec.
use-sse.ts didn't handle that: onerror only recorded a metric, and the
dead instance stayed in eventSourceRef, so openSSE() early-returned
forever. Only a tab hide/show or an authState change cleared the ref, so
a visible desktop tab silently lost every realtime update — agent status,
terminal-state banner, media/review refreshes, injection-hold badge —
while the app looked healthy.
onerror now branches on readyState: CONNECTING means the browser is
retrying on its own and is left alone; CLOSED means the instance is dead,
so it's dropped from the ref and reopened on a capped backoff (1s doubling
to 30s). The backoff resets when the stream delivers an event (the server
sends a snapshot on every connect) and when the tab is foregrounded.
No heartbeat watchdog: the server's keepalives are SSE comments, which
EventSource never surfaces to onmessage, so a client-side liveness timer
couldn't tell a hung proxy from a genuinely idle stream.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both reviewers independently found the same hole: the reset was keyed on
"any delivered event", but the server writes the snapshot unconditionally
the moment it accepts the connection (events-routes.ts:42-48), so a
delivered event only proves the *connect* succeeded — not that the stream
is healthy.
That made a no-backoff loop reachable by the exact scenario this PR
targets: the browser's own transient retry lands on a half-started server
that accepts and sends a snapshot, the stream dies, the next retry hits
the 502 window and goes CLOSED — but the snapshot already reset us to the
1s floor. A server restarting in a loop kept every open tab cycling near
the floor, so the cap never engaged in the case it exists for, and each
cycle carried a full agent-list re-render plus the invalidation burst.
The reset now requires the connection to have lasted STABLE_CONNECTION_MS
(10s) first. It stays keyed on a delivered event rather than a bare timer,
so a hung proxy holding the socket open still can't clear the backoff.
Also fixes the test harness: this vitest config sets neither globals nor
setupFiles, so RTL's auto-cleanup never registered and every rendered hook
stayed mounted for the rest of the file. The new hidden-tab test sees 7
connections instead of 2 without the explicit cleanup() — six leaked hooks
answering the same visibilitychange.
Tests: flapping server keeps backing off; tab hidden with a retry pending
does not reconnect behind the user's back and recovers exactly once on
foreground.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Architecture review follow-ups. No behavior change.
The backoff reset lived at the top of handleSSEMessage, which is otherwise
pure event dispatch — it depends only on queryClient/jotaiStore and is the
obvious thing to lift out of this hook later. Putting lifecycle mutation
there meant the dispatch concern closed over backoff state and couldn't
move without dragging it along. It's now noteStreamActivity(), wrapped at
the onmessage assignment site alongside the rest of the backoff mutation.
The EventSource lived in a component-lifetime useRef while the three
variables describing the same connection were effect-scoped locals — one
state machine stored two ways, where establishing that the ref can't
outlive the locals took non-local reasoning. The ref is never read during
render and never wanted across effect re-runs, so it's now a local like
the rest, and the mutual exclusion of "live source" and "pending timer"
is stated instead of implied.
connectedAt -> connectStartedAt: it's stamped at construction, so it marks
when the connect was initiated, not when it was established. The old name
made `Date.now() - connectedAt >= STABLE_CONNECTION_MS` read as exact when
it's generous by the establishment time.
STABLE_CONNECTION_MS's rationale moves to its declaration and cites the
server code it depends on, so whoever tunes or deletes the constant reads
the why at the same place as the what.
Tests adopt the sibling stream test's vi.stubGlobal/unstubAllGlobals, and
document.hidden is now a restored spy rather than a permanent redefine.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second architecture review caught a semantics gap in the stability gate.
`connectStartedAt` was stamped only in `openSSE`, but the browser
reconnects transiently under the *same* instance without re-running it.
So a connect-time snapshot delivered by an internal retry passed the
10s check whenever the instance happened to be old — clearing the
backoff on exactly the event the gate exists to discount.
Reachable trace: elevated backoff -> our reconnect creates an instance ->
it survives 10s (possibly silent) -> mixed flapping starts (transient
drop, internal retry, snapshot, fatal 502) -> the internal-retry snapshot
resets the delay and the fatal that follows starts at 1s instead of the
remembered one. Bounded (each cycle costs 10s+ of instance life) but the
code no longer meant what STABLE_CONNECTION_MS said.
`open` fires on every establishment, including internal retries, so
stamping there makes the gate measure true connection age. The stamp in
`openSSE` stays as the pre-open fallback: generous by the establishment
latency, but never 0 — a zero would make the check trivially true.
Renamed `connectionAliveSince` to match what it now holds.
Also closes a defensive asymmetry in `onerror`: the `source === opened`
identity check guarded the null-out but not `scheduleReconnect()`, so the
(unreachable) stale path would have scheduled a retry beside a live
connection and doubled the delay for nothing. The whole branch is behind
the check now, with the unreachability stated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@selfcontained
selfcontained merged commit d561a3b into mainAug 10, 2026
1 check passed
@selfcontained
selfcontained deleted the agt_8dac8d0b7908/build-sse-eventsource-reconnect branch August 10, 2026 03:23
selfcontained added a commit that referenced this pull request Aug 15, 2026
The reconnect state machine added in #921 was already well covered, but
`handleSSEMessage` — the table that routes ~20 server push types into
react-query cache writes — was not. Only `snapshot` was exercised, and
only as a delivery vehicle for the backoff tests, with nothing asserted
about what it does to the cache. Every realtime update in the app flows
through this function.
Adds 23 tests driving the real hook through the fake EventSource, so the
routing itself is what gets pinned: which payload reaches which key, the
`exact` scoping that keeps one agent's event from refetching every other
agent's list, the review-detail invalidation predicate, the ack that must
only fire when a notification was actually shown, and the tolerance of an
unparseable or unrecognized frame. Also covers the connection gate — the
stream must not open before authentication, including on the foreground
path, which reopens without re-running the effect.
Mutation battery: 50 mutants, 47 killed. The three survivors are provably
unobservable rather than uncovered — `patchAgentHasStream`'s no-op guard
and `media.seen`'s `!file.seen` guard are each redundant with react-query
structural sharing, and `applyReviewCreated`'s null guard only ever skips
an element-identical rewrite. An earlier draft asserted referential
stability to chase the first two; those assertions were dropped because no
change to use-sse.ts can fail them, which made them a test of react-query
rather than of this hook.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@selfcontained