-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(client): align Streamable HTTP session-id lifecycle with the spec #2466
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
c403c96
ec450a1
ebe1bde
af924bb
2602078
0c0aaf2
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,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| `StreamableHTTPClientTransport` session-id handling now follows the spec's assignment model: the `Mcp-Session-Id` response header is captured only from a successful response to a POST that carried an `InitializeRequest` — the spec assigns the id "at initialization time ... on the HTTP response containing the `InitializeResult`", and is silent on the header elsewhere. Session headers on any other response are now ignored entirely (previously any 2xx reply could install one), so an error reply, a late reply from an older session, or a stray header on a normal request contributes nothing to session state. Previously a 404 probe reply carrying a session id made the legacy fallback `initialize` present an id the server never issued, which strict stateful servers reject. A 404 to a POST that carried the current id now clears it (the session is gone per spec), and `terminateSession()` treats 404 as successful termination. On a legacy-era connection the client then starts a new session automatically — a fresh session-less `InitializeRequest`, per the spec's session-management requirement — and retries the failed request or notification once (pagination continuations re-establish the session but surface the 404 — the dead session's cursor is meaningless to the new one); server-side per-session state (e.g. resource subscriptions) does not carry over. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,9 @@ | |
| LoggingLevel, | ||
| MessageExtraInfo, | ||
| NonCompleteResultFlow, | ||
| Notification, | ||
| NotificationMethod, | ||
| NotificationOptions, | ||
| ProtocolEra, | ||
| ProtocolOptions, | ||
| ReadResourceRequest, | ||
|
|
@@ -43,6 +45,7 @@ | |
| RequestOptions, | ||
| ResolvedInputRequiredDriverConfig, | ||
| Result, | ||
| ResultTypeMap, | ||
| ServerCapabilities, | ||
| StandardSchemaV1, | ||
| SubscribeRequest, | ||
|
|
@@ -74,6 +77,7 @@ | |
| scanXMcpHeaderDeclarations, | ||
| SdkError, | ||
| SdkErrorCode, | ||
| SdkHttpError, | ||
| SUBSCRIPTION_ID_META_KEY, | ||
| SUPPORTED_MODERN_PROTOCOL_VERSIONS | ||
| } from '@modelcontextprotocol/core-internal'; | ||
|
|
@@ -897,6 +901,92 @@ | |
| } | ||
| } | ||
|
|
||
| private _sessionReinit?: Promise<void>; | ||
|
|
||
| override request<M extends RequestMethod>( | ||
| request: { method: M; params?: Record<string, unknown> }, | ||
| options?: RequestOptions | ||
| ): Promise<ResultTypeMap[M]>; | ||
| override request<T extends StandardSchemaV1>( | ||
| request: { method: string; params?: Record<string, unknown> }, | ||
| resultSchema: T, | ||
| options?: RequestOptions | ||
| ): Promise<StandardSchemaV1.InferOutput<T>>; | ||
| override async request( | ||
| request: { method: string; params?: Record<string, unknown> }, | ||
| schemaOrOptions?: StandardSchemaV1 | RequestOptions, | ||
| maybeOptions?: RequestOptions | ||
| ): Promise<unknown> { | ||
|
claude[bot] marked this conversation as resolved.
|
||
| try { | ||
| return await super.request(request as never, schemaOrOptions as never, maybeOptions); | ||
| } catch (error) { | ||
| // Spec (Session Management): a 404 to a request that carried the | ||
| // session id means the session is gone — start a new one with a | ||
| // fresh InitializeRequest, then retry the failed request once. | ||
| if (request.method === 'initialize' || !this._isSessionTerminated(error)) { | ||
| throw error; | ||
| } | ||
| await this._recoverSession(error); | ||
| // A cursor minted by the dead session is meaningless to the new | ||
| // one: the session is recovered, but the 404 surfaces so the | ||
| // caller restarts pagination. | ||
| if (request.params !== undefined && 'cursor' in request.params) { | ||
| throw error; | ||
| } | ||
| return await super.request(request as never, schemaOrOptions as never, maybeOptions); | ||
|
Check warning on line 936 in packages/client/src/client/client.ts
|
||
|
Comment on lines
+930
to
+936
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. 🟡 The default no-cursor Extended reasoning...What happens. The cursor guard in the new The code path, step by step.
Why this is a gap in the PR's own design, not just documented behavior. The changeset does say "pagination continuations re-establish the session but surface the 404," but the guard's justification — the caller restarts pagination — presumes the caller can. For the aggregate walk, the caller is Impact and why it's a nit. The failure is transient and self-healing: the session is re-established as part of the failed call, so an immediate host retry of How to fix. In |
||
| } | ||
| } | ||
|
|
||
| override async notification(notification: Notification, options?: NotificationOptions): Promise<void> { | ||
| try { | ||
| return await super.notification(notification, options); | ||
| } catch (error) { | ||
| // `notifications/initialized` is part of the recovery handshake | ||
| // itself — recovering on it would await the in-flight recovery. | ||
| if (notification.method === 'notifications/initialized' || !this._isSessionTerminated(error)) { | ||
| throw error; | ||
| } | ||
| await this._recoverSession(error); | ||
| return await super.notification(notification, options); | ||
| } | ||
| } | ||
|
Comment on lines
+940
to
+952
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. 🔴 Two send paths escape the new session-recovery fence and end up POSTing session-less, which the SDK's own stateful server answers with a 400 that can never be marked Extended reasoning...Path 1 — debounced notifications bypass the |
||
|
|
||
|
Check failure on line 953 in packages/client/src/client/client.ts
|
||
|
Comment on lines
+941
to
+953
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. 🔴 This Extended reasoning...What the bug is. The new The trigger is routine — no opt-in needed. Step-by-step proof.
Why this is a regression from this PR. Pre-PR, a 404 to the cancel POST was harmless: session state stayed intact, every later request kept carrying Not a duplicate. The existing comment on this override flags two escape paths — the How to fix. Either route the cancel-notification send's failure through the same recovery/marking-aware path (a Protocol-level hook the Client can override, mirroring how the awaited notification path was fixed in 2602078), or — the option that closes all three sibling paths at once — lazily re-initialize on the next |
||
| private async _recoverSession(cause: unknown): Promise<void> { | ||
| this._sessionReinit ??= this._reinitializeSession().finally(() => { | ||
| this._sessionReinit = undefined; | ||
| }); | ||
| try { | ||
| await this._sessionReinit; | ||
| } catch (reinitError) { | ||
| // Re-establishment failed (the client is closed at this | ||
| // point): surface the request's own 404; the handshake | ||
| // failure goes to onerror. | ||
| this.onerror?.(reinitError instanceof Error ? reinitError : new Error(String(reinitError))); | ||
| throw cause; | ||
| } | ||
| } | ||
|
|
||
|
Check warning on line 968 in packages/client/src/client/client.ts
|
||
|
Comment on lines
+955
to
+968
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. 🟡 When N concurrent requests/notifications are deduped into one shared recovery handshake via Extended reasoning...What the bug is. The code path that triggers it. This is exactly the scenario the dedup exists for: (1) session S1 expires server-side; (2) N in-flight requests/notifications each get a terminated-session 404; (3) all N enter Step-by-step proof. Take the PR's own dedup test ( Why nothing prevents it. There is no Impact — why this is a nit, not blocking. Nothing functionally breaks: the client closes once, every waiter still rejects with its own 404, and How to fix. Report once at the source instead of per-waiter — e.g. when creating this._sessionReinit ??= this._reinitializeSession()
.catch(e => {
this.onerror?.(e instanceof Error ? e : new Error(String(e)));
throw e;
})
.finally(() => { this._sessionReinit = undefined; });and reduce each waiter's catch to just |
||
| private _isSessionTerminated(error: unknown): boolean { | ||
| return ( | ||
| error instanceof SdkHttpError && | ||
| error.data.status === 404 && | ||
| error.data['sessionTerminated'] === true && | ||
| // Legacy era only: the 2026-07-28 handshake is discover-based. | ||
| this._negotiatedProtocolVersion !== undefined && | ||
| legacyProtocolVersions([this._negotiatedProtocolVersion]).length === 1 | ||
| ); | ||
|
Comment on lines
+969
to
+977
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. 🔴 The new session recovery never arms on session-resume reconnects: Extended reasoning...What the bug is. The recovery gate |
||
| } | ||
|
|
||
| private async _reinitializeSession(): Promise<void> { | ||
| const transport = this.transport; | ||
| if (transport === undefined) { | ||
| throw new SdkError(SdkErrorCode.ConnectionClosed, 'Cannot reinitialize: not connected'); | ||
| } | ||
| // No caller options: the handshake is shared connection state, so no | ||
| // single request's signal/timeout/onprogress may govern it. | ||
| await this._legacyHandshake(transport); | ||
| } | ||
|
|
||
| /** | ||
| * Connects to a server via the given transport and performs the MCP initialization handshake. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { | |
| createFetchWithInit, | ||
| encodeMcpParamValue, | ||
| isInitializedNotification, | ||
| isInitializeRequest, | ||
| isJSONRPCErrorResponse, | ||
| isJSONRPCRequest, | ||
| isJSONRPCResultResponse, | ||
|
|
@@ -315,13 +316,15 @@ export class StreamableHTTPClientTransport implements Transport { | |
| private _fetch?: FetchLike; | ||
| private _fetchWithInit: FetchLike; | ||
| private _sessionId?: string; | ||
| private _terminatedSessionId?: string; | ||
| private _reconnectionOptions: StreamableHTTPReconnectionOptions; | ||
| private _protocolVersion?: string; | ||
| private _onInsufficientScope: 'reauthorize' | 'throw'; | ||
| private _maxStepUpRetries: number; | ||
| private _serverRetryMs?: number; // Server-provided retry delay from SSE retry field | ||
| private readonly _reconnectionScheduler?: ReconnectionScheduler; | ||
| private _cancelReconnection?: () => void; | ||
| private _pendingReconnectOptions?: StartSSEOptions; | ||
|
|
||
| onclose?: () => void; | ||
| onerror?: (error: Error) => void; | ||
|
|
@@ -652,6 +655,7 @@ export class StreamableHTTPClientTransport implements Transport { | |
|
|
||
| const reconnect = (): void => { | ||
| this._cancelReconnection = undefined; | ||
| this._pendingReconnectOptions = undefined; | ||
| // Honour BOTH the transport-wide abort and the per-request abort | ||
| // (a listen subscription closed during the backoff delay): do not | ||
| // resurrect a stream the caller already tore down. | ||
|
|
@@ -674,6 +678,7 @@ export class StreamableHTTPClientTransport implements Transport { | |
| const handle = setTimeout(reconnect, delay); | ||
| this._cancelReconnection = () => clearTimeout(handle); | ||
| } | ||
| this._pendingReconnectOptions = options; | ||
| } | ||
|
|
||
| private _handleSseStream(stream: ReadableStream<Uint8Array> | null, options: StartSSEOptions, isReconnectable: boolean): void { | ||
|
|
@@ -971,15 +976,46 @@ export class StreamableHTTPClientTransport implements Transport { | |
| signal | ||
| }; | ||
|
|
||
| // 404s are judged against the id this request carried — late | ||
| // replies from an older session race `_sessionId`. | ||
| const sentSessionId = headers.get('mcp-session-id'); | ||
|
|
||
| const response = await (this._fetch ?? fetch)(this._url, init); | ||
|
|
||
| // Handle session ID received during initialization | ||
| const sessionId = response.headers.get('mcp-session-id'); | ||
| if (sessionId) { | ||
| this._sessionId = sessionId; | ||
| // Spec: the session id is assigned "at initialization time ... on the HTTP | ||
| // response containing the InitializeResult" — non-handshake echoes are ignored. | ||
| if (response.ok && (Array.isArray(message) ? message.some(m => isInitializeRequest(m)) : isInitializeRequest(message))) { | ||
| const sessionId = response.headers.get('mcp-session-id'); | ||
| if (sessionId) { | ||
| this._sessionId = sessionId; | ||
| this._terminatedSessionId = undefined; | ||
| } | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| // 404 to the current session id: session is gone (spec) — | ||
| // drop it. POSTs only; SSE reconnect GETs 404 transiently. | ||
| // `_terminatedSessionId` marks raced in-flight requests of the | ||
| // same dead session until a new one is assigned; the stale | ||
| // version header is dropped so the recovery initialize goes | ||
| // out like a fresh connect. | ||
| let sessionTerminated = false; | ||
| if (response.status === 404 && sentSessionId !== null && sentSessionId === this._sessionId) { | ||
| this._terminatedSessionId = sentSessionId; | ||
| this._sessionId = undefined; | ||
| this._protocolVersion = undefined; | ||
| // A reconnect pending for the dead session's SSE stream | ||
| // would fire into the new session with a stale | ||
| // last-event-id — drop it, and settle any per-request | ||
| // caller waiting on that stream (it is definitively gone). | ||
| this._cancelReconnection?.(); | ||
| this._cancelReconnection = undefined; | ||
| this._pendingReconnectOptions?.onRequestStreamEnd?.(); | ||
|
Comment on lines
+1005
to
+1013
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. 🟡 The stale-reconnect teardown added on the 404 session-clear is single-slot while multiple SSE reconnects can be pending at once: Extended reasoning...What the bug is. The 404 teardown in The escape paths. Two orderings escape the teardown:
Why nothing else stops the escaped reconnect. The reconnect closure gates only on the transport-wide abort and the per-request Step-by-step proof. (1) Legacy-era connect under session S1; standalone GET stream A open; a long-running One correction to the raw finding, and why this is a nit. The overwritten per-request caller is not left waiting forever against a conforming server: the orphaned timer still fires, its GETs fail, retries exhaust at How to fix. Track pending reconnects in a |
||
| this._pendingReconnectOptions = undefined; | ||
| sessionTerminated = true; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| } else if (response.status === 404 && sentSessionId !== null && sentSessionId === this._terminatedSessionId) { | ||
| sessionTerminated = true; | ||
| } | ||
| if (response.status === 401 && this._authProvider) { | ||
| // Store WWW-Authenticate params for interactive finishAuth() path | ||
| if (response.headers.has('www-authenticate')) { | ||
|
|
@@ -1060,7 +1096,8 @@ export class StreamableHTTPClientTransport implements Transport { | |
| throw new SdkHttpError(SdkErrorCode.ClientHttpNotImplemented, `Error POSTing to endpoint: ${text}`, { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| text | ||
| text, | ||
| ...(sessionTerminated ? { sessionTerminated: true } : {}) | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -1145,8 +1182,9 @@ export class StreamableHTTPClientTransport implements Transport { | |
| * HTTP `DELETE` to the MCP endpoint with the `Mcp-Session-Id` header to explicitly | ||
| * terminate the session. | ||
| * | ||
| * The server MAY respond with HTTP `405 Method Not Allowed`, indicating that | ||
| * the server does not allow clients to terminate sessions. | ||
| * A `405 Method Not Allowed` (server does not allow client termination) | ||
| * or `404 Not Found` (session already expired) response also resolves; | ||
| * the stored session id is cleared in every resolving case. | ||
| */ | ||
| async terminateSession(): Promise<void> { | ||
| if (!this._sessionId) { | ||
|
|
@@ -1166,9 +1204,9 @@ export class StreamableHTTPClientTransport implements Transport { | |
| const response = await (this._fetch ?? fetch)(this._url, init); | ||
| await response.text?.().catch(() => {}); | ||
|
|
||
| // We specifically handle 405 as a valid response according to the spec, | ||
| // meaning the server does not support explicit session termination | ||
| if (!response.ok && response.status !== 405) { | ||
| // 405: server does not support explicit termination (spec). | ||
| // 404: session already gone — the goal state; still drop the id. | ||
| if (!response.ok && response.status !== 405 && response.status !== 404) { | ||
| throw new SdkHttpError( | ||
| SdkErrorCode.ClientHttpFailedToTerminateSession, | ||
| `Failed to terminate session: ${response.statusText}`, | ||
|
|
||
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.
🟡 The closing sentence of the new session-
404bullet — "Host code keyed off that404now only sees it when the automatic recovery has also failed, at which point the client is closed" (echoed at line ~1627) — is false for pagination continuations: per the cursor guard inClient.request, a cursor-carrying request surfaces the original 404 even when recovery succeeded, with the client open and fully usable (the PR's own test asserts a follow-uptools/listsucceeds after the surfaced 404). A host following the guide as written would tear down a healthy, just-recovered client on every cursor-page session expiry; qualify both sentences to carve out the pagination case.Extended reasoning...
What the docs claim vs. what the code does. The new migration-guide bullet (
docs/migration/upgrade-to-v2.md:1569-1570) ends with: "Host code keyed off that404now only sees it when the automatic recovery has also failed, at which point the client is closed." The same claim is repeated at :1625-1627: "hosts only observe it when that recovery fails." But the code has a third outcome the sentence doesn't cover: in theClient.requestoverride (packages/client/src/client/client.ts:923-935),await this._recoverSession(error)runs before the cursor guard, and only after recovery succeeds does the'cursor' in request.paramscheck rethrow the original 404. So for a pagination continuation the host observes a terminated-session 404 while the client is open, recovered, and fully usable.The code path, concretely:
client.listTools()returnsnextCursor; the session then expires server-side.client.request({ method: 'tools/list', params: { cursor } })POSTs with the dead session id → 404 withsessionTerminated: true.await this._recoverSession(error)— a fresh session-lessinitialize/initializedhandshake succeeds; the transport now holds a new live session id.The PR's own test pins this.
sessionReinitialize.test.ts's "recovers the session on a pagination continuation but does not retry it" asserts exactly the sequence above: the cursor request rejects with the 404, the wire trace shows['initialize', 'tools/list', 'initialize'](recovery ran), and a follow-uptools/liston the same client then succeeds. The changeset (.changeset/shttp-session-capture-on-ok.md) even describes it accurately — "pagination continuations re-establish the session but surface the 404" — so the migration guide contradicts the PR's own more careful wording. The bullet's earlier parenthetical ("pagination continuations are not retried") hints at the exception, but the closing sentence then asserts the opposite unconditionally.Why it matters. The migration guide is the document hosts are told to code against (the :1623-1625 prose explicitly directs client code to "key off the HTTP
404status"). A host that follows the sentence literally — "a surfaced terminated-session 404 means recovery failed and the client is closed, so rebuild client + transport" — will discard a healthy, just-recovered client every time a cursor page hits session expiry. That is precisely the reconnect churn the automatic recovery feature was added to eliminate, and mid-pagination expiry is a realistic ordering (long list walks are exactly when a session TTL is likely to lapse). Note this isn't confined to manual pagination:_listAllPages(behind the default no-cursorlistTools/listPrompts/listResourcesaggregate walk) issues{...params, cursor}requests for pages 2+, so the most common list path can surface this 404 with the client open.Why nit, not blocking. This is docs-only — nothing in the SDK misbehaves at runtime; the damage requires a host to code to the sentence literally, and the fix is a one-line qualification in each of the two places.
How to fix. Qualify both sentences, e.g. at :1569-1570: "Host code keyed off that
404now only sees it when the automatic recovery has also failed (the client is closed at that point) — or when the failed request was a pagination continuation, in which case the session has been re-established and the client remains usable; restart pagination from the first page." And at :1627: "hosts only observe it when that recovery fails, or on a pagination continuation (session recovered; restart pagination)."