Skip to content

fix(sessions): stream session events over a WebSocket to dodge HTTP connection exhaustion - #4342

Open
TomeHirata wants to merge 3 commits into
mainfrom
fix/stream-exhaustion-ws
Open

fix(sessions): stream session events over a WebSocket to dodge HTTP connection exhaustion#4342
TomeHirata wants to merge 3 commits into
mainfrom
fix/stream-exhaustion-ws

Conversation

@TomeHirata

Copy link
Copy Markdown
Contributor

Related issue

N/A

Summary

Follow-up to #3699, which added a banner warning that open tabs are exhausting the browser's connection pool. That explains the stall; this PR removes it.

Each open conversation holds one long-lived GET /v1/sessions/{id}/stream SSE request. Held SSE GETs count against the browser's ~6 HTTP/1.1 connections-per-origin cap, shared across all tabs, so with 5–6 conversations open every slot is taken and unrelated requests queue behind them.

  • Add a dedicated per-conversation event WebSocket (WS /v1/sessions/{id}/stream/ws) carrying the same ServerStreamEvent payloads. WebSockets use a separate, effectively unbounded pool, so the HTTP cap no longer applies.
  • The web client defaults to the WebSocket; the SSE route stays as a fallbackVITE_EVENT_STREAM_TRANSPORT=sse at build time, or the omnigent.eventStream.transport localStorage override per tab. No hard cutover.
  • Server: factor the SSE generator into a transport-agnostic _iter_session_events core (presence + session_stream.subscribe + validation). The SSE wire output stays byte-identical; the WS route reuses the same snapshot-on-connect (_build_resource_snapshot) and presence semantics.
  • Client: split the pump into pumpParsedEvents (transport-agnostic reduce/tap/flush) plus pumpStreamEvents (unchanged SSE wrapper); add the sessionEventSocket transport; startStreamPump selects via eventStreamUsesWebSocket().

ELI5

The browser only lets ~6 long-lived HTTP connections talk to one site at once, and every tab shares that budget. Each open chat held one forever just to receive live updates — six chats, no slots left, whole app freezes. WebSockets don't count against that budget, so live updates now ride one.

Before:  6 tabs x 1 SSE stream  ->  6/6 HTTP slots used  ->  navigation/API queue (hang)
After:   6 tabs x 1 WS stream   ->  0/6 HTTP slots used  ->  HTTP pool free

Test Plan

  • Server: python -m pytest tests/server/routes/test_session_stream_ws.py tests/server/test_stream_events.py -q27 passed. test_session_stream_ws.py drives the real WS route: subscription-ack heartbeat, live event fan-out, resource snapshot-on-connect, and unauthenticated + unauthorized handshake rejection.
  • Web: cd web && pnpm vitest run src/store/chatStore.test.ts src/lib/sessionEventSocket.test.ts313 passed. sessionEventSocket.test.ts covers parsed-event yield, clean-close vs drop, and abort; pumpParsedEvents tests lock the WS clean-close/drop mapping.
  • E2E: tests/e2e_ui/chat/test_event_stream_websocket.py drives a full turn over the event WebSocket and asserts the SSE endpoint is never opened — verified as a real guard by inverting it to SSE, where it fails.
  • The rest of the e2e suite pins VITE_EVENT_STREAM_TRANSPORT=sse, because much of it drives the SSE transport directly (intercepting GET /stream to inject 404s, monkeypatching window.fetch for a controllable ReadableStream) and Playwright's extra_http_headers do not apply to a WebSocket handshake, so header-auth'd multi-viewer presence tests cannot identify themselves over WS.
  • Rebased onto latest main and re-verified all of the above green.

Demo

N/A — no visible UI change; this is a transport swap. Verify via DevTools → Network: the event stream connects as …/stream/ws rather than an EventStream.

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

Manual verification: confirmed via DevTools → Network that the event stream connects as a WebSocket and that many open tabs no longer stall unrelated HTTP requests; confirmed the SSE fallback still streams under VITE_EVENT_STREAM_TRANSPORT=sse.

Two things worth a reviewer's eye:

  1. Permanent-failure detection is coarser over WS. A WS handshake collapses 401/403/404 into an opaque abnormal close, so a forbidden/deleted session is detected via repeated empty opens (MAX_WS_EMPTY_OPENS = 8) rather than an immediate status code. It terminates correctly, just slower to give up than the SSE path. Happy to add a distinct server close code mapped to immediate client failure if preferred.
  2. The e2e SSE pin is load-bearing. Four workflows build the SPA for the Playwright suite and all four pin SSE; dropping the pin from any one of them will fail that shard on the presence and transient-404 tests.

Changelog

Session live updates now stream over a WebSocket, so opening many conversation tabs no longer stalls the app.

…onnection exhaustion

Each open tab held its session event stream on a fetch-based SSE GET
(`/v1/sessions/{id}/stream`), which counts against the browser's ~6
HTTP/1.1 connections-per-origin cap shared across all tabs. With 5-6
tabs open, the SSE streams filled every slot and unrelated navigation /
API requests queued behind them, so the UI appeared hung.

Add a dedicated per-conversation event WebSocket
(`WS /v1/sessions/{id}/stream/ws`) that carries the same
`ServerStreamEvent` payloads, moving the stream onto the browser's
separate, effectively unbounded WebSocket pool. The web client defaults
to it; the SSE route stays as a fallback (`VITE_EVENT_STREAM_TRANSPORT=sse`).

Server: factor the SSE generator into a transport-agnostic
`_iter_session_events` core (presence + subscribe + validation) so the
SSE wire output is byte-identical and the WS route shares the same
snapshot-on-connect and presence semantics.

Client: split the pump into `pumpParsedEvents` (transport-agnostic) plus
`pumpStreamEvents` (SSE wrapper); add the `sessionEventSocket` transport;
`startStreamPump` selects transport via `useEventStreamWebSocket()`.

Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Rebase onto main brought the routes_events explicit-import refactor, so
`_iter_session_events` needed adding to that import list and to the
sessions package re-exports.

CI fixes:
- Rename `useEventStreamWebSocket` to `eventStreamUsesWebSocket`: the
  `use` prefix made oxlint's react-hooks rule treat it as a React Hook
  called from a non-component function.
- Annotate `_iter_session_events` as `AsyncGenerator` so it satisfies
  `contextlib.aclosing`'s `_SupportsAclose` bound.
- Build the e2e SPA with `VITE_EVENT_STREAM_TRANSPORT=sse`. Much of that
  suite drives the SSE transport directly (intercepting `GET /stream` to
  inject 404s, monkeypatching `window.fetch` for a controllable
  ReadableStream), and Playwright's `extra_http_headers` don't apply to a
  WebSocket handshake, so header-auth'd multi-viewer presence tests can't
  identify themselves over WS.

Add a localStorage override (`omnigent.eventStream.transport`) so a
transport can be flipped per-tab without a rebuild, and use it for new
coverage of the default transport: an e2e turn over the event WebSocket
that also asserts the SSE endpoint is never opened, plus two unit tests
for transport selection.

Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
The conftest's own build sets VITE_EVENT_STREAM_TRANSPORT=sse, but every
UI workflow builds the SPA in a dedicated step and then runs pytest with
--ui-skip-build, so that fixture never ran and the suite kept exercising
the WebSocket default — failing the presence tests, which rely on
Playwright's extra_http_headers (not applied to a WS handshake).

Set the env var on the build step in each workflow that runs the e2e_ui
suite: e2e-ui, flake-stress-ui, ui-snapshot, and ui-snapshot-update.

The WebSocket default keeps its own coverage: the localStorage override
survives an SSE-pinned build, so test_event_stream_websocket.py still
opts back in and asserts the SSE endpoint is never opened.

Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Copilot AI lite review requested due to automatic review settings August 7, 2026 08:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

finally:
recv_task.cancel()
with contextlib.suppress(asyncio.CancelledError, Exception):
await recv_task
)
with contextlib.suppress(RuntimeError):
await websocket.close(code=status.WS_1001_GOING_AWAY)
except WebSocketDisconnect:
@omnigent-ci

omnigent-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Missing visual demonstration

This PR fixes a user-facing failure (open tabs exhausting the ~6-per-origin HTTP/1.1 pool and stalling the whole app) and the description itself points to DevTools → Network as the way to see the result. A short before/after Network-panel capture — SSE GETs filling every connection slot vs. the event stream riding …/stream/ws with the HTTP pool free — would make the fix verifiable without a checkout. The Demo section is N/A; a network screenshot/gif is the natural evidence here.

Blocking issues

None. I traced the server route, the shared _iter_session_events core, the client transport, and the startStreamPump WS branch:

  • Presence is cleaned up on every exit path: _iter_session_events registers/disconnects presence in a try/finally, and the WS route wraps it in contextlib.aclosing, so a client drop, subscriber overflow, or an unexpected send error still runs the disconnect.
  • Auth/authz gates fire before accept() and map both the unauthenticated and unauthorized cases to a 1008 close (covered by the two reject tests); the session id is never trusted for authz.
  • The WS route reuses the SSE snapshot (_build_resource_snapshot) and the same viewer_user_id=_attribution_user(user_id) / presence_root_id=root semantics, so parity holds. _build_resource_snapshot is defined after stream_session but resolves fine as a closure free-var at call time.
  • Empty-open failure detection is sound: session.heartbeat parses to null client-side and is not queued, but the snapshot events (session.changed_files.invalidated, session.presence) parse non-null, so sawEvent flips on any real connect — only genuinely empty closes count toward MAX_WS_EMPTY_OPENS.
  • CSWSH is not a new gap: the global WebSocketOriginMiddleware (app.py) already guards every @router.websocket route, including this one.

Security vulnerabilities

None found. The new route inherits the app-wide WS Origin enforcement, gates identity pre-accept, and access-checks the session at LEVEL_READ. No secrets, deserialization, or path-traversal surface introduced. No lockfile/dependency or extras changes in the diff.

Non-blocking notes

  • Disconnect latency vs. send-after-close. _watch_disconnect flips the flag on WebSocketDisconnect, but the emit loop only re-checks disconnected.is_set() when the next event/heartbeat arrives (≤ heartbeat interval). In that window a send_text to a just-closed socket can raise a RuntimeError that isn't in the except (SubscriberOverflowError, WebSocketDisconnect) list; it propagates but the finally still closes cleanly and aclosing still runs presence cleanup, so the only cost is a logged error. Consider contextlib.suppress-ing the send or broadening the except for tidiness.
  • Forbidden-session UX under the default transport. The SSE path detects 401/403 on the preflight and gives up immediately; the WS path can only infer a permanent 403 from MAX_WS_EMPTY_OPENS (8) empty closes with backoff between them, so a genuinely forbidden session shows a spinner noticeably longer before flipping to failed. Documented and acceptable, but worth keeping the cap low.
  • e2e coverage is default-transport-only in one direction. The suite pins VITE_EVENT_STREAM_TRANSPORT=sse and a single e2e opts back into WS; the many SSE-driven interception tests no longer exercise the production default. The dedicated WS server/client tests mitigate this, but the broad reconnect/interception coverage now runs against the fallback path rather than the shipped one.

Summary

A well-structured, defensively-written transport addition. The SSE generator is cleanly refactored into a transport-agnostic core with byte-identical SSE output, the WS route faithfully mirrors the SSE auth/presence/snapshot contract, and the client pump is split so both transports share the reduce/flush logic. Test coverage (server WS route, client socket, shared pumpParsedEvents, transport selection, and an end-to-end guard that asserts the SSE endpoint is never opened) is thorough. No blocking correctness or security issues; the only real asks are a Network-panel demo and a couple of minor robustness/UX refinements.


Automated review by Polly · workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Pull request size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants