fix: harden SSE keep-alive lifecycle (v1.x backport of the #2541 review fixes) by mattzcarey · Pull Request #2543 · modelcontextprotocol/typescript-sdk

mattzcarey · 2026-07-24T10:25:01Z

Follow-up to #2538 (merged). Review of the v2 port (#2541) found lifecycle defects in the keep-alive implementation after #2538 had already merged, so v1.x carries them; this PR backports the fixes.

Fix

Close races: deferred replay/POST continuations re-check closed state before registering streams; requests on a terminated transport receive the spec's 404 Session not found response.
POST error-path cleanup: keep-alive is armed only after the fallible priming write and message dispatch. If either fails, the discarded response's stream and request mappings are reclaimed as well as its timer. Request-map deletion is ownership-checked so a failed retry cannot erase a successor using the same JSON-RPC id.
Safe interval validation: sub-millisecond, non-finite, non-positive, and overflowing values disable keep-alive instead of being clamped to a ~1 ms frame flood.
Proxy delivery: GET, POST, and replay SSE responses use Cache-Control: no-cache, no-transform and X-Accel-Buffering: no, matching v2.

Verification

transport suite: 170 tests passed
typecheck and lint passed
ESM and CJS builds passed
close-race test is mutation-verified (removing the _closed guard fails it)
error-path tests cover timer teardown, empty maps, successor-map ownership, and post-await close races
patch changeset included

This is intentionally a targeted hardening backport of #2541's review fixes. The separate pre-existing request-scoped replay defect in untouched send() behavior is tracked by #2151.

Backports to v1.x the keep-alive hardening that landed on main after the original v1 fix (#2538) merged: - startKeepAlive is a no-op after transport close, so a deferred arm (e.g. an event-store replayEventsAfter await straddling close()) cannot create a timer the close sweep already missed. - The POST SSE path arms keep-alive after the fallible awaits (priming event write, message dispatch) instead of before them, so an error path that discards the Response cannot leak a permanently-firing timer against a stream nothing can cancel. - The guard uses the !(keepAliveMs > 0) polarity, so a non-finite value (e.g. parseInt of an unset env var) disables keep-alive instead of arming a Node-clamped ~1ms interval that floods every stream. Each fix has a fake-timer regression test.

changeset-bot · 2026-07-24T10:25:07Z

🦋 Changeset detected

Latest commit: 6479960

The changes in this PR will be included in the next version bump.

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

pkg-pr-new · 2026-07-24T10:25:36Z

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2543

commit: 6479960

Addresses the remaining review findings on the same paths: - The POST SSE error path now reclaims the stream bookkeeping too, not just the timer: a failure after registration (failed priming event write, throwing handler) runs the registered cleanup and retires the request mappings, so repeated failures during an event-store outage no longer accrete orphaned stream entries for the session's lifetime. - The replay continuation checks _closed after the replayEventsAfter await: when close() raced the await, the stream is ended instead of re-registered on the dead transport, so the client observes stream end (rather than hanging) and a later resume isn't 409-blocked by a stale entry. - The keep-alive guard rejects all non-finite intervals: Infinity (and any out-of-range delay) is clamped by setInterval to ~1ms just like NaN, so the guard now requires Number.isFinite as well as > 0. Tests extended accordingly: the non-finite case is parameterized over NaN and Infinity, and the error-path tests assert the internal stream maps are empty, not just the timer count.

claude

Additional findings (outside current diff — PR may have been updated during review):

🟣 src/server/webStandardStreamableHttp.ts:975-977 — Pre-existing issue (not introduced by this PR): requests arriving after close() are still accepted — close() sets the new _closed flag but never clears sessionId/_initialized, and neither handleRequest nor validateSession checks _closed, so a POST/GET carrying the terminated session id passes validation, executes handler side effects on a dead session, and returns 200 plus an SSE stream that never carries a response, never ends, and (new with this PR) emits zero keep-alive frames. The MCP Streamable HTTP spec requires 404 for a terminated session id; since this PR introduces exactly the needed primitive, a one-line _closed check at the top of handleRequest (or in validateSession) returning the 404 'Session not found' error would close the gap.
Extended reasoning...

The bug. close() (src/server/webStandardStreamableHttp.ts:975) now sets _closed = true and sweeps streams/timers, but it does not clear this.sessionId or this._initialized — and neither handleRequest nor validateSession consults _closed. validateSession only checks _initialized and session-id equality, both of which still pass after close. So a POST or GET carrying the terminated session id is accepted and processed as if the session were alive, where the MCP Streamable HTTP spec says the server MUST respond 404 Not Found to requests with a terminated session id.

Why the request still reaches the handler. On the Protocol side, Protocol._onclose (src/shared/protocol.ts:644) sets this._transport = undefined but never detaches transport.onmessage (attached at protocol.ts:628), so the closed transport still dispatches straight into the connected Server, whose _requestHandlers survive _onclose. _onrequest then captures capturedTransport = this._transport (protocol.ts:694) — which is undefined post-close — and every response send is optional-chained (capturedTransport?.send(response), protocol.ts:811). The tool handler executes its side effects on a terminated session, and the response is silently swallowed.

Resulting client-visible failure. Because transport.send() is never invoked, the allResponsesReady cleanup in send() never runs — and close() has already swept — so the freshly registered _streamMapping/_requestToStreamMapping entries and their live controller leak forever. The client gets HTTP 200 and an SSE stream that never carries a response and never ends. New with this PR: startKeepAlive now no-ops on _closed, so that zombie stream also carries zero keep-alive frames, making it indistinguishable from a healthy idle stream until an intermediary kills it. A GET with the old session id likewise gets 200 + a fresh standalone SSE stream registered on the dead transport.

Why nothing else prevents it. The only reuse guard in handleRequest is the stateless-mode _hasHandledRequest check, which is inert in stateful mode. Multi-session example apps dodge this at the routing layer by deleting the transport from their session map after onsessionclosed, but the class docstring's own single-transport deployments (Hono app.all, Cloudflare Workers fetch) route every request to the one instance — and the transport itself violates the spec's 404 requirement regardless.

Step-by-step proof (realistic race, no client bug required):
1. A client with an auto-reconnecting SSE stream (or a retried tool call) sends DELETE. handleDeleteRequest awaits onsessionclosed + close(); _closed = true, streams swept — but sessionId/_initialized unchanged.
2. A tick later, the client's queued reconnect GET or retried tools/call POST lands on the same transport instance with the old session id.
3. validateSession passes (id matches, _initialized true). For a POST, the SSE mapping is registered, onmessage dispatches into Protocol, and the tool handler runs its side effects on the terminated session.
4. capturedTransport?.send(response) is a no-op (_transport is undefined), so no response is written and no cleanup fires. The client holds a 200 SSE stream that never ends and — post-PR — never even sees a keep-alive frame. The stale mapping entries leak for the process lifetime.
Not a duplicate. This is distinct from the other findings on this PR: those concern close() racing an in-flight request (JSON-mode POST, replay await). This is a request arriving after close() completes and being fully accepted and executed.

Fix. At the top of handleRequest (or in validateSession), if this._closed, return the 404 Session not found JSON-RPC error via createJsonErrorResponse(404, -32001, 'Session not found') — a one-line use of the _closed flag this PR already adds. Alternatively (or additionally), clear sessionId/_initialized in close(). Note the multi-node onsessionclosed pattern is unaffected: in that pattern a closed transport never receives requests, so gating on _closed breaks nothing.

Severity: pre-existing. handleRequest never consulted closed state before this PR either; the PR only adds _closed = true to close() and uses it in startKeepAlive. It should not block merge — flagged because the PR introduces exactly the primitive that makes the fix trivial.
🟣 src/server/webStandardStreamableHttp.ts:975-980 — In JSON response mode (enableJsonResponse: true), the Promise<Response> returned by handlePostRequest can only settle via send()'s resolveJson call, but the stream's cleanup() only deletes the _streamMapping entry — so this close() sweep during an in-flight POST (e.g. a DELETE or server shutdown while a long-running tool call runs) drops the pending resolver and the client hangs on the socket forever. This is a pre-existing issue (the PR only adds the _closed flag here), flagged because this PR hardens exactly the close-during-in-flight lifecycle and JSON mode is the one stream mode left broken; the fix is for JSON-mode cleanup (or the close() sweep) to resolve pending resolveJson entries with a terminal response before deleting the mapping.
Extended reasoning...

Note: this is a pre-existing issue — the only change this PR makes to close() is the one-line _closed flag. It's surfaced here because the PR's entire purpose is hardening the close-during-in-flight-request lifecycle (close race, error-path leak), and JSON response mode is the one stream mode whose close behavior remains broken.

The bug. In JSON response mode, handlePostRequest returns new Promise<Response>(resolve => ...) and registers the stream as:
```
this._streamMapping.set(streamId, {
    resolveJson: resolve,
    cleanup: () => {
        this._streamMapping.delete(streamId);
    }
});
```
Note the asymmetry with SSE mode: the SSE cleanup closures call streamController.close(), so a swept SSE client at least observes stream end. The JSON-mode cleanup never calls resolve — it only deletes the map entry. resolveJson is invoked in exactly one place in the file: send()'s allResponsesReady branch, which requires this._streamMapping.get(streamId) to still return the entry.

The code path. close() does this._streamMapping.forEach(({ cleanup }) => cleanup()) and then this._streamMapping.clear(). For a pending JSON-mode POST this permanently drops the only resolver: the Promise<Response> returned from handleRequest never settles. The Node wrapper (StreamableHTTPServerTransport via @hono/node-server's getRequestListener) awaits that promise before writing anything to the socket, so no HTTP response is ever written and the client hangs until an external timeout.

Why existing safeguards don't catch it. The new _closed flag is checked only in startKeepAlive; nothing on the JSON path consults it. And the response isn't merely delayed — it's lost either way: close() does not clear _requestToStreamMapping, so if the tool handler later completes, send() resolves the streamId, finds _streamMapping.get(streamId) === undefined, computes allResponsesReady as true, and hits if (!stream) throw new Error('No connection established for request ID: ...'). The same defect is also reachable through the public closeSSEStream(requestId) API while in JSON mode, since it invokes the same non-resolving cleanup().

Step-by-step proof.
1. Construct the transport with enableJsonResponse: true and a session ID generator; initialize a session.
2. Client POSTs tools/call (id call-1) for a long-running tool. handlePostRequest registers { resolveJson: resolve, cleanup: delete-only } under streamId, maps call-1 → streamId, dispatches onmessage, and returns the pending Promise<Response>. The HTTP layer awaits it.
3. While the tool runs, the client sends DELETE (or the server shuts down). handleDeleteRequest awaits this.close(); the sweep calls the JSON entry's cleanup() — map entry deleted, resolve never called.
4. The POST's promise now has no reachable resolver. The client socket for the tools/call request hangs indefinitely.
5. When the tool finally completes, send() throws No connection established for request ID: call-1 — the response is unrecoverable.
Distinct from the three existing review comments, which cover the POST SSE path's map/controller leak, the SSE replay re-insertion after close, and the Infinity guard gap — none involve JSON response mode or a never-settling Promise<Response>; fixing all three leaves this hang intact.

Fix. Make the JSON-mode cleanup() settle the promise before deleting the mapping — e.g. resolve(new Response(null, { status: 503 })) or a JSON-RPC error body — and/or have close() resolve all pending resolveJson entries during its sweep, mirroring how SSE cleanup closes its controller.

Reject timer delays above Node's maximum in addition to NaN and Infinity, and keep the lifecycle regression focused on cleanup rather than the transport's pre-existing error-status mapping.

mattzcarey · 2026-07-24T11:57:33Z

Final review-prep update pushed in 15a556f3:

rejects NaN, Infinity, non-positive, and overflowing timer delays
replay-close continuation ends cleanly rather than re-registering on a closed transport
POST failures reclaim timers plus stream/request bookkeeping
regression tests stay focused on lifecycle cleanup rather than pinning the transport's pre-existing catch-all error status

Local verification: 167 transport tests, typecheck, lint, and ESM/CJS builds pass. Broader pre-existing transport lifecycle findings are explicitly documented as out of scope for this targeted #2541 hardening backport. Fresh CI is running; this will be marked ready for maintainer review when it is green.

claude

Beyond the inline findings, this pass also examined the new keepAliveMs guard for fractional values in (0, 1) — they pass the guard and Node clamps them to ~1ms, but that matches pre-PR behavior for an explicitly sub-millisecond value rather than an accidental NaN/Infinity/overflow, so it was ruled out as a regression.

Extended reasoning...

Bugs were found this run (one pre-existing send() resumption gap, one nit on the new reclaim closure) and are posted as inline comments, so no approval and no restatement of them here. This note only records the one additional candidate examined and refuted — a sub-1ms fractional keepAliveMs slipping past the hardened guard — so a later review pass doesn't re-explore it: such a value is an explicit user choice, behaves identically before and after this PR, and is outside the accidental-invalid-input class (NaN, Infinity, overflow) the guard targets.

Reject sub-millisecond intervals that Node clamps to 1ms and disable nginx-style buffering on GET, POST, and replay SSE responses so heartbeat frames reach clients.

mattzcarey · 2026-07-24T14:39:36Z

Quality review follow-up in cca65fc7:

rejects sub-millisecond delays that Node clamps to 1 ms
adds X-Accel-Buffering: no to GET, POST, and replay SSE responses so heartbeat frames reach clients behind nginx-style buffering
adds direct assertions for all three response forms

Local transport suite is 168/168; typecheck, lint, and ESM/CJS builds passed.

Reject work resuming after transport close, ownership-guard request-map cleanup, and align no-transform response headers across GET, POST, and replay streams.

claude

Beyond the inline findings, this pass also examined and ruled out three candidates in the new POST-path guards: (1) close() straddling the writePrimingEvent await — the close sweep's cleanup() already closes the registered controller and empties the mapping, and the controller-identity check prevents a late keep-alive arm; (2) the mid-POST _closed guard sitting after the initialize commit — the guard still blocks registration/dispatch, and the uncleared sessionId/_initialized state is the pre-existing rollback question already discussed and scoped out above; (3) JSON-response mode bypassing the new error-path reclaim — that branch returns before the fallible priming write, so no post-registration error path exists there.

Extended reasoning...

Two findings are posted inline this run (a pre-existing DELETE-retry double-teardown window in handleDeleteRequest, and a diagnostics nit about the new _closed 404s skipping onerror), so per policy this is an informational deferral, not a verdict. The note records three additional candidate issues in this PR's new code that were examined and refuted this run — none previously mentioned in the thread — so a later pass or the author does not re-derive them from scratch. It is not a guarantee of correctness and does not restate the inline findings.

claude · 2026-07-24T15:38:39Z

+            // Request parsing and session initialization may await user/runtime
+            // work. Do not register or dispatch after close() has swept state.
+            if (this._closed) {
+                return this.createJsonErrorResponse(404, -32001, 'Session not found');
+            }


🟡 The two new _closed rejection sites (the handleRequest front-door check and this post-parse re-check) return their 404 without invoking this.onerror, unlike every other rejection path in this file — including the semantically identical 404 -32001 'Session not found' in validateSession. This makes exactly the post-close race traffic this PR hardens against invisible to operators while indistinguishable stale-session-id 404s are still reported; adding this.onerror?.(new Error('Session not found')) (or a more specific message) before each return restores the convention.

Extended reasoning...

What the bug is. The transport has a uniform convention: every request rejection reports through this.onerror?.() before returning the error Response. All ~20 rejection paths in webStandardStreamableHttp.ts follow it — the DNS-rebinding 403s, the 406/415 header checks, the 400 parse errors, the 405, the 409 conflicts, all four validateSession/validateProtocolVersion errors (including the semantically identical 404 -32001 'Session not found': this.onerror?.(new Error('Session not found'))), both replayEvents error paths, and the final POST catch. The two _closed rejection sites added by this PR are the only exceptions: the front-door check at the top of handleRequest and the post-parse re-check in handlePostRequest both return createJsonErrorResponse(404, -32001, 'Session not found') silently.

The code path that triggers it. These guards fire on precisely the races this PR's lifecycle work targets: a client GET or POST racing a DELETE in the standard session-map pattern (the app looks up the transport before onclose evicts it), or a POST whose req.json() / onsessioninitialized await straddles close() — the PR's own new test ('should not register a POST stream after close races session initialization') exercises the second site.

Why existing code doesn't hide the gap. The silence is observable, not academic. Protocol.connect() wraps transport.onerror to forward into Protocol._onerror → the application's Server.onerror (src/shared/protocol.ts:621-625), and Protocol._onclose clears _transport but never detaches the transport object's onerror property — so post-close onerror calls still reach the application handler (and any transport-level onerror installed directly, as the SDK's own tests do). Nothing in the PR suggests the suppression is intentional: the new comments explain the 404 but not the missing report, and the new tests assert only the 404 status.

Impact. An operator debugging 'clients intermittently get Session not found' gets asymmetric telemetry: the rejection appears in onerror when it comes from validateSession (stale session id presented) but not when it comes from the new _closed guards (request hit a just-closed transport) — even though the two cases are byte-for-byte indistinguishable to the client, and the second is exactly the post-close traffic this PR is about.

Step-by-step proof. (1) App uses the standard session-map pattern; client holds session S. (2) Client issues DELETE for S while its reconnect logic concurrently re-opens the standalone GET stream. (3) DELETE wins: handleDeleteRequest → close() sets _closed = true; the app's map still holds the transport until onclose runs. (4) The racing GET reaches handleRequest, hits the new _closed guard, and returns 404 — onerror never fires; the app's error handler (still wired per protocol.ts:621-625) sees nothing. (5) Compare: the same client retrying a moment later against a new transport with a stale session id gets the same 404 via validateSession — and that one is reported. Same client-visible symptom, half the telemetry.

How to fix. One line at each site, mirroring validateSession:

if (this._closed) { this.onerror?.(new Error('Session not found')); // or 'Request received after transport close' return this.createJsonErrorResponse(404, -32001, 'Session not found'); }

Severity. Nit: the client receives the correct, spec-appropriate 404 either way, cleanup is correct, and nothing functionally breaks — this is purely a diagnostics/consistency gap in the PR's new code with a one-line fix per site. It should not block merge.

claude · 2026-07-24T15:38:39Z

+        if (this._closed) {
+            return this.createJsonErrorResponse(404, -32001, 'Session not found');
+        }


🟣 Pre-existing (not introduced by this PR): the new _closed front-door gate is inert for the entire duration of the onsessionclosed await in handleDeleteRequest, because _closed is only set at the top of close() — which runs after that await. A retried DELETE (idempotent per RFC 9110, so HTTP clients/proxies retry freely) arriving during a slow async onsessionclosed callback passes the gate and validateSession, and re-runs the whole teardown: onsessionclosed, close(), and onclose all fire twice. One-line completion of this PR's own idiom: set _closed (or a _closing flag checked by the gate) in handleDeleteRequest before awaiting _onsessionclosed.

Extended reasoning...

The bug. handleDeleteRequest runs await Promise.resolve(this._onsessionclosed?.(this.sessionId!)) before await this.close(), and this._closed = true is committed only at the top of close(). So for the entire wall-clock duration of the user callback — which the docs pitch as the place to "clean up resources associated with the session", i.e. real apps do async DB/Redis/billing work there — the transport is mid-teardown but _closed is still false. The PR's new front-door gate (handleRequest, lines 385-387) therefore protects only the post-close() portion of teardown, not the (potentially much longer) callback portion.

The triggering code path. DELETE is defined as idempotent by RFC 9110, so HTTP clients and proxies retry it freely on timeout. A retried DELETE arriving while the first is parked on the onsessionclosed await: (1) passes the _closed gate (false), (2) passes validateSession — sessionId and _initialized are never cleared anywhere, (3) passes validateProtocolVersion, and (4) re-enters handleDeleteRequest, invoking this._onsessionclosed a second time for the same session id. Both continuations then call close(), which has no reentrancy guard, so this.onclose?.() fires twice — double-invoking Protocol._onclose and the application's onclose handler. Both DELETEs return 200.

Step-by-step proof. (1) App configures onsessionclosed that deletes session rows in a slow database. (2) Client sends DELETE; server enters handleDeleteRequest, passes all validation, parks on the callback await. _closed is still false. (3) The client's HTTP layer times out and retries the DELETE (standard retry policy for an idempotent method). (4) The retry passes the front-door gate, validateSession, and validateProtocolVersion — nothing distinguishes it from a first DELETE — and invokes onsessionclosed again for the same session. (5) Both requests proceed to close(); the second sweep is empty, but onclose?.() runs unconditionally both times. Every per-session cleanup hook the SDK exposes (onsessionclosed, transport onclose, Protocol onclose) double-runs — apps that decrement connection counters, emit per-session audit/billing events, or release external resources double-run that logic.

Why this is pre-existing rather than introduced. handleDeleteRequest is untouched by this PR, and close()'s non-idempotency predates it. Before this PR there was no _closed flag at all, so two concurrent/retried DELETEs double-ran onsessionclosed/close()/onclose identically — and post-close requests weren't rejected either. The PR strictly narrows the vulnerable window (sequential post-close DELETEs now correctly 404). This is also not a duplicate of the other findings on this PR: bug_001/bug_002 concern POST-path awaits racing close(), and the earlier front-door-gate comment concerned requests arriving strictly after close() completes; here the second request arrives before close() runs, so none of those fixes prevent the double teardown.

Impact if left as-is. No SDK-internal corruption — the second sweep over the empty maps is harmless. The harm is at the application layer: double-invocation of session-close callbacks (counters, audit, billing, external resource release), plus Protocol._onclose's response-handler rejection sweep running twice. Merging this PR as-is is strictly no worse than pre-PR behavior.

How to fix. A one-line completion of the PR's own idiom: set this._closed = true (or a dedicated _closing flag that the front-door gate also checks) at the top of handleDeleteRequest before awaiting _onsessionclosed. A retried DELETE then gets the same 404 the gate already produces after close, and the session-close callbacks are guaranteed to run exactly once. Since the maintainer has scoped broader lifecycle changes out of this targeted backport, this works equally well as an immediate follow-up.

mattzcarey · 2026-07-24T17:05:08Z

Closing as superseded by #2547.

mattzcarey requested a review from a team as a code owner July 24, 2026 10:25

chore: add changeset

a333f8b