fix(client): treat HTTP 404 with session ID as session expiry - #2660
Open
Azazi wants to merge 3 commits into
Open
fix(client): treat HTTP 404 with session ID as session expiry#2660Azazi wants to merge 3 commits into
Azazi wants to merge 3 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) 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 (snapshotted before the fetch, and before the isHandshake header-stripping check, so a sessionless initialize is never misclassified): on a 404 when the request carried a session ID, clear the stale session ID (so a subsequent connect() issues a fresh initialize) and throw SdkHttpError with the new SdkErrorCode.ClientHttpSessionExpired. A 404 without a session ID is unchanged and still surfaces as ClientHttpNotImplemented. Not applied to the standalone GET SSE stream, whose failure must not tear down an otherwise healthy session. terminateSession() now also treats a 404 (session already gone server-side) the same as the existing 405 (termination unsupported) case: it resolves instead of throwing ClientHttpFailedToTerminateSession, since the session being already gone is exactly the caller's intent. Rebases the essential behavior of modelcontextprotocol#2125 onto current main, resolving the conflict introduced by modelcontextprotocol#2469 (the isHandshake / SdkHttpError changes did not exist when modelcontextprotocol#2125 was opened) and updating the newly-added errorSurfacePins test and migration guide per docs/behavior-surface-pins.md's protocol for a deliberate SdkErrorCode membership change. Co-authored-by: David Soria Parra <davidsp@anthropic.com>
…est coverage The existing 'should handle 404 response when session expires' test never established a session before sending the 404-triggering request, so despite its name it only ever exercised the generic ClientHttpNotImplemented fallback -- it did not cover session expiry at all. Renamed to describe what it actually tests and kept as regression coverage for the "404 with no active session" case, which is intentionally unchanged by this fix. Added: - The actual session-expiry case: establish a session, get a 404, assert ClientHttpSessionExpired is thrown, the session ID is cleared, and a subsequent request no longer carries a session ID. - A regression guard for the standalone GET SSE stream: a 404 there must not clear the session. - terminateSession() treating a 404 (session already gone) as success, mirror of the existing 405 test.
modelcontextprotocol#2125 (which this rebases) never included one.
🦋 Changeset detectedLatest commit: 9a88b3f The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
This was referenced Aug 13, 2026
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Picks up #2125 (thank you @dsp-ant for the original work) — rebased onto current
mainto resolve the merge conflict with #2469, which touched the same lines after #2125 was opened and left it stuck asCONFLICTINGfor a while. The behavior is the same as #2125's final state:Mcp-Session-Idmeans the session has expired or been terminated server-side.StreamableHTTPClientTransportnow detects this, clears the stale session ID, and throws a new, distinguishableSdkErrorCode.ClientHttpSessionExpiredinstead of the genericClientHttpNotImplemented— so a subsequentconnect()starts a fresh session automatically.terminateSession()now also treats a 404 (session already gone) the same as the existing 405 case: resolves instead of throwing.Relates to #1708.
What changed since #2125 (the actual diff, not just a merge)
git cherry-pickof #2125's three commits doesn't apply cleanly against currentmain— worth calling out so this doesn't read as reinventing the fix:packages/core/src/errors/sdkErrors.ts(where fix(client): treat HTTP 404 with session ID as session expiry #2125 added the new error code) no longer exists — error classes moved topackages/core-internal/src/errors/sdkErrors.tssince fix(client): treat HTTP 404 with session ID as session expiry #2125 was opened.mainalready usesSdkHttpError(notSdkError) at every touched call site, predating and unrelated to fix(client): treat HTTP 404 with session ID as session expiry #2125.isHandshakeheader-stripping logic from fix(client): initialize requests never carry a session id; capture only from the initialize response #2469 (initialize requests never carry a session ID) didn't exist when fix(client): treat HTTP 404 with session ID as session expiry #2125 was written. The session-ID snapshot taken before the 404 check now also accounts for it (!isHandshake && this._sessionId !== undefined), so a 404 on an initialize request is never misclassified as session expiry.Given that, I reproduced the behavior fresh against current
mainrather than fighting three-way merges through a stale branch, and credited David viaCo-authored-byon the fix commit.Also, beyond #2125's original scope:
packages/core-internal/test/types/errorSurfacePins.test.ts— this repo added an explicit ABI pin over the fullSdkErrorCodemembership since fix(client): treat HTTP 404 with session ID as session expiry #2125 was opened (docs/behavior-surface-pins.md), which the new enum member correctly turns red without a pin update..changeset/streamable-http-404-session-expiry.md) — fix(client): treat HTTP 404 with session ID as session expiry #2125 didn't have one.docs/migration/upgrade-to-v2.md, perdocs/behavior-surface-pins.md's protocol for a deliberate, consumer-facingSdkErrorCodechange.should handle 404 response when session expires) that, despite its name, never actually established a session before triggering the 404 — it was only ever exercising the unrelated generic-fallback path. Renamed it to describe what it actually tests, and added real coverage for the session-bound case, the GET-stream non-clearing regression guard, andterminateSession()'s 404 handling.Scope note
This intentionally does not include #2150's transparent reinit-and-retry-original-request behavior or its new
onsessionexpiredhook — that's a substantively different (and larger) design question. #2466's closing comment framed exactly this kind of minimal fix as the thing worth landing now, with fuller recovery "available... if we want it as its own PR later." Happy to be pointed at a different direction if maintainers prefer #2150's approach instead — this is meant to unblock the minimal, already-reviewed fix, not to preempt that decision.Test plan
pnpm check:all(typecheck + lint across the whole monorepo) — cleanpnpm test:all— all tests green in every package this change touches or that depends on it (client: 800/800,core-internal: 1433/1433 including the updated pin); two pre-existing, unrelated failures observed intest/integration(IPv6/host-header) andtest/e2e(protocol:timeout:max-total, a fake-timer race) were confirmed present and unrelated to this diff by reproducing them on a clean, unmodifiedmaincheckoutHousekeeping
Since this rebases #2125, I've left a comment there and on #1708 pointing here — feel free to close #2125 in favor of this one if that's easier, or redirect review however's most convenient.