| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…ransport Idle SSE streams (the standalone GET stream in particular, but also POST response streams during long-running tool calls) are killed by intermediaries and server idle timeouts (Node's requestTimeout defaults to 300s), which clients observe as "SSE stream disconnected: TypeError: terminated" roughly every 5 minutes, followed by a reconnect loop. The server transport now writes an SSE comment frame (`: keepalive`) to every open SSE stream every keepAliveMs milliseconds (default 15000, per the WHATWG SSE spec recommendation; set 0 to disable). Comment frames are dropped by SSE parsers and never surface as protocol messages. The timer is unref'd so it never holds the process open, and is cleared on stream cleanup/cancel and transport close. Fixes #1211
🦋 Changeset detectedLatest 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 |
Sorry, something went wrong.
npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2538 commit: 2bdf8c5 |
Sorry, something went wrong.
| private startKeepAlive(streamId: string, controller: ReadableStreamDefaultController<Uint8Array>, encoder: TextEncoder): void { | ||
| if (this._keepAliveMs <= 0) { | ||
| return; | ||
| } | ||
| const timer = setInterval(() => { | ||
| try { | ||
| controller.enqueue(encoder.encode(': keepalive\n\n')); | ||
| } catch { | ||
| this.stopKeepAlive(streamId); | ||
| } | ||
| }, this._keepAliveMs); | ||
| // Don't let the keep-alive timer hold the process open (Node.js only) | ||
| (timer as { unref?: () => void }).unref?.(); | ||
| this._keepAliveTimers.set(streamId, timer); | ||
| } |
There was a problem hiding this comment.
🔴 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...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 trigger is the replay path, and it's realistic:
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.
Two concrete failures per reconnect cycle:
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.
Sorry, something went wrong.
…er the same id A replayed stream can re-register under the same stream id when the event store doesn't implement the optional getStreamIdForEventId (the 409 conflict check is skipped). startKeepAlive now clears any timer already armed for the stream id before arming a new one, so the predecessor's timer can't leak or — via its failing write's stopKeepAlive on the shared id — silently disable the live stream's keep-alive. Also formats the changeset.
|
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. |
Sorry, something went wrong.
| this.startKeepAlive(streamId, streamController!, encoder); | ||
|
|
||
| // Write priming event if event store is configured (after mapping is set up) | ||
| await this.writePrimingEvent(streamController!, encoder, streamId, clientProtocolVersion); |
There was a problem hiding this comment.
🔴 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...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.
The PR has three cleanup mechanisms for keep-alive timers, and this path defeats all of them:
Note that unref() only prevents the timer from holding the Node process open; it does not stop the interval from firing or free anything.
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.
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.
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.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.
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.