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 4 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/codemod
@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.
…ession-expiry-404 # Conflicts: # docs/migration-SKILL.md # packages/client/src/client/streamableHttp.ts # packages/client/test/client/streamableHttp.test.ts
The standalone GET SSE stream is the optional server->client notification channel. A 404 on its (re)connection must not tear down the session — the client should keep the session and continue issuing POST requests, as the reconnect-failure-onerror e2e scenario asserts. Limiting session-expiry detection to the POST path (_send) fixes a regression where a failing GET reconnect cleared the session and broke subsequent tool calls. Genuine expiry is still detected when a real POST request gets a 404.
| ClientHttpFailedToTerminateSession = 'CLIENT_HTTP_FAILED_TO_TERMINATE_SESSION', | ||
| /** | ||
| * Server returned HTTP 404 for a request that carried an `Mcp-Session-Id`. | ||
| * Per the MCP spec (Streamable HTTP, Session Management), this means the | ||
| * session has expired or been terminated server-side and the client must | ||
| * start a new session. The transport clears its stale session ID before | ||
| * throwing this, so reconnecting issues a fresh `initialize`. | ||
| */ | ||
| ClientHttpSessionExpired = 'CLIENT_HTTP_SESSION_EXPIRED' |
There was a problem hiding this comment.
🟡 No changeset was added for this PR (the changeset-bot reports "No Changeset found"), but it introduces a new public enum member (SdkErrorCode.ClientHttpSessionExpired) and changes user-visible runtime behavior of StreamableHTTPClientTransport (404-with-session-ID now throws the new code and clears sessionId; terminateSession() now resolves on 404 instead of throwing). Please add a changeset (patch or minor) for @modelcontextprotocol/client and @modelcontextprotocol/core so the change produces a version bump and changelog entry, mirroring .changeset/add-sdk-http-error.md from the prior SdkHttpError change.
Extended reasoning...
Missing changeset for a consumer-visible API/behavior change
This repository uses changesets to drive releases: .changeset/ contains dozens of per-PR entries, .github/workflows/release.yml runs changesets/action, and the packages are currently in alpha pre-release mode (.changeset/pre.json, mode: "pre", tag alpha), where each merged changeset is what produces the next 2.0.0-alpha.N bump and its changelog entry. The changeset-bot comment on this PR (latest commit dcc1757) confirms "No Changeset found", so as it stands merging will not bump any package or record this change in a changelog.
Why this PR warrants one
The PR is not internal-only — it changes the published API surface and runtime behavior that consumers will observe:
- New public enum member:
SdkErrorCode.ClientHttpSessionExpiredis added topackages/core/src/errors/sdkErrors.tsand is re-exported through@modelcontextprotocol/client(version2.0.0-alpha.2, published with public access — the pkg-pr-new bot even builds an installable preview of it for this PR). - Changed runtime behavior of
StreamableHTTPClientTransport: a 404 in response to a POST that carried anMcp-Session-Idnow throwsSdkHttpErrorwith the new code and clearstransport.sessionId(previously it surfaced asClientHttpNotImplementedand left the session ID intact), andterminateSession()now resolves on a 404 and clears the session ID instead of throwingClientHttpFailedToTerminateSession. - Both behaviors are documented as part of the public API in
docs/migration.mdanddocs/migration-SKILL.mdin this very PR — consumers are explicitly told to writeswitch (error.code)handlers for the new code, so they need a release that actually contains it.
Repo convention
Directly comparable prior changes shipped changesets. The closest analogue, .changeset/add-sdk-http-error.md (the PR that introduced SdkHttpError and the SdkErrorCode.ClientHttp* handling), declared a minor bump for both @modelcontextprotocol/core and @modelcontextprotocol/client. Other client/core fixes in the directory (e.g. fix-session-status-codes.md, the related server-side 404 fix) follow the same pattern. This PR touches exactly the same surface and should follow suit.
Concrete walk-through of the impact
- This PR merges with no changeset.
- The next time the release workflow runs
changeset version, only the other pending changesets are consumed; nothing records the new error code or the changed 404/terminateSession()semantics. @modelcontextprotocol/clientis published at whatever version the other changesets produce, with a CHANGELOG that never mentionsClientHttpSessionExpired— even though the migration guide added in this PR tells users to handle it.- A consumer pinning by changelog/release notes has no signal that 404 handling and
terminateSession()behavior changed between alpha versions.
Nuance and how to fix
One nuance: @modelcontextprotocol/core is marked "private": true in its package.json, so the bump that matters for npm consumers is the one for @modelcontextprotocol/client. Including core in the changeset is still consistent with the existing convention (add-sdk-http-error.md lists both) and is harmless. The fix is a single file, e.g. .changeset/client-http-session-expired.md:
---
'@modelcontextprotocol/client': minor
'@modelcontextprotocol/core': minor
---
Treat HTTP 404 with a session ID as session expiry: `StreamableHTTPClientTransport` now throws `SdkHttpError` with the new `SdkErrorCode.ClientHttpSessionExpired` and clears its session ID, and `terminateSession()` resolves on 404 instead of throwing.(Patch vs minor is the author's call; add-sdk-http-error.md used minor for the analogous error-code addition.) This is a release-process completeness issue rather than a code defect, and the changeset-bot already surfaces it on the PR, so it is non-blocking — but it should be added before merge so the release notes and version bump reflect the change.
| #### Session expiry now surfaces as `ClientHttpSessionExpired` | ||
|
|
||
| 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` now detects this by status code alone — it no longer | ||
| inspects the response body, so servers that report expiry with a non-reference body | ||
| (a different JSON-RPC error code, plain text, or HTML) are handled correctly. On such | ||
| a `404` the transport clears its stale session ID (so `transport.sessionId` becomes | ||
| `undefined` and a subsequent `client.connect(transport)` issues a fresh `initialize`) | ||
| and throws `SdkHttpError` with `SdkErrorCode.ClientHttpSessionExpired`. | ||
|
|
There was a problem hiding this comment.
🟡 The "Session expiry now surfaces as ClientHttpSessionExpired" prose states the rule unconditionally — "when a client receives an HTTP 404 in response to a request that carried an Mcp-Session-Id ... throws SdkErrorCode.ClientHttpSessionExpired" — but after dcc1757 the standalone GET SSE stream (which does carry the Mcp-Session-Id header) deliberately throws ClientHttpFailedToOpenStream and preserves the session instead. The same over-broad claim appears in docs/migration-SKILL.md line 144 ("thrown on HTTP 404 when a session ID was set") and the ClientHttpSessionExpired JSDoc in packages/core/src/errors/sdkErrors.ts. Qualify the prose to say expiry detection happens only on the POST request path, mirroring the inline NOTE in _startOrAuthSse.
Extended reasoning...
What the docs claim vs. what the code does
Commit dcc1757 ("fix(client): don't treat GET-stream 404 as session expiry") changed _startOrAuthSse so a 404 on the standalone GET SSE stream is deliberately not treated as session expiry: it throws SdkErrorCode.ClientHttpFailedToOpenStream and leaves _sessionId intact. The new inline NOTE and the test 'does NOT treat a 404 on the standalone GET stream as session expiry' both confirm this is intentional. However, that commit only touched streamableHttp.ts and the test file — the prose added earlier in this PR was not updated, and it still describes the rule unconditionally in terms of any request that carried an Mcp-Session-Id:
docs/migration.md(lines ~781–793): "when a client receives an HTTP404in response to a request that carried anMcp-Session-Id... On such a404the transport clears its stale session ID ... and throwsSdkHttpErrorwithSdkErrorCode.ClientHttpSessionExpired."docs/migration-SKILL.mdline 144:ClientHttpSessionExpiredis "thrown on HTTP 404 when a session ID was set; transport clearssessionId".packages/core/src/errors/sdkErrors.ts— the new JSDoc onSdkErrorCode.ClientHttpSessionExpired: "Server returned HTTP 404 for a request that carried anMcp-Session-Id... The transport clears its stale session ID before throwing this."
Why the GET stream falls under the prose but not the code
_commonHeaders() sets the mcp-session-id header whenever this._sessionId is set, and _startOrAuthSse builds its GET request from those headers — so the standalone GET stream (and resumeStream() / resumption-token reconnects, which also go through _startOrAuthSse) does carry the Mcp-Session-Id. Per the prose, a 404 there should clear the session ID and surface as ClientHttpSessionExpired. Per the code at HEAD, it does neither.
Step-by-step proof
- Construct
new StreamableHTTPClientTransport(url, { sessionId: 'existing-session-id' })and callstart(). _startOrAuthSse({})(the standalone GET stream, or aresumeStream()reconnect) calls_commonHeaders(), which addsmcp-session-id: existing-session-id— the request carries the session ID, so the docs' precondition is met.- The server responds
404 Not Found(e.g. it evicted the session). - In the
!response.okblock, the 401 and 405 branches are skipped, and execution reaches the catch-all under the new NOTE:throw new SdkHttpError(SdkErrorCode.ClientHttpFailedToOpenStream, ...).this._sessionIdis never cleared. - A consumer who read the migration guide / SKILL doc / JSDoc and wrote
catch (e) { if (e.code === SdkErrorCode.ClientHttpSessionExpired) reconnect(); }around the reconnect path never hits that branch — they getClientHttpFailedToOpenStreamand a still-settransport.sessionId, the opposite of what the prose promises. The unit test added in dcc1757 asserts exactly this outcome (ClientHttpFailedToOpenStream,sessionIdstill'existing-session-id').
Why existing review comments don't cover this
The earlier inline comment on docs/migration.md line 794 is about the negative sentence (a 404 without a session ID surfacing as ClientHttpNotImplemented vs ClientHttpFailedToOpenStream on the GET path), and both prior bot comments predate dcc1757 — the commit that created this particular prose-vs-code divergence. The affirmative "404 with a session ID ⇒ ClientHttpSessionExpired" claim becoming inaccurate is new and unflagged. (The terminateSession() DELETE path is fine as documented: the very next paragraph in migration.md describes its 404-resolves-silently behavior, so only the POST/GET asymmetry needs a fix.)
How to fix
Qualify the three locations to scope expiry detection to the POST request path, mirroring the inline NOTE in _startOrAuthSse. For example, in migration.md: "...detects this by status code alone on POST requests: on such a 404 the transport clears its stale session ID ... A 404 on the standalone GET SSE stream (the optional notification channel, including resumeStream() reconnects) is deliberately not treated as expiry — it surfaces as ClientHttpFailedToOpenStream and leaves the session intact." Add an analogous parenthetical to the SKILL-doc bullet and a sentence to the ClientHttpSessionExpired JSDoc. This is a documentation-precision fix — a few sentences across the three files — but worth doing before merge since the contradiction was introduced within this same PR.
|
Picked this up as #2660 — rebased onto current Hope that's a useful head start — happy to adjust anything you'd want done differently, and equally happy to close #2660 in favor of you picking your own branch back up if you'd rather. |
2 participants
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.