Skip to content

Commit c729b12

Browse files
fix(agentex-ui): abort the upstream request when the BFF client disconnects (#383)
## Problem The `/api/agentex` catch-all BFF proxy streams SSE through unbuffered but passes no `signal` to `fetch`. When the browser goes away — closed tab, navigation, or the 300s Istio route timeout on the UI's `virtualService` — undici keeps the upstream request open. There is no timeout on the call either, so an orphan never expires, and the browser's `EventSource` then reconnects and opens a fresh one. Each orphan leaves an `agentex` handler blocked in a Redis `XREAD`, pinning one connection from that pod's redis-py pool (`REDIS_MAX_CONNECTIONS`, default 200). They accumulate until the pools are exhausted, at which point `agentex` fails to serve new requests and only a restart recovers it. Reported on Mayo Nonprod ([Slack thread](https://scaleapi.slack.com/archives/C05KKGVD2QP/p1785278044417629)). At saturation Redis showed 1,094 `connected_clients` with 943 `blocked_clients`, while each of the two `agentex-ui` pods held ~570 established sockets to the `agentex` ClusterIP and served one inbound client apiece. Restarting only `agentex-ui` dropped backend Redis connections from ~1,094 to 279 without touching `agentex`, confirming the UI as the holder. Introduced in 0f07dc7 (#350). ## Fix Pass `req.signal`. App Router aborts it on client disconnect, which propagates through undici and destroys the upstream request — and makes the existing Istio route timeout an effective ceiling on upstream lifetime. A fixed timeout would be wrong here, since it would cut legitimate long-lived SSE streams. The abort also rejects the in-flight `fetch`, so that case is answered with a 499 instead of propagating as a 500 — a departed client isn't an upstream failure. A genuine transport error still propagates exactly as before. ## Verification Built this branch, pointed `AGENTEX_API_URL` at a connection-counting SSE upstream, opened 5 streaming clients and killed them: | | upstream live while connected | 6s after clients killed | | --- | --- | --- | | current `main` shape | 5 | **5** ← leak | | this branch | 5 | **0** | No `ResponseAborted` stack traces in the server log, server healthy afterward. The main risk of adding a signal is truncating a response that is still streaming after the handler returns, so that was checked explicitly on a production build: - a 2 MB body streamed from upstream over ~2s arrived at exactly 2,048,011 bytes, 3/3 runs - a 666,669-byte streamed `POST` with `duplex: 'half'` echoed back intact `tsc --noEmit`, `eslint --max-warnings=0`, and `prettier --check` are clean. ## Follow-ups for the Agents team (not this PR) Two `agentex` backend issues from the same thread remain open: 1. **`cleanup_stream` in the `finally` deletes a task-wide stream** ([`streams_use_case.py:194`](https://github.com/scaleapi/scale-agentex/blob/main/agentex/src/domain/use_cases/streams_use_case.py#L194)). The topic is keyed by task id alone, so it is shared by every viewer. Worth landing with or right after this PR: today that `finally` almost never runs *because* disconnects don't propagate, and this change makes it run on every tab close. If something looks like a regression from this PR, it is that. 2. **SSE subscriptions have no terminal condition.** A tab left open on a finished task still pins a pool connection indefinitely, polling `XREAD` every ~2.1s. The naive fix is unsafe: the client's retry loop (`custom-subscribe-task-state.tsx`, `while (!signal?.aborted)`) treats a clean server-side stream end as "reconnect immediately" — no backoff, no error-counter increment — so ending the stream server-side alone would produce a reconnect storm. Needs a coordinated client change. Separately and unrelated to the leak: `AGENTEX_API_URL` falls back silently to `http://localhost:5003`, so a rename on either side yields a UI that can't reach the backend with no configuration error. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes a connection-pool exhaustion bug where client disconnects (tab close, navigation, Istio timeout) left orphaned upstream SSE connections open indefinitely. The fix passes `req.signal` to the upstream `fetch` so that undici tears down the upstream request when the browser goes away, and wraps the fetch in a try/catch to return HTTP 499 for clean disconnects rather than propagating a 500. - **Signal forwarding**: `signal: req.signal` is added to the `fetch` options, making the Istio route timeout an effective ceiling on upstream lifetime and eliminating the Redis connection leak described in the incident. - **Abort discrimination**: The catch block uses identity comparison (`error === req.signal.reason`) rather than `error.name === 'AbortError'`, correctly targeting Next.js's `ResponseAborted` abort reason while letting genuine transport errors propagate unchanged. <details><summary><h3>Confidence Score: 5/5</h3></summary> Safe to merge — a targeted, well-tested fix that eliminates the orphaned upstream connection leak with no changes to the happy path. The change is small and surgical: one new try/catch block, one added fetch option. The abort discrimination via identity comparison correctly handles Next.js's ResponseAborted abort reason without relying on error names. The PR description documents explicit verification that streaming bodies are not truncated and that 0 upstream connections survive after all clients disconnect. **Files Needing Attention:** No files require special attention. </details> <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | agentex-ui/app/api/agentex/[...path]/route.ts | Wraps upstream fetch in try/catch, adds req.signal to propagate client disconnects, and returns 499 on abort using identity comparison against req.signal.reason. | </details> <sub>Reviews (2): Last reviewed commit: ["fix(agentex-ui): match the abort by reje..."](c78215b) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=48166959)</sub> <!-- /greptile_comment --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0011d2c commit c729b12

1 file changed

Lines changed: 20 additions & 8 deletions

File tree

  • agentex-ui/app/api/agentex/[...path]

agentex-ui/app/api/agentex/[...path]/route.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,26 @@ async function proxy(
3434

3535
const method = req.method.toUpperCase();
3636
const hasBody = method !== 'GET' && method !== 'HEAD';
37-
const upstream = await fetch(target, {
38-
method,
39-
headers,
40-
body: hasBody ? req.body : undefined,
41-
redirect: 'manual',
42-
// @ts-expect-error `duplex` is required to stream a request body (undici)
43-
duplex: 'half',
44-
});
37+
let upstream: Response;
38+
try {
39+
upstream = await fetch(target, {
40+
method,
41+
headers,
42+
body: hasBody ? req.body : undefined,
43+
redirect: 'manual',
44+
// A client disconnect must tear down the upstream request; undici otherwise holds it
45+
// open and the upstream handler keeps running.
46+
signal: req.signal,
47+
// @ts-expect-error `duplex` is required to stream a request body (undici)
48+
duplex: 'half',
49+
});
50+
} catch (error) {
51+
// The disconnect above rejects the fetch with the signal's abort reason itself, so identity —
52+
// not `error.name` — separates it from a transport failure that coincides with a disconnect.
53+
// Next's reason is a `ResponseAborted`, so an `AbortError` check would never fire.
54+
if (error === req.signal.reason) return new Response(null, { status: 499 });
55+
throw error;
56+
}
4557

4658
// Pass the upstream body through unbuffered so SSE / streaming responses work.
4759
const resHeaders = new Headers(upstream.headers);

0 commit comments

Comments
 (0)