-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: send SSE keep-alive comment frames from Streamable HTTP server transport (v1.x) #2538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@modelcontextprotocol/sdk': patch | ||
| --- | ||
|
|
||
| `StreamableHTTPServerTransport` and `WebStandardStreamableHTTPServerTransport` now write SSE keep-alive comment frames (`: keepalive`) to open SSE streams so idle connections (e.g. the standalone GET stream, or a POST stream during a long-running tool call) are not killed by | ||
| intermediaries or server idle timeouts. Configurable via the new `keepAliveMs` option (default 15000; set 0 to disable). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,8 +146,24 @@ export interface WebStandardStreamableHTTPServerTransportOptions { | |
| * client reconnection timing for polling behavior. | ||
| */ | ||
| retryInterval?: number; | ||
|
|
||
| /** | ||
| * Interval in milliseconds between SSE keep-alive comment frames (`: keepalive`) | ||
| * written to open SSE streams. Keep-alive frames prevent idle streams (e.g. the | ||
| * standalone GET stream, or a POST stream during a long-running tool call) from | ||
| * being killed by intermediaries and server idle timeouts, which clients observe | ||
| * as `SSE stream disconnected: TypeError: terminated`. | ||
| * | ||
| * Comment frames are ignored by SSE parsers and never surface as messages. | ||
| * Defaults to 15000 (per the WHATWG SSE spec recommendation of roughly every | ||
| * 15 seconds). Set to 0 to disable keep-alive frames. | ||
| */ | ||
| keepAliveMs?: number; | ||
| } | ||
|
|
||
| /** Default interval between SSE keep-alive comment frames. */ | ||
| const DEFAULT_KEEP_ALIVE_MS = 15_000; | ||
|
|
||
| /** | ||
| * Options for handling a request | ||
| */ | ||
|
|
@@ -225,6 +241,8 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| private _allowedOrigins?: string[]; | ||
| private _enableDnsRebindingProtection: boolean; | ||
| private _retryInterval?: number; | ||
| private _keepAliveMs: number; | ||
| private _keepAliveTimers: Map<string, ReturnType<typeof setInterval>> = new Map(); | ||
|
|
||
| sessionId?: string; | ||
| onclose?: () => void; | ||
|
|
@@ -241,6 +259,43 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| this._allowedOrigins = options.allowedOrigins; | ||
| this._enableDnsRebindingProtection = options.enableDnsRebindingProtection ?? false; | ||
| this._retryInterval = options.retryInterval; | ||
| this._keepAliveMs = options.keepAliveMs ?? DEFAULT_KEEP_ALIVE_MS; | ||
| } | ||
|
|
||
| /** | ||
| * Arms a keep-alive interval for an SSE stream that periodically writes an SSE | ||
| * comment frame so intermediaries and idle timeouts don't kill the connection. | ||
| * Replaces any timer already armed for the same stream id (a resumed stream | ||
| * re-registered under the same id supersedes its predecessor's timer). The | ||
| * timer is cleared via stopKeepAlive when the stream is cleaned up, and | ||
| * clears itself if a write fails (stream already closed/cancelled). | ||
| */ | ||
| private startKeepAlive(streamId: string, controller: ReadableStreamDefaultController<Uint8Array>, encoder: TextEncoder): void { | ||
| if (this._keepAliveMs <= 0) { | ||
| return; | ||
| } | ||
| this.stopKeepAlive(streamId); | ||
| 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); | ||
| } | ||
|
|
||
| /** | ||
| * Clears the keep-alive interval for a stream, if one is armed. | ||
| */ | ||
| private stopKeepAlive(streamId: string): void { | ||
| const timer = this._keepAliveTimers.get(streamId); | ||
| if (timer !== undefined) { | ||
| clearInterval(timer); | ||
| this._keepAliveTimers.delete(streamId); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -425,6 +480,7 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| }, | ||
| cancel: () => { | ||
| // Stream was cancelled by client | ||
| this.stopKeepAlive(this._standaloneSseStreamId); | ||
| this._streamMapping.delete(this._standaloneSseStreamId); | ||
| } | ||
| }); | ||
|
|
@@ -445,6 +501,7 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| controller: streamController!, | ||
| encoder, | ||
| cleanup: () => { | ||
| this.stopKeepAlive(this._standaloneSseStreamId); | ||
| this._streamMapping.delete(this._standaloneSseStreamId); | ||
| try { | ||
| streamController!.close(); | ||
|
|
@@ -454,6 +511,8 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| } | ||
| }); | ||
|
|
||
| this.startKeepAlive(this._standaloneSseStreamId, streamController!, encoder); | ||
|
|
||
| return new Response(readable, { headers }); | ||
| } | ||
|
|
||
|
|
@@ -528,6 +587,7 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| controller: streamController!, | ||
| encoder, | ||
| cleanup: () => { | ||
| this.stopKeepAlive(replayedStreamId); | ||
| this._streamMapping.delete(replayedStreamId); | ||
| try { | ||
| streamController!.close(); | ||
|
|
@@ -537,6 +597,8 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| } | ||
| }); | ||
|
|
||
| this.startKeepAlive(replayedStreamId, streamController!, encoder); | ||
|
|
||
| return new Response(readable, { headers }); | ||
| } catch (error) { | ||
| this.onerror?.(error as Error); | ||
|
|
@@ -743,6 +805,7 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| }, | ||
| cancel: () => { | ||
| // Stream was cancelled by client | ||
| this.stopKeepAlive(streamId); | ||
| this._streamMapping.delete(streamId); | ||
| } | ||
| }); | ||
|
|
@@ -766,6 +829,7 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| controller: streamController!, | ||
| encoder, | ||
| cleanup: () => { | ||
| this.stopKeepAlive(streamId); | ||
| this._streamMapping.delete(streamId); | ||
| try { | ||
| streamController!.close(); | ||
|
|
@@ -778,6 +842,8 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| } | ||
| } | ||
|
|
||
| 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); | ||
|
Comment on lines
+842
to
845
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In the POST SSE path, Extended reasoning...The bugIn Why none of the PR's safety nets catch thisThe PR has three cleanup mechanisms for keep-alive timers, and this path defeats all of them:
Note that ImpactEach failed POST-with-request arms a new interval under a fresh Step-by-step proof
Why this is distinct from the earlier claude[bot] findingThe timeline comment describes the replay-path same-streamId timer overwrite; its suggested fix — FixEither arm the keep-alive only after |
||
|
|
||
|
|
@@ -901,6 +967,10 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { | |
| }); | ||
| this._streamMapping.clear(); | ||
|
|
||
| // Clear any keep-alive timers not already cleared by stream cleanup | ||
| this._keepAliveTimers.forEach(timer => clearInterval(timer)); | ||
| this._keepAliveTimers.clear(); | ||
|
|
||
| // Clear any pending responses | ||
| this._requestResponseMap.clear(); | ||
| this.onclose?.(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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 optionalgetStreamIdForEventId(including the SDK's own InMemoryEventStore). On a reconnect the orphaned timer's write eventually throws and its catch callsstopKeepAlive(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: callthis.stopKeepAlive(streamId)at the top ofstartKeepAlive().Extended reasoning...
The bug
startKeepAlive()(src/server/webStandardStreamableHttp.ts:271-285) unconditionally doesthis._keepAliveTimers.set(streamId, timer). If a timer is already registered under thatstreamId, the oldsetIntervalhandle is overwritten in the map but never cleared. Since bothstopKeepAlive()andclose()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:
replayEvents()only performs its 409 "Stream already has an active connection" check when the EventStore implements the optionalgetStreamIdForEventId()(lines 529-542). The SDK's ownInMemoryEventStore(src/examples/shared/inMemoryEventStore.ts) does not implement it, so servers built on it skip the conflict check entirely.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.Last-Event-IDfrom the same original stream re-entersreplayEvents()with the samereplayedStreamId. The second pass overwrites the_streamMappingentry (pre-existing behavior) andstartKeepAlive(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 callsstopKeepAlive, and POST streams use a freshcrypto.randomUUID()— so the flaw is confined to, but reliably triggered on, the replay path.Impact
Two concrete failures per reconnect cycle:
: keepaliveinto an abandoned controller (or throwing, see below). Leaks accumulate unboundedly across reconnects in a long-lived session.enqueuethrows and itscatchcallsthis.stopKeepAlive(streamId)— which clears timer B, the live resumed stream's keep-alive (they share the samestreamIdkey). 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
eventStore: new InMemoryEventStore()(nogetStreamIdForEventId). Client holds an event ID from streamS.GETwithLast-Event-ID→replayEvents()runs, no 409 check, mapsS, arms timer A forS.cancel()is a no-op → mapping forSand timer A both survive; the controller is now cancelled.Last-Event-IDstill resolving toS→replayEvents()runs again, mapsS(overwrite), callsstartKeepAlive(S, controllerB, …)→_keepAliveTimers.set(S, timerB)overwrites timer A without clearing it.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 (subsequentstopKeepAlive(S)calls are no-ops).Fix
One line — make
startKeepAliveidempotent per stream: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.