fix: send SSE keep-alive comment frames from WebStandardStreamableHTTPServerTransport - #2541
Conversation
🦋 Changeset detectedLatest commit: 3c4e82f 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).
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.
2ec4fda to
5b10de0
Compare
There was a problem hiding this comment.
No new bugs found this run, and I verified the two previous-round findings are now fixed at the PR head — but given this changes default wire behavior for every SSE deployment and reworks close-race lifecycle in the core transport, it warrants a human maintainer's sign-off.
What was reviewed this round:
- Confirmed the previously-flagged guards now exist: handleRequest rejects closed transports with 404, handlePostRequest re-checks
_closedboth before the initialization branch and after theonsessioninitializedawait, and the claimed regression tests (body-parse race, session-init race) are present. - Confirmed the listen leg is migrated onto
armSseKeepAlive(invalid-delay tests included) andunsubscribe?.()is wrapped so a throwing custom bus can no longer skip timer/slot cleanup. - Checked handlePostRequest for a
_closedre-check race after thewritePrimingEventawait — ruled out (the close sweep's cleanup closes the registered controller, so no stranded stream results).
Extended reasoning...
Overview
PR #2541 adds configurable SSE keep-alive comment frames (keepAliveMs, default 15000, 0 disables) to WebStandardStreamableHTTPServerTransport, PerRequestHTTPServerTransport, and the subscriptions/listen router, threading createMcpHandler's existing option through all serving legs. It centralizes timer policy in a new sseKeepAlive.ts helper (finite/positive/2^31-1 validation, unref shim), adds anti-buffering headers, hardens several close-race paths (closed-transport 404s in handleRequest/handlePostRequest, replay-leg post-await guard, try/finally around onsessionclosed, try/catch around bus unsubscribe), and adds substantial test coverage across five test files.
Security risks
None identified. The change emits SSE comment frames and headers; keepAliveMs is validated against non-finite/overflow values on all three legs, no user input reaches timer arming unvalidated, and the new closed-transport guards reduce (not add) dangling-state exposure.
Level of scrutiny
High. This is core HTTP transport lifecycle code, it changes default wire behavior for every existing deployment (keep-alive on by default), it adds public API surface (options on two exported interfaces, inherited by NodeStreamableHTTPServerTransport), and the PR went through seven review rounds with real findings (orphaned-timer close races, guard-polarity bugs, un-migrated legs, resolved-thread claims that did not match the head at one point). That history is exactly the profile where a human maintainer should make the final call, even though the current head checks out.
Other factors
This run's bug-hunting pass found nothing. I independently verified the two red findings from the previous run are fixed at the current head: the _closed guards and both regression tests exist in streamableHttp.ts/streamableHttp.test.ts, and listenRouter.ts now uses armSseKeepAlive with the invalid-delay listen tests present. The one candidate raised this run (close racing the writePrimingEvent await in handlePostRequest) was adversarially examined and refuted. Test coverage is strong (fake-timer coverage on all three legs, close-race regressions, header pinning), and the changeset is correctly classified as minor.
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.
5b10de0 to
3c4e82f
Compare
There was a problem hiding this comment.
No bugs found, but this changes default behavior on the Streamable HTTP transport path (keep-alive on by default for every SSE stream, new 404s on closed transports), so it deserves a human look at the design choices.
What was reviewed:
- Keep-alive timer lifecycle across all four SSE surfaces (standalone GET, POST request streams, Last-Event-ID replay, listen router, per-request transport) — every arm site has a matching clear in cancel/cleanup/close, and timers are unref'd.
- Close races: the new
_closedchecks after async body-parse/init points, the replay path's predecessor cleanup before remapping, and the DELETEtry/finallyall guard correctly; keep-alive is only armed when the mapping still points at the live controller. - Checked
keepAliveMs: 0disables the timer end-to-end (?? DEFAULTnever swallows 0), and that removingDEFAULT_LISTEN_KEEPALIVE_MSbreaks no public export (legacyStatelessFallback's signature is preserved via an internal wrapper).
Extended reasoning...
Overview
The PR adds SSE keep-alive comment frames (default 15s, configurable via keepAliveMs, 0 disables) to WebStandardStreamableHTTPServerTransport, PerRequestHTTPServerTransport, and the listen router, unifying the constant in a new sseKeepAlive.ts module. It threads createMcpHandler's existing keepAliveMs option through the modern per-request exchange and the legacy stateless fallback. Alongside, it adds X-Accel-Buffering: no / Cache-Control: no-transform headers, new _closed guards that return 404 after close races, a try/finally around _onsessionclosed in the DELETE path, predecessor-stream cleanup in the Last-Event-ID replay path, and a changeset + troubleshooting doc entry.
Security risks
Low. No auth, crypto, or input-handling changes. The keep-alive frames are SSE comments (ignored by clients, not stored in event stores, so resumability is unaffected). The new 404-on-closed checks slightly tighten behavior rather than loosen it. armSseKeepAlive clamps the interval to the 32-bit timer max, avoiding Node's 1ms-overflow footgun.
Level of scrutiny
High — this is the production HTTP transport lifecycle, one of the most critical paths in the SDK, and the diff is ~480 insertions with several deliberate behavior changes: keep-alive is now ON by default for every SSE stream the SDK serves, closed transports now answer 404 at multiple new points, the replay path now cleans up a predecessor mapping instead of silently overwriting it, and the DELETE handler reorders close relative to the 200 response. I traced each timer arm/clear pairing and each close-race guard and found them correct (arming is conditional on the mapping still holding the live controller; every cleanup/cancel/close path clears its timer; the fallible-enqueue catch self-clears). The listen-router unsubscribe is now wrapped per the repo's teardown convention. legacyStatelessFallback's public signature is unchanged, and the removed DEFAULT_LISTEN_KEEPALIVE_MS was never exported from a package index. Still, defaults-on heartbeats and new 4xx responses are design decisions a maintainer should sign off on, so I am not shadow-approving.
Other factors
Test coverage is strong: fake-timer tests for all four SSE surfaces, keepAliveMs: 0 and invalid-interval disabling, timer-leak assertions (vi.getTimerCount() === 0 after close), two close-race tests (body-parse race, session-init race), and a throwing-unsubscribe teardown test. The changeset and troubleshooting doc match the implemented behavior, including the 0-to-disable claim. The bug hunting system reported no findings, consistent with my own read.
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
LGTM, thanks for picking up the changes.
2 participants
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.