Skip to content

fix(sse): bound slow keepalive path with absolute last-resort deadline - #14808

Merged
diegosouzapw merged 5 commits into
diegosouzapw:release/v3.8.51from
maxmad64bis:fix/keepalive-deadline-last-resort
Sep 25, 2026
Merged

diegosouzapw merged 5 commits into
diegosouzapw:release/v3.8.51from
maxmad64bis:fix/keepalive-deadline-last-resort

Conversation

@maxmad64bis

@maxmad64bis maxmad64bis commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ base-red inherited: #14547

Summary

A handler that never resolves before first byte holds the early-keepalive slow path open forever, pinning the client spinner and concurrency slots. The slow path only ticks keepalives until the handler settles, while legitimate waits (queue, cooldown, park-and-resume, readiness) all fit under a derived bound. At 33 minutes the wrapper now emits the route error frame, closes the stream, aborts the handler, and warns once with correlation.

Related Issues

Related to #12685

Upstream census (2026-09-24)
# State Title / maintainer position
#12685 closed keepalive-only hang; diegosouzapw 2026-09-10 confirmed the frame is internal keepalive, 2026-09-17 closed stale
#9153 closed per-attempt deadline; diegosouzapw 2026-08-21 viable, in progress
#12741 closed fail fast on heartbeat-only streams; closed by diegosouzapw 2026-09-10
#4105 closed clear keepalive timer on abort; diegosouzapw 2026-06-17 merged
#14025 open readiness budget ignores reasoning effort; no position yet
#14792 open thread correlationId into chat keepalive; overlaps this diff
#14377 open strict SSE switches; diegosouzapw 2026-09-22 building opt-out
#13850 open valid responses keepalives; diegosouzapw 2026-09-22 changes asked

Validation

  • Change type: sse
  • Focused tests and category gates from the golden path
  • npm run lint
  • Reconciled with the current active release base; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR

Tests Added Or Updated

  • tests/unit/early-stream-keepalive.test.ts — 4 new tests: expiry emits the error frame, aborts the deadline controller, and warns once with correlation; resolve under the deadline forwards with no warn; slowPathDeadlineMs <= 0 disables the deadline; client abort before the deadline keeps the existing abort path with no deadline warn.

Coverage Notes

  • The 4 new tests cover the added branches in open-sse/utils/earlyStreamKeepalive.ts (timer start, expiry path, opt-out, abort interplay); targeted suites are green (keepalive 22/22); the full suite runs in CI.

Reviewer Notes

Maintainer rework (merge-batch 2026-09-24)

  • Unbounded token map fixed (c8b0144). deadlineControllersByToken is keyed by strings, so each request left an entry behind for the life of the process. Now:
    • the entry is released when the keepalive wrapper finishes (fast path, settle, client abort, cancel or deadline expiry) through the new releaseDeadlineController();
    • a FinalizationRegistry backstop covers requests that never reach the wrapper (non-streaming paths);
    • a hard cap of 10,000 entries acts as the last resort.
  • New regression test. 20 rounds of fast, slow, expired and client-aborted requests must leave the map at its original size, and a released token must no longer resolve through the header lookup. With the release disabled the test fails with 45 entries left over. With the fix it passes.
  • Doc comment restored. The SettledHandler comment had lost one line (the strictNullChecks: false sentence). It is back.
  • correlationId: reqId moved in the chat route (73f896b). It now sits on the same line where fix(api): thread correlationId into chat completions early keepalive #14792 adds it, so the two merge into a single property instead of a duplicate key (TS1117).
  • Merged the current release/v3.8.51, which includes fix(api): thread correlationId into chat completions early keepalive #14792. The combined state has been checked.
  • Gates run:
    • Focused keepalive/route tests are green, including fix(api): thread correlationId into chat completions early keepalive #14792's.
    • typecheck:core and check:open-sse-typecheck are clean.
    • ESLint on the changed files is clean.
    • The JON-562 /v1/messages retention canary passes on this branch. Timeouts had to be raised for that run because the devbox was overloaded; the tip fails the same way under that load.
    • check-file-size flags only open-sse/executors/opencode.ts. That file is identical to the tip, so the failure is inherited from the base.

@maxmad64bis
maxmad64bis force-pushed the fix/keepalive-deadline-last-resort branch from e186199 to 9b5f5d6 Compare September 24, 2026 22:00
A handler that never resolves before first byte no longer holds the
early-keepalive slow path open forever: at 33 minutes the wrapper
emits the route error frame, closes the stream, aborts the handler,
and warns once with correlation.
@maxmad64bis
maxmad64bis marked this pull request as ready for review September 25, 2026 08:13
@maxmad64bis
maxmad64bis force-pushed the fix/keepalive-deadline-last-resort branch from 9b5f5d6 to cc87bd5 Compare September 25, 2026 08:14
…row unbounded

withDeadlineSignal registered every request under a string token in a
module-level Map that nothing ever pruned, so each streamed request left an
entry behind for the life of the process. Release the entry when the
keepalive wrapper finishes (fast path, settle, client abort, cancel or
deadline expiry), add a FinalizationRegistry backstop for requests that
never reach the wrapper, and cap the map size as a last resort.

Also restore the doc-comment line on SettledHandler that the original
commit dropped by accident.
…4792 adds it

diegosouzapw#14792 threads the same correlationId: reqId option into the chat
completions keepalive call, one line above extraHeaders. Putting this
branch's copy on the same line lets the two changes merge into a single
property instead of a duplicate key (TS1117) when diegosouzapw#14792 lands first.
@diegosouzapw
diegosouzapw merged commit 8c05ec4 into diegosouzapw:release/v3.8.51 Sep 25, 2026
12 of 16 checks passed
HouMinXi added a commit to HouMinXi/OmniRoute that referenced this pull request Sep 26, 2026
…quest proxy

The Next.js App Router hands route handlers a Proxy around the inbound
request for dynamic "auto" routes. Passing that proxy as the Request
constructor input makes undici read #state on the proxy receiver;
ECMAScript gives a Proxy no [[PrivateFieldValues]], so every request on
/v1/chat/completions, /v1/messages and /v1/responses answered HTTP 500
with "TypeError: Cannot read private member #state ..." after diegosouzapw#14808
wired withDeadlineSignal into those routes.

Rebuild the wrapped request field-by-field (url/method/headers/body/
signal/duplex) — property reads go through the proxy get trap safely.

Adds a regression test that wraps the input in a Next-style trap and
asserts the wrap survives, with bug-injection verified (reverting to
the constructor-input form fails the test).

Signed-off-by: Minxi Hou <houminxi@gmail.com>
hugoharman pushed a commit to hugoharman/OmniRoute that referenced this pull request Sep 27, 2026
withDeadlineSignal() wrapped the inbound request with
`new Request(request, { signal, headers })`. Next.js hands App Router
handlers that have no `dynamic` export a Proxy around the NextRequest
(proxyNextRequest), and the Request constructor reads the input's private
#state field, which cannot be reached through a Proxy. Every call to
/v1/chat/completions, /v1/messages and /v1/responses therefore failed with
"TypeError: Cannot read private member #state from an object whose class
did not declare it" and HTTP 500 (regression from diegosouzapw#14808).

Rebuild the request from its public accessors (url, method, headers,
redirect, body stream with duplex "half"), matching rebuildRequest() in
chatBodyAdmission. Adds regression tests with a Proxy-wrapped request and
a body-less GET.
@maxmad64bis
maxmad64bis deleted the fix/keepalive-deadline-last-resort branch September 30, 2026 00:21
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.

2 participants