fix: send SSE keep-alive comment frames from WebStandardStreamableHTTPServerTransport#2541
fix: send SSE keep-alive comment frames from WebStandardStreamableHTTPServerTransport#2541mattzcarey wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 5b10de0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the candidate concern that the keepAliveMs: 0 disable test is vacuous (Promise.race against an already-resolved promise) — it is not: race subscribes to reader.read() first, so when a frame is buffered its already-resolved promise's reaction is queued ahead of Promise.resolve('pending') and the test would correctly fail if frames were written.
Extended reasoning...
Bugs were found and posted as inline comments (keep-alive timer armed before the fallible writePrimingEvent await, and the close-during-replayEventsAfter race), so human review is already signalled. This note only records the one additional candidate examined and ruled out this run — the supposed vacuity of the keepAliveMs: 0 test's Promise.race — which was verified to be a sound test via microtask reaction ordering. Nothing here should be read as a guarantee of correctness of the rest of the diff.
There was a problem hiding this comment.
Beyond the inline findings, I verified at head c9b0215 that the round-4 fixes genuinely landed this time (given the earlier accidental revert): the NaN-safe !(keepAliveMs > 0) guard is present in both streamableHttp.ts:306 and perRequestTransport.ts:427 (matching listenRouter.ts:221), and the replay-await regression test's mock now returns _GET_stream, so it genuinely reaches the keep-alive arm and exercises the closed-transport guard. I also checked the timer lifecycle for leak paths beyond the inline findings: stopKeepAlive is wired into every stream cleanup/cancel path (standalone GET, replay, POST), and close() sweeps the timer map after stream cleanup — no unswept path found for finite in-domain intervals.
Extended reasoning...
This run's five findings are all nit-severity (test-coverage gaps, an Infinity edge on an operator-config input, a pre-existing error-code mapping a new test cements, a missing X-Accel-Buffering header on pre-existing response sites, and a docs misattribution) and are already posted inline. This note records the concrete items examined and ruled out on top of them, chiefly re-verifying that the previously-claimed-then-reverted guard fix is actually present at the current head — informational only, not a correctness guarantee. The PR touches transport lifecycle code across four review rounds, so it stays with the inline findings and human judgment rather than an approval.
There was a problem hiding this comment.
Beyond the inline findings, this run also examined whether the round-4 troubleshooting correction (client body-idle timeout, not server.requestTimeout) needed to propagate into the keepAliveMs JSDoc and the changeset, which still say "intermediaries and server idle timeouts" — ruled out: that prose names the client-observed symptom (TypeError: terminated) and the intermediary/watchdog class generically, without repeating the specific incorrect server.requestTimeout attribution the docs fix removed.
Extended reasoning...
This run's findings (the pre-existing handlePostRequest close-race gap and the two nits) are already posted inline, so no summary of them belongs here. The one candidate examined and refuted this run was a claimed inconsistency between the corrected troubleshooting entry and the keepAliveMs JSDoc/changeset wording; verifiers concluded the surviving prose is symptom-level and accurate (it does not re-assert the refuted server.requestTimeout mechanism), so it is not a defect. Recording it so a later pass does not re-litigate the wording after the round-4 doc correction.
|
Quality review follow-up in
The refactor removes duplicated policy while preserving each transport's distinct teardown ownership. Local server suite is 472/472 and full workspace pre-push build/typecheck/lint passed. |
There was a problem hiding this comment.
Beyond the inline findings, this run also examined a close() racing the writePrimingEvent await on the POST SSE leg and ruled it out: the stream mapping is registered before that await, so close()'s sweep runs its cleanup (controller closed, mapping deleted, timer stopped); the continuation's keep-alive arm is skipped by the registration identity check at streamableHttp.ts:1016, and the client observes stream end rather than a hang.
Extended reasoning...
Bugs were found this round (two nits, one pre-existing), so the inline comments carry the substantive feedback and no approval/deferral body is warranted. This note records one concrete close-race candidate that was examined and refuted — the writePrimingEvent-await window in handlePostRequest — since prior rounds repeatedly surfaced close-race variants in this function and this record should keep a later pass from re-exploring it. Verified against the current code: registration precedes the await, close()'s sweep closes the controller and deletes the mapping, and the post-await keep-alive arm identity-checks the mapping (startKeepAlive is additionally _closed-guarded). This is informational only, not a correctness guarantee, and is distinct from the filed nit about the earlier req.json() window (session mint + onsessioninitialized firing before the 404).
| } | ||
| this._streamMapping.clear(); | ||
|
|
||
| // Clear any keep-alive timers not already cleared by stream cleanup | ||
| for (const timer of this._keepAliveTimers.values()) { | ||
| clearInterval(timer); | ||
| } | ||
| this._keepAliveTimers.clear(); | ||
|
|
There was a problem hiding this comment.
🟣 Pre-existing issue (the JSON-mode cleanup shape and the close() sweep both predate this PR): in enableJsonResponse mode, the stream entry's cleanup only deletes the _streamMapping entry and never settles the 'new Promise(resolve => ...)' returned by handlePostRequest, so close()'s sweep landing while a JSON-mode request handler is in flight (client-drivable: DELETE on the session during a slow tools/call POST) silently drops the resolveJson sink and the POST's handleRequest promise pends forever — a byte-less hung HTTP exchange plus a leaked promise and captured request context. Since this PR already fixed the equivalent close-race hang on every SSE leg, settling the deferred in the JSON entry's cleanup (e.g. resolve with the 404 'Session not found' body the transport now answers for closed transports) would complete the invariant the new lifecycle tests pin.
Extended reasoning...
What the bug is. In enableJsonResponse mode, handlePostRequest returns new Promise<Response>(resolve => ...) and registers the JSON stream entry (streamableHttp.ts:904-910) as { resolveJson: resolve, cleanup: () => { this._streamMapping.delete(streamId); } }. The cleanup deletes the mapping but never settles the deferred. close()'s sweep (streamableHttp.ts:1108-1121, extended by this PR to also clear keep-alive timers) runs for (const { cleanup } of this._streamMapping.values()) cleanup() — so a close that lands while a JSON-mode request handler is still in flight silently drops the resolveJson sink, and nothing can ever settle the returned Promise<Response> afterwards.
Why nothing downstream saves it. When transport.onclose fires, Protocol._onclose sets its transport reference to undefined and aborts in-flight handlers, so the handler's eventual response typically never reaches transport.send() at all. Even if a late send() did run, the all-responses-ready path would find stream === undefined and — in the JSON-mode branch — throw 'No connection established for request ID' into onerror without resolving anything (close() clears _requestResponseMap but not _requestToStreamMapping, so the lookup succeeds and reaches the throw). The Promise<Response> returned by handleRequest stays pending forever.
Step-by-step proof.
- Sessionful transport with
enableJsonResponse: true; client POSTs a slowtools/call. The handler is dispatched and the JSON entry is registered — so the pre-registration_closedre-check this PR added at streamableHttp.ts:875 (commit 8a6a55a) has already passed. - The same or another client sends
DELETEwith the session id →handleDeleteRequestawaitsthis.close()→ the sweep invokes the JSON entry's cleanup, deleting the mapping without resolving the deferred. - The tool handler is aborted (or completes into a nulled transport); no code path holds the
resolveanymore. - The POST's
handleRequestpromise never settles: in a Node adapter (await handler(req, res)inNodeStreamableHTTPServerTransport.handleRequest), the HTTP exchange stays open with zero bytes written until the client's own socket/body timeout kills it — and server-side the pending promise plus its captured request context leak for the life of the process.
Contrast with SSE mode — why this survives this PR. In SSE mode the same sweep's cleanup closes the stream controller, so the client observes stream end — the exact invariant this PR's new close-race lifecycle tests pin ('the client observes stream end instead of hanging on a dead session'). JSON mode is the one response shape where close() still produces an eternal hang instead of a terminated exchange, i.e. a surviving sibling of exactly the hang class this PR eliminated on every SSE leg (the repo's Completeness recurring catch: partial fixes leaving sibling code paths with the bug the PR addresses).
Not a duplicate of the round-3 finding. The earlier report's root cause was registering mappings after close()'s once-only sweep — fixed in 8a6a55a by the pre-registration _closed re-check and the handleRequest 404 gate. This is the post-registration window: the entry exists when the sweep runs, the guards pass, and the fix is different (settle the promise, not re-check before registering).
How to fix. Make the JSON entry's cleanup settle the deferred before deleting the mapping — e.g. resolve(this.createJsonErrorResponse(404, -32_001, 'Session not found')), matching what the transport now answers for requests on a closed transport — or, equivalently, resolve all resolveJson sinks in close()'s sweep. Either way the exchange terminates with a well-formed body instead of hanging, restoring parity with the SSE legs.
Why pre_existing. The resolveJson/non-settling-cleanup shape and the sweep loop both predate this PR; the diff only adds the keep-alive timer clears to close() and did not add or alter the JSON-mode cleanup. That said, since this PR reworks this exact function's close-race lifecycle throughout, it is the natural place to close out the last surviving hang shape.
d221ae9 to
2ec4fda
Compare
There was a problem hiding this comment.
Beyond the inline findings, I re-verified both against the PR head (2ec4fda): grep _closed in streamableHttp.ts matches only the declaration, startKeepAlive, the replay-leg check, and close() — the guards described in the resolved threads are indeed absent — and listenRouter.ts:221-227 still uses the raw keepAliveMs > 0 guard. One adjacent residue the inline comments don't cover: listenRouter.ts:254 still sends Cache-Control: no-cache without no-transform, although the resolved thread states POST, listen, and per-request streams were all updated to no-cache, no-transform in 8a6a55a — worth folding into the same listenRouter follow-up.
Extended reasoning...
Bugs were found and are posted as inline comments, so this is an informational note only, not a verdict on the PR. I independently confirmed both findings at the PR head rather than trusting the finder output: the close-race guards and regression test the resolved review threads attribute to 8a6a55a are not present in the merged streamableHttp.ts, and listenRouter.ts was never migrated onto armSseKeepAlive. In the course of that verification I also checked the listen stream's response headers, since the same resolved-thread text ("POST, listen, and per-request SSE responses now use Cache-Control: no-cache, no-transform") covers it — the listen leg still ships plain no-cache, so that thread's claim is only two-thirds backed by the code. Human review is needed regardless, given two resolved threads describe fixes the diff does not contain.
Add stream-owned keep-alive timers to session and per-request HTTP transports, propagate createMcpHandler's existing interval across its HTTP legs, and prevent proxy buffering.
2ec4fda to
5b10de0
Compare
1 participant
Refs #1211. v2 counterpart to the v1 keep-alive work in #2538/#2547.
Problem
An HTTP SSE stream can remain silent long enough for a client body-idle timeout or intermediary to terminate it. Clients observe
SSE stream disconnected: TypeError: terminatedand reconnect in a loop.Fix
keepAliveMstoWebStandardStreamableHTTPServerTransportandPerRequestHTTPServerTransport(default15000;0disables).: keepaliveSSE comments while GET, POST, replay, modern per-request, andsubscriptions/listenstreams are open.createMcpHandler's existingkeepAliveMssetting to modern exchanges and its internal legacy fallback without changing the exportedlegacyStatelessFallbackAPI.Cache-Control: no-cache, no-transformandX-Accel-Buffering: no.mainfrom fix(server): streamableHttp stores per-request events while stream disconnected (SEP-1699 poll-and-replay) #2342 rather than reimplementing it here.Verification
@modelcontextprotocol/server: 468 tests passedKeep-alive is default-on to fix existing deployments without configuration changes. Hibernation-sensitive runtimes can set
keepAliveMs: 0.