Skip to content

feat(web): warn when open tabs exhaust the browser connection pool - #3699

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

feat(web): warn when open tabs exhaust the browser connection pool#3699
TomeHirata wants to merge 2 commits into
mainfrom
fix/stream-exhaustion

Conversation

@TomeHirata

@TomeHirata TomeHirata commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Related issue

N/A

Summary

Each open conversation holds one long-lived GET /v1/sessions/{id}/stream SSE request for as long as it's bound. Browsers cap HTTP/1.1 connections at ~6 per origin, and that budget is shared across every tab in the profile — so with 5–6 conversations open the held streams occupy every slot and unrelated requests (navigation, API calls) queue behind them. The UI appears hung, with nothing in the app explaining why.

This PR makes the cause visible:

  • Each bound stream holds a uniquely-named Web Lock, so any tab can count how many streams the profile holds via navigator.locks.query() (web/src/lib/streamTabRegistry.ts).
  • At 5 stream-holding tabs, a banner explains the browser limit and names the one remedy the user has — close a few tabs (web/src/components/StreamTabLimitBanner.tsx).

Scope — this is advisory. It explains the stall; it does not remove it. Lifting the limit needs a transport that doesn't consume an HTTP connection per conversation, which is a follow-up PR (see below).

Design notes

  • Web Locks, not a heartbeat. The browser releases a lock automatically when its tab goes away — crash and force-quit included — so there are no phantom tabs and no expiry window to tune. A localStorage/BroadcastChannel heartbeat would need both.
  • Counted per bound stream, not per tab. A tab on the sidebar, settings, or inbox holds no stream and consumes no connection, so it must not inflate the count. startStreamPump owns acquire/release for exactly the window its stream exists — verified that this is the single place a stream is opened (one caller; switchTo aborts the prior stream, so a tab never holds two).
  • Suppressed on multiplexed connections. HTTP/2 and HTTP/3 carry every request over one connection, so the cap doesn't bind — and warning anyway would be a false alarm on the Databricks Apps ingress (whose HTTP/2 stream cap is what the pump already reconnects around). Gated on the navigation entry's ALPN id; an unknown protocol is treated as HTTP/1.1 so the warning fails toward being shown on the setups most likely to stall.
  • Re-arms only on worsening. Dismissing at 5 won't re-nag at 5, but crossing to 6 is new information.
  • Degrades to inert where Web Locks is unavailable (count reads 0 → banner never shows).

ELI5

The browser only lets ~6 long-lived connections talk to one site at a time, and every tab shares that budget. Each open chat holds one open forever to receive live updates. Open six chats and there's nothing left for anything else, so the app freezes. This PR notices that situation and says so, instead of leaving you with a mysterious hang.

5 conversation tabs -> 5/6 HTTP slots used -> banner: "close a few tabs"
6 conversation tabs -> 6/6 slots used      -> requests queue (the hang)

Test Plan

  • cd web && pnpm vitest run src/lib/streamTabRegistry.test.ts src/components/StreamTabLimitBanner.test.tsx15 passed. Covers: counting this tab's stream, counting peer tabs' streams, ignoring non-stream locks, prompt release on stream end, unique lock names (a shared name would pin the count at 1), no-op without Web Locks, and the HTTP/1.1-vs-h2/h3 gate including the unknown-protocol default.
  • cd web && pnpm vitest run src/store/chatStore.test.ts305 passed (the startStreamPump acquire/release wiring changes nothing about the pump's behavior).
  • Full web unit suite: 5389 passed, 3 pre-existing failures in src/shell/NewChatDialog.test.tsx — confirmed identical on clean origin/main (stashed this branch's changes and re-ran), so unrelated to this PR.
  • Prettier, oxlint, TypeScript type-check, and pre-commit all clean.
  • E2E: tests/e2e_ui/chat/test_stream_tab_limit_banner.py opens 5 conversation tabs in one browser context (tabs in one context share both the connection pool and the Web Locks scope, like real tabs in one profile), asserts the banner appears, then closes a tab and asserts it clears.
  • Manual: just dev, open 5 conversations in separate tabs against the local HTTP/1.1 server → banner appears; close one → it clears. With h2 in front, the banner stays hidden.

Demo

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 the banner appears at 5 stream-holding tabs on a local HTTP/1.1 server, clears when a tab closes, and stays hidden behind an HTTP/2 front end. Automated coverage is unit-level: the lock registry (count, release, foreign locks, unique naming, unsupported-browser fallback, protocol gate) and the banner (threshold, dismiss, re-arm on worsening, multiplexed suppression).

E2E coverage opens 5 conversation tabs in a single browser context and asserts both the appearance and the self-clearing of the banner. Before writing it I confirmed the premise empirically with a standalone Playwright probe: 3 pages in one context see all 3 stream locks, a page in a second context sees none of them (so separate contexts would have silently counted only themselves), and closing a page drops its lock — which is exactly the auto-release property that makes Web Locks the right primitive here.

I could not execute the e2e test locally. The local harness currently fails to bring a runner online within its 30s budget — test_smoke.py, which this PR does not touch and which passes in CI, fails identically on this machine, so it is environmental rather than caused by this test. The e2e result in CI is therefore the first real execution; if it fails I will fix it rather than waive the gate.

Reviewer note — this explains the hang rather than fixing it. At 6 simultaneously-visible conversation tabs the UI will still stall; the banner just makes the cause legible. The transport fix that actually removes the ceiling is split out per review feedback: #4342 (event WebSocket — server route + client transport + tests, already implemented and green).

Changelog

[UI] Omnigent now warns when enough tabs are open to hit the browser's connection limit and slow the app down.

@github-actions github-actions Bot added the size/XL Pull request size: XL label Jul 31, 2026
Comment thread omnigent/server/routes/sessions/routes_events.py Fixed
Comment thread omnigent/server/routes/sessions/routes_events.py Fixed
@omnigent-ci

omnigent-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: stream session events over a WebSocket

Blocking issues

None. The transport swap is well-factored: the SSE wire output is preserved byte-for-byte (_stream_live_events still appends [DONE] and only SSE-formats pairs from the shared core), presence connect/disconnect moved cleanly into _iter_session_events's try/finally under aclosing (so the subscriber slot and presence token are released on WS teardown too), and the WS route enforces the same LEVEL_READ authz and pre-accept auth rejection as the SSE route. Auth/authz is not weakened: unauthenticated handshakes are closed 1008 before any session id is probed, and the id is never trusted for authorization. Snapshot-on-connect is genuinely shared via _build_resource_snapshot, and the client clean-close (1000) → server_closed vs. drop → dropped mapping matches the SSE [DONE] semantics.

Security vulnerabilities

None introduced. Identity rides the WS handshake via the ingress (browsers can't set X-Forwarded-Email on an upgrade) — same trust model as the existing session_updates WS, not a new boundary. No injection, deserialization, or path-traversal surface added; the WS payload is json.dumps of an already-validated ServerStreamEvent. No lockfile/pyproject/extras changes in this diff.

Non-blocking notes

  • Pre-accept exception mapping is incomplete. Only _require_access_and_level is wrapped to a WebSocketException. _get_runner_client / _ensure_runner_relay_ready run before websocket.accept() and can raise OmnigentError; those would surface as an ungraceful handshake failure rather than the clean 1008-style close the access path produces. Consider extending the try to cover them for consistent client-visible close semantics.
  • send_text after disconnect. The emit loop guards with disconnected.is_set() before each send, but a disconnect landing between that check and await websocket.send_text(...) can raise a non-WebSocketDisconnect error (e.g. Starlette's "close message already sent" RuntimeError), which neither except clause catches and which would propagate past cleanup. finally still releases presence/recv_task, so it's not a leak — but broadening the catch (or wrapping the send) would keep it quiet.
  • Reconnect backoff vs. empty-open guard. Because _build_resource_snapshot always emits at least session.changed_files.invalidated + a presence frame, sawEvent flips true on any accepted socket, so MAX_WS_EMPTY_OPENS only ever fires for the "accept but zero frames" case. A server that accepts, sends the snapshot, then repeatedly drops would reconnect with failedOpens === 0 (no backoff) — a hot loop. This mirrors the existing SSE behavior (healthy-connect-then-drop also reconnects instantly), so it's not a regression, just a latent edge worth being aware of.
  • session.heartbeat ack isn't a client-visible event. parseEvent has no session.heartbeat case, so the WS ready-ack is dropped client-side (same as SSE) and doesn't count toward sawEvent; the snapshot events carry that role instead. Correct as written, but the "acks with heartbeat" server test and the sawEvent logic depend on this indirection — a note for future maintainers.

Summary

Clean, low-risk transport addition that solves a real HTTP/1.1 connection-exhaustion problem by moving the per-conversation stream onto the WS pool, with the SSE path retained as an env-gated fallback. The shared-core refactor (_iter_session_events, _build_resource_snapshot, pumpParsedEvents) is tasteful and preserves the existing SSE contract, and coverage is solid on both sides (new WS route integration tests, new client transport unit tests, and the existing SSE reconnect suite pinned to the fallback via test-setup.ts). No blocking correctness or security issues found; the notes above are hardening opportunities, not gating. Since this is a network-transport fix with no rendering change (verified via DevTools → Network), the N/A demo is reasonable.


Automated review by Polly · workflow run

Copilot AI lite review requested due to automatic review settings August 5, 2026 09:14
@TomeHirata
TomeHirata force-pushed the fix/stream-exhaustion branch from da757d0 to 7648ceb Compare August 5, 2026 09:14

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.

@TomeHirata
TomeHirata force-pushed the fix/stream-exhaustion branch from 7648ceb to 5ccdd5d Compare August 5, 2026 09:22
Copilot AI review requested due to automatic review settings August 5, 2026 09:22

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.

Each open conversation holds one long-lived
`GET /v1/sessions/{id}/stream` SSE request for as long as it is bound.
Browsers cap HTTP/1.1 connections at ~6 per origin and that budget is
shared across every tab in the profile, so with 5-6 conversations open
the held streams occupy every slot and unrelated requests (navigation,
API calls) queue behind them — the UI appears hung with nothing in the
app explaining why.

Surface the cause. Each bound stream holds a uniquely-named Web Lock, so
any tab can count how many streams the profile is holding via
`navigator.locks.query()`; at 5 a banner explains the browser limit and
points at the one remedy the user has (close a few tabs).

Web Locks rather than a localStorage/BroadcastChannel heartbeat: the
browser releases a lock automatically when its tab goes away, including
a crash or force-quit, so there are no phantom tabs and no expiry window
to tune.

Two things keep it from crying wolf. The count is per BOUND STREAM, not
per tab, so a tab on the sidebar or settings — which holds no stream and
no connection — never inflates it. And it renders only when the page was
served over HTTP/1.1: HTTP/2 and HTTP/3 multiplex over a single
connection, so the cap does not bind on the Databricks Apps ingress.

This is advisory. It explains the stall rather than removing it; lifting
the limit needs a transport that doesn't consume an HTTP connection per
conversation, which is a follow-up.

Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Copilot AI review requested due to automatic review settings August 7, 2026 08:11
@TomeHirata
TomeHirata force-pushed the fix/stream-exhaustion branch from 5ccdd5d to 8fad061 Compare August 7, 2026 08:11

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.

@TomeHirata TomeHirata changed the title fix(sessions): stream session events over a WebSocket to dodge HTTP connection exhaustion feat(web): warn when open tabs exhaust the browser connection pool Aug 7, 2026
Adds the Playwright coverage the E2E UI Required gate asks for: open
enough conversation tabs to threaten the browser's per-origin connection
budget, assert the banner explains it, then close a tab and assert it
retires itself.

Several pages in ONE browser context, deliberately: Web Locks (which back
the count) are scoped to a browsing-context group, and pages in one
Playwright context share a lock manager — the same scope as real tabs in
one browser profile. Verified empirically that 3 pages in one context see
all 3 locks, a page in a second context sees none of them, and closing a
page drops its lock.

The same conversation opened N times is realistic: every tab runs its own
store and stream pump, so N tabs on one session hold N streams and
consume N connections just as N tabs on N sessions would.

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

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.

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