fix(client): recover streamable HTTP session on 404 for session-bound requests#1718
Conversation
🦋 Changeset detectedLatest commit: 0b7f219 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
Thanks for this. Could we simplify? The spec (Session Management, item 4) says:
When a client receives HTTP 404 in response to a request containing an
MCP-Session-Id, it MUST start a new session by sending a newInitializeRequestwithout a session ID attached.
The transport's piece is "without a session ID attached". Clear _sessionId so the next request omits the header. Client.connect() already handles the re-init when transport.sessionId is undefined (client.ts:497).
Suggested minimal change (both _startOrAuthSse and send, before the existing throw):
if (response.status === 404 && this._sessionId !== undefined) {
this._sessionId = undefined;
}# Conflicts: # packages/client/src/client/streamableHttp.ts
d2d6587 to
6078c54
Compare
|
Thanks for the guidance — updated to the minimal approach.
|
|
@claude review |
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
LGTM — both previously raised issues (changeset doc mismatch and post-response race on this._sessionId) are addressed in the latest commit.
Extended reasoning...
Overview
This PR adds session-bound 404 recovery to StreamableHTTPClientTransport in two locations: the POST path (_send) and the GET SSE path (_startOrAuthSse). Three files changed: the implementation, its tests, and a changeset entry.
Security risks
No security-sensitive code is touched. The change only affects how a stale session ID is cleared on HTTP 404; no auth credentials, token handling, or permission logic is modified.
Level of scrutiny
The change is small and surgical — four lines of production code across two methods plus the pre-fetch capture variable. The logic is straightforward and well-tested. Both previous review cycles caught real issues that were fully addressed: (1) changeset description now accurately reflects the implementation (no sessionExpired property), and (2) the 404 guard correctly uses sentWithSession = headers.has("mcp-session-id") captured before await fetch(), eliminating the concurrent-mutation race.
Other factors
The bug hunting system found no remaining issues. All inline comment threads are resolved. Tests cover the POST session-bound path, the GET session-bound path, and the non-session-bound 404 baseline. The implementation matches the spec requirement that a 404 for a session-carrying request signals an expired/invalid session.
Summary
Handle session-expired 404s in
StreamableHTTPClientTransportwhen requests are session-bound.What changed
send()(POST path), when a request is sent withmcp-session-idand receives404:SdkErrortagged withsessionExpired: true_startOrAuthSse()(GET path), apply the same 404 session-expiry handling.@modelcontextprotocol/client.Why
Per MCP session management requirements, a 404 for a request carrying
Mcp-Session-Idindicates an invalid/expired session. The client should not keep sending a stale session ID.Testing
pnpm --filter @modelcontextprotocol/client test -- test/client/streamableHttp.test.tspnpm exec prettier --check packages/client/src/client/streamableHttp.ts packages/client/test/client/streamableHttp.test.ts .changeset/gentle-maps-smile.mdCloses #1708