-
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 |
|---|---|---|
|
|
@@ -288,6 +288,17 @@ | |
| return; | ||
| } | ||
|
|
||
| // A 404 on the standalone GET stream while a session ID is set 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 && this._sessionId !== undefined) { | ||
| 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 | ||
|
|
@@ -629,6 +640,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 && this._sessionId !== undefined) { | ||
| this._sessionId = undefined; | ||
| throw new SdkError(SdkErrorCode.ClientHttpSessionExpired, `Session expired (HTTP 404): ${text}`, { | ||
| status: 404, | ||
| text | ||
| }); | ||
| } | ||
|
|
||
|
Check warning on line 658 in packages/client/src/client/streamableHttp.ts
|
||
|
Comment on lines
+643
to
+658
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 PR adds 404-with-session-ID handling to the POST ( Extended reasoning...What the bug isThis PR introduces the rule "a 404 in response to a request carrying The code path that triggers itasync terminateSession(): Promise<void> {
if (!this._sessionId) {
return; // No session to terminate
}
// ... DELETE with Mcp-Session-Id header via _commonHeaders() ...
if (!response.ok && response.status !== 405) {
throw new SdkError(SdkErrorCode.ClientHttpFailedToTerminateSession, ...);
}
this._sessionId = undefined; // never reached on 404
}On a 404 the method falls into the Step-by-step proof
Why this is the same wedge the PR is fixingThe PR description frames the motivation as "the session stays wedged until the app is restarted" because consumers couldn't distinguish session expiry from other HTTP failures. Why the impact is bounded (and why this is a nit rather than a blocker)
That said, the inconsistency is real and the misleading error is user-visible: callers that surface or log termination failures will report a spurious error. How to fix itTreat 404 like the existing 405 special-case: when the DELETE returns 404, clear // Option A: silent success (preferred — spec considers the session already terminated)
if (!response.ok && response.status !== 405 && response.status !== 404) {
throw new SdkError(SdkErrorCode.ClientHttpFailedToTerminateSession, ...);
}
this._sessionId = undefined; |
||
| throw new SdkError(SdkErrorCode.ClientHttpNotImplemented, `Error POSTing to endpoint: ${text}`, { | ||
| status: response.status, | ||
| text | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,32 +220,80 @@ | |
| await expect(transport.terminateSession()).resolves.not.toThrow(); | ||
| }); | ||
|
|
||
| it('should handle 404 response when session expires', async () => { | ||
| describe('session expiry (HTTP 404)', () => { | ||
| const message: JSONRPCMessage = { | ||
| jsonrpc: '2.0', | ||
| method: 'test', | ||
| params: {}, | ||
| id: 'test-id' | ||
| }; | ||
|
|
||
| (globalThis.fetch as Mock).mockResolvedValueOnce({ | ||
| ok: false, | ||
| status: 404, | ||
| statusText: 'Not Found', | ||
| text: () => Promise.resolve('Session not found'), | ||
| headers: new Headers() | ||
| }); | ||
| // Per the MCP spec (Streamable HTTP, Session Management): a 404 in response | ||
| // to a request that carried an `Mcp-Session-Id` means the session expired and | ||
| // a new one must be started. Detection is by status code alone — non-reference | ||
| // servers report it with varying bodies, so we must not require a body shape. | ||
| it.each([ | ||
| ['plain text', 'Session not found'], | ||
| ['JSON-RPC -32002 (Figma Desktop)', '{"jsonrpc":"2.0","error":{"code":-32002,"message":"Session not found"},"id":null}'], | ||
| ['JSON-RPC -32001 (reference server)', '{"jsonrpc":"2.0","error":{"code":-32001,"message":"Session not found"},"id":null}'], | ||
| ['arbitrary HTML', '<html><body>404 page not found</body></html>'], | ||
| ['empty body', ''] | ||
|
Check warning on line 240 in packages/client/test/client/streamableHttp.test.ts
|
||
|
Comment on lines
+223
to
+240
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 new Extended reasoning...Test-coverage gap: GET SSE (
|
||
| ])('treats 404 with a session ID as session expiry regardless of body (%s)', async (_label, body) => { | ||
| const sessionTransport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { | ||
| sessionId: 'existing-session-id' | ||
| }); | ||
|
|
||
| const errorSpy = vi.fn(); | ||
| transport.onerror = errorSpy; | ||
| (globalThis.fetch as Mock).mockResolvedValueOnce({ | ||
| ok: false, | ||
| status: 404, | ||
| statusText: 'Not Found', | ||
| text: () => Promise.resolve(body), | ||
| headers: new Headers() | ||
| }); | ||
|
|
||
| const errorSpy = vi.fn(); | ||
| sessionTransport.onerror = errorSpy; | ||
|
|
||
| const error = await sessionTransport.send(message).then( | ||
| () => null, | ||
| e => e | ||
| ); | ||
|
|
||
| expect(error).toBeInstanceOf(SdkError); | ||
| expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpSessionExpired); | ||
| expect((error as SdkError).data).toEqual({ status: 404, text: body }); | ||
| expect(errorSpy).toHaveBeenCalled(); | ||
| // The dead session ID is cleared so a subsequent reconnect issues a fresh `initialize`. | ||
| expect(sessionTransport.sessionId).toBeUndefined(); | ||
|
|
||
| await expect(transport.send(message)).rejects.toThrow( | ||
| new SdkError(SdkErrorCode.ClientHttpNotImplemented, 'Error POSTing to endpoint: Session not found', { | ||
| await sessionTransport.close().catch(() => {}); | ||
| }); | ||
|
|
||
| it('treats a 404 without a session ID as a generic HTTP error, not session expiry', async () => { | ||
| // No session ID was ever established (e.g. a 404 on the initial connect, or a | ||
| // wrong URL). The spec rule only applies to requests carrying an Mcp-Session-Id, | ||
| // so this must remain a generic error rather than triggering a session reset. | ||
| (globalThis.fetch as Mock).mockResolvedValueOnce({ | ||
| ok: false, | ||
| status: 404, | ||
| text: 'Session not found' | ||
| }) | ||
| ); | ||
| expect(errorSpy).toHaveBeenCalled(); | ||
| statusText: 'Not Found', | ||
| text: () => Promise.resolve('Not Found'), | ||
| headers: new Headers() | ||
| }); | ||
|
|
||
| const errorSpy = vi.fn(); | ||
| transport.onerror = errorSpy; | ||
|
|
||
| const error = await transport.send(message).then( | ||
| () => null, | ||
| e => e | ||
| ); | ||
|
|
||
| expect(error).toBeInstanceOf(SdkError); | ||
| expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpNotImplemented); | ||
| expect(errorSpy).toHaveBeenCalled(); | ||
| expect(transport.sessionId).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle non-streaming JSON response', async () => { | ||
|
|
||
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 404 session-expiry check reads
this._sessionIdafter the response'smcp-session-idheader has already been written into it a few lines above, so it tests post-response state rather than whether the request carried a session ID (which is what the spec rule and the inline comment describe). Snapshotconst hadSessionId = this._sessionId !== undefinedbefore theawait fetch(or check the requestheaders) and branch on that. The same hunk in_startOrAuthSsereads consistently because the GET path never writes the response header back, but using the same snapshot pattern there keeps the two branches symmetric.Extended reasoning...
What the check actually tests
The new 404 branch in
_sendis intended to implement the MCP spec rule: "When a client receives HTTP 404 in response to a request containing anMcp-Session-Id, it MUST start a new session." The inline comment above the branch restates this exactly — "a 404 in response to a request that carried anMcp-Session-Id".But the predicate
this._sessionId !== undefinedis evaluated after the response header handling that runs unconditionally at the top of_send's response processing:So the check answers "is a session ID set now, after the response has been processed?" — not "did the request we just sent carry one?". Those two questions diverge in two ways.
How they diverge
(1) A 404 response that itself carries an
mcp-session-idheader. A request goes out with no session ID; the server responds 404 and includesmcp-session-idin the response headers. Lines a few up store it intothis._sessionId; the 404 check then sees a non-undefined session ID, misclassifies the error asClientHttpSessionExpired, and immediately clears the session ID it just stored. Per the spec this should beClientHttpNotImplemented(the request had no session). The PR's negative test (treats a 404 without a session ID as a generic HTTP error) only mocksheaders: new Headers(), so this path is untested.(2) Concurrency. Two
_sendcalls in flight: request A is aninitializewhose 200 response setsthis._sessionId; request B was dispatched earlier with no session ID and gets a 404. If B's response is processed after A's, the 404 check reads A's freshly-established session ID, misclassifies B's 404, and clears the session A just created. The session is wedged until the caller reconnects.Step-by-step proof (scenario 1)
sessionIdoption) callssend(initialize)._commonHeaders()returns headers withoutmcp-session-idbecausethis._sessionIdisundefined.404 Not Foundwith headermcp-session-id: abc.response.headers.get('mcp-session-id')→'abc'→this._sessionId = 'abc'.!response.ok→ enter error block. The 404 check seesthis._sessionId === 'abc' !== undefined→ true.SdkError(ClientHttpSessionExpired)and clearsthis._sessionId. Per the spec this is wrong — the request had noMcp-Session-Id, so it should be a genericClientHttpNotImplemented.Why this is low-severity
Both triggers are unlikely in practice: real servers don't set
mcp-session-idon 404 error responses, and theProtocollayer serializesinitializeso the concurrency window is narrow (it requires directtransport.send()use outsideClient.connect()). I'm filing this as a nit — a spec-precision issue with a one-line fix, not a likely real-world failure.Fix
Snapshot the request-side state before the fetch and branch on that:
Or equivalently, check
headers.has('mcp-session-id')on the already-built request headers.The mirrored hunk in
_startOrAuthSsehappens to read correctly today (the GET path never writes the responsemcp-session-idheader back intothis._sessionId), but using the same snapshot pattern there keeps the two branches symmetric and future-proofs against someone adding session-header handling to the GET path.