fix: send SSE keep-alive comment frames from Streamable HTTP server transport (v1.x)

[mattzcarey]

Fixes #1211

Problem

Idle SSE streams from the Streamable HTTP server transport are killed by idle-connection timeouts — Node's server.requestTimeout defaults to 300s, and reverse proxies / cloud LBs have similar watchdogs. The standalone GET stream is the worst case (it can sit silent forever), but POST response streams during long-running tool calls hit the same thing. Clients observe SSE stream disconnected: TypeError: terminated every ~5 minutes and enter a reconnect loop.

Fix

WebStandardStreamableHTTPServerTransport (and therefore the Node StreamableHTTPServerTransport wrapper) now writes an SSE comment frame (: keepalive) to every open SSE stream — standalone GET, POST response streams, and replay streams — on an interval.

• New transport option keepAliveMs, defaulting to 15000 (the WHATWG SSE spec recommends a comment line roughly every 15 seconds). Set 0 to disable. Naming matches v2's keepAliveMs on createMcpHandler.
• Comment frames are dropped by SSE parsers before event dispatch, so they never surface as protocol messages — default-on is wire-transparent to any compliant client.
• The timer is unref'd (where supported) so it never holds a Node process open, is cleared on stream cleanup/cancel and transport close(), and clears itself if a write fails on an already-closed stream.

This mirrors the fix we shipped for the same symptom in Cloudflare's agents MCP worker transport (cloudflare/agents#1583), where the edge closes idle SSE responses after ~5 minutes.

Tests

Added a WebStandardStreamableHTTPServerTransport SSE keep-alive suite (fake timers): frames on idle GET stream, custom interval, keepAliveMs: 0 disables, timers fully cleared on close, and frames on a POST SSE stream while a tool call is pending. Full suite (1611 tests), typecheck, and lint pass.

[changeset-bot]

🦋 Changeset detected

Latest commit: 2bdf8c5

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]

Open in StackBlitz

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

commit: 2bdf8c5

[claude]

🔴 startKeepAlive() registers a new interval with this._keepAliveTimers.set(streamId, timer) without clearing any existing timer for that streamId, so a second call for the same stream orphans the first interval — reachable via replayEvents(), where the 409 conflict check is skipped whenever the EventStore doesn't implement the optional getStreamIdForEventId (including the SDK's own InMemoryEventStore). On a reconnect the orphaned timer's write eventually throws and its catch calls stopKeepAlive(streamId), clearing the live resumed stream's timer — silently disabling keep-alive on exactly the resumability path this PR targets, while leaking one interval per reconnect. Fix: call this.stopKeepAlive(streamId) at the top of startKeepAlive().

Extended reasoning...

The bug

startKeepAlive() (src/server/webStandardStreamableHttp.ts:271-285) unconditionally does this._keepAliveTimers.set(streamId, timer). If a timer is already registered under that streamId, the old setInterval handle is overwritten in the map but never cleared. Since both stopKeepAlive() and close() only clear timers currently in the map, the old interval becomes permanently unreachable and runs for the life of the process.

The code path that triggers it

The trigger is the replay path, and it's realistic:

1. replayEvents() only performs its 409 "Stream already has an active connection" check when the EventStore implements the optional getStreamIdForEventId() (lines 529-542). The SDK's own InMemoryEventStore (src/examples/shared/inMemoryEventStore.ts) does not implement it, so servers built on it skip the conflict check entirely.
2. The replay stream's ReadableStream.cancel() handler is a no-op (// Cleanup will be handled by the mapping, lines 562-565) — so when the client disconnects, the mapping stays and the keep-alive timer stays armed.
3. Reconnecting with a Last-Event-ID from the same original stream re-enters replayEvents() with the same replayedStreamId. The second pass overwrites the _streamMapping entry (pre-existing behavior) and startKeepAlive(replayedStreamId, …) overwrites the timer entry (new in this PR), orphaning timer A.

Why nothing prevents it

The only defenses are the 409 check (skipped without getStreamIdForEventId) and cleanup on cancel (a no-op on the replay stream). The other call sites happen to be safe — the standalone GET has its own 409 guard and a cancel handler that calls stopKeepAlive, and POST streams use a fresh crypto.randomUUID() — so the flaw is confined to, but reliably triggered on, the replay path.

Impact

Two concrete failures per reconnect cycle:

• Leaked interval: orphaned timer A keeps firing every 15s forever, enqueueing : keepalive into an abandoned controller (or throwing, see below). Leaks accumulate unboundedly across reconnects in a long-lived session.
• Keep-alive silently disabled on the live stream: once the abandoned stream's controller is cancelled, timer A's enqueue throws and its catch calls this.stopKeepAlive(streamId) — which clears timer B, the live resumed stream's keep-alive (they share the same streamId key). The resumed stream then goes idle and gets killed by the very proxy/idle timeouts this PR exists to defeat — reintroducing SSE stream disconnected: TypeError: terminated #1211 precisely on the resumability path.

Step-by-step proof

1. Server uses eventStore: new InMemoryEventStore() (no getStreamIdForEventId). Client holds an event ID from stream S.
2. Client sends GET with Last-Event-ID → replayEvents() runs, no 409 check, maps S, arms timer A for S.
3. Client's connection drops (proxy timeout, network blip). The replay stream's cancel() is a no-op → mapping for S and timer A both survive; the controller is now cancelled.
4. Client reconnects with a Last-Event-ID still resolving to S → replayEvents() runs again, maps S (overwrite), calls startKeepAlive(S, controllerB, …) → _keepAliveTimers.set(S, timerB) overwrites timer A without clearing it.
5. ≤15s later, timer A fires: controllerA.enqueue() throws (cancelled stream) → catch → stopKeepAlive(S) → clearInterval(timerB) and deletes the map entry. The live resumed stream now has no keep-alive; timer A keeps firing and throwing forever (subsequent stopKeepAlive(S) calls are no-ops).
6. Each further reconnect repeats steps 2-5, orphaning one more permanent interval.

Fix

One line — make startKeepAlive idempotent per stream:

private startKeepAlive(streamId: string, controller: ..., encoder: TextEncoder): void {
    if (this._keepAliveMs <= 0) {
        return;
    }
    this.stopKeepAlive(streamId);   // clear any existing timer for this stream
    const timer = setInterval(() => { ... });
    ...
}

This is severity-normal rather than a nit because the failure isn't just a resource leak: the PR's own fix silently stops working in its target scenario (long-lived sessions behind idle-killing intermediaries, using resumability), and nothing surfaces the breakage — the client just resumes seeing SSE stream disconnected: TypeError: terminated.

[mattzcarey]

Good catch — fixed: startKeepAlive now clears any existing timer for the stream id before arming a new one (making it idempotent per stream), with a regression test covering the replay-reconnect path with an event store that lacks getStreamIdForEventId. Also fixed the changeset formatting that failed the build.

[claude]

🔴 In the POST SSE path, startKeepAlive(streamId, ...) is armed immediately before await this.writePrimingEvent(...), which can reject when eventStore.storeEvent() fails; the rejection lands in the outer catch, which returns a 400 without calling stopKeepAlive(streamId) — and since the Response(readable) is never returned, the stream has no consumer, so the timer's enqueue never throws and its self-cleanup catch never fires. Each failed POST leaks a permanently-firing interval (fresh UUID streamId per request) that enqueues : keepalive into an unconsumed queue every 15s until transport close. Fix: arm the keep-alive only after writePrimingEvent succeeds, or call stopKeepAlive(streamId)/the mapping cleanup in the catch before returning the error response.

Extended reasoning...

The bug

In handlePostRequest's SSE branch, this.startKeepAlive(streamId, streamController!, encoder) (line 842) is called immediately before await this.writePrimingEvent(streamController!, encoder, streamId, clientProtocolVersion) (line 845). writePrimingEvent awaits this._eventStore.storeEvent(streamId, {}) — an async call into user-provided storage (Redis, a database, etc.) that can reject under transient failure. When it does, the rejection propagates to the outer catch (lines 870–874), which returns a 400 JSON error response without calling stopKeepAlive(streamId) or running the stream mapping's cleanup(). The interval armed three lines earlier keeps firing forever.

Why none of the PR's safety nets catch this

The PR has three cleanup mechanisms for keep-alive timers, and this path defeats all of them:

1. The timer's self-cleanup catch requires controller.enqueue() to throw. But the Response(readable) is never returned to the caller on this path — the 400 error response is returned instead — so the ReadableStream has no consumer and is never closed, errored, or cancelled. Per the Streams spec, enqueue() on a live controller always succeeds (the internal queue is unbounded; desiredSize just goes negative). The : keepalive chunk is appended to an unconsumed queue every 15s, forever, and the catch never fires.
2. cancel() / mapping cleanup() never run: nobody holds the response, so cancel() can't be invoked; and cleanup() is only reached via send()'s all-responses-ready branch or the closeSSEStream callbacks built in the onmessage dispatch loop (line 848+) — which is after the throwing await, so no handler ever receives these messages and no response is ever produced for these request ids.
3. transport.close() does clear _keepAliveTimers, but only at session end — a stateful session can live for hours or days.

Note that unref() only prevents the timer from holding the Node process open; it does not stop the interval from firing or free anything.

Impact

Each failed POST-with-request arms a new interval under a fresh crypto.randomUUID() streamId, so leaks accumulate one per failed request. A client retry loop against a flapping event store leaks an interval per retry, each retaining its controller, encoder, and an ever-growing chunk queue — an unbounded timer + memory leak on a long-lived transport. This is new in this PR: pre-PR, a storeEvent rejection on this path leaked only inert _streamMapping/_requestToStreamMapping entries; the actively-firing interval and unbounded enqueue are introduced here.

Step-by-step proof

1. Server: stateful transport with eventStore backed by Redis; Redis has a transient outage.
2. Client (protocol version >= 2025-11-25, so writePrimingEvent actually calls storeEvent) POSTs a tools/call request.
3. handlePostRequest reaches the SSE branch: creates readable, sets the stream mapping, calls startKeepAlive(streamId, ...) — interval armed.
4. await this.writePrimingEvent(...) → await this._eventStore.storeEvent(...) rejects.
5. Outer catch returns createJsonErrorResponse(400, -32700, 'Parse error', ...). The readable is abandoned with zero consumers; no stopKeepAlive, no cleanup().
6. Every 15s the interval enqueues : keepalive into the unconsumed queue; enqueue succeeds, so the self-cleanup catch never runs.
7. Client retries → steps 3–6 repeat with a fresh UUID → a second leaked interval. Repeat for the duration of the outage.
8. All leaked intervals fire until transport.close(), potentially hours/days later.

Why this is distinct from the earlier claude[bot] finding

The timeline comment describes the replay-path same-streamId timer overwrite; its suggested fix — stopKeepAlive(streamId) at the top of startKeepAlive() — does not fix this bug, because here every leaked timer has a fresh UUID streamId that is never passed to startKeepAlive again.

Fix

Either arm the keep-alive only after writePrimingEvent succeeds (moving line 842 below line 845 — the other two call sites already follow this arm-last pattern), or wrap the post-arm section so the catch calls stopKeepAlive(streamId) (or the mapping's cleanup()) before returning the error response.