-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(client): treat HTTP 404 with session ID as session expiry #2125
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -246,6 +246,12 @@ | |
| headers.set('last-event-id', resumptionToken); | ||
| } | ||
|
|
||
| // Capture whether this request carried a session ID (see `_send` for the | ||
| // 404 session-expiry rule). The GET path does not write a response | ||
| // `mcp-session-id` back into `this._sessionId`, but snapshotting keeps the | ||
| // two branches symmetric. | ||
| const requestHadSessionId = this._sessionId !== undefined; | ||
|
|
||
| const response = await (this._fetch ?? fetch)(this._url, { | ||
| ...this._requestInit, | ||
| method: 'GET', | ||
|
|
@@ -288,6 +294,18 @@ | |
| return; | ||
| } | ||
|
|
||
| // A 404 on the standalone GET stream for a request that carried a session | ||
| // ID means the session expired server-side (same spec rule as the POST | ||
| // path in `_send`). Clear the dead session ID and surface the | ||
| // session-expired error code. | ||
| if (response.status === 404 && requestHadSessionId) { | ||
| this._sessionId = undefined; | ||
| throw new SdkError(SdkErrorCode.ClientHttpSessionExpired, 'Failed to open SSE stream: session expired (HTTP 404)', { | ||
| status: 404, | ||
| statusText: response.statusText | ||
| }); | ||
| } | ||
|
|
||
| throw new SdkError(SdkErrorCode.ClientHttpFailedToOpenStream, `Failed to open SSE stream: ${response.statusText}`, { | ||
| status: response.status, | ||
| statusText: response.statusText | ||
|
|
@@ -552,6 +570,12 @@ | |
| signal: this._abortController?.signal | ||
| }; | ||
|
|
||
| // Capture whether *this request* carried a session ID before processing the | ||
| // response — the response handling below may write a new `mcp-session-id` | ||
| // into `this._sessionId`, and the 404 session-expiry rule is defined in terms | ||
| // of the request, not the post-response state. | ||
| const requestHadSessionId = this._sessionId !== undefined; | ||
|
|
||
| const response = await (this._fetch ?? fetch)(this._url, init); | ||
|
|
||
| // Handle session ID received during initialization | ||
|
|
@@ -629,6 +653,22 @@ | |
| } | ||
| } | ||
|
|
||
| // Per the MCP spec (Streamable HTTP, Session Management): a 404 in | ||
| // response to a request that carried an `Mcp-Session-Id` means the | ||
| // session has expired or been terminated server-side, and the client | ||
| // must start a new session. Detect this by the status code alone — | ||
| // not the response body — since non-reference servers report it with | ||
| // varying bodies (different JSON-RPC error codes, plain text, HTML). | ||
| // Clear the dead session ID so a subsequent reconnect issues a fresh | ||
| // `initialize`, and surface a distinct, body-agnostic error code. | ||
| if (response.status === 404 && requestHadSessionId) { | ||
| this._sessionId = undefined; | ||
| throw new SdkError(SdkErrorCode.ClientHttpSessionExpired, `Session expired (HTTP 404): ${text}`, { | ||
| status: 404, | ||
| text | ||
| }); | ||
| } | ||
|
|
||
|
claude[bot] marked this conversation as resolved.
claude[bot] marked this conversation as resolved.
|
||
| throw new SdkError(SdkErrorCode.ClientHttpNotImplemented, `Error POSTing to endpoint: ${text}`, { | ||
| status: response.status, | ||
| text | ||
|
|
@@ -722,9 +762,12 @@ | |
| 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 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) { | ||
|
Check warning on line 770 in packages/client/src/client/streamableHttp.ts
|
||
|
Comment on lines
+765
to
+770
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 Extended reasoning...What changed vs. what the JSDoc saysThis PR changes // 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,
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 Step-by-step
FixAdd one sentence to the JSDoc at This keeps the public-facing doc in sync with both the implementation and the migration guide. Pure documentation-completeness nit; non-blocking. |
||
| throw new SdkError(SdkErrorCode.ClientHttpFailedToTerminateSession, `Failed to terminate session: ${response.statusText}`, { | ||
| status: response.status, | ||
| statusText: 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 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 toSdkErrorCode.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
ClientHttpSessionExpiredstates unconditionally: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
sessionIdoption, sothis._sessionIdisundefined._startOrAuthSse({})runs. The new snapshotconst requestHadSessionId = this._sessionId !== undefinedevaluates tofalse.404 Not Found.response.okisfalse, so the error block runs. The 401, 405, and the new404 && requestHadSessionIdchecks are all skipped (the last one becauserequestHadSessionIdisfalse).throw new SdkError(SdkErrorCode.ClientHttpFailedToOpenStream, \Failed to open SSE stream: ${response.statusText}`, ...)`.So the GET path produces
ClientHttpFailedToOpenStream, not theClientHttpNotImplementedthe 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
ClientHttpSessionExpiredvs.ClientHttpNotImplementedto 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:
ClientHttpNotImplementedis new and misleading.How to fix
Qualify the sentence or list both codes. For example: