Skip to content

fix(sse): propagate client abort to the upstream request after headers - #14342

Merged
diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.51from
VIPKaiser:fix/propagate-client-abort-upstream
Sep 24, 2026
Merged

diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.51from
VIPKaiser:fix/propagate-client-abort-upstream

Conversation

@VIPKaiser

Copy link
Copy Markdown
Contributor

Summary

When a client aborts a streamed /v1/chat/completions, OmniRoute never cancels the upstream
request. A local upstream (oMLX) keeps prefilling/decoding the abandoned request and blocks the
next one.

Observed (oMLX server log, single-slot local backend):

  • Client aborted at T; the upstream still decoded 1041 more tokens for the dead request.
  • A request aborted 435 ms in still ran a full 34 s prefill, and the next request queued behind it.

Three independent breaks, all fixed here:

  1. open-sse/handlers/chatCore/upstreamTimeouts.ts executeWithUpstreamStartTimeout: links the
    client signal to the fetch signal, but its finally removed that listener as soon as the
    Promise.race settled (headers received). The timeout is meant to bound time-to-headers only;
    a later client abort no longer reached the fetch. The abort link now stays for the fetch
    lifetime on success; only the timer is cleared. It is still dropped when the attempt fails.
    (The once listener lives on the per-request signal, so it is bounded.)
  2. open-sse/utils/streamHandler.ts createDisconnectAwareStream().pull(): on
    !isConnected() it closed the client-facing stream but never cancelled the upstream reader
    (only cancel() did). It now cancels the reader/writer with client_disconnected first,
    except after a client terminal marker or while a completed tool handoff drain owns the reader.
  3. open-sse/services/combo/validateQuality.ts validateResponseQuality peek loop: await reader.read() was not abort-aware; a stalled prefill held the combo attempt until the first
    byte. It now takes an optional signal (passed from runtimeUnits.ts), races each read against
    it, cancels the peek reader on abort and returns { valid: true } (a client abort is not a
    provider failure, so no failover/cooldown; the combo loop's existing signal.aborted check then
    returns 499).

Usage accounting

No change to conventions. handleDisconnect already finalizes the pending call as 499
client_disconnected before either abort propagates, and that is the recorded outcome. A cancelled
upstream never delivers its final usage chunk, so an aborted stream records no upstream usage; I did
not add partial-token estimation. Non-aborted streams take the same code path as before (covered by
tests). Happy to add estimation from tokens seen if maintainers want it.

Tests Added Or Updated

  • tests/unit/client-abort-propagates-upstream.test.ts (new, 8 tests):
    • Real HTTP fake upstream sending headers + chunks: abort after headers, assert the upstream
      socket closes early (fails without fix 1).
    • Non-aborted wrapper result unchanged; failed attempt does not keep the abort link.
    • pipeWithDisconnect: disconnect cancels the upstream body; a normal stream is forwarded
      intact and not cancelled.
    • validateResponseQuality: exits promptly and cancels the reader on abort (stalled body), returns
      immediately on an already-aborted signal, unchanged for a normal stream with a signal.
  • 4 of 8 fail on the base without the source changes; all pass with them.
  • Ran: new test, stream-handler*, stream-disconnect*, codex-tool-handoff*,
    streaming-empty*, combo* (389 tests, 0 fail), npm run typecheck:core,
    npm run check:open-sse-typecheck, eslint + prettier on touched files.

Reviewer Notes

  • Fix 2 is belt-and-braces once fix 1 lands (abort already flows through streamController.signal),
    but covers executors that ignore the signal.
  • Cancelling a Response.clone() tee branch does not by itself abort the upstream; that is why
    fix 1 is the load-bearing change. Peek cancellation intentionally is not awaited.
  • Changelog fragment added: changelog.d/fixes/0000-propagate-client-abort-upstream.md
    (rename to the PR number).

Three breaks let an upstream keep working on an abandoned streamed request:
- executeWithUpstreamStartTimeout removed its client-abort link as soon as
  headers arrived, so a later disconnect never reached the fetch signal.
- createDisconnectAwareStream.pull() closed the client stream on disconnect
  without cancelling the upstream reader.
- The combo streaming quality peek awaited reader.read() with no abort
  awareness, holding the attempt open until the first upstream byte.
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks @VIPKaiser — this closes a real gap: previously a client abort that arrived after
headers (mid-stream, or during the combo quality-peek) never reached the upstream fetch, so the
provider kept generating for a client that was already gone. I probed the new test suite
(tests/unit/client-abort-propagates-upstream.test.ts) against your head commit with a real
node:http fake upstream: all 8 cases pass, covering the timeout-wrapper abort link, the
disconnect-aware stream pipe cancellation, and the combo quality-peek abort race. Clean, well
scoped, well commented. Looks merge-ready to me.

@diegosouzapw
diegosouzapw merged commit 5e5773a into cheaper-inference:release/v3.8.51 Sep 24, 2026
3 checks passed
SCys pushed a commit to SCys/OmniRoute that referenced this pull request Sep 25, 2026
…no live body

Root cause: cheaper-inference#14342 changed executeWithUpstreamStartTimeout to keep the
client-signal `abortListener` (the link that aborts combinedController)
whenever the start-timeout race settled successfully, so a client abort
after headers could still reach the upstream fetch. It kept the link for
every resolved result, including results that carry no streaming body, so
one listener stayed on the client signal after the race settled. That
broke the cheaper-inference#12406 guard ("every listener registered for the race must be
removed once it settles", 1 !== 0).

Fix: keep the link only when the settled result (a Response or the
executor's `{ response }` wrapper) still has an unconsumed body that will
stream on the combined signal. Otherwise remove it in the finally, as for
failed attempts. The abortPromise listener is still always removed. The
cheaper-inference#14342 post-headers propagation is unchanged for streaming responses.

Tests: the cheaper-inference#12406 guard is green again; two new cases pin both sides
(a body-less Response releases the link; a streaming body keeps exactly
one link, propagates the abort, and it self-removes on abort).

Refs cheaper-inference#14547
diegosouzapw added a commit that referenced this pull request Sep 29, 2026
…leak, MCP bundle deadlocks, sidebar keys, flush-empty-retry, proxy-status and pack-policy tests (#14820)

Fixes the reds still present on the tip: client-abort listener leak (#14342 vs #12406), MCP bundle deadlock (quotaCache init cycle via the quotaCacheState leaf, plus the new #14892 proxyLogs -> upstreamStatusCapture -> usage/migrations cycle via the zero-import timingMs leaf), sidebar Model catalog keys, and the stale flush-empty-retry / proxy-status / pack-policy tests. Combo pre-content retry and zh-TW glossary were already fixed on the tip and dropped. Merged tree: 102/102 focused incl. mcp-bundle-startup (red on the tip), typecheck:core clean, open-sse typecheck 0, check:cycles OK, file-size OK.
thinh0704hcm pushed a commit to thinh0704hcm/OmniRoute that referenced this pull request Oct 4, 2026
…per-inference#14342)

Our port rewrote the resolve-settles test to expect an unconditional
kept abort link, but upstream's rule is conditional: the link is kept
only while the settled Response still has a live body. The rewritten
expectation failed (0 listeners vs 1) for a plain string result.
Upstream's own keep-link tests already cover post-resolve abort
propagation, and all 9 pass against our null-safe implementation.
Sign up for free to 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.

3 participants