Skip to content

fix(client): align Streamable HTTP session-id lifecycle with the spec#2466

Closed
felixweinberger wants to merge 6 commits into
mainfrom
fweinberger/shttp-session-capture-on-ok
Closed

fix(client): align Streamable HTTP session-id lifecycle with the spec#2466
felixweinberger wants to merge 6 commits into
mainfrom
fweinberger/shttp-session-capture-on-ok

Conversation

@felixweinberger

@felixweinberger felixweinberger commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Aligns StreamableHTTPClientTransport session-id handling with the spec's session-management rules:

  • Capture only from successful responses (the spec assigns the id on the InitializeResult response), and only when the request carried no id or the still-current one — an error reply, or a late reply from an older session, contributes nothing to session state. Previously the header was stored from any response before the status check, so e.g. a 404 answer to a version-negotiation probe carrying a session id made the fallback initialize present an id the server never issued, which strict stateful servers reject.
  • 404 to the current session id clears it (POSTs only; SSE reconnect GETs can 404 transiently while the session is live), and the terminated id is remembered until a replacement is assigned so raced in-flight requests are recognized too.
  • The client then starts a new session automatically — a fresh session-less InitializeRequest, per the spec's "MUST start a new session" requirement — and retries the failed request once (deduped across concurrent failures; runs under the failing request's timeout/signal; pagination continuations are not retried since a dead session's cursor is meaningless to the new one). If re-establishment fails, the request's own 404 surfaces and the handshake error goes to onerror. Server-side per-session state (e.g. resource subscriptions) does not carry over.
  • terminateSession() treats 404 as successful termination (the session is already gone — the goal state) and drops the stored id.

Motivation and Context

Auto-mode version negotiation made the capture-before-status-check pattern reachable on every connect (legacy servers by definition mishandle the probe), and the client-transport:http:session-404-reinitialize e2e requirement — the spec sentence verbatim — was a knownFailure. It now passes; 404-surfaces narrows to the unrecoverable case.

How Has This Been Tested?

Unit: error-reply non-capture (fails on main), session-less follow-up, 202 rotation unchanged, 404 clear, raced-sibling marking with late-reply guard, reinit dedup (two concurrent failures → one handshake), failure surface, cursor guard, initialize exclusion. E2e: the session-404-reinitialize scenario passes live (transparent recovery, new session id, both initializes session-less on the wire); full client suite and e2e matrix green.

Breaking Changes

None for spec-conforming servers. Requests that previously failed permanently on session expiry now transparently recover on legacy-era connections; callers relying on seeing the 404 still get it when re-establishment fails.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

2026-07-28-era connections keep their existing behavior (the modern handshake is discover-based); recovery there is a follow-up.

@felixweinberger felixweinberger requested a review from a team as a code owner July 8, 2026 16:01
@changeset-bot

changeset-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0c0aaf2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modelcontextprotocol/client Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Jul 8, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2466

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2466

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2466

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2466

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2466

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2466

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2466

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2466

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2466

commit: 0c0aaf2

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — small, spec-anchored guard on session-id capture with solid regression coverage; the inline note on the second test's unmocked fire-and-forget GET is a minor test-hygiene follow-up, not a blocker.

Extended reasoning...

Overview

The PR is a 3-file change: a ~10-line guard in StreamableHTTPClientTransport._send that moves the mcp-session-id header capture inside a response.ok check, two new regression tests, and an accurate changeset. No API surface changes, no new abstractions, no behavior change for spec-conforming servers.

Correctness

The change is spec-correct: the Streamable HTTP spec assigns the session id on the successful InitializeResult response, and error responses have no sanctioned role in session assignment (a 404 in fact mandates a fresh session-less re-initialize). Keeping the capture for all 2xx (rather than narrowing to initialize-only) preserves session-rotation behavior, which the second test covers explicitly. The changeset prose matches the implementation. The failure mode being fixed — auto-mode probe 404s carrying a session header poisoning the fallback handshake — is concrete and reproduced by the first test, which the author states fails on main.

Security risks

None introduced. If anything the change is mildly hardening: it stops a misbehaving server or intermediary from injecting session state via error responses. No auth, crypto, or permission logic is touched (the 401/403 handling paths are unchanged).

Level of scrutiny

The file is a production-critical transport, so I read the full _send flow around the edit; but the edit itself is a minimal, well-commented guard whose semantics are unambiguous. The two bug-hunter findings are (a) a test-hygiene nit — the rotated-session test's final notifications/initialized 202 triggers an unmocked fire-and-forget GET that falls through the fetch spy to the real network (verified: the suite's beforeEach spy has no default implementation) — and (b) an explicitly pre-existing 404/stale-session issue this PR neither introduces nor touches. Neither affects the correctness of the shipped change; the nit is worth fixing in a follow-up commit but doesn't warrant holding the fix.

Other factors

No prior reviews or outstanding comments on the timeline. Test coverage is good (poisoning regression, recovery after error, 2xx rotation), the changeset is present and accurate, and the change aligns with the repo's minimalism principle — it removes unsanctioned behavior rather than adding surface.

Comment thread packages/client/test/client/streamableHttp.test.ts Outdated
Comment thread packages/client/src/client/streamableHttp.ts Outdated
Comment thread packages/client/src/client/streamableHttp.ts Outdated
Comment thread packages/client/src/client/streamableHttp.ts Outdated
Comment thread packages/client/src/client/streamableHttp.ts Outdated
Comment thread .changeset/shttp-session-capture-on-ok.md Outdated
Comment on lines +973 to +977

const response = await (this._fetch ?? fetch)(this._url, init);

// Handle session ID received during initialization
const sessionId = response.headers.get('mcp-session-id');
if (sessionId) {
this._sessionId = sessionId;
// Capture the session id only from successful responses — the
// spec assigns it on the InitializeResult response. An error

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟣 Pre-existing issue (not introduced by this PR): _send's early-return resume branch destructures onresumptiontoken from the send options but calls _startOrAuthSse without it (and without onRequestStreamEnd), so a send() with resumptionToken — the documented resumability pattern — never reports new resumption tokens for the lifetime of the resumed stream, freezing the persisted token at its pre-disconnect value. A second disconnect then resumes from the stale token, replaying already-delivered events (or failing if the old event was evicted); the one-line follow-up fix is to thread onresumptiontoken and onRequestStreamEnd through the resume branch's _startOrAuthSse call.

Extended reasoning...

What the bug is. At the top of _send (packages/client/src/client/streamableHttp.ts:921-935), when options.resumptionToken is set the transport takes an early-return branch that resumes the SSE stream via GET + Last-Event-ID. Line 921 destructures onresumptiontoken from options, but the branch then calls _startOrAuthSse({ resumptionToken, replayMessageId, requestSignal: options?.requestSignal }) — dropping both onresumptiontoken and onRequestStreamEnd. Inside _handleSseStream, new event ids are delivered to the caller only via onresumptiontoken?.(event.id), which is undefined on this path, so the caller's token-persistence callback never fires for the resumed stream. The reconnect loop (_scheduleReconnection_startOrAuthSse) re-threads the same StartSSEOptions object, so the callback stays undefined across every subsequent auto-reconnect of that stream, too.\n\nWhy it's an omission, not a design choice. TransportSendOptions.onresumptiontoken (packages/core-internal/src/shared/transport.ts:65-69) is documented as the mechanism for clients to "persist the latest token for potential reconnection", and Protocol.request threads it through to transport.send on every request. Crucially, the public resumeStream() API (streamableHttp.ts:1214-1218) does pass onresumptiontoken to _startOrAuthSse for the identical underlying operation — only the send()-with-resumptionToken path drops it. The non-resume POST path also passes it into _handleSseStream. This branch is the single inconsistent site. The recent requestSignal-threading comment on lines 925-928 shows this branch was audited for exactly this class of per-request-option omission, and onresumptiontoken was still missed.\n\nConcrete trigger (in-repo caller). examples/repl/client.ts:688-693 is the documented resumability pattern: client.callTool({...}, { resumptionToken: notificationsToolLastEventId, onresumptiontoken: onLastEventIdUpdate }).\n\nStep-by-step proof.\n1. First callTool (no token yet): the POST path passes onresumptiontoken into _handleSseStream; each SSE event id fires the callback; the caller persists token T1, T2, … up to T5.\n2. The stream drops. The caller re-issues the call with resumptionToken: 'T5'.\n3. _send takes the resume branch and calls _startOrAuthSse({ resumptionToken: 'T5', replayMessageId, requestSignal }) — no onresumptiontoken.\n4. The resumed stream delivers events T6T20; _handleSseStream evaluates onresumptiontoken?.(event.id) with undefined each time. The caller's persisted token stays frozen at T5.\n5. The stream drops a second time. The caller resumes from the stale T5: the server replays T6T20 (duplicate notifications delivered to onmessage), or — if the server's event store has evicted T5 — the resume fails outright.\n\nWhy nothing else prevents it. There is no other channel for token delivery on this path: _handleSseStream is the only place event ids reach the caller, and its internal reconnects propagate the same options object with onresumptiontoken: undefined. onRequestStreamEnd is likewise dropped, though no current in-repo caller combines it with resumptionToken, so the token callback is the concrete harm.\n\nWhy pre-existing. This PR edits only the response-status handling further down in _send (session-id capture on response.ok and the 404 clear) and does not touch the resume branch; the identical omission ships in v1.29 (dist/esm/client/streamableHttp.js ~line 293). It's flagged here because the PR audits precisely this function's per-request option/state threading. Not blocking.\n\nHow to fix (one-liner follow-up). Thread both dropped options through:\nts\nthis._startOrAuthSse({\n resumptionToken,\n onresumptiontoken,\n replayMessageId: isJSONRPCRequest(message) ? message.id : undefined,\n requestSignal: options?.requestSignal,\n onRequestStreamEnd: options?.onRequestStreamEnd\n}).catch(error => this.onerror?.(error));\n\nThis matches what resumeStream() already does and what the non-resume POST path passes to _handleSseStream.

…nses

StreamableHTTPClientTransport stored the mcp-session-id response header
before checking response.ok, so a session id on an error reply became the
connection's session state. With version negotiation in auto mode this
became reachable on every connect: a legacy server whose 404 to the probe
carried a session id made the fallback initialize present a session id
the server never issued — spec-conforming stateful servers reject that,
failing a connect that plain legacy mode completes. The stale id also
made a retry connect() on the same transport take the session-resuming
branch and skip the handshake entirely.

Capture now happens only on successful responses, matching the spec's
assignment point (the InitializeResult response). Additionally, a 404 to
a POST that carried the session id clears the stored id — per spec the
session is gone and the next attempt must start a fresh handshake. POST
only: SSE reconnect GETs can 404 for transient infra reasons while the
session is still live.

Regression tests pin the error-path non-capture, the session-less
follow-up request, the 404 clear, and that successful capture (including
202 rotation) is unchanged.
@felixweinberger felixweinberger force-pushed the fweinberger/shttp-session-capture-on-ok branch from d867807 to c403c96 Compare July 9, 2026 09:40
Comment thread .pack-stage/rewrite-and-repack.mjs Outdated
Comment thread packages/client/src/client/streamableHttp.ts Outdated
…s; treat DELETE 404 as terminated

Review follow-ups: interpret each response against the session id its own
request carried (a late success from a previous session no longer installs
its stale id over a rotated one, and a late 404 no longer wipes a newer
session); terminateSession() now treats 404 as successful termination (the
session is already gone — the goal state — and the stored id is dropped
instead of left dead behind a throw); changeset motivation reworded to the
reconnect pattern that actually reproduces the resume-branch hazard; e2e
knownFailure note refreshed to describe the post-fix behavior.
@felixweinberger felixweinberger force-pushed the fweinberger/shttp-session-capture-on-ok branch 2 times, most recently from e9e6cb7 to ec450a1 Compare July 9, 2026 10:10
…rrent session

Per the spec's session-management requirement, a 404 to a request that
carried the session id means the session is gone and the client must
start a new one with a fresh session-less InitializeRequest. The
transport marks that condition on the thrown error; on a legacy-era
connection the client then re-runs the initialize handshake (deduped
across concurrent failures) and retries the failed request once.
Server-side per-session state (e.g. resource subscriptions) does not
carry over. The session-404-reinitialize e2e requirement now passes and
its knownFailure entry is removed; 404-surfaces narrows to the
unrecoverable case (re-establishment also fails). Comments and the
changeset tightened throughout.
- The terminated session id is remembered until a new one is assigned, so
  every in-flight request of the dead session recovers (previously only
  the first-processed 404 was marked and the dedup was unreachable).
- The stale protocol-version header is dropped with the session, so the
  recovery initialize goes out like a fresh connect.
- A failed re-establishment surfaces the request's own 404 (the handshake
  failure goes to onerror) instead of replacing it.
- The recovery handshake runs under the failing request's options, so
  caller timeouts/signals bound it.
- Pagination continuations are not retried — a dead session's cursor is
  meaningless to the new one.
- connect()'s doc block reattached to connect(); unit tests for the
  marker, sibling marking, dedup, failure surface, and cursor guard.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Additional findings (outside current diff — PR may have been updated during review):

  • 🟣 packages/client/src/client/streamableHttp.ts:1195-1200 — Pre-existing issue (not introduced by this PR): the if (transport.sessionId) 405-detection branch in examples/repl/client.ts:538-543 is unreachable dead code — every resolving path of terminateSession() (2xx, 405, and now 404 with this PR) unconditionally clears _sessionId, so the REPL prints "Session terminated successfully" / "Session ID has been cleared" even when the server declined the DELETE with 405. Since this PR widens the silent-resolve set to 404, the example's premise drifts further; the fix is to drop the dead branch in the example (or note that 405/404 also resolve silently).

    Extended reasoning...

    What the bug is. examples/repl/client.ts (the terminateSession() command handler, lines 532-550) awaits transport.terminateSession() and then branches on if (transport.sessionId) to detect the case where the server answered the session DELETE with 405 Method Not Allowed:

    await transport.terminateSession();
    console.log('Session terminated successfully');
    
    // Check if sessionId was cleared after termination
    if (transport.sessionId) {
        console.log('Server responded with 405 Method Not Allowed (session termination not supported)');
        console.log('Session ID is still active:', transport.sessionId);
    } else {
        console.log('Session ID has been cleared');
        ...
    }

    That branch can never execute. In StreamableHTTPClientTransport.terminateSession() (packages/client/src/client/streamableHttp.ts:1195-1211), the guard this PR modifies — if (!response.ok && response.status !== 405 && response.status !== 404) throw ... — means the method resolves on 2xx, 405, and (new in this PR) 404, and every resolving path falls through to the unconditional this._sessionId = undefined. The only other resolving path is the early return when _sessionId is already unset. So after any resolved terminateSession(), transport.sessionId is always undefined, and the example's only discriminator is useless.

    Step-by-step proof. (1) REPL connects to a stateless-topology server that declines session DELETEs; _sessionId = 'S1'. (2) User runs the terminate command; the DELETE goes out with mcp-session-id: S1. (3) Server answers 405. (4) In terminateSession(), !response.ok is true but status === 405, so the throw is skipped and execution reaches this._sessionId = undefined; the method resolves. (5) Back in the example, transport.sessionId is undefined, so the else branch runs: the REPL prints "Session terminated successfully" and "Session ID has been cleared", then closes the transport and nulls the client — telling the user termination succeeded when the server actually refused it, and the 405-specific message the branch was written to print never appears.

    Why this is pre-existing. Verified against pre-PR history (git show HEAD~5): the v1/pre-PR guard was if (!response.ok && response.status !== 405) followed by the same unconditional clear, so 405 already fell through to _sessionId = undefined and the example branch was already dead before this PR. This PR does not introduce the defect — it edits the exact guard whose resolve semantics the example misreads, and widens the silent-resolve set from {2xx, 405} to {2xx, 405, 404}, so the example's assumption (that a resolved call plus sessionId state distinguishes "terminated" from "declined") drifts one status further from reality.

    Why nothing prevents it. terminateSession() exposes no way to observe which status resolved it — 2xx (terminated), 405 (declined), and 404 (already gone) all resolve identically and clear the id identically. The example's transport.sessionId probe is the only signal available, and it is constant across all three outcomes.

    Impact. Example-only: the REPL misinforms users about what the server did (claims success + clears state on a declined DELETE), and the dead branch teaches readers a detection pattern the SDK does not support. No SDK runtime behavior is wrong.

    How to fix. Update the example: drop the dead if (transport.sessionId) branch (or replace it with a comment noting that terminateSession() also resolves silently on 405/404 and always clears sessionId). A status-discriminating return value on terminateSession() would be the API-level alternative, but that is a larger design question and out of scope for this PR; per repo minimalism the example-side fix is the right scope.

Comment thread packages/client/src/client/client.ts Outdated
Comment thread packages/client/src/client/client.ts Outdated
Comment thread packages/client/src/client/client.ts
Comment thread packages/client/src/client/streamableHttp.ts
@felixweinberger felixweinberger changed the title fix(client): only capture the session id header from successful responses fix(client): align Streamable HTTP session-id lifecycle with the spec Jul 9, 2026
…inuations

Session recovery after a terminated-session 404 previously covered only
plain requests. A pagination continuation or a client-sent notification
that was the first to observe a dead session left the client without a
session id and with no re-initialize, wedging it (a stateful server
answers subsequent session-less requests with 400, which never triggers
recovery). Now:

- Cursor continuations re-establish the session before surfacing the
  404, so the caller can restart pagination on a live session.
- notification() recovers and resends once, mirroring request();
  notifications/initialized is exempt since it is part of the recovery
  handshake itself.
- The recovery handshake runs without the failing request's options, so
  a per-request signal, timeout, or onprogress cannot cancel or
  decorate the shared initialize that concurrent recoveries depend on.
- The transport's 404 session-clear cancels a pending SSE stream
  reconnect (it would fire into the new session with a stale
  last-event-id) and fires its onRequestStreamEnd so a per-request
  caller waiting on the resume settles instead of hanging.

Also documents the 404 behavior changes in the migration guide and the
terminateSession JSDoc, and drops the repl example's unreachable 405
detection branch (terminateSession always clears the session id on
every resolving path).
The spec assigns Mcp-Session-Id at initialization time, on the HTTP
response containing the InitializeResult, and is silent on the header
elsewhere. Key the client transport's capture on the outgoing POST
carrying an InitializeRequest instead of guarding by the sent session
id: a session header on any other successful response is now ignored
entirely. The sent-id guard remains only where it still matters — the
404 termination transition, which must be attributed to the session
the request actually presented.

Drop the tests that blessed mid-session rotation via ordinary 2xx
replies and replace them with handshake-keyed coverage: capture from
an initialize response (single and batched), no capture from any
non-initialize response (same or different id), and the late-404
race now recovers through a real re-initialize handshake. Document
the behavior change in the v2 migration guide.
Comment on lines +940 to +952
override async notification(notification: Notification, options?: NotificationOptions): Promise<void> {
try {
return await super.notification(notification, options);
} catch (error) {
// `notifications/initialized` is part of the recovery handshake
// itself — recovering on it would await the in-flight recovery.
if (notification.method === 'notifications/initialized' || !this._isSessionTerminated(error)) {
throw error;
}
await this._recoverSession(error);
return await super.notification(notification, options);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Two send paths escape the new session-recovery fence and end up POSTing session-less, which the SDK's own stateful server answers with a 400 that can never be marked sessionTerminated (both marking branches require sentSessionId !== null), so recovery can never arm afterwards: (1) a notification debounced via debouncedNotificationMethods (e.g. notifications/roots/list_changed, the option's own JSDoc example) is sent in an un-awaited microtask whose error goes only to _onerror (protocol.ts:1637) — this override's catch never sees the terminated-session 404, the notification is silently lost, and the client is permanently wedged; (2) new requests/notifications initiated during the recovery window (after the transport's 404-clear, before the recovery initialize restores _sessionId) go out session-less and fail with a hard 400 instead of joining the in-flight _sessionReinit. Route the debounced send's failure through the same recovery path, and fence new sends on the in-flight _sessionReinit (e.g. if (this._sessionReinit) await this._sessionReinit.catch(() => {}) at the top of both overrides).

Extended reasoning...

Path 1 — debounced notifications bypass the notification() override entirely (permanent wedge). The override recovers by catching the error from await super.notification(...), but Protocol. _notificationViaCodec has a second send path the catch cannot observe: when the client is constructed with debouncedNotificationMethods (ProtocolOptions, protocol.ts:89) and the notification is simple (no params, no relatedRequestId), the send is deferred to a microtask and fired un-awaited — this._transport?.send(jsonrpcNotification, options).catch(error => this._onerror(error)) (protocol.ts:1626-1638). super.notification() returns success immediately, so the terminated-session 404 goes only to onerror and _recoverSession never runs. sendRootsListChanged() (client.ts:2610-2612) sends exactly the eligible shape, and the option's own JSDoc example is a list_changed method — the trigger is the documented usage.\n\nStep-by-step proof (path 1).\n1. Client created with debouncedNotificationMethods: ['notifications/roots/list_changed'] on a legacy-era connection to a stateful server; session S1 established.\n2. The server expires S1. The host calls client.sendRootsListChanged() as the first observer of the dead session.\n3. The debounced microtask POSTs with mcp-session-id: S1 → 404. The transport's new 404 branch fires (streamableHttp.ts, the sentSessionId === this._sessionId clear): _sessionId and _protocolVersion wiped, _terminatedSessionId = S1, error marked sessionTerminated — but the error dies in _onerror. No _recoverSession, and the notification is silently lost.\n4. Every subsequent request goes out session-less via _commonHeaders(). The SDK's own stateful server rejects a session-less non-initialize POST with 400 Mcp-Session-Id header is required (server streamableHttp.ts:940-941) — never a 404, and never with sentSessionId !== null, so neither transport marking branch can ever fire again.\n5. _isSessionTerminated requires status 404 and sessionTerminated === true, so the automatic recovery is permanently unreachable; the migration guide's 404-keyed host recovery also never matches (all failures are now 400), and a second connect() on the same transport throws already started. The client is wedged until the host builds a brand-new transport.\n\nPath 2 — traffic during the recovery window falls into an unhandled third category (transient). The 404-clear wipes _sessionId/_protocolVersion synchronously in the failing request's continuation, and the new id only returns with the recovery initialize's 2xx capture — so there is a ≥1-RTT window with no session id. The request()/notification() overrides have no fence: a call issued during the window goes straight to super.transport.send_commonHeaders(), emitting a POST with no mcp-session-id and no mcp-protocol-version header, which the stateful server 400s. That 400 is recognized by nothing — it is not deduped into _sessionReinit (the dedup keys on the terminated-session 404 via _terminatedSessionId, which only marks requests that went out with the dead id), _isSessionTerminated is false so the override rethrows raw, and the docs-prescribed 404-keyed host recovery does not match either. Concretely: request A 404s and starts recovery; a second tool call C issued while the handshake round-trips gets a hard, caller-visible SdkHttpError(400) — a spurious failure the machinery was actively (and successfully) absorbing for A. This half self-heals (the next attempt carries the fresh id), but it converts a previously host-recoverable 404 into a 400 nothing recovers, precisely inside the feature's own window.\n\nWhy existing code doesn't prevent either. Both failures land in the same unrecoverable state through the same mechanism: once the transport-side clear runs, any send that bypasses the recovery-aware catch goes out session-less, and a session-less request can never re-arm the sessionTerminated marking (both branches require sentSessionId !== null). The changeset's "deduped across concurrent failures" claim covers only raced siblings that went out with the dead id. Path 1 is the repo's recurring partial-migration pattern: commit 2602078 added recovery to the awaited notification path after the earlier review comment, but the debounced sibling path at protocol.ts:1637 was left with the identical gap; no test covers debounced + terminated-session. Path 2 is a timing hole in the dedup's own target scenario.\n\nNew-in-PR surface. Pre-PR, a debounced notification's 404 was merely a lost notification with session state intact — every later request still carried the id, still 404d, and the documented 404-keyed host recovery kept matching. The PR's unconditional transport-side clear converts that into a silent, permanently session-less wedge, while the changeset promises notification recovery. Likewise for path 2, the same concurrent request pre-PR failed with the documented 404 rather than an unrecognizable 400.\n\nHow to fix. (1) Route the debounced send's failure through the recovery path — e.g. have the deferred send's rejection call a hook the Client can override instead of going straight to _onerror, or trigger a lazy reinit on the next request() when the transport reports _terminatedSessionId set with _sessionId === undefined (or simply skip debouncing in Client.notification on legacy sessions). (2) Fence new work on the in-flight recovery: at the top of both overrides, before the first super. attempt, if (this._sessionReinit) { await this._sessionReinit.catch(() => {}); } — new calls then wait the handshake out and go out carrying the fresh id. Severity reflects path 1: it is gated on the opt-in debouncedNotificationMethods option, but when triggered the client silently and permanently wedges — the same failure class as the cursor/notification wedge already fixed in this PR, and it should be closed alongside it since the PR introduces the clear that creates it.

Comment on lines +969 to +977
private _isSessionTerminated(error: unknown): boolean {
return (
error instanceof SdkHttpError &&
error.data.status === 404 &&
error.data['sessionTerminated'] === true &&
// Legacy era only: the 2026-07-28 handshake is discover-based.
this._negotiatedProtocolVersion !== undefined &&
legacyProtocolVersions([this._negotiatedProtocolVersion]).length === 1
);

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 recovery never arms on session-resume reconnects: _isSessionTerminated requires this._negotiatedProtocolVersion !== undefined, but both resume branches (the plain legacy connect at client.ts:1034-1043 and _connectNegotiated's resume at :1141-1148) skip the handshake and never set it — so on the documented { sessionId, protocolVersion } reconnect pattern the gate always returns false. When the resumed session later expires, the transport's 404-clear still wipes _sessionId/_protocolVersion, every subsequent request goes out session-less and gets a 400 (never a sessionTerminated 404) from the SDK's stateful server, permanently wedging the client with both automatic and documented 404-keyed host recovery unreachable. Fix: seed _negotiatedProtocolVersion from transport.protocolVersion in the resume branches, or let the gate fall back to the transport's protocol version.

Extended reasoning...

What the bug is. The recovery gate _isSessionTerminated (packages/client/src/client/client.ts:969-977) requires this._negotiatedProtocolVersion !== undefined && legacyProtocolVersions([...]).length === 1. But on the documented session-resume reconnect pattern — construct a new StreamableHTTPClientTransport with { sessionId, protocolVersion } (per the protocolVersion option JSDoc) and connect a fresh Client_negotiatedProtocolVersion is never populated. Both resume branches only read it and return early: the plain legacy connect resume branch (client.ts:1034-1043) and _connectNegotiated's resume branch (client.ts:1141-1148). The only writers are _legacyHandshake completion (:1116, skipped by design on resume), the modern negotiated path (:1185), and _connectFromPrior (:1305) — none of which run on a resume connect — and _resetConnectionState (:533) nulls it. So on a resumed connection the field stays undefined for the connection's lifetime, and this is exactly the path the typescript:client-transport:http:session-id-option e2e test exercises (fresh Client + transport constructed with {sessionId}).\n\nThe failure sequence. When the resumed session later expires server-side:\n\n1. Host reconnects: new StreamableHTTPClientTransport(url, { sessionId: 'S1', protocolVersion: '2025-03-26' }), fresh Client, connect() takes the resume branch — no handshake, _negotiatedProtocolVersion stays undefined. Requests work fine (the transport sends the headers from its constructor options).\n2. The server expires S1 (TTL, restart, eviction — the persist-and-reconnect deployment is precisely where this is most likely).\n3. The next request POSTs with mcp-session-id: S1 → 404. The transport's 404-clear does run (streamableHttp.tssentSessionId === this._sessionId, both from the constructor option): it wipes _sessionId and _protocolVersion and marks the error sessionTerminated: true.\n4. Client.request's catch calls _isSessionTerminated, which returns false purely because _negotiatedProtocolVersion is undefined — no recovery handshake is attempted, and the raw 404 surfaces once.\n5. Every subsequent request goes out with neither mcp-session-id nor mcp-protocol-version. The SDK's own stateful server rejects a session-less non-initialize POST with 400 "Bad Request: Mcp-Session-Id header is required" (packages/server/src/server/streamableHttp.ts:938-941) — never a 404, and never sessionTerminated (both transport marking branches require sentSessionId !== null). So recovery can never arm later either; a same-instance connect() retry throws "already started". The client is permanently wedged.\n\nWhy this is a regression, not just a missing feature. Pre-PR, every request against the dead session kept 404ing, so the docs-prescribed host recovery ("key off the HTTP 404 status", docs/migration/upgrade-to-v2.md) matched from any observer. Post-PR, the first 404 flips the failure mode to 400s — 404-keyed host recovery silently stops matching after the first failure. The unconditional transport-side clear and the era-gated client-side recovery, both added by this PR, disagree about when recovery will run, and the gap between them is the wedge.\n\nWhy it contradicts the PR's own docs. The changeset and docs/migration/upgrade-to-v2.md (session 404 bullet) promise: "on a pre-2026 connection the client re-initializes automatically and retries the failed request once." A resumed connection is definitionally pre-2026 — only the legacy era has session ids — and the gate's own comment ("Legacy era only: the 2026-07-28 handshake is discover-based") shows the intent is era-scoping, not resume-exclusion. This is an oversight, not a design choice: _reinitializeSession_legacyHandshake(transport) does not depend on _negotiatedProtocolVersion at all, so recovery would work fine if the gate let it through.\n\nWhy the tests miss it. sessionReinitialize.test.ts always runs a real initialize first (so _negotiatedProtocolVersion is set), and the session-id-option e2e test never expires the resumed session — no test covers resume-connect + expiry.\n\nHow to fix. Small change, two equivalent options: (a) seed _negotiatedProtocolVersion from transport.protocolVersion in the two resume branches (the getter exists on StreamableHTTPClientTransport), or (b) make _isSessionTerminated fall back to the transport's protocol version when the negotiated one is undefined (an undefined negotiated version on a session-carrying transport is by construction a legacy connection — the era gate is effectively redundant with the sessionTerminated flag, which can only arise from legacy Mcp-Session-Id mechanics). Either way, a test covering resume-connect followed by session expiry would pin the behavior.

Comment on lines +1567 to +1570
response (including error replies), so a server that (re)assigned session ids on
ordinary responses could silently rotate the client's session. The spec defines
that single assignment point and is silent on the header elsewhere, so the client
now ignores it on every other response — a server that wants a session must

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 closing sentence of the new session-404 bullet — "Host code keyed off that 404 now only sees it when the automatic recovery has also failed, at which point the client is closed" (echoed at line ~1627) — is false for pagination continuations: per the cursor guard in Client.request, a cursor-carrying request surfaces the original 404 even when recovery succeeded, with the client open and fully usable (the PR's own test asserts a follow-up tools/list succeeds after the surfaced 404). A host following the guide as written would tear down a healthy, just-recovered client on every cursor-page session expiry; qualify both sentences to carve out the pagination case.

Extended reasoning...

What the docs claim vs. what the code does. The new migration-guide bullet (docs/migration/upgrade-to-v2.md:1569-1570) ends with: "Host code keyed off that 404 now only sees it when the automatic recovery has also failed, at which point the client is closed." The same claim is repeated at :1625-1627: "hosts only observe it when that recovery fails." But the code has a third outcome the sentence doesn't cover: in the Client.request override (packages/client/src/client/client.ts:923-935), await this._recoverSession(error) runs before the cursor guard, and only after recovery succeeds does the 'cursor' in request.params check rethrow the original 404. So for a pagination continuation the host observes a terminated-session 404 while the client is open, recovered, and fully usable.

The code path, concretely:

  1. Host paginates: client.listTools() returns nextCursor; the session then expires server-side.
  2. client.request({ method: 'tools/list', params: { cursor } }) POSTs with the dead session id → 404 with sessionTerminated: true.
  3. The override's catch runs await this._recoverSession(error) — a fresh session-less initialize/initialized handshake succeeds; the transport now holds a new live session id.
  4. Only then does the cursor guard match and rethrow the original 404 (the dead session's cursor is meaningless to the new session).
  5. The host's promise rejects with a terminated-session 404 — while the client is connected and the next request works.

The PR's own test pins this. sessionReinitialize.test.ts's "recovers the session on a pagination continuation but does not retry it" asserts exactly the sequence above: the cursor request rejects with the 404, the wire trace shows ['initialize', 'tools/list', 'initialize'] (recovery ran), and a follow-up tools/list on the same client then succeeds. The changeset (.changeset/shttp-session-capture-on-ok.md) even describes it accurately — "pagination continuations re-establish the session but surface the 404" — so the migration guide contradicts the PR's own more careful wording. The bullet's earlier parenthetical ("pagination continuations are not retried") hints at the exception, but the closing sentence then asserts the opposite unconditionally.

Why it matters. The migration guide is the document hosts are told to code against (the :1623-1625 prose explicitly directs client code to "key off the HTTP 404 status"). A host that follows the sentence literally — "a surfaced terminated-session 404 means recovery failed and the client is closed, so rebuild client + transport" — will discard a healthy, just-recovered client every time a cursor page hits session expiry. That is precisely the reconnect churn the automatic recovery feature was added to eliminate, and mid-pagination expiry is a realistic ordering (long list walks are exactly when a session TTL is likely to lapse). Note this isn't confined to manual pagination: _listAllPages (behind the default no-cursor listTools/listPrompts/listResources aggregate walk) issues {...params, cursor} requests for pages 2+, so the most common list path can surface this 404 with the client open.

Why nit, not blocking. This is docs-only — nothing in the SDK misbehaves at runtime; the damage requires a host to code to the sentence literally, and the fix is a one-line qualification in each of the two places.

How to fix. Qualify both sentences, e.g. at :1569-1570: "Host code keyed off that 404 now only sees it when the automatic recovery has also failed (the client is closed at that point) — or when the failed request was a pagination continuation, in which case the session has been re-established and the client remains usable; restart pagination from the first page." And at :1627: "hosts only observe it when that recovery fails, or on a pagination continuation (session recovered; restart pagination)."

Comment on lines +1005 to +1013
this._sessionId = undefined;
this._protocolVersion = undefined;
// A reconnect pending for the dead session's SSE stream
// would fire into the new session with a stale
// last-event-id — drop it, and settle any per-request
// caller waiting on that stream (it is definitively gone).
this._cancelReconnection?.();
this._cancelReconnection = undefined;
this._pendingReconnectOptions?.onRequestStreamEnd?.();

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 stale-reconnect teardown added on the 404 session-clear is single-slot while multiple SSE reconnects can be pending at once: _cancelReconnection/_pendingReconnectOptions are overwritten by every _scheduleReconnection call, so when a server restart drops the standalone GET stream and a primed POST SSE stream together, only the last-scheduled reconnect is cancelled — the orphaned timer (or a reconnect already in flight, whose catch re-arms itself) later fires into the recovered session with the dead session's last-event-id under the new session id, exactly the cross-session leak the new comment says this block prevents. Consider tracking pending reconnects in a Set of {cancel, options} entries, or adding a session-generation check inside the reconnect closure so reconnects armed under a since-terminated session refuse to fire.

Extended reasoning...

What the bug is. The 404 teardown in _send (packages/client/src/client/streamableHttp.ts:1005-1013) cancels "the" pending SSE reconnection via this._cancelReconnection?.() and settles "the" waiting per-request caller via this._pendingReconnectOptions?.onRequestStreamEnd?.(). But both are single instance fields that every _scheduleReconnection call overwrites (lines 675-680, in both the scheduler branch and the setTimeout branch), while multiple reconnections can genuinely be pending simultaneously: this transport is hasPerRequestStream = true, so _handleSseStream schedules a reconnect per dropped stream — the standalone GET stream (isReconnectable = true) and every primed per-request POST SSE stream (hasPrimingEvent). A server restart that kills the session — the exact trigger this teardown was written for — typically drops all of them at once.

The escape paths. Two orderings escape the teardown:

  1. Overwritten slot. GET stream drops → _scheduleReconnection arms timer A and stores its cancel handle; a primed POST stream drops → timer B overwrites the slot, making A's cancel handle unreachable. When a pending POST then 404s with the current session id, the new block cancels only B and fires only B's onRequestStreamEnd. Timer A stays armed.
  2. In-flight window. reconnect() clears both slots at its start (lines 656-657) before awaiting _startOrAuthSse. A 404 arriving while that reconnect GET is in flight finds nothing to cancel; _startOrAuthSse has no 404 session-clear of its own (the new comment explicitly scopes the clear to "POSTs only"), so the GET fails generically and its .catch (lines 662-670) calls _scheduleReconnection(options, attempt + 1) — re-arming the stale-token reconnect after the teardown already ran.

Why nothing else stops the escaped reconnect. The reconnect closure gates only on the transport-wide abort and the per-request requestSignal — neither fires during session recovery (unlike close(), where the same single-slot limitation is harmless because _abortController.abort() makes every closure bail). _startOrAuthSse rebuilds headers via _commonHeaders() at attempt time, so the escaped GET goes out with the new session id plus the dead session's last-event-id — precisely the cross-session resumption-token leak the comment at lines 1006-1009 claims this block prevents.

Step-by-step proof. (1) Legacy-era connect under session S1; standalone GET stream A open; a long-running tools/call holds primed POST stream B. (2) Server restarts, killing S1: A and B both drop; A's reconnect is scheduled (slot ← A), then B's (slot ← B; A's cancel handle orphaned). (3) A pending POST 404s with S1 → the teardown cancels B only, fires B's onRequestStreamEnd only. (4) The Client-level recovery re-initializes → _sessionId = S2. (5) A's timer fires (~1s backoff, comfortably straddling the fast handshake); both abort checks pass; _startOrAuthSse sends GET with mcp-session-id: S2 + last-event-id: <S1 token>. Against the SDK's own server this gets a 400 (unknown event id) or races the recovery's own standalone GET for the one-stream slot (409 to whichever loses); against a permissive non-conforming server it can produce a second live standalone stream and duplicate notification delivery.

One correction to the raw finding, and why this is a nit. The overwritten per-request caller is not left waiting forever against a conforming server: the orphaned timer still fires, its GETs fail, retries exhaust at maxRetries (default 2), and _scheduleReconnection (lines 645-649) then fires options.onRequestStreamEnd?.() — so settlement is late and noisy (spurious onerror, wasted GETs), not lost; a true hang needs a non-conforming server that accepts the stale token as a live stream. That bounded, self-limiting impact — plus the narrow trigger (multi-stream simultaneous drop racing a terminated-session 404 within the ~1s backoff window) — is why this is a nit rather than blocking. The single-slot pattern itself predates this PR (close() shares it, harmlessly), but this PR is what makes same-transport session replacement a supported flow and adds the correctness claim that depends on the teardown being complete, so the residual gap belongs here rather than as pre-existing.

How to fix. Track pending reconnects in a Set of { cancel, options } entries instead of one slot (the 404 teardown drains the whole set; close() can share it), and/or add a session-generation counter captured in the reconnect closure and re-checked before firing — the same deferred-callback-must-recheck-state shape the surrounding code already applies to the capture/clear guards. Either closes both the overwritten-slot and the in-flight re-arm paths.

@felixweinberger

Copy link
Copy Markdown
Contributor Author

Closing: this grew far past its purpose. Superseding with a minimal fix — initialize requests never carry a session id, and the id is only captured from the initialize response (both per spec §Session Management). The session-recovery work here remains available on the branch if we want it as its own PR later.

Comment on lines +941 to +953
try {
return await super.notification(notification, options);
} catch (error) {
// `notifications/initialized` is part of the recovery handshake
// itself — recovering on it would await the in-flight recovery.
if (notification.method === 'notifications/initialized' || !this._isSessionTerminated(error)) {
throw error;
}
await this._recoverSession(error);
return await super.notification(notification, options);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 This notification() override never sees the notifications/cancelled POST that Protocol.request's cancel closure sends directly on the transport (protocol.ts:1453-1465, taken on every legacy-era abort or request timeout, since streamCloseCancels requires the modern era) — so when that POST is the first observer of a dead session, the transport's 404-clear wipes _sessionId/_protocolVersion but the sessionTerminated error dies in _onerror and _recoverSession never runs. Every subsequent request then goes out session-less and gets a hard 400 (never a sessionTerminated 404), permanently wedging the client with no opt-in required. This is a third sibling of the two escape paths already flagged (debounced notifications, recovery-window sends); a lazy reinit on the next request when the transport holds _terminatedSessionId with _sessionId === undefined would close all three.

Extended reasoning...

What the bug is. The new Client.notification() override (client.ts:941-953) adds session recovery for notifications, but Protocol's request-cancellation path never goes through it. In Protocol.request, the cancel closure (packages/core-internal/src/shared/protocol.ts:1446-1473) checks requestAbort === undefined — which is always true on a legacy-era connection, because streamCloseCancels requires codec.era === MODERN_WIRE_REVISION (protocol.ts:1416) — and then calls this._transport?.send({ method: 'notifications/cancelled', ... }).catch(error => this._onerror(...)) directly on the transport (protocol.ts:1453-1465). The send bypasses Client.notification() entirely, so its rejection can only ever reach _onerror; _recoverSession never runs.

The trigger is routine — no opt-in needed. cancel() fires from two everyday sources: the caller's options.signal abort (protocol.ts:1549-1550) and every request timeout (protocol.ts:1552-1555, default 60s). A realistic ordering: a long-running tools/call is in flight when the server restarts and drops the session; the request's SSE stream dies with it, the timeout fires, and the resulting notifications/cancelled POST — carrying the dead session id — becomes the first observer of the terminated session. Unlike the already-flagged debouncedNotificationMethods path, nothing has to be configured for this to happen.

Step-by-step proof.

  1. Legacy-era connection to a stateful server; session S1 established. Server expires S1 (TTL, restart, eviction).
  2. The host aborts an in-flight tools/call (or its timeout fires). cancel() runs; since requestAbort is undefined on legacy era, the closure POSTs notifications/cancelled directly via this._transport.send(...) with mcp-session-id: S1.
  3. The server answers 404. The transport's new clear branch fires (streamableHttp.ts, the sentSessionId === this._sessionId arm): _sessionId and _protocolVersion are wiped, _terminatedSessionId = S1, the error is marked sessionTerminated: true — but the rejection lands in the closure's .catch_onerror. No recovery hook observes it. (The aborted request itself can't rescue anything either: cancel() already rejected its promise with SdkError(RequestTimeout), so its own late rejection is a no-op on a settled promise.)
  4. The next client.callTool() goes out with no mcp-session-id and no mcp-protocol-version. The SDK's own stateful server rejects session-less non-initialize POSTs with 400 "Bad Request: Mcp-Session-Id header is required" (packages/server/src/server/streamableHttp.ts:938-941) — never a 404.
  5. _isSessionTerminated requires a 404 with sessionTerminated === true, and both transport marking branches require sentSessionId !== null — which session-less requests can never satisfy. So recovery can never re-arm, the docs-prescribed 404-keyed host recovery stops matching (all failures are now 400s), and a same-instance connect() retry throws already started. The client is permanently wedged until the host constructs a brand-new transport.

Why this is a regression from this PR. Pre-PR, a 404 to the cancel POST was harmless: session state stayed intact, every later request kept carrying S1 and kept 404ing, so host-side 404-keyed recovery worked from any observer. The PR's unconditional transport-side clear plus the recovery hooks living only in the request()/notification() overrides is what converts this into a silent, permanent wedge.

Not a duplicate. The existing comment on this override flags two escape paths — the debouncedNotificationMethods microtask (protocol.ts:1637) and new sends during the recovery window. This is a third, distinct send site: the direct transport.send inside Protocol.request's cancel closure. Fixing exactly the two flagged paths (e.g. routing the debounced send's failure through recovery, fencing new sends on _sessionReinit) would leave this one broken — and it's the broadest of the three, requiring no options at all.

How to fix. Either route the cancel-notification send's failure through the same recovery/marking-aware path (a Protocol-level hook the Client can override, mirroring how the awaited notification path was fixed in 2602078), or — the option that closes all three sibling paths at once — lazily re-initialize on the next request()/notification() when the transport reports _terminatedSessionId set with _sessionId === undefined. A test pinning legacy-era cancel/timeout racing session expiry would prevent this partial-migration pattern from recurring.

Comment on lines +955 to +968
this._sessionReinit ??= this._reinitializeSession().finally(() => {
this._sessionReinit = undefined;
});
try {
await this._sessionReinit;
} catch (reinitError) {
// Re-establishment failed (the client is closed at this
// point): surface the request's own 404; the handshake
// failure goes to onerror.
this.onerror?.(reinitError instanceof Error ? reinitError : new Error(String(reinitError)));
throw cause;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 When N concurrent requests/notifications are deduped into one shared recovery handshake via _sessionReinit ??= and that single initialize fails, each waiter's own catch in _recoverSession calls this.onerror?.(reinitError) — so the host's onerror receives the same handshake error N times for one failure (the client was closed exactly once). Report the reinit error once where _sessionReinit is created (or guard with a reported flag) and have each waiter's catch just throw cause.

Extended reasoning...

What the bug is. _recoverSession (packages/client/src/client/client.ts:954-967) deliberately deduplicates concurrent terminated-session recoveries into one shared handshake: this._sessionReinit ??= this._reinitializeSession().finally(...). That dedup is correct and pinned by this PR's own deduplicates concurrent recoveries into one handshake test. But the failure reporting is per-waiter: every caller that awaits the shared promise has its own catch (reinitError) block running this.onerror?.(reinitError instanceof Error ? reinitError : new Error(String(reinitError))). When the one recovery initialize fails, that rejected promise is observed by every deduped waiter, and each one independently reports the same error object to onerror.

The code path that triggers it. This is exactly the scenario the dedup exists for: (1) session S1 expires server-side; (2) N in-flight requests/notifications each get a terminated-session 404; (3) all N enter _recoverSession, the first creates _sessionReinit, the rest join it; (4) the recovery handshake fails (e.g. server returns 500 to the fresh initialize); (5) _legacyHandshake's catch (client.ts:1121-1125) runs void this.close() — once — and rethrows with no onerror of its own; (6) all N waiters' catches fire, each calling this.onerror?.(reinitError). One handshake, one close, N identical onerror invocations.

Step-by-step proof. Take the PR's own dedup test (sessionReinitialize.test.ts, two concurrent callTools failing on the same dead session) and flip on failReinit: true while collecting client.onerror calls in an array: both tool-call promises reject with their own 404 (correct), but the reported array contains the status-500 handshake error twice — same Error instance, two callbacks. The existing failure-surface test only exercises a single failing waiter and asserts with reported.some(...), so the duplication is invisible to the suite.

Why nothing prevents it. There is no reported flag or once-guard anywhere on this path; the per-waiter catch is the sole reporting site (_legacyHandshake closes and rethrows without touching onerror). The code comment's own intent is singular — "the handshake failure goes to onerror" — and the close happens exactly once, so N reports of one event is an accuracy defect in the diagnostics, not intentional fan-out. Note per-request failure already surfaces to each caller via the rethrown cause (the original 404), so the duplicated onerror calls are strictly redundant.

Impact — why this is a nit, not blocking. Nothing functionally breaks: the client closes once, every waiter still rejects with its own 404, and onerror in this SDK is an out-of-band diagnostic channel with no exactly-once contract (reconnect attempts and transport failures already report per occurrence). The cost is noise: a host that counts, alerts, logs, or triggers reconnect logic from onerror sees N duplicate reports of a single failure event.

How to fix. Report once at the source instead of per-waiter — e.g. when creating _sessionReinit, attach the report in the shared chain:

this._sessionReinit ??= this._reinitializeSession()
    .catch(e => {
        this.onerror?.(e instanceof Error ? e : new Error(String(e)));
        throw e;
    })
    .finally(() => { this._sessionReinit = undefined; });

and reduce each waiter's catch to just throw cause. (A boolean reported flag guarding the existing call works equally well.) Extending the dedup test with failReinit: true and expect(reported).toHaveLength(1) would pin the once-only behavior.

Comment on lines +930 to +936
// A cursor minted by the dead session is meaningless to the new
// one: the session is recovered, but the 404 surfaces so the
// caller restarts pagination.
if (request.params !== undefined && 'cursor' in request.params) {
throw error;
}
return await super.request(request as never, schemaOrOptions as never, maybeOptions);

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 default no-cursor listTools()/listPrompts()/listResources()/listResourceTemplates() all route through _listAllPages, whose page-2+ requests carry a cursor — so a session expiring mid-walk makes the whole aggregate call reject with the terminated-session 404 (per the cursor guard here) even though _recoverSession already succeeded and the client is healthy. Since _listAllPages holds baseParams and the accumulator, consider catching the terminated-session 404 on a page continuation there and restarting the walk once from page 1, making the SDK-owned list path as transparently recoverable as callTool() already is.

Extended reasoning...

What happens. The cursor guard in the new Client.request override (client.ts:930-936) rethrows the terminated-session 404 for any request whose params contain cursor, on the rationale that "the caller restarts pagination." That rationale holds for a host-managed cursor — but the SDK's most common list path is itself that caller: the default listTools() (client.ts:2506), listPrompts() (:1601), listResources() (:1641), and listResourceTemplates() (:1674) all delegate to _listAllPages (:1701), which issues this.request({ method, params: { ...baseParams, cursor } }, options) for every page ≥ 2 (:1730) with no catch in the walk loop.

The code path, step by step.

  1. Legacy-era stateful server with a multi-page tool list; host calls client.listTools() (no cursor).
  2. Page 1 goes out as {...baseParams} — no cursor key — so if the session were already dead here, recovery + retry would work transparently.
  3. The session expires between page 1 and page 2 (long list walks are exactly where a TTL lapse lands).
  4. The page-2 request POSTs with the dead id → 404 with sessionTerminated: true. The override's catch runs _recoverSession — a fresh session-less initialize/initialized handshake succeeds.
  5. The cursor guard then matches ('cursor' in request.params) and rethrows the original 404. _listAllPages has no catch, so the entire listTools() promise rejects with a raw SdkHttpError 404 — while the client is open, recovered, and the very next call works.

Why this is a gap in the PR's own design, not just documented behavior. The changeset does say "pagination continuations re-establish the session but surface the 404," but the guard's justification — the caller restarts pagination — presumes the caller can. For the aggregate walk, the caller is _listAllPages itself: it holds baseParams and the accumulator and could trivially restart from page 1 after the recovery it just performed. The external host called listTools() with no cursor, never manages pagination, and receives an opaque 404 from a convenience API whose whole purpose is hiding pagination. Worse, per the migration guide's (separately flagged) claim that a surfaced 404 means "recovery has also failed, at which point the client is closed," a guide-following host would tear down a healthy, just-recovered client. The PR's own pagination test (sessionReinitialize.test.ts, "recovers the session on a pagination continuation but does not retry it") pins only the manual client.request({ ..., params: { cursor } }) case — the aggregate walk is untested.

Impact and why it's a nit. The failure is transient and self-healing: the session is re-established as part of the failed call, so an immediate host retry of listTools() succeeds; nothing wedges or closes. It's also not a regression — pre-PR the same mid-walk expiry rejected listTools() with a 404 and no recovery at all — and the trigger window is narrow (expiry must land exactly between pages of a typically sub-second walk). Merging as-is is strictly better than main; this is incomplete coverage of the new recovery feature on one common path, worth a follow-up rather than a blocker.

How to fix. In _listAllPages, catch the terminated-session 404 (SdkHttpError with status === 404 && data.sessionTerminated === true) on a page-continuation request and restart the walk once from page 1 — the session was already re-established by the request() override, so the restart just re-runs the loop with the retained baseParams and a fresh accumulator. (Alternatively, route the walk's internal page requests around the cursor guard with a single internal retry.) Note this is distinct from the earlier docs comment on upgrade-to-v2.md:1570, which asks only to qualify the migration-guide prose; this is the complementary code-level fix that would make the qualification largely moot for the SDK-owned path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant