Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/migration-SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ Two error classes now exist:
| 403 after upscoping | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpForbidden` |
| Unexpected content type | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpUnexpectedContent` |
| Session termination failed | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToTerminateSession` |
| Session expired (404 w/ session) | `StreamableHTTPError` (status 404) | `SdkError` with `SdkErrorCode.ClientHttpSessionExpired` |
| Response result fails schema | `ZodError` (raw) | `SdkError` with `SdkErrorCode.InvalidResult` |

New `SdkErrorCode` enum values:
Expand All @@ -139,6 +140,7 @@ New `SdkErrorCode` enum values:
- `SdkErrorCode.ClientHttpUnexpectedContent` = `'CLIENT_HTTP_UNEXPECTED_CONTENT'`
- `SdkErrorCode.ClientHttpFailedToOpenStream` = `'CLIENT_HTTP_FAILED_TO_OPEN_STREAM'`
- `SdkErrorCode.ClientHttpFailedToTerminateSession` = `'CLIENT_HTTP_FAILED_TO_TERMINATE_SESSION'`
- `SdkErrorCode.ClientHttpSessionExpired` = `'CLIENT_HTTP_SESSION_EXPIRED'` (thrown on HTTP 404 when a session ID was set; transport clears `sessionId` so reconnect re-`initialize`s; detection is status-only, body-agnostic)

Update error handling:

Expand Down
23 changes: 23 additions & 0 deletions docs/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,7 @@ The new `SdkErrorCode` enum contains string-valued codes for local SDK errors:
| `SdkErrorCode.ClientHttpUnexpectedContent` | Unexpected content type in HTTP response |
| `SdkErrorCode.ClientHttpFailedToOpenStream` | Failed to open SSE stream |
| `SdkErrorCode.ClientHttpFailedToTerminateSession` | Failed to terminate session |
| `SdkErrorCode.ClientHttpSessionExpired` | Server returned 404 for a request carrying a session ID — the session expired, start a new one |

#### `StreamableHTTPError` removed

Expand Down Expand Up @@ -760,6 +761,12 @@ try {
case SdkErrorCode.ClientHttpFailedToOpenStream:
console.log('Failed to open SSE stream');
break;
case SdkErrorCode.ClientHttpSessionExpired:
// Server returned 404 for a request carrying a session ID.
// The transport already cleared its session ID; reconnect to
// start a fresh session (per the MCP spec, Session Management).
console.log('Session expired — reconnecting');
break;
case SdkErrorCode.ClientHttpNotImplemented:
console.log('HTTP request failed');
break;
Expand All @@ -770,6 +777,22 @@ try {
}
```

#### 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 `SdkError` with `SdkErrorCode.ClientHttpSessionExpired`.

A `404` for a request that did **not** carry a session ID (for example a wrong URL on
the initial connection) is unchanged: it still surfaces as `SdkErrorCode.ClientHttpNotImplemented`.

#### Why this change?

Previously, `ErrorCode.RequestTimeout` (-32001) and `ErrorCode.ConnectionClosed` (-32000) were used for local timeout/connection errors. However, these errors never cross the wire as JSON-RPC responses - they are rejected locally. Using protocol error codes for local errors was
Expand Down
27 changes: 27 additions & 0 deletions packages/client/src/client/streamableHttp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

View check run for this annotation

Claude / Claude Code Review

404 session-expiry check reads this._sessionId after response headers may have mutated it

The 404 session-expiry check reads `this._sessionId` *after* the response's `mcp-session-id` header 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). Snapshot `const hadSessionId = this._sessionId !== undefined` before the `await fetch` (or check the request `headers`) and branch on that. The same hunk in `_startOrAuthSse` reads consistently beca

Check warning on line 658 in packages/client/src/client/streamableHttp.ts

View check run for this annotation

Claude / Claude Code Review

terminateSession DELETE path does not handle 404 session expiry

The PR adds 404-with-session-ID handling to the POST (`_send`) and standalone GET SSE (`_startOrAuthSse`) paths, but `terminateSession()` — which always sends a DELETE carrying `Mcp-Session-Id` since it returns early when there is no session ID — still treats a 404 as a generic failure: it throws `ClientHttpFailedToTerminateSession` before reaching `this._sessionId = undefined`, so the dead session ID is retained and the caller gets a misleading error even though the session is in fact already g
Comment on lines +643 to +658
Copy link
Copy Markdown

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._sessionId after the response's mcp-session-id header 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). Snapshot const hadSessionId = this._sessionId !== undefined before the await fetch (or check the request headers) and branch on that. The same hunk in _startOrAuthSse reads 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 _send is intended to implement the MCP spec rule: "When a client receives HTTP 404 in response to a request containing an Mcp-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 an Mcp-Session-Id".

But the predicate this._sessionId !== undefined is evaluated after the response header handling that runs unconditionally at the top of _send's response processing:

const sessionId = response.headers.get('mcp-session-id');
if (sessionId) {
    this._sessionId = sessionId;          // mutation happens here, even on !response.ok
}

if (!response.ok) {
    // ...401/403 handling...
    if (response.status === 404 && this._sessionId !== undefined) {  // reads post-mutation state
        this._sessionId = undefined;
        throw new SdkError(SdkErrorCode.ClientHttpSessionExpired, ...);
    }
}

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-id header. A request goes out with no session ID; the server responds 404 and includes mcp-session-id in the response headers. Lines a few up store it into this._sessionId; the 404 check then sees a non-undefined session ID, misclassifies the error as ClientHttpSessionExpired, and immediately clears the session ID it just stored. Per the spec this should be ClientHttpNotImplemented (the request had no session). The PR's negative test (treats a 404 without a session ID as a generic HTTP error) only mocks headers: new Headers(), so this path is untested.

(2) Concurrency. Two _send calls in flight: request A is an initialize whose 200 response sets this._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)

  1. A fresh transport (no sessionId option) calls send(initialize). _commonHeaders() returns headers without mcp-session-id because this._sessionId is undefined.
  2. The (misbehaving) server responds 404 Not Found with header mcp-session-id: abc.
  3. response.headers.get('mcp-session-id')'abc'this._sessionId = 'abc'.
  4. !response.ok → enter error block. The 404 check sees this._sessionId === 'abc' !== undefined → true.
  5. Throws SdkError(ClientHttpSessionExpired) and clears this._sessionId. Per the spec this is wrong — the request had no Mcp-Session-Id, so it should be a generic ClientHttpNotImplemented.

Why this is low-severity

Both triggers are unlikely in practice: real servers don't set mcp-session-id on 404 error responses, and the Protocol layer serializes initialize so the concurrency window is narrow (it requires direct transport.send() use outside Client.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:

const hadSessionId = this._sessionId !== undefined;   // before await fetch
// ...
if (response.status === 404 && hadSessionId) { ... }

Or equivalently, check headers.has('mcp-session-id') on the already-built request headers.

The mirrored hunk in _startOrAuthSse happens to read correctly today (the GET path never writes the response mcp-session-id header back into this._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.

Comment on lines +643 to +658
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The PR adds 404-with-session-ID handling to the POST (_send) and standalone GET SSE (_startOrAuthSse) paths, but terminateSession() — which always sends a DELETE carrying Mcp-Session-Id since it returns early when there is no session ID — still treats a 404 as a generic failure: it throws ClientHttpFailedToTerminateSession before reaching this._sessionId = undefined, so the dead session ID is retained and the caller gets a misleading error even though the session is in fact already gone. Per the same spec rule applied on the other two paths, a 404 here should clear _sessionId and likely succeed silently (the session is already terminated server-side, which is what the caller asked for).

Extended reasoning...

What the bug is

This PR introduces the rule "a 404 in response to a request carrying Mcp-Session-Id means the session expired server-side" and applies it to two of the three code paths in StreamableHTTPClientTransport that can carry a session ID: _send (POST) and _startOrAuthSse (GET). The third path, terminateSession(), also always carries a session ID — it returns early at the top of the method when !this._sessionId — but is left untouched.

The code path that triggers it

async 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 !response.ok && status !== 405 branch and throws, so this._sessionId = undefined on the next line never executes.

Step-by-step proof

  1. Client establishes a session; _sessionId = 'abc'.
  2. Server-side, the session expires (idle timeout, restart, etc.).
  3. Client calls transport.terminateSession().
  4. The early-return guard passes (_sessionId is set), so a DELETE is sent with Mcp-Session-Id: abc.
  5. Server replies 404 Not Found because it has no record of session abc.
  6. response.ok is false, response.status !== 405, so ClientHttpFailedToTerminateSession is thrown.
  7. this._sessionId is still 'abc' — the dead ID survives, and the caller is told termination failed even though the session is already gone (which is exactly the outcome the caller wanted).

Why this is the same wedge the PR is fixing

The 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. terminateSession() exhibits the identical symptom: after step 7 the transport still holds a stale session ID, and the surfaced error code (ClientHttpFailedToTerminateSession) is misleading — it implies an actionable failure when in fact nothing went wrong.

Why the impact is bounded (and why this is a nit rather than a blocker)

  • terminateSession() is typically called on shutdown, so the lingering _sessionId is usually about to be discarded anyway.
  • With this PR's other changes in place, if the caller does keep the transport alive and issues a subsequent POST, the new _send 404 handler will now clear the stale ID — so the wedge is no longer permanent the way it was before this PR.

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 it

Treat 404 like the existing 405 special-case: when the DELETE returns 404, clear _sessionId and return successfully (the user's intent — terminating the session — has already been achieved server-side). Alternatively, throw ClientHttpSessionExpired after clearing _sessionId for consistency with the other two paths, but silent success is closer to the spec semantics for DELETE. Either way, _sessionId must be cleared.

// 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
Expand Down
80 changes: 64 additions & 16 deletions packages/client/test/client/streamableHttp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

View check run for this annotation

Claude / Claude Code Review

No test coverage for the GET (_startOrAuthSse) 404-with-session-ID path

The new `session expiry (HTTP 404)` test group only exercises the POST path (`_send` via `sessionTransport.send()`); the GET SSE path (`_startOrAuthSse`), which this PR also modified to handle 404-with-session-ID and which constructs a different `SdkError` data shape (`{ status, statusText }` vs `{ status, text }`), has no test. Consider adding a GET-path test (e.g. `transport.start()` + `_startOrAuthSse({})` with a sessionId set and a 404 mock) mirroring the POST cases.
Comment on lines +223 to +240
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The new session expiry (HTTP 404) test group only exercises the POST path (_send via sessionTransport.send()); the GET SSE path (_startOrAuthSse), which this PR also modified to handle 404-with-session-ID and which constructs a different SdkError data shape ({ status, statusText } vs { status, text }), has no test. Consider adding a GET-path test (e.g. transport.start() + _startOrAuthSse({}) with a sessionId set and a 404 mock) mirroring the POST cases.

Extended reasoning...

Test-coverage gap: GET SSE (_startOrAuthSse) 404-with-session-ID path is untested

This PR adds the "404 with a session ID means session expiry" handling to two code paths in packages/client/src/client/streamableHttp.ts:

  1. _send (POST) — around line 649: throws SdkError(SdkErrorCode.ClientHttpSessionExpired, Session expired (HTTP 404): ${text}, { status: 404, text })
  2. _startOrAuthSse (standalone GET SSE) — around line 291: throws SdkError(SdkErrorCode.ClientHttpSessionExpired, 'Failed to open SSE stream: session expired (HTTP 404)', { status: 404, statusText: response.statusText })

These are distinct branches with different error messages and different data shapes ({ status, text } vs { status, statusText }). The new describe('session expiry (HTTP 404)') block in packages/client/test/client/streamableHttp.test.ts only exercises path (1):

  • The parametrized it.each(...) test calls sessionTransport.send(message), which routes through _send (POST).
  • The negative case ("404 without a session ID") also calls transport.send(message) (POST).

Nothing in the test file ever invokes _startOrAuthSse (or transport.start() + a GET SSE attempt) with a session ID set and a mocked 404 response, so the GET-side branch is unverified.

Why this matters

The repo's review checklist requires that "new behavior has vitest coverage including error paths." Because the GET branch is a separate if with its own message and a divergent data shape, a future regression there (e.g. someone removing the GET-side branch during a refactor, or changing the data shape) would not be caught by the existing tests — they'd all stay green.

Concrete walk-through

  1. Construct a StreamableHTTPClientTransport with { sessionId: 'existing-session-id' }.
  2. Call await transport.start() then await (transport as any)._startOrAuthSse({}) (the test file already uses this private-method-access pattern for several other GET-path tests).
  3. Mock fetch to resolve with { ok: false, status: 404, statusText: 'Not Found', text: () => Promise.resolve(''), headers: new Headers() }.
  4. Currently no test asserts that the rejection is an SdkError with code === SdkErrorCode.ClientHttpSessionExpired, that error.data equals { status: 404, statusText: 'Not Found' }, or that transport.sessionId is cleared afterward.

Suggested fix

Add a GET-path test inside the existing describe('session expiry (HTTP 404)') group, something like:

it('treats 404 on the standalone GET SSE stream with a session ID as session expiry', async () => {
    const sessionTransport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
        sessionId: 'existing-session-id'
    });

    (globalThis.fetch as Mock).mockResolvedValueOnce({
        ok: false,
        status: 404,
        statusText: 'Not Found',
        text: () => Promise.resolve(''),
        headers: new Headers()
    });

    await sessionTransport.start();
    const error = await (sessionTransport as unknown as { _startOrAuthSse: (o: StartSSEOptions) => Promise<void> })
        ._startOrAuthSse({})
        .then(() => null, e => e);

    expect(error).toBeInstanceOf(SdkError);
    expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpSessionExpired);
    expect((error as SdkError).data).toEqual({ status: 404, statusText: 'Not Found' });
    expect(sessionTransport.sessionId).toBeUndefined();

    await sessionTransport.close().catch(() => {});
});

A matching negative case (GET 404 with no session ID still produces ClientHttpFailedToOpenStream) would round out the symmetry with the POST tests.

This is a test-coverage gap, not a runtime defect — the production GET-path code looks correct as written.

])('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 () => {
Expand Down
10 changes: 9 additions & 1 deletion packages/core/src/errors/sdkErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,15 @@ export enum SdkErrorCode {
ClientHttpForbidden = 'CLIENT_HTTP_FORBIDDEN',
ClientHttpUnexpectedContent = 'CLIENT_HTTP_UNEXPECTED_CONTENT',
ClientHttpFailedToOpenStream = 'CLIENT_HTTP_FAILED_TO_OPEN_STREAM',
ClientHttpFailedToTerminateSession = 'CLIENT_HTTP_FAILED_TO_TERMINATE_SESSION'
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'
}

/**
Expand Down
Loading