Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(client): abort legacy SSE reconnect chain when the originating request times out#2616
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
295267e29c4e86e00d771473bb37c1b55ebec94475248cb693b486e8d3bc0a3adefd713f66066e0ab79e2728d040e6543396364856bf58a387ebeceFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| '@modelcontextprotocol/server': patch | ||
| --- | ||
| Stop the legacy-era (2025-11-25) Streamable HTTP SSE reconnect chain when its originating request settles. Previously, when a request on a legacy session timed out or was aborted, the client POSTed `notifications/cancelled` but the transport kept resuming the request's SSE stream via GET + Last-Event-ID indefinitely (each successful resume reset the retry counter), and a late resumed response surfaced as "Received a response for an unknown message ID". The protocol layer now threads a request-scoped abort signal to per-request-stream transports on the legacy era too, and aborts it on EVERY settlement path — cancel/timeout, `maxTotalTimeout` exceeded, and successful completion (which also releases the per-request abort listener otherwise leaked on Node 20.0–20.2) — the wire cancellation mechanism per era is unchanged (legacy connections still POST `notifications/cancelled`; modern 2026-07-28 connections still cancel via per-request stream close), and `maxTotalTimeout` settlements — which previously settled without any wire cancel signal at either era — now route through the request's cancel path and emit the era's signal (the `notifications/cancelled` POST on legacy connections and modern single-channel transports, the stream-close cancel on modern per-request-stream connections) while the caller still sees the original maxTotalTimeout error. This lives in the shared `Protocol` base, so server-initiated requests (`createMessage`, `elicitInput`) gain the same maxTotalTimeout cancellation signal. Additionally, the `notifications/cancelled` send no longer inherits the original request's `resumptionToken`, which on Streamable HTTP short-circuited the send into an SSE resume and silently swallowed the cancellation. For a request re-issued with `resumptionToken` on the SDK's own Streamable HTTP transport that POST is best-effort: the re-issued request's JSON-RPC id never reached the server (the send resumed via GET instead of POSTing), so the server cannot correlate the cancellation and such requests are only torn down locally; custom per-request-stream transports that POST the re-issued body normally give the server a correlatable id. (Same asymmetry inbound, pre-existing: progress notifications replayed on a resumed stream carry the original request's `progressToken`, so `onprogress`/`resetTimeoutOnProgress` do not survive a `resumptionToken` re-issue on this transport.) The client transport's `onerror` contract is also tightened: each failed SSE reconnect leg now reports exactly once with the underlying error (the `"Failed to reconnect SSE stream:"` wrapper message is gone), deliberate teardown (`close()` landing mid-POST/mid-GET/mid-DELETE, or a settled request's signal aborting its resume) no longer surfaces an `AbortError` through `onerror`, and `onRequestStreamEnd` now fires when a `resumptionToken` resume fails to open (a terminal outcome that previously reported only through `onerror`). | ||
| Two hardening details in the same machinery: `close()` now disarms every pending reconnect chain even when a custom `ReconnectionScheduler` cancel throws (the first error still propagates, after all chains are disarmed) and releases each chain's settlement listener, so a user-supplied cancel runs at most once across `close()` and the request's own settlement; and each request chain's reconnect legs now reuse one composed transport+request abort signal instead of composing a fresh one per leg (on Node 20.0-20.2, where `AbortSignal.any` is unavailable, per-leg composition stranded one abort-listener pair per completed resume leg on both input signals until the request settled). Two adjacent lifecycle/boundary fixes from the same review sweep: `maxTotalTimeout: 0` is now honored as the strictest budget (rejecting on the first progress-driven timeout reset) instead of being silently disabled by a falsy check; and on the legacy HTTP+SSE transport (`SSEClientTransport`), a `close()` that lands while a mid-session 401 token refresh (`onUnauthorized`) is pending no longer lets the recovery continuation open a new EventSource that nothing can tear down — the continuation now rejects with `UnauthorizedError('Transport closed during re-authentication')`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1503,15 +1503,30 @@ rewrite required unless noted. | ||
| - **Unchanged, for re-baselining relief:** timeout rejections still carry | ||
| `data.timeout` / `data.maxTotalTimeout` exactly as v1 `McpError` did — v1 assertions | ||
| on those survive verbatim. The cancelled-on-timeout signal is unchanged on legacy-era | ||
| connections and on stdio/in-memory at any era; on 2026-era Streamable HTTP the cancel | ||
| signal is the per-request stream close instead of a `notifications/cancelled` POST | ||
| on those survive verbatim. For per-leg timeouts and caller aborts, the | ||
| cancelled-on-timeout signal is unchanged on legacy-era connections and on | ||
| stdio/in-memory at any era; on 2026-era Streamable HTTP the cancel signal is the | ||
| per-request stream close instead of a `notifications/cancelled` POST | ||
| (see [support-2026-07-28.md](./support-2026-07-28.md)). | ||
| - **Also unchanged: SSE reconnection exhaustion.** `StreamableHTTPClientTransport`'s | ||
| standalone GET-stream reconnection behavior and its exhaustion signal carry over from | ||
| v1: when retries run out, the transport emits `onerror` with a plain `Error` whose | ||
| message is `Maximum reconnection attempts (N) exceeded.` — there is no typed error | ||
| class for this condition, so monitors that match the message text keep working. | ||
| - **Changed: `maxTotalTimeout` settlements now emit the cancel signal.** In v1 a | ||
| request settling because `maxTotalTimeout` was exceeded put nothing on the wire. | ||
| It now routes through the same cancel path as a plain timeout and emits the | ||
| connection's cancel signal — `notifications/cancelled` on legacy-era connections | ||
| and on single-channel transports at any era, the per-request stream close on | ||
| 2026-era Streamable HTTP — while the caller still sees the same | ||
| `Maximum total timeout exceeded` rejection. | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| - **Also unchanged: the SSE reconnection exhaustion message.** When | ||
| `StreamableHTTPClientTransport` runs out of retries, it still emits `onerror` with a | ||
| plain `Error` whose message is `Maximum reconnection attempts (N) exceeded.` — there | ||
| is no typed error class for this condition, so monitors that match that message text | ||
| keep working. | ||
| - **Changed: per-leg reconnect failures and intentional aborts.** Each failed | ||
| reconnect leg now reports through `onerror` exactly once with the underlying error | ||
| (e.g. `Failed to open SSE stream: …`) — the v1 `Failed to reconnect SSE stream:` | ||
| wrapper message is gone, so monitors matching that prefix need re-baselining — and | ||
| deliberate teardown (transport `close()` landing mid-POST/mid-GET/mid-DELETE, or a | ||
| settled request's teardown aborting its resume) no longer surfaces an `AbortError` | ||
| through `onerror`. | ||
| - **Also unchanged: elicitation response validation.** `elicitInput`'s local validation | ||
| of elicitation responses against `requestedSchema`, the resulting `-32602` error | ||
| message wording (`Elicitation response content does not match requested schema: …`), | ||
| @@ -1553,8 +1568,11 @@ rewrite required unless noted. | ||
| so an abort fired in the same tick can land before the frame is ever sent: the call | ||
| rejects with `SdkError(RequestTimeout, reason)` and **no `notifications/cancelled` is | ||
| emitted** (nothing was in flight). v1 sent the frame synchronously from these verbs. | ||
| Once the frame is on the wire, aborting still sends `notifications/cancelled` before | ||
| rejecting. | ||
| Once the frame is on the wire, aborting still emits the era's cancel signal before | ||
| rejecting: a `notifications/cancelled` POST on legacy-era (2025-11-25) connections and | ||
| on single-channel transports (stdio / in-memory) at any era; on a 2026-07-28 | ||
| Streamable HTTP connection the per-request stream close is itself the cancellation | ||
| and no `notifications/cancelled` is sent. | ||
| - **Protocol-version pinning is a first-class option.** | ||
| `ProtocolOptions.supportedProtocolVersions` pins the legacy `initialize` handshake: | ||
| the **first** pre-2026 entry in the list is offered (list order is preference order), | ||
| @@ -1825,8 +1843,11 @@ where an entry notes its own signature change: | ||
| wrappers, test doubles, decorators) compile and run against v2 with only the import | ||
| path updated. v2 adds **optional** members only — `hasPerRequestStream` and | ||
| `setSupportedProtocolVersions` on the interface, `requestSignal` / `headers` / | ||
| `onRequestStreamEnd` on `TransportSendOptions` — which matter only for 2026-era | ||
| per-request-stream cancellation and `Mcp-Param-*` header attachment | ||
| `onRequestStreamEnd` on `TransportSendOptions` — used for per-request | ||
| cancellation and teardown at either protocol version on per-request-stream | ||
| transports (on a 2026-era connection the `requestSignal` abort IS the spec | ||
| cancel signal; on a 2025-era connection it is local teardown accompanying the | ||
| `notifications/cancelled` POST) and for `Mcp-Param-*` header attachment | ||
| ([support-2026-07-28.md](./support-2026-07-28.md)). | ||
| - All TypeScript **type** definitions from `types.ts` (except the aliases listed under | ||
| [Removed type aliases](#removed-type-aliases) and the `experimental` capability | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -209,7 +209,12 @@ export class SSEClientTransport implements Transport { | ||
| return response; | ||
| } | ||
| }); | ||
| this._abortController = new AbortController(); | ||
| // One transport-lifetime controller: `_startOrAuth` also runs on | ||
| // the mid-session 401 recovery path, and REPLACING the controller | ||
| // there would orphan the signal already captured by any POST in | ||
| // flight — close() could no longer cancel that POST, and _send's | ||
| // intentional-abort guard would consult the wrong controller. | ||
| this._abortController ??= new AbortController(); | ||
| this._eventSource.onerror = event => { | ||
| if (event.code === 401 && this._authProvider) { | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| @@ -219,13 +224,36 @@ export class SSEClientTransport implements Transport { | ||
| this._eventSource?.close(); | ||
| this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then( | ||
| // onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject. | ||
| () => this._startOrAuth().then(resolve, reject), | ||
| () => { | ||
| // Deferred continuation after an arbitrarily | ||
| // long refresh await: a close() that landed in | ||
| // the meantime must not be undone by opening a | ||
| // brand-new EventSource — the ES wrapper fetch | ||
| // never carries the transport-lifetime signal, | ||
| // and close() already ran against the old | ||
| // instance, so nothing could ever tear the | ||
| // resurrected stream down. | ||
| if (this._abortController?.signal.aborted === true) { | ||
| reject(new UnauthorizedError('Transport closed during re-authentication')); | ||
| return; | ||
| } | ||
| this._startOrAuth().then(resolve, reject); | ||
| }, | ||
| // onUnauthorized failed → not yet reported. Auth-seam | ||
| // stamp: covers the SDK's OAuth flow and custom | ||
| // callbacks alike. | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| (error: unknown) => { | ||
| markAuthSeamEscape(error); | ||
| this.onerror?.(error as Error); | ||
| // Mirror the success arm's closed-state guard: | ||
| // a refresh that rejects AFTER close() (token | ||
| // endpoint unreachable at shutdown, abandoned | ||
| // interactive flow) must not surface a | ||
| // spurious auth error through onerror | ||
| // post-shutdown. Still reject so a pending | ||
| // start() settles. | ||
| if (this._abortController?.signal.aborted !== true) { | ||
| this.onerror?.(error as Error); | ||
| } | ||
| reject(error); | ||
| } | ||
| ); | ||
| @@ -338,9 +366,15 @@ export class SSEClientTransport implements Transport { | ||
| } | ||
| async close(): Promise<void> { | ||
| this._abortController?.abort(); | ||
| this._eventSource?.close(); | ||
| this.onclose?.(); | ||
| try { | ||
| this._abortController?.abort(); | ||
| this._eventSource?.close(); | ||
| } finally { | ||
| // onclose is the ONLY trigger for Protocol._onclose (which settles | ||
| // every pending request with ConnectionClosed) — it must fire even | ||
| // if the EventSource teardown throws. | ||
| this.onclose?.(); | ||
| } | ||
| } | ||
| async send(message: JSONRPCMessage): Promise<void> { | ||
| @@ -407,7 +441,13 @@ export class SSEClientTransport implements Transport { | ||
| // Release connection - POST responses don't have content we need | ||
| await response.text?.().catch(() => {}); | ||
| } catch (error) { | ||
| this.onerror?.(error as Error); | ||
| // The POST runs on the transport-lifetime signal alone, so a | ||
| // close() landing mid-flight rejects with an intentional | ||
| // AbortError — a clean shutdown, not a transport error. Still | ||
| // rethrow so callers see the failure. | ||
| if (this._abortController?.signal.aborted !== true) { | ||
| this.onerror?.(error as Error); | ||
| } | ||
| throw error; | ||
| } | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.