fix(client): treat HTTP 404 with session ID as session expiry#2125
fix(client): treat HTTP 404 with session ID as session expiry#2125dsp-ant wants to merge 2 commits into
Conversation
Per the MCP spec (Streamable HTTP, Session Management), when a client receives an HTTP 404 in response to a request that carried an Mcp-Session-Id, the session has expired or been terminated server-side and the client must start a new session. StreamableHTTPClientTransport previously surfaced every non-401/403 error status — including 404 — as a generic ClientHttpNotImplemented (POST) or ClientHttpFailedToOpenStream (GET) error, with no way to distinguish session expiry from other failures. Consumers were left matching the response body, which only works against the reference server; servers that report expiry with a different body (e.g. a -32002 JSON-RPC code, or a plain-text/HTML proxy response) slipped through. Detect session expiry by status code alone, scoped to requests that actually carried a session ID: on a 404 when a session ID is set, clear the stale session ID (so a subsequent connect() issues a fresh initialize) and throw SdkError with the new SdkErrorCode.ClientHttpSessionExpired. A 404 without a session ID (e.g. wrong URL on initial connect) is unchanged and still surfaces as ClientHttpNotImplemented. Applied to both the POST (_send) and standalone GET SSE (_startOrAuthSse) paths; documents the new code in the migration guides.
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… in terminateSession Snapshot whether the request carried a session ID before the fetch, so the 404 session-expiry check reflects the request rather than post-response state (the response handler may write a new mcp-session-id into _sessionId). Apply the same snapshot in the GET path for symmetry. terminateSession() now treats a 404 like the existing 405 case: the session is already gone server-side, which is the caller's intent, so it clears the session ID and resolves instead of throwing ClientHttpFailedToTerminateSession. Add tests for the GET-stream 404 path (distinct error data shape) and the terminateSession 404 path.
|
Thanks for the review — addressed all three in 6cc7726:
Full workspace typecheck/build/lint pass. |
There was a problem hiding this comment.
Round 1 feedback fully addressed in 6cc7726 — request-side session-ID snapshot, terminateSession() 404-as-success, and GET-path test coverage all look correct. Only doc nits remain; note the changeset bot is still flagging a missing changeset for this behavior change in the client package.
Extended reasoning...
Overview
This PR adds spec-mandated handling for HTTP 404 responses on requests that carried an Mcp-Session-Id: the transport clears the stale session ID and surfaces a new SdkErrorCode.ClientHttpSessionExpired. Changes span packages/client/src/client/streamableHttp.ts (POST, GET SSE, and terminateSession() paths), packages/core/src/errors/sdkErrors.ts (new enum value), tests, and migration docs.
Security risks
None. The change narrows error classification by HTTP status code on the client transport. No new auth surface, no input parsing, no injection vectors. The requestHadSessionId snapshot taken before fetch correctly avoids the post-response state-mutation issue raised in round 1.
Level of scrutiny
Medium — this is a public client transport behavior change in error handling, which downstream consumers branch on. However, the change is anchored directly to the MCP spec (Streamable HTTP, Session Management), the author bundled a migration-docs update, and tests cover the POST path, GET SSE path, the no-session-ID negative case, and terminateSession() 404. All three of my prior review comments were addressed precisely as suggested.
Other factors
The two remaining inline comments are documentation-completeness nits (JSDoc on terminateSession() not mentioning the new 404 case; migration.md slightly overstating the no-session-ID error code as POST-only behavior). Neither blocks correctness. The changeset-bot has flagged a missing changeset; given this changes observable error codes in @modelcontextprotocol/client and adds a new public enum value to @modelcontextprotocol/core, a patch changeset is warranted before merge.
| // 405 Method Not Allowed: per the spec the server does not support explicit | ||
| // session termination — treat as success. | ||
| // 404 Not Found: the session is already gone server-side, which is exactly | ||
| // what the caller asked for — treat as success rather than a failure. In both | ||
| // cases fall through to clear the local session ID. | ||
| if (!response.ok && response.status !== 405 && response.status !== 404) { |
There was a problem hiding this comment.
🟡 The terminateSession() JSDoc still only documents the 405 Method Not Allowed special case, but this PR adds a sibling success case: a 404 now also resolves and clears the session ID instead of throwing ClientHttpFailedToTerminateSession. Add a one-sentence note mirroring the 405 sentence so the public-facing JSDoc matches the implementation.
Extended reasoning...
What changed vs. what the JSDoc says
This PR changes terminateSession() so that an HTTP 404 response is treated as a second success case alongside the existing 405:
// 405 Method Not Allowed: per the spec the server does not support explicit
// session termination — treat as success.
// 404 Not Found: the session is already gone server-side, ... — treat as success ...
if (!response.ok && response.status !== 405 && response.status !== 404) {
throw new SdkError(SdkErrorCode.ClientHttpFailedToTerminateSession, ...);
}
this._sessionId = undefined;The behavior is documented in three places: the inline implementation comment, docs/migration.md ("Session expiry now surfaces as ClientHttpSessionExpired" section), and docs/migration-SKILL.md. But the public-facing JSDoc on terminateSession() — the text consumers see in editor hover tooltips and generated API docs — still reads:
The server MAY respond with HTTP
405 Method Not Allowed, indicating that the server does not allow clients to terminate sessions.
It enumerates exactly one tolerated non-OK status, which now reads as a complete list when it isn't.
Why it matters (and why it's a nit, not a blocker)
The refutation correctly notes there's no contradiction — the JSDoc never claimed 404 throws, and the 405 sentence is a near-verbatim spec quote about server behavior, not a claim about SDK error handling. So this isn't a prose-vs-implementation mismatch in the strict sense.
But the JSDoc does go out of its way to call out one specific non-OK status that is silently tolerated. By enumerating one special case, it implies a closed set. Adding a sibling special case in the implementation while leaving the doc untouched makes the doc misleadingly incomplete: a consumer reading only the hover tooltip would reasonably assume any non-405, non-OK response (including 404) throws ClientHttpFailedToTerminateSession, and might write try/catch logic for a case that no longer fires.
Step-by-step
- A consumer with an active session calls
transport.terminateSession()while the server has already evicted the session (idle timeout, restart, etc.). - The server returns 404. Pre-PR this threw
ClientHttpFailedToTerminateSession; post-PR it resolves and clears_sessionId. - A consumer who reads the JSDoc to understand what failure modes to handle sees only the 405 carve-out and doesn't learn that 404 is now a silent success — they'd have to dig into the migration guide or the source to discover it.
Fix
Add one sentence to the JSDoc at packages/client/src/client/streamableHttp.ts around lines 757–759, mirroring the existing 405 line:
The server may also respond with HTTP `404 Not Found` if the session has
already expired or been terminated; this is treated as successful termination.
This keeps the public-facing doc in sync with both the implementation and the migration guide. Pure documentation-completeness nit; non-blocking.
|
|
||
| A `404` for a request that did **not** carry a session ID (for example a wrong URL on |
There was a problem hiding this comment.
🟡 The new sentence stating a 404 without a session ID "still surfaces as SdkErrorCode.ClientHttpNotImplemented" is only true for the POST path (_send); on the standalone GET SSE path (_startOrAuthSse) the same scenario falls through to SdkErrorCode.ClientHttpFailedToOpenStream. Consider qualifying the sentence as POST-only or listing both codes so consumers writing recovery logic from the docs handle both paths.
Extended reasoning...
What the prose claims vs. what the code does
The added paragraph under Session expiry now surfaces as ClientHttpSessionExpired states unconditionally:
A
404for a request that did not carry a session ID (for example a wrong URL on the initial connection) is unchanged: it still surfaces asSdkErrorCode.ClientHttpNotImplemented.
That sentence reads as a general claim about every 404-without-session path in StreamableHTTPClientTransport, but it is only accurate for the POST path. The transport has two places where the new 404 branch was added, and their fall-through error codes differ:
_send(POST): after the newif (response.status === 404 && requestHadSessionId)branch, the catch-all throwsSdkError(SdkErrorCode.ClientHttpNotImplemented, ...)— matches the prose._startOrAuthSse(standalone GET SSE): after the same new 404 branch, the catch-all throwsSdkError(SdkErrorCode.ClientHttpFailedToOpenStream, ...)— notClientHttpNotImplemented.
Step-by-step proof for the GET path
- A transport is constructed with no
sessionIdoption, sothis._sessionIdisundefined. _startOrAuthSse({})runs. The new snapshotconst requestHadSessionId = this._sessionId !== undefinedevaluates tofalse.- The server replies
404 Not Found. response.okisfalse, so the error block runs. The 401, 405, and the new404 && requestHadSessionIdchecks are all skipped (the last one becauserequestHadSessionIdisfalse).- Execution reaches the fall-through:
throw new SdkError(SdkErrorCode.ClientHttpFailedToOpenStream, \Failed to open SSE stream: ${response.statusText}`, ...)`.
So the GET path produces ClientHttpFailedToOpenStream, not the ClientHttpNotImplemented the prose promises.
Why it matters (and why it's only a nit)
A consumer writing reconnect/recovery logic from the migration guide could reasonably write a handler that branches on ClientHttpSessionExpired vs. ClientHttpNotImplemented to decide "session expired — reconnect" vs. "plain 404 — give up." The GET path's plain-404 case would slip through neither branch.
That said, this is a low-impact documentation imprecision rather than a code bug:
- The prose's own parenthetical example ("a wrong URL on the initial connection") points the reader at the POST initialize path, where the claim is correct.
- The GET-without-session-then-404 scenario is rare in practice — the standalone GET stream is normally opened post-initialize against the same URL, by which point a session ID would be set if the server uses sessions.
- The GET path's 404-without-session error code is unchanged from before this PR; only the unqualified attribution to
ClientHttpNotImplementedis new and misleading.
How to fix
Qualify the sentence or list both codes. For example:
A
404for a request that did not carry a session ID is unchanged: it still surfaces asSdkErrorCode.ClientHttpNotImplementedon POST andSdkErrorCode.ClientHttpFailedToOpenStreamon the standalone GET stream.
1 participant
Motivation
The MCP spec (Streamable HTTP, Session Management) states:
StreamableHTTPClientTransportdid not implement this. Every non-401/403 error status — including404— was surfaced as a genericSdkErrorCode.ClientHttpNotImplemented(POST) orSdkErrorCode.ClientHttpFailedToOpenStream(GET), with no way for a caller to tell session expiry apart from any other HTTP failure.In practice consumers worked around this by string-matching the response body (e.g. looking for a
-32001JSON-RPC code). That only matches the reference server. Servers that report an expired session with a different body slip through and the session stays wedged until the app is restarted, for example:-32002)Change
Detect session expiry by status code alone, scoped to requests that actually carried a session ID (exactly what the spec describes):
404whenthis._sessionIdis set, the transport clears the stale session ID and throwsSdkErrorwith a newSdkErrorCode.ClientHttpSessionExpired. Clearing the ID means a subsequentclient.connect(transport)issues a freshinitializeinstead of a reconnect (Client.connect()branches ontransport.sessionId).404for a request that did not carry a session ID (e.g. a wrong URL on the initial connect) is unchanged and still surfaces asClientHttpNotImplemented.Applied to both the POST (
_send) and standalone GET SSE (_startOrAuthSse) paths.Tests
packages/client/test/client/streamableHttp.test.ts— replaced the single 404 test with asession expiry (HTTP 404)group:ClientHttpSessionExpiredfor a 404 with a session ID across varied bodies (plain text,-32002,-32001, HTML, empty), and assertingtransport.sessionIdis clearedClientHttpNotImplementedFull workspace typecheck, build, and lint pass (pre-push hook).
Docs
Added
SdkErrorCode.ClientHttpSessionExpiredand the new behavior todocs/migration.mdanddocs/migration-SKILL.md.