From 295267e24375edefc75ad329ff68544c58200470 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 05:24:51 +0000 Subject: [PATCH 01/17] fix(client): abort legacy SSE reconnect chain when the originating request settles On a legacy (2025-11-25) Streamable HTTP session, a request that settled via timeout or caller abort POSTed notifications/cancelled but left the transport's request-scoped SSE reconnect chain (GET + Last-Event-ID resumption) running: the per-request AbortController was only created when stream-close IS the cancel signal (modern era), so the transport's requestSignal guards never fired, each successful resume reset the retry counter, and a late resumed response surfaced as 'Received a response for an unknown message ID' (#2615). Create the request-scoped AbortController for every per-request-stream transport regardless of era, and abort it when the request settles. On the legacy era this is purely local teardown alongside the (unchanged) notifications/cancelled POST; the modern-era stream-close-cancels path and single-channel transports (stdio/in-memory) are byte-identical to before. --- .../legacy-sse-reconnect-after-timeout.md | 5 + .../client/test/client/streamableHttp.test.ts | 209 ++++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 33 ++- .../test/shared/protocol.test.ts | 57 ++++- 4 files changed, 291 insertions(+), 13 deletions(-) create mode 100644 .changeset/legacy-sse-reconnect-after-timeout.md diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md new file mode 100644 index 0000000000..028c256db0 --- /dev/null +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': 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 alongside the `notifications/cancelled` POST when the request settles — the wire-visible cancellation behavior is unchanged for every (era × transport) combination. diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index a36bbc0ad3..027cf28237 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -4,6 +4,7 @@ import type { Mock, Mocked } from 'vitest'; import type { OAuthClientProvider } from '../../src/client/auth'; import { UnauthorizedError } from '../../src/client/auth'; +import { Client } from '../../src/client/client'; import type { ReconnectionScheduler, StartSSEOptions, StreamableHTTPReconnectionOptions } from '../../src/client/streamableHttp'; import { StreamableHTTPClientTransport } from '../../src/client/streamableHttp'; @@ -1464,6 +1465,57 @@ describe('StreamableHTTPClientTransport', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + it('per-request requestSignal abort while a reconnect is scheduled: the pending reconnect never fires (#2615)', async () => { + // ARRANGE — a POST stream that is primed (SSE event id) and then + // closes gracefully WITHOUT delivering the response, so the + // transport schedules a GET+Last-Event-ID reconnect. The abort + // lands in the window between "reconnect scheduled" and "reconnect + // fires" — the shape a request timeout produces. + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 5, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const primedClosingStream = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('id: ev-1\ndata: \n\n')); + controller.close(); + } + }); + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: primedClosingStream + }); + + const requestAbort = new AbortController(); + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { requestSignal: requestAbort.signal } + ); + // Let the stream close and the reconnect get scheduled (delay 10ms). + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // ACT — the request settles (timeout/cancel) before the reconnect fires. + requestAbort.abort(); + await vi.advanceTimersByTimeAsync(100); + + // ASSERT — the scheduled reconnect saw the aborted requestSignal + // and bailed: no GET resume, no onerror. + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(errorSpy).not.toHaveBeenCalled(); + }); + it('onRequestStreamEnd fires when the per-request POST stream ends gracefully without reconnecting', async () => { // ARRANGE — a POST stream with NO priming event id (so the // graceful-close path does NOT schedule a reconnect): the @@ -2737,3 +2789,160 @@ describe('StreamableHTTPClientTransport', () => { }); }); }); + +/** + * End-to-end regression for #2615: on a legacy (2025-11-25) session, the + * transport's request-scoped SSE reconnect chain (GET + Last-Event-ID + * resumption) must stop once the originating request settles via timeout. + * Before the fix, the chain kept resuming forever (every successful resume + * resets the retry counter), and a late resumed GET carrying the original + * JSON-RPC response surfaced as "Received a response for an unknown message + * ID". + */ +describe('legacy era (2025-11-25): request timeout stops the SSE reconnect chain (#2615)', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.spyOn(globalThis, 'fetch'); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + }); + + const encoder = new TextEncoder(); + const sseResponse = (chunks: string[]) => ({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + for (const chunk of chunks) { + controller.enqueue(encoder.encode(chunk)); + } + controller.close(); + } + }) + }); + const jsonResponse = (message: JSONRPCMessage) => ({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'application/json' }), + json: async () => message, + text: async () => JSON.stringify(message) + }); + const accepted = () => ({ ok: true, status: 202, headers: new Headers(), text: async () => '' }); + const methodNotAllowed = () => ({ + ok: false, + status: 405, + statusText: 'Method Not Allowed', + headers: new Headers(), + text: async () => '' + }); + + it('stops resuming once the request times out; the late response never surfaces as an unknown message ID', async () => { + let pingId: string | number | undefined; + let eventSeq = 0; + let settled = false; + let resumesAfterSettle = 0; + const cancelledPosts: JSONRPCMessage[] = []; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementation(async (_url, init: RequestInit) => { + if (init.method === 'GET') { + const lastEventId = (init.headers as Headers).get('last-event-id'); + // Standalone notification stream: not offered by this server. + if (lastEventId === null) { + return methodNotAllowed(); + } + // Request-scoped resume. Once the request has settled, hand + // back the late original response — before the fix this is + // the resumed GET that surfaced "unknown message ID". + if (settled) { + resumesAfterSettle++; + return sseResponse([`id: evt-${++eventSeq}\ndata: {"jsonrpc":"2.0","id":${JSON.stringify(pingId)},"result":{}}\n\n`]); + } + // Keep the chain alive: a priming event id, then a graceful + // close without the response (the server expects the client + // to resume via GET + Last-Event-ID). + return sseResponse([`id: evt-${++eventSeq}\ndata: \n\n`]); + } + const message = JSON.parse(init.body as string) as JSONRPCMessage; + if ('method' in message) { + if (message.method === 'initialize' && 'id' in message) { + return jsonResponse({ + jsonrpc: '2.0', + id: message.id, + result: { + protocolVersion: '2025-11-25', + capabilities: {}, + serverInfo: { name: 'legacy-server', version: '1.0.0' } + } + }); + } + if (message.method === 'notifications/cancelled') { + cancelledPosts.push(message); + return accepted(); + } + if (message.method === 'ping' && 'id' in message) { + pingId = message.id; + // SSE response: retry hint + priming event id, then a + // graceful close without the response. + return sseResponse([`retry: 10\nid: evt-${++eventSeq}\ndata: \n\n`]); + } + } + return accepted(); + }); + + const transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 2, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + const errors: Error[] = []; + client.onerror = error => errors.push(error); + + await client.connect(transport); + + const resumeGetCount = () => + fetchMock.mock.calls.filter(call => call[1]?.method === 'GET' && (call[1].headers as Headers).get('last-event-id') !== null) + .length; + + let settledError: unknown; + const pending = client.ping({ timeout: 100 }).catch(error => { + settled = true; + settledError = error; + }); + + // Let the reconnect chain run a few resume cycles before the timeout. + await vi.advanceTimersByTimeAsync(50); + expect(resumeGetCount()).toBeGreaterThan(0); + expect(settled).toBe(false); + + // Cross the request timeout. + await vi.advanceTimersByTimeAsync(100); + await pending; + expect(settled).toBe(true); + expect(String(settledError)).toContain('Request timed out'); + + // The legacy wire cancel signal is unchanged: exactly one + // notifications/cancelled POST. + expect(cancelledPosts).toHaveLength(1); + + // Give an orphaned chain ample time to keep resuming (before the fix + // it reconnected forever — each successful resume resets the retry + // counter, so maxRetries never binds). + await vi.advanceTimersByTimeAsync(2000); + + // THE KEY ASSERTIONS: no resumed GET after the request settled, and + // the late response never surfaced as an unknown message ID. + expect(resumesAfterSettle).toBe(0); + expect(errors.map(e => e.message)).not.toContainEqual(expect.stringContaining('unknown message ID')); + + await client.close(); + }); +}); diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 0a19770082..863af5bc8d 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -1413,9 +1413,18 @@ export abstract class Protocol { // POSTing `notifications/cancelled`. Every other (era × transport) // combination — legacy era on any transport, modern era on stdio / // in-memory — keeps today's `notifications/cancelled` POST path - // unchanged. + // unchanged (the legacy era on a per-request-stream transport + // additionally aborts `requestSignal` locally; see below). const streamCloseCancels = codec.era === MODERN_WIRE_REVISION && this._transport.hasPerRequestStream === true; - const requestAbort = streamCloseCancels ? new AbortController() : undefined; + // The per-request AbortController exists on EVERY per-request-stream + // transport, not just when stream-close is the spec cancel signal. + // On the legacy era the `notifications/cancelled` POST below stays + // the wire signal, but the transport still owns a per-request SSE + // reconnect chain (GET + Last-Event-ID resumption) that nothing + // else tears down: without this signal, a request that settles via + // timeout or caller abort leaves orphaned reconnects running until + // the late response surfaces as "unknown message ID" (#2615). + const requestAbort = this._transport.hasPerRequestStream === true ? new AbortController() : undefined; const messageId = this._requestMessageId++; cleanupMessageId = messageId; @@ -1450,7 +1459,7 @@ export abstract class Protocol { } this._progressHandlers.delete(messageId); - if (requestAbort === undefined) { + if (!streamCloseCancels) { this._transport ?.send( this._envelopeOutbound({ @@ -1464,14 +1473,18 @@ export abstract class Protocol { { relatedRequestId, resumptionToken, onresumptiontoken } ) .catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`))); - } else { - // Modern-era per-request-stream transport: aborting the - // request's underlying stream IS the spec cancel signal. - // The transport already swallows the resulting AbortError - // (no spurious `onerror`); a post-abort send() rejection - // re-hits an already-settled promise below and is a no-op. - requestAbort.abort(); } + // Aborting the request-scoped signal is either the spec cancel + // signal itself (modern era: closing the per-request stream IS + // the cancellation, so no `notifications/cancelled` above) or a + // purely local teardown alongside the POST (legacy era: it + // stops the transport's SSE reconnect chain for this request — + // #2615). The transport already swallows the resulting + // AbortError (no spurious `onerror`); a post-abort send() + // rejection re-hits an already-settled promise below and is a + // no-op. The cancelled POST above does not carry this signal, + // so aborting here cannot cut off that send. + requestAbort?.abort(); // Wrap the reason in an SdkError if it isn't already const error = reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason)); diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 2ecdc40adc..d06842d42c 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -881,7 +881,7 @@ describe('protocol tests', () => { expect(cancelledSent(sent)).toHaveLength(1); }); - test('legacy era + per-request-stream transport: behavior unchanged — POSTs notifications/cancelled, no requestSignal', async () => { + test('legacy era + per-request-stream transport: POSTs notifications/cancelled AND aborts the requestSignal (#2615)', async () => { const tx = new PerRequestStreamTransport(); const proto = createTestProtocol(); await proto.connect(tx); @@ -890,13 +890,64 @@ describe('protocol tests', () => { const ac = new AbortController(); const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { signal: ac.signal }); - // Legacy path is byte-identical to before: no requestSignal threaded. - expect(tx.lastRequestSignal).toBeUndefined(); + // The requestSignal is threaded on the legacy era too — it is not + // the spec cancel signal there (the POST below is), but the + // transport needs it to tear down the request's SSE reconnect + // chain (GET + Last-Event-ID resumption) when the request settles. + const requestSignal = tx.lastRequestSignal; + expect(requestSignal).toBeInstanceOf(AbortSignal); + expect(requestSignal?.aborted).toBe(false); ac.abort('user cancel'); await expect(pending).rejects.toThrow(); + // The wire signal is unchanged (spec cancel = notifications/cancelled)… expect(cancelledSent(tx.sent)).toHaveLength(1); + // …and the request-scoped abort additionally stops any reconnect + // chain the transport still owns for this request (#2615). + expect(requestSignal?.aborted).toBe(true); + }); + + test('legacy era + per-request-stream transport: timeout POSTs notifications/cancelled AND aborts the requestSignal (#2615)', async () => { + const tx = new PerRequestStreamTransport(); + const proto = createTestProtocol(); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { timeout: 0 }); + const requestSignal = tx.lastRequestSignal; + expect(requestSignal).toBeInstanceOf(AbortSignal); + + await expect(pending).rejects.toThrow('Request timed out'); + + expect(cancelledSent(tx.sent)).toHaveLength(1); + expect(requestSignal?.aborted).toBe(true); + }); + + test('legacy era + single-channel transport (no hasPerRequestStream): POSTs notifications/cancelled, no requestSignal', async () => { + // stdio / in-memory shape: hasPerRequestStream is undefined. + const sent: JSONRPCMessage[] = []; + let sawRequestSignal: AbortSignal | undefined; + const tx = new MockTransport(); + tx.send = async (m: JSONRPCMessage, opts?: TransportSendOptions) => { + sent.push(m); + if (opts?.requestSignal !== undefined) { + sawRequestSignal = opts.requestSignal; + } + }; + const proto = createTestProtocol(); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const ac = new AbortController(); + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { signal: ac.signal }); + ac.abort('user cancel'); + await expect(pending).rejects.toThrow(); + + // No per-request stream to tear down — the legacy single-channel + // path stays byte-identical: cancelled POST only, no requestSignal. + expect(cancelledSent(sent)).toHaveLength(1); + expect(sawRequestSignal).toBeUndefined(); }); test('modern era + per-request-stream transport: timeout aborts the stream, NO notifications/cancelled', async () => { From 29c4e86d2f0f321899d5fa9fce2b909a6efdb798 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 05:55:44 +0000 Subject: [PATCH 02/17] fix(client): cover all settlement paths for the request-scoped abort; stop cancellation POST inheriting resumptionToken Review follow-ups on #2616: - Abort the request-scoped signal in the request funnel's .finally() so EVERY settlement path releases it: a maxTotalTimeout hit settles via the response handler directly (never through cancel()) and left the per-request SSE reconnect chain orphaned (same #2615 symptom), and a successful completion previously never released the controller, pinning one abort listener per request on the transport-lifetime signal via the Node 20.0-20.2 anySignal fallback. - Stop forwarding the original request's resumptionToken / onresumptiontoken into the notifications/cancelled send: on Streamable HTTP a truthy resumptionToken short-circuits send() into a GET+Last-Event-ID resume WITHOUT posting the message, silently swallowing the cancellation and spawning a fresh, unguarded reconnect chain. A notification is not a resumable request; only relatedRequestId is kept. requestSignal is deliberately NOT threaded into that POST - cancel() aborts it immediately afterwards, which would cut the cancellation off mid-flight. - Update the three prose sites still describing the 2026-only requestSignal contract (hasPerRequestStream JSDoc, docs/advanced/custom-transports.md, docs/migration/ support-2026-07-28.md): requestSignal is threaded on every request for per-request-stream transports; on 2026-era connections the abort IS the spec cancel, on 2025-era it is local teardown accompanying the notifications/cancelled POST, and transports forwarding it into fetch should swallow the intentional AbortError. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 +- docs/advanced/custom-transports.md | 3 +- docs/migration/support-2026-07-28.md | 13 ++- .../client/test/client/streamableHttp.test.ts | 93 +++++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 25 ++++- .../core-internal/src/shared/transport.ts | 14 ++- .../test/shared/protocol.test.ts | 93 +++++++++++++++++++ 7 files changed, 230 insertions(+), 13 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index 028c256db0..dc8f5da5be 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/client': 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 alongside the `notifications/cancelled` POST when the request settles — the wire-visible cancellation behavior is unchanged for every (era × transport) combination. +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-visible cancellation behavior is unchanged for every (era × transport) combination. 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. diff --git a/docs/advanced/custom-transports.md b/docs/advanced/custom-transports.md index 242d55937e..ee1e95dcad 100644 --- a/docs/advanced/custom-transports.md +++ b/docs/advanced/custom-transports.md @@ -1,6 +1,7 @@ --- shape: how-to --- + # Custom transports A **transport** moves `JSONRPCMessage` values in both directions over a channel the SDK knows nothing about. Implement the `Transport` interface and `connect()` accepts it like a built-in one. @@ -180,7 +181,7 @@ async send(message: JSONRPCMessage, options?: TransportSendOptions): Promise { + // Regression for the cancel-path resumption leak: transport.send() + // with a resumptionToken short-circuits into a GET+Last-Event-ID + // resume WITHOUT posting the message. If the request's own + // resumptionToken were forwarded into the notifications/cancelled + // send, the cancellation would be silently swallowed (no POST) and a + // fresh SSE reconnect chain — without the request-scoped abort signal + // — would be spawned in its place. + let eventSeq = 0; + const cancelledPosts: JSONRPCMessage[] = []; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementation(async (_url, init: RequestInit) => { + if (init.method === 'GET') { + const lastEventId = (init.headers as Headers).get('last-event-id'); + // Standalone notification stream: not offered by this server. + if (lastEventId === null) { + return methodNotAllowed(); + } + // Request-scoped resume: a priming event id, then a graceful + // close without the response, so the chain keeps resuming + // until the client tears it down. + return sseResponse([`retry: 10\nid: evt-${++eventSeq}\ndata: \n\n`]); + } + const message = JSON.parse(init.body as string) as JSONRPCMessage; + if ('method' in message) { + if (message.method === 'initialize' && 'id' in message) { + return jsonResponse({ + jsonrpc: '2.0', + id: message.id, + result: { + protocolVersion: '2025-11-25', + capabilities: {}, + serverInfo: { name: 'legacy-server', version: '1.0.0' } + } + }); + } + if (message.method === 'notifications/cancelled') { + cancelledPosts.push(message); + return accepted(); + } + } + return accepted(); + }); + + const transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 2, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + const errors: Error[] = []; + client.onerror = error => errors.push(error); + + await client.connect(transport); + + const resumeGetCount = () => + fetchMock.mock.calls.filter(call => call[1]?.method === 'GET' && (call[1].headers as Headers).get('last-event-id') !== null) + .length; + + // Issue the request WITH a resumption token: the transport resumes the + // request's stream via GET + Last-Event-ID instead of POSTing it. + let settled = false; + const pending = client.ping({ timeout: 100, resumptionToken: 'evt-0' }).catch(() => { + settled = true; + }); + + await vi.advanceTimersByTimeAsync(50); + expect(resumeGetCount()).toBeGreaterThan(0); + expect(settled).toBe(false); + + // Cross the request timeout. + await vi.advanceTimersByTimeAsync(100); + await pending; + expect(settled).toBe(true); + + // THE KEY ASSERTION: the cancellation actually reached the wire as a + // POST — it was not swallowed into another GET resume. + expect(cancelledPosts).toHaveLength(1); + + // And no fresh (unguarded) reconnect chain was spawned by the + // cancellation send: once the request settled, the resume GET count + // stays flat. + const resumesAtSettle = resumeGetCount(); + await vi.advanceTimersByTimeAsync(2000); + expect(resumeGetCount()).toBe(resumesAtSettle); + + await client.close(); + }); }); diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 863af5bc8d..f18df24e8b 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -1372,6 +1372,7 @@ export abstract class Protocol { let onAbort: (() => void) | undefined; let cleanupMessageId: number | undefined; + let requestAbort: AbortController | undefined; // Send the request return new Promise>((resolve, reject) => { @@ -1424,7 +1425,7 @@ export abstract class Protocol { // else tears down: without this signal, a request that settles via // timeout or caller abort leaves orphaned reconnects running until // the late response surfaces as "unknown message ID" (#2615). - const requestAbort = this._transport.hasPerRequestStream === true ? new AbortController() : undefined; + requestAbort = this._transport.hasPerRequestStream === true ? new AbortController() : undefined; const messageId = this._requestMessageId++; cleanupMessageId = messageId; @@ -1470,7 +1471,15 @@ export abstract class Protocol { reason: String(reason) } }), - { relatedRequestId, resumptionToken, onresumptiontoken } + // Deliberately NOT forwarding the original request's + // resumptionToken/onresumptiontoken: a notification is + // not a resumable request, and on Streamable HTTP a + // resumption token short-circuits send() into a + // GET+Last-Event-ID resume WITHOUT posting the message + // — the cancellation would be silently swallowed and a + // fresh, unguarded SSE reconnect chain spawned in its + // place. + { relatedRequestId } ) .catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`))); } @@ -1587,6 +1596,18 @@ export abstract class Protocol { this._responseHandlers.delete(cleanupMessageId); this._cleanupTimeout(cleanupMessageId); } + // Release the request-scoped signal on EVERY settlement path, not + // just cancel(): a maxTotalTimeout hit settles via the response + // handler directly (never through cancel()) and would otherwise + // leave the transport's per-request SSE reconnect chain orphaned + // (#2615), and a successful completion would otherwise pin one + // abort listener per request on the transport-lifetime signal via + // the Node 20.0–20.2 anySignal fallback. Idempotent after the + // cancel() abort above; a no-op for single-channel transports + // (requestAbort is undefined). Aborting after a completed response + // is pure local teardown — the transport treats it as intentional + // (no onerror, no reconnect) and nothing is put on the wire. + requestAbort?.abort(); }); } diff --git a/packages/core-internal/src/shared/transport.ts b/packages/core-internal/src/shared/transport.ts index 226f6ab0bd..d2d83bc8ef 100644 --- a/packages/core-internal/src/shared/transport.ts +++ b/packages/core-internal/src/shared/transport.ts @@ -130,11 +130,15 @@ export interface Transport { * `true` when this transport opens one underlying request per outbound * JSON-RPC request (the Streamable HTTP POST-per-request model) and * therefore honors {@linkcode TransportSendOptions.requestSignal}. The - * 2026-07-28 spec makes closing that per-request stream the cancellation - * signal — the protocol layer aborts `requestSignal` instead of POSTing - * `notifications/cancelled` when this flag is set on a 2026-era - * connection. Transports that share a single channel (stdio, in-memory) - * leave it `undefined`. + * protocol layer threads a request-scoped `requestSignal` into every + * outbound request on such transports and aborts it when the request + * settles (response, error, timeout, or caller abort). On a 2026-07-28 + * connection that abort IS the spec cancellation signal — the 2026-07-28 + * spec makes closing the per-request stream the cancellation, so no + * `notifications/cancelled` is sent. On a 2025-era connection it is + * purely local teardown (it stops the request's SSE reconnect chain) + * accompanying the `notifications/cancelled` POST. Transports that share + * a single channel (stdio, in-memory) leave it `undefined`. */ readonly hasPerRequestStream?: boolean; diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index d06842d42c..4d6dad3c2a 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -962,6 +962,99 @@ describe('protocol tests', () => { expect(tx.lastRequestSignal?.aborted).toBe(true); expect(cancelledSent(tx.sent)).toHaveLength(0); }); + + test.each(['2025-11-25', '2026-07-28'])( + '%s era + per-request-stream transport: maxTotalTimeout settlement aborts the requestSignal (#2615)', + async era => { + const tx = new PerRequestStreamTransport(); + const proto = createTestProtocol(); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, era); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { + timeout: 1000, + maxTotalTimeout: 1, + resetTimeoutOnProgress: true, + onprogress: () => {} + }); + const requestSignal = tx.lastRequestSignal; + expect(requestSignal).toBeInstanceOf(AbortSignal); + expect(requestSignal?.aborted).toBe(false); + + // Cross the total budget, then deliver a progress notification: + // the maxTotalTimeout check fires on the timeout-reset path + // inside _onprogress and settles the request via the response + // handler directly — it never goes through cancel(), so the + // request-scoped abort must be covered by the settlement + // cleanup, not only by cancel(). + await new Promise(resolve => setTimeout(resolve, 5)); + tx.onmessage?.({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { progressToken: 0, progress: 1 } + }); + + await expect(pending).rejects.toThrow('Maximum total timeout exceeded'); + expect(requestSignal?.aborted).toBe(true); + } + ); + + test('per-request-stream transport: successful completion releases (aborts) the request-scoped signal', async () => { + const tx = new PerRequestStreamTransport(); + const proto = createTestProtocol(); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({})); + const requestSignal = tx.lastRequestSignal; + expect(requestSignal).toBeInstanceOf(AbortSignal); + expect(requestSignal?.aborted).toBe(false); + + tx.onmessage?.({ jsonrpc: '2.0', id: 0, result: {} }); + await expect(pending).resolves.toEqual({}); + + // A completed request sends no wire cancel of any kind… + expect(cancelledSent(tx.sent)).toHaveLength(0); + // …but the request-scoped signal is released (aborted) so the + // transport can drop its per-request state — otherwise every + // successful request leaks an abort listener on the transport- + // lifetime signal via the Node 20.0–20.2 anySignal fallback. + expect(requestSignal?.aborted).toBe(true); + }); + + test('legacy era: cancellation POST does not inherit the original request resumptionToken', async () => { + // On Streamable HTTP, transport.send() with a resumptionToken + // short-circuits into a GET+Last-Event-ID resume WITHOUT posting + // the message. A notification is not a resumable request, so the + // cancellation send must never carry the original request's + // resumption options — forwarding them silently swallows the + // cancellation and spawns a fresh, unguarded SSE reconnect chain. + class RecordingTransport extends MockTransport { + readonly hasPerRequestStream = true; + calls: { message: JSONRPCMessage; options?: TransportSendOptions }[] = []; + override async send(message: JSONRPCMessage, options?: TransportSendOptions): Promise { + this.calls.push({ message, options }); + } + } + const tx = new RecordingTransport(); + const proto = createTestProtocol(); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const ac = new AbortController(); + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { + signal: ac.signal, + resumptionToken: 'evt-42', + onresumptiontoken: () => {} + }); + ac.abort('user cancel'); + await expect(pending).rejects.toThrow(); + + const cancelled = tx.calls.find(c => 'method' in c.message && c.message.method === 'notifications/cancelled'); + expect(cancelled).toBeDefined(); + expect(cancelled?.options?.resumptionToken).toBeUndefined(); + expect(cancelled?.options?.onresumptiontoken).toBeUndefined(); + }); }); }); From e00d771b428cb69dd9dc47e00d8b53a7151a9c13 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 06:33:05 +0000 Subject: [PATCH 03/17] fix(client): stop the resumptionToken send path double-reporting and swallowing callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 2: - The resumptionToken short-circuit in StreamableHTTPClientTransport._send ended in a bare .catch(error => this.onerror?.(error)). _startOrAuthSse's own catch already reports genuine failures via onerror (and deliberately stays silent on an intentional abort) before rethrowing, so the outer catch double-fired onerror for real failures and — now that legacy-era settlements abort requestSignal — surfaced a spurious AbortError through client.onerror whenever a request issued with options.resumptionToken settled while its resume GET was in flight. Swallow the rethrow instead. - The same short-circuit destructured onresumptiontoken but forwarded neither it nor onRequestStreamEnd into _startOrAuthSse, unlike every sibling call site (normal POST path, reconnect legs, resumeStream()): a request resumed via resumptionToken never reported newer event IDs to the caller's persistence hook, and its stream-end callback never fired on a terminal non-resumable outcome. Thread both through. - Regression tests: abort landing mid-resume-GET surfaces no onerror; a genuine resume GET failure reports onerror exactly once; both callbacks are forwarded; the existing e2e resumptionToken test now asserts the collected client.onerror list stays empty. - Rescope the changeset's wire-behavior claim: the cancellation mechanism per era is unchanged, and modern-era maxTotalTimeout settlements now emit the previously-omitted stream-close cancel. Update the fourth stale prose site (upgrade-to-v2.md Transport interface contract bullet) to the either-era requestSignal contract adopted in the other three sites. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 +- docs/migration/upgrade-to-v2.md | 7 +- packages/client/src/client/streamableHttp.ts | 19 ++- .../client/test/client/streamableHttp.test.ts | 126 ++++++++++++++++++ 4 files changed, 149 insertions(+), 5 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index dc8f5da5be..93de5494bf 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/client': 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-visible cancellation behavior is unchanged for every (era × transport) combination. 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. +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 modern-era `maxTotalTimeout` settlements now emit the stream-close cancel signal they previously omitted. 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. diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 19f4127733..e6cffb3560 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1825,8 +1825,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 diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index ace0663158..822c026e28 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -954,11 +954,26 @@ export class StreamableHTTPClientTransport implements Transport { // same per-request abort as the original POST — modern-era // cancel-via-stream-close routes through `requestSignal`, and // without it a resumed long-running request would not cancel. + // `onresumptiontoken` / `onRequestStreamEnd` are forwarded like + // every other `_startOrAuthSse` call site, so a resumed request + // keeps reporting newer event IDs to the caller's persistence + // hook and still fires the stream-end callback on a terminal + // non-resumable outcome. this._startOrAuthSse({ resumptionToken, + onresumptiontoken, replayMessageId: isJSONRPCRequest(message) ? message.id : undefined, - requestSignal: options?.requestSignal - }).catch(error => this.onerror?.(error)); + requestSignal: options?.requestSignal, + onRequestStreamEnd: options?.onRequestStreamEnd + }).catch(() => { + // Swallow the rethrow: `_startOrAuthSse`'s own catch already + // surfaced genuine failures via `onerror` before rethrowing + // (and deliberately stayed silent on an intentional abort — + // transport close or a settled request's `requestSignal`). + // Reporting here would double-fire `onerror` for real + // failures and turn a clean per-request teardown into a + // spurious `AbortError`. + }); return; } diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index f21205df0f..d7ce245a46 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1516,6 +1516,127 @@ describe('StreamableHTTPClientTransport', () => { expect(errorSpy).not.toHaveBeenCalled(); }); + it('resumptionToken send: requestSignal abort while the resume GET is in flight surfaces no spurious onerror (#2615)', async () => { + // ARRANGE — a request re-issued with options.resumptionToken + // short-circuits send() into a GET+Last-Event-ID resume. The + // server hangs on that GET (the exact scenario request timeouts + // exist for), so the settlement abort lands MID-FETCH — not in + // the scheduled-reconnect window the sibling test covers. + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementation( + (_url, init: RequestInit) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => reject(new DOMException('The operation was aborted', 'AbortError'))); + }) + ); + + const requestAbort = new AbortController(); + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { resumptionToken: 'evt-42', requestSignal: requestAbort.signal } + ); + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // ACT — the request settles (timeout/cancel) while the resume GET + // is still in flight; the fetch rejects with an AbortError. + requestAbort.abort(); + await vi.advanceTimersByTimeAsync(100); + + // ASSERT — a deliberate per-request teardown is a clean shutdown, + // not a transport error. + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('resumptionToken send: a genuine resume GET failure surfaces onerror exactly once (no double-report)', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const failure = new TypeError('fetch failed'); + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockRejectedValueOnce(failure); + + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { resumptionToken: 'evt-42' } + ); + await vi.advanceTimersByTimeAsync(5); + + // _startOrAuthSse's own catch reports the failure; the send() + // short-circuit must not report it a second time. + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith(failure); + }); + + it('resumptionToken send: forwards onresumptiontoken so the resumed stream reports newer event IDs', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + // The resumed GET replays a newer event carrying the response. + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + controller.enqueue( + new TextEncoder().encode('id: evt-43\ndata: {"jsonrpc":"2.0","id":"request-1","result":{}}\n\n') + ); + controller.close(); + } + }) + }); + + const tokens: string[] = []; + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { resumptionToken: 'evt-42', onresumptiontoken: token => tokens.push(token) } + ); + await vi.advanceTimersByTimeAsync(5); + + // The caller's persistence hook saw the newer event ID. + expect(tokens).toEqual(['evt-43']); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('resumptionToken send: forwards onRequestStreamEnd so a terminal non-resumable outcome settles the caller', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + // 405 on the resume GET: terminal, non-resumable — the stream-end + // callback must fire so the caller can settle. + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 405, + statusText: 'Method Not Allowed', + headers: new Headers(), + text: async () => '' + }); + + const onStreamEnd = vi.fn(); + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { resumptionToken: 'evt-42', onRequestStreamEnd: onStreamEnd } + ); + await vi.advanceTimersByTimeAsync(5); + + expect(onStreamEnd).toHaveBeenCalledTimes(1); + expect(errorSpy).not.toHaveBeenCalled(); + }); + it('onRequestStreamEnd fires when the per-request POST stream ends gracefully without reconnecting', async () => { // ARRANGE — a POST stream with NO priming event id (so the // graceful-close path does NOT schedule a reconnect): the @@ -3036,6 +3157,11 @@ describe('legacy era (2025-11-25): request timeout stops the SSE reconnect chain await vi.advanceTimersByTimeAsync(2000); expect(resumeGetCount()).toBe(resumesAtSettle); + // The settlement abort landed on a deliberately-resumed request: a + // clean teardown, so no error — spurious AbortError included — may + // surface through client.onerror. + expect(errors).toEqual([]); + await client.close(); }); }); From 473bb3741079c0c58dd8ee0928f778539608e74d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 07:04:19 +0000 Subject: [PATCH 04/17] fix(client): sweep the last bare _startOrAuthSse catch; scope resumed-cancel prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 3: - The 202/initialized branch in _send kept the catch shape the previous round removed from the resumptionToken short-circuit: a bare .catch(error => this.onerror?.(error)) after _startOrAuthSse, which double-fires onerror for genuine standalone-GET failures (the inner catch already reported before rethrowing) and surfaces a spurious AbortError when transport.close() lands while that GET is in flight. Swallow the rethrow, with regression tests for both facets. - Update the fifth and last stale prose site: the class-level JSDoc on StreamableHTTPClientTransport.hasPerRequestStream now carries the same either-era requestSignal contract as the Transport interface JSDoc. - Scope the resumed-request cancellation prose (e2e test KEY ASSERTION comment and changeset): on the SDK's own transport a request re-issued with resumptionToken never POSTs its fresh JSON-RPC id, so the notifications/cancelled POST carries an id the server cannot correlate — cancellation is best-effort there and resumed requests are only torn down locally; the POST stays because custom per-request-stream transports that POST the re-issued body normally give the server a correlatable id. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 +- packages/client/src/client/streamableHttp.ts | 19 ++++-- .../client/test/client/streamableHttp.test.ts | 60 ++++++++++++++++++- 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index 93de5494bf..6e45fbf723 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/client': 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 modern-era `maxTotalTimeout` settlements now emit the stream-close cancel signal they previously omitted. 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. +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 modern-era `maxTotalTimeout` settlements now emit the stream-close cancel signal they previously omitted. 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. diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index 822c026e28..00dda24a6f 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -334,9 +334,12 @@ export class StreamableHTTPClientTransport implements Transport { /** * Streamable HTTP opens one POST (and SSE response stream) per outbound - * request and honors `TransportSendOptions.requestSignal`. On a 2026-era - * connection the protocol layer aborts that per-request stream as the - * spec cancellation signal instead of POSTing `notifications/cancelled`. + * request and honors `TransportSendOptions.requestSignal`. The protocol + * layer threads `requestSignal` into every outbound request and aborts it + * when the request settles — on a 2026-era connection that abort IS the + * spec cancellation signal (no `notifications/cancelled` is sent); on a + * 2025-era connection it is purely local teardown (it stops the request's + * SSE reconnect chain) accompanying the `notifications/cancelled` POST. */ readonly hasPerRequestStream = true; @@ -1126,8 +1129,14 @@ export class StreamableHTTPClientTransport implements Transport { // if the accepted notification is initialized, we start the SSE stream // if it's supported by the server if (isInitializedNotification(message)) { - // Start without a lastEventId since this is a fresh connection - this._startOrAuthSse({ resumptionToken: undefined }).catch(error => this.onerror?.(error)); + // Start without a lastEventId since this is a fresh connection. + // Swallow the rethrow: `_startOrAuthSse`'s own catch already + // surfaced genuine failures via `onerror` before rethrowing + // (and deliberately stayed silent on an intentional abort — + // transport close). Reporting here would double-fire + // `onerror` for real failures and turn a clean shutdown + // into a spurious `AbortError`. + this._startOrAuthSse({ resumptionToken: undefined }).catch(() => {}); } return; } diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index d7ce245a46..1be65dd1fb 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1637,6 +1637,57 @@ describe('StreamableHTTPClientTransport', () => { expect(errorSpy).not.toHaveBeenCalled(); }); + it('202-initialized standalone GET: a genuine failure surfaces onerror exactly once (no double-report)', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const failure = new TypeError('fetch failed'); + const fetchMock = globalThis.fetch as Mock; + // The notifications/initialized POST is 202-accepted... + fetchMock.mockResolvedValueOnce({ ok: true, status: 202, headers: new Headers(), text: async () => '' }); + // ...and the standalone GET it triggers fails genuinely. + fetchMock.mockRejectedValueOnce(failure); + + await transport.start(); + await transport.send({ jsonrpc: '2.0', method: 'notifications/initialized' }); + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(2); + + // _startOrAuthSse's own catch reports the failure; the 202 branch + // must not report it a second time. + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith(failure); + }); + + it('202-initialized standalone GET: transport.close() while the GET is in flight surfaces no spurious onerror', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockResolvedValueOnce({ ok: true, status: 202, headers: new Headers(), text: async () => '' }); + // The standalone GET hangs until its signal aborts. + fetchMock.mockImplementationOnce( + (_url, init: RequestInit) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => reject(new DOMException('The operation was aborted', 'AbortError'))); + }) + ); + + await transport.start(); + await transport.send({ jsonrpc: '2.0', method: 'notifications/initialized' }); + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(2); + + // ACT — deliberate shutdown while the standalone GET is in flight. + await transport.close(); + await vi.advanceTimersByTimeAsync(5); + + // ASSERT — a clean shutdown, not a transport error. + expect(errorSpy).not.toHaveBeenCalled(); + }); + it('onRequestStreamEnd fires when the per-request POST stream ends gracefully without reconnecting', async () => { // ARRANGE — a POST stream with NO priming event id (so the // graceful-close path does NOT schedule a reconnect): the @@ -3147,7 +3198,14 @@ describe('legacy era (2025-11-25): request timeout stops the SSE reconnect chain expect(settled).toBe(true); // THE KEY ASSERTION: the cancellation actually reached the wire as a - // POST — it was not swallowed into another GET resume. + // POST — it was not swallowed into another GET resume. Note this is + // best-effort on the SDK's own transport: the POST carries the + // re-issued request's fresh JSON-RPC id, which never reached the + // server here (the send itself short-circuited into a GET resume), so + // the server cannot correlate it and a resumed request can only be + // torn down locally (the requestSignal abort). The POST is kept + // because custom per-request-stream transports that POST the + // re-issued body normally DO give the server a correlatable id. expect(cancelledPosts).toHaveLength(1); // And no fresh (unguarded) reconnect chain was spawned by the From c1b55ebef1b892a107f01192c52876917b9e507e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 07:30:08 +0000 Subject: [PATCH 05/17] fix(client,core): finish the onerror discipline sweep; wire cancel for maxTotalTimeout at either era MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 4: - _send's main POST-path outer catch guarded onerror with only the requestSignal half despite its comment claiming parity with isIntentionalAbort: transport.close() landing mid-POST (deterministic for notification sends, which carry no requestSignal) surfaced a spurious AbortError through onerror. Add the transport-signal half; the rethrow is kept so callers still settle. - _scheduleReconnection's reconnect() catch re-fired onerror after _startOrAuthSse's inner catch had already reported the same genuine failure, double-reporting every failed reconnect leg. Drop the duplicate report; retry scheduling and the scheduleError catch stay. - A genuine open failure on the resumptionToken short-circuit was terminal (initial-open failures never enter the reconnect loop) but fired no onRequestStreamEnd — and send() had already resolved fire-and-forget, leaving direct transport callers of the documented resume pattern with no per-request settlement. Fire the stream-end callback for non-intentional failures in the short-circuit's catch, mirroring the maxRetries-exhaustion branch; never on intentional aborts. - A maxTotalTimeout settlement never routed through cancel(), so legacy sessions got no notifications/cancelled POST for that settlement while plain-timeout and caller-abort did (and the modern era got its stream-close cancel on this path earlier in this PR). _onprogress now settles through the request's stored cancel path with the original error: legacy emits the cancelled POST (correct requestId), modern keeps the stream-close abort alone, and the caller still sees the maxTotalTimeout SdkError unchanged. - Regression tests for all four: close-mid-POST notification send surfaces no onerror; failed reconnect leg reports exactly once then retries; resumed-path open failure fires onRequestStreamEnd exactly once (and not on intentional abort); the era-matrix maxTotalTimeout test now asserts the cancelled POST count and requestId per era. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 +- packages/client/src/client/streamableHttp.ts | 34 ++++-- .../client/test/client/streamableHttp.test.ts | 104 +++++++++++++++++- packages/core-internal/src/shared/protocol.ts | 46 +++++--- .../test/shared/protocol.test.ts | 20 +++- 5 files changed, 173 insertions(+), 33 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index 6e45fbf723..e5f83a3237 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/client': 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 modern-era `maxTotalTimeout` settlements now emit the stream-close cancel signal they previously omitted. 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. +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, the stream-close cancel on modern ones) while the caller still sees the original maxTotalTimeout error. 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. diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index 00dda24a6f..4924ebe159 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -687,9 +687,13 @@ export class StreamableHTTPClientTransport implements Transport { // (a listen subscription closed during the backoff delay): do not // resurrect a stream the caller already tore down. if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return; - this._startOrAuthSse(options).catch(error => { + this._startOrAuthSse(options).catch(() => { if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return; - this.onerror?.(new Error(`Failed to reconnect SSE stream: ${error instanceof Error ? error.message : String(error)}`)); + // No onerror here: `_startOrAuthSse`'s own catch already + // reported the genuine failure once before rethrowing (and + // stayed silent on an intentional abort — caught above). + // Reporting again would double-fire onerror for every failed + // reconnect leg. Just schedule the next attempt. try { this._scheduleReconnection(options, attemptCount + 1); } catch (scheduleError) { @@ -976,6 +980,17 @@ export class StreamableHTTPClientTransport implements Transport { // Reporting here would double-fire `onerror` for real // failures and turn a clean per-request teardown into a // spurious `AbortError`. + // + // A genuine open failure IS terminal for the resumed + // stream (an initial-open failure never enters the + // reconnect loop) and this send() already resolved + // fire-and-forget — fire the stream-end callback so the + // caller can settle, mirroring `_scheduleReconnection`'s + // maxRetries-exhaustion branch. Not on intentional aborts: + // the contract excludes deliberate teardown. + if (options?.requestSignal?.aborted !== true && this._abortController?.signal.aborted !== true) { + options?.onRequestStreamEnd?.(); + } }); return; } @@ -1185,13 +1200,14 @@ export class StreamableHTTPClientTransport implements Transport { await response.text?.().catch(() => {}); } } catch (error) { - // Intentional per-request abort BEFORE response headers (the - // `subscriptions/listen` driver aborting its `requestSignal`): - // fetch rejects with AbortError. Same guard as - // `_handleSseStream`'s `isIntentionalAbort` — do not surface a - // misleading onerror; still rethrow so `listen()`'s send-catch - // settles the per-subscription state machine. - if (options?.requestSignal?.aborted !== true) { + // Intentional abort BEFORE response headers — a per-request abort + // (the `subscriptions/listen` driver aborting its `requestSignal`) + // or a transport-wide close() landing mid-POST: fetch rejects with + // AbortError. Same guard as `_handleSseStream`'s + // `isIntentionalAbort` (BOTH signal halves) — do not surface a + // misleading onerror; still rethrow so `listen()`'s send-catch and + // the protocol layer settle their state machines. + if (options?.requestSignal?.aborted !== true && this._abortController?.signal.aborted !== true) { this.onerror?.(error as Error); } throw error; diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 1be65dd1fb..84a5bfc24b 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1535,10 +1535,11 @@ describe('StreamableHTTPClientTransport', () => { ); const requestAbort = new AbortController(); + const onStreamEnd = vi.fn(); await transport.start(); await transport.send( { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, - { resumptionToken: 'evt-42', requestSignal: requestAbort.signal } + { resumptionToken: 'evt-42', requestSignal: requestAbort.signal, onRequestStreamEnd: onStreamEnd } ); await vi.advanceTimersByTimeAsync(5); expect(fetchMock).toHaveBeenCalledTimes(1); @@ -1548,9 +1549,11 @@ describe('StreamableHTTPClientTransport', () => { requestAbort.abort(); await vi.advanceTimersByTimeAsync(100); - // ASSERT — a deliberate per-request teardown is a clean shutdown, - // not a transport error. + // ASSERT — a deliberate per-request teardown is a clean shutdown: + // no transport error, and no stream-end callback (the contract + // excludes deliberate requestSignal aborts). expect(errorSpy).not.toHaveBeenCalled(); + expect(onStreamEnd).not.toHaveBeenCalled(); }); it('resumptionToken send: a genuine resume GET failure surfaces onerror exactly once (no double-report)', async () => { @@ -1562,10 +1565,11 @@ describe('StreamableHTTPClientTransport', () => { const fetchMock = globalThis.fetch as Mock; fetchMock.mockRejectedValueOnce(failure); + const onStreamEnd = vi.fn(); await transport.start(); await transport.send( { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, - { resumptionToken: 'evt-42' } + { resumptionToken: 'evt-42', onRequestStreamEnd: onStreamEnd } ); await vi.advanceTimersByTimeAsync(5); @@ -1573,6 +1577,12 @@ describe('StreamableHTTPClientTransport', () => { // short-circuit must not report it a second time. expect(errorSpy).toHaveBeenCalledTimes(1); expect(errorSpy).toHaveBeenCalledWith(failure); + + // A genuine open failure is TERMINAL for the resumed stream (no + // reconnect is ever scheduled for an initial-open failure) and + // send() already resolved fire-and-forget — the stream-end + // callback is the caller's only per-request settlement signal. + expect(onStreamEnd).toHaveBeenCalledTimes(1); }); it('resumptionToken send: forwards onresumptiontoken so the resumed stream reports newer event IDs', async () => { @@ -1688,6 +1698,92 @@ describe('StreamableHTTPClientTransport', () => { expect(errorSpy).not.toHaveBeenCalled(); }); + it('notification POST: transport.close() while the POST is in flight surfaces no spurious onerror', async () => { + // Notification sends carry no requestSignal, so the POST's fetch + // signal is the transport-lifetime signal alone — close() landing + // mid-POST must read as a clean shutdown, not a transport error. + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementationOnce( + (_url, init: RequestInit) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => reject(new DOMException('The operation was aborted', 'AbortError'))); + }) + ); + + await transport.start(); + let sendError: unknown; + const pending = transport.send({ jsonrpc: '2.0', method: 'notifications/roots/list_changed' }).catch(error => { + sendError = error; + }); + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // ACT — deliberate shutdown while the POST is in flight. + await transport.close(); + await pending; + + // ASSERT — the rethrow is kept (send() rejects so callers settle), + // but no onerror fires for a deliberate shutdown. + expect(sendError).toBeInstanceOf(DOMException); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('failed reconnect leg surfaces onerror exactly once (no double-report), then retries', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 5, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const failure = new TypeError('fetch failed'); + const fetchMock = globalThis.fetch as Mock; + // POST stream: primed (SSE event id), then a graceful close + // without the response — schedules a GET reconnect at +10ms. + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('id: ev-1\ndata: \n\n')); + controller.close(); + } + }) + }); + // The reconnect GET fails genuinely. + fetchMock.mockRejectedValueOnce(failure); + // The retried leg after it hangs, so the count stays deterministic. + fetchMock.mockImplementation(() => new Promise(() => {})); + + await transport.start(); + await transport.send({ jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }); + // Let the stream close and the reconnect get scheduled... + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(1); + // ...then fire the reconnect leg and let it fail. + await vi.advanceTimersByTimeAsync(10); + expect(fetchMock).toHaveBeenCalledTimes(2); + + // _startOrAuthSse's own catch is the single reporting site — the + // reconnect scheduler must not report the same failure again. + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith(failure); + + // The retry itself still happens (attempt 2 fires the next GET). + await vi.advanceTimersByTimeAsync(15); + expect(fetchMock).toHaveBeenCalledTimes(3); + expect(errorSpy).toHaveBeenCalledTimes(1); + }); + it('onRequestStreamEnd fires when the per-request POST stream ends gracefully without reconnecting', async () => { // ARRANGE — a POST stream with NO priming event id (so the // graceful-close path does NOT schedule a reconnect): the diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index f18df24e8b..f9854ea1f8 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -519,7 +519,14 @@ type TimeoutInfo = { timeout: number; maxTotalTimeout?: number; resetTimeoutOnProgress: boolean; - onTimeout: () => void; + /** + * Settles the request through its cancel path. Called with no argument by + * the per-leg `setTimeout` (plain "Request timed out"); called with the + * `maxTotalTimeout` error by `_onprogress` so that settlement takes the + * same cancel path — emitting the era's wire cancel signal — while the + * caller still sees the original maxTotalTimeout error. + */ + onTimeout: (error?: Error) => void; }; /* @@ -736,7 +743,7 @@ export abstract class Protocol { messageId: number, timeout: number, maxTotalTimeout: number | undefined, - onTimeout: () => void, + onTimeout: (error?: Error) => void, resetTimeoutOnProgress: boolean = false ) { this._timeoutInfo.set(messageId, { @@ -1176,11 +1183,18 @@ export abstract class Protocol { try { this._resetTimeout(messageId); } catch (error) { - // Clean up if maxTotalTimeout was exceeded - this._responseHandlers.delete(messageId); - this._progressHandlers.delete(messageId); - this._cleanupTimeout(messageId); - responseHandler(error as Error); + // maxTotalTimeout exceeded. Settle through the request's + // cancel path (stored as `onTimeout`) rather than the response + // handler directly, so this settlement emits the same wire + // cancel signal as a plain timeout on the same session — the + // `notifications/cancelled` POST on a legacy (2025-11-25) + // connection, the stream-close abort alone on a modern + // (2026-07-28) one. cancel() rejects with an SdkError reason + // unchanged, so the caller still sees the original + // maxTotalTimeout error; handler/timeout cleanup runs in the + // request funnel's `.finally()`, exactly as for a plain + // timeout settlement. + timeoutInfo.onTimeout(error as Error); return; } } @@ -1574,7 +1588,8 @@ export abstract class Protocol { options?.signal?.addEventListener('abort', onAbort, { once: true }); const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC; - const timeoutHandler = () => cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout })); + const timeoutHandler = (error?: Error) => + cancel(error ?? new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout })); this._setupTimeout(messageId, timeout, options?.maxTotalTimeout, timeoutHandler, options?.resetTimeoutOnProgress ?? false); @@ -1597,13 +1612,14 @@ export abstract class Protocol { this._cleanupTimeout(cleanupMessageId); } // Release the request-scoped signal on EVERY settlement path, not - // just cancel(): a maxTotalTimeout hit settles via the response - // handler directly (never through cancel()) and would otherwise - // leave the transport's per-request SSE reconnect chain orphaned - // (#2615), and a successful completion would otherwise pin one - // abort listener per request on the transport-lifetime signal via - // the Node 20.0–20.2 anySignal fallback. Idempotent after the - // cancel() abort above; a no-op for single-channel transports + // just cancel(): a successful completion (and a send() failure) + // never goes through cancel() and would otherwise pin one abort + // listener per request on the transport-lifetime signal via the + // Node 20.0–20.2 anySignal fallback — and leave the transport's + // per-request SSE reconnect chain orphaned (#2615) on any + // settlement cancel() missed. Idempotent after the cancel() abort + // above (timeouts, caller aborts, and maxTotalTimeout all route + // through cancel()); a no-op for single-channel transports // (requestAbort is undefined). Aborting after a completed response // is pure local teardown — the transport treats it as intentional // (no onerror, no reconnect) and nothing is put on the wire. diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 4d6dad3c2a..dfa513565b 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -983,10 +983,10 @@ describe('protocol tests', () => { // Cross the total budget, then deliver a progress notification: // the maxTotalTimeout check fires on the timeout-reset path - // inside _onprogress and settles the request via the response - // handler directly — it never goes through cancel(), so the - // request-scoped abort must be covered by the settlement - // cleanup, not only by cancel(). + // inside _onprogress, which routes the settlement through the + // request's cancel path — so this settlement emits the same + // wire cancel signal as a plain timeout, while the caller + // still sees the original maxTotalTimeout error. await new Promise(resolve => setTimeout(resolve, 5)); tx.onmessage?.({ jsonrpc: '2.0', @@ -996,6 +996,18 @@ describe('protocol tests', () => { await expect(pending).rejects.toThrow('Maximum total timeout exceeded'); expect(requestSignal?.aborted).toBe(true); + + // The wire cancel signal matches the era, exactly like a plain + // timeout settlement on the same session: legacy POSTs + // notifications/cancelled (carrying the request's id); modern + // cancels via the stream-close abort alone. + const cancelled = cancelledSent(tx.sent); + if (era === '2025-11-25') { + expect(cancelled).toHaveLength(1); + expect((cancelled[0] as { params?: { requestId?: unknown } }).params?.requestId).toBe(0); + } else { + expect(cancelled).toHaveLength(0); + } } ); From ec9447501431a265b12c5d999b615f6152967d0c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 07:36:20 +0000 Subject: [PATCH 06/17] test(e2e): update reconnect-failure onerror assertions to the report-once contract The reconnect-failure-onerror story pinned the pre-c1b55eb double-report: two "Failed to reconnect SSE stream" wrappers on top of the open failures. Each failed reconnect leg now reports exactly once via the transport's single reporting site ("Failed to open SSE stream: ..."), so assert two of those, zero wrappers, and the unchanged budget-exhausted report. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- test/e2e/scenarios/transport-http.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/e2e/scenarios/transport-http.test.ts b/test/e2e/scenarios/transport-http.test.ts index 7cda0f28aa..ab7f627b7e 100644 --- a/test/e2e/scenarios/transport-http.test.ts +++ b/test/e2e/scenarios/transport-http.test.ts @@ -1482,7 +1482,12 @@ verifies('client-transport:http:reconnect-failure-onerror', async (_args: TestAr await vi.waitFor(() => expect(transportErrors.filter(e => e.message === 'Maximum reconnection attempts (2) exceeded.')).toHaveLength(1) ); - expect(transportErrors.filter(e => e.message.startsWith('Failed to reconnect SSE stream:'))).toHaveLength(2); + // Each failed reconnect leg reports exactly once — the SSE open + // failure itself, from the transport's single reporting site; the + // scheduler no longer wraps the same failure in a duplicate + // "Failed to reconnect SSE stream" onerror. + expect(transportErrors.filter(e => e.message.startsWith('Failed to open SSE stream:'))).toHaveLength(2); + expect(transportErrors.filter(e => e.message.startsWith('Failed to reconnect SSE stream:'))).toHaveLength(0); expect(records.filter(r => r.method === 'GET')).toHaveLength(3); // The reconnection failure stays on onerror: an unrelated request issued afterwards still succeeds From 248cb69704b2db854a60f24a3e09a7059d02af96 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 07:53:20 +0000 Subject: [PATCH 07/17] fix(core,client): disarm the leg timer on maxTotalTimeout; guard terminateSession onerror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 5: - The maxTotalTimeout-through-cancel reroute (c1b55eb) left the per-leg timeout timer armed: _resetTimeout's over-budget branch deleted the _timeoutInfo entry and threw without clearTimeout (making the funnel's .finally() _cleanupTimeout a no-op), and cancel() never marked the request settled — so the orphaned timer later re-ran cancel() and POSTed a SECOND notifications/cancelled for the same requestId on legacy-era transports. Fixed both ways: clearTimeout in the over-budget branch, and cancel() now sets a shared `settled` flag (renamed from responseReceived — it covers both settlement channels) making cancellation idempotent against any late timer or future double-cancel path. The era-matrix maxTotalTimeout test now crosses the leg timeout after rejection and re-asserts the cancelled count stays 1 (legacy) / 0 (modern). - terminateSession's catch was the last unguarded onerror site: its DELETE runs on the transport-lifetime signal alone, so close() landing mid-flight surfaced a spurious AbortError during a clean shutdown. Same guard as the POST path (transport-signal half only — no requestSignal exists here), rethrow kept, with a mirroring regression test. - Scope the inbound half of the resumed-request id asymmetry (pre-existing): _handleSseStream remaps replayMessageId onto responses only, so progress notifications replayed on a resumed stream carry the original request's progressToken and the fresh onprogress handler never fires (resetTimeoutOnProgress never resets). The transport cannot remap — callers persist SSE event ids, never the original wire id — so this is documented rather than changed: RequestOptions.onprogress JSDoc, the StartSSEOptions.replayMessageId JSDoc, the short-circuit comment, and the changeset now state that onprogress/resetTimeoutOnProgress do not survive a resumptionToken re-issue on this transport. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 +- packages/client/src/client/streamableHttp.ts | 23 +++++++++++- .../client/test/client/streamableHttp.test.ts | 35 +++++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 26 ++++++++++++-- .../test/shared/protocol.test.ts | 12 ++++++- 5 files changed, 92 insertions(+), 6 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index e5f83a3237..0002e5255a 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/client': 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, the stream-close cancel on modern ones) while the caller still sees the original maxTotalTimeout error. 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. +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, the stream-close cancel on modern ones) while the caller still sees the original maxTotalTimeout error. 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.) diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index 4924ebe159..e907c2514c 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -71,6 +71,12 @@ export interface StartSSEOptions { /** * Override Message ID to associate with the replay message * so that the response can be associated with the new resumed request. + * + * Only JSON-RPC RESPONSES are remapped. Notifications replayed on the + * resumed stream (e.g. `notifications/progress`) pass through verbatim, + * still carrying the original request's identifiers — the transport never + * learns the original wire id (callers persist SSE event ids, not message + * ids), so it has nothing to remap `params.progressToken` with. */ replayMessageId?: string | number; @@ -966,6 +972,14 @@ export class StreamableHTTPClientTransport implements Transport { // keeps reporting newer event IDs to the caller's persistence // hook and still fires the stream-end callback on a terminal // non-resumable outcome. + // + // Known limitation: `onprogress` / `resetTimeoutOnProgress` do + // NOT survive a resumptionToken re-issue on this transport. + // The re-issued request's fresh progressToken never reaches + // the wire (this path resumes via GET instead of POSTing), + // and replayed `notifications/progress` carry the ORIGINAL + // request's token — see the `replayMessageId` JSDoc: only + // responses are remapped. this._startOrAuthSse({ resumptionToken, onresumptiontoken, @@ -1262,7 +1276,14 @@ export class StreamableHTTPClientTransport implements Transport { this._sessionId = undefined; } catch (error) { - this.onerror?.(error as Error); + // Same guard as the POST path: the DELETE runs on the + // transport-lifetime signal alone, so close() landing mid-flight + // (or terminateSession() called after close()) rejects with an + // intentional AbortError — a clean shutdown, not a transport + // error. Still rethrow so the caller sees the failure. + if (this._abortController?.signal.aborted !== true) { + this.onerror?.(error as Error); + } throw error; } } diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 84a5bfc24b..f5e61a9d4c 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1732,6 +1732,41 @@ describe('StreamableHTTPClientTransport', () => { expect(errorSpy).not.toHaveBeenCalled(); }); + it('terminateSession: transport.close() while the DELETE is in flight surfaces no spurious onerror', async () => { + // The session-termination DELETE runs on the transport-lifetime + // signal alone — close() landing mid-flight must read as a clean + // shutdown, not a transport error. (A sessionId is required or + // terminateSession() no-ops without fetching.) + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { sessionId: 'session-1' }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementationOnce( + (_url, init: RequestInit) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => reject(new DOMException('The operation was aborted', 'AbortError'))); + }) + ); + + await transport.start(); + let terminateError: unknown; + const pending = transport.terminateSession().catch(error => { + terminateError = error; + }); + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // ACT — deliberate shutdown while the DELETE is in flight. + await transport.close(); + await pending; + + // ASSERT — the rethrow is kept (terminateSession() rejects so the + // caller sees the outcome), but no onerror fires. + expect(terminateError).toBeInstanceOf(DOMException); + expect(errorSpy).not.toHaveBeenCalled(); + }); + it('failed reconnect leg surfaces onerror exactly once (no double-report), then retries', async () => { transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { reconnectionOptions: { diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index f9854ea1f8..a3473344ac 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -101,6 +101,13 @@ export const DEFAULT_REQUEST_TIMEOUT_MSEC = 60_000; export type RequestOptions = { /** * If set, requests progress notifications from the remote end (if supported). When progress notifications are received, this callback will be invoked. + * + * Does not survive a `resumptionToken` re-issue on the SDK's Streamable + * HTTP transport: the resumed send never POSTs the fresh progress token, + * and progress notifications replayed on the resumed stream carry the + * original request's token, so this callback (and + * {@linkcode RequestOptions.resetTimeoutOnProgress | resetTimeoutOnProgress}) + * will not fire for the resumed request. */ onprogress?: ProgressCallback; @@ -762,6 +769,12 @@ export abstract class Protocol { const totalElapsed = Date.now() - info.startTime; if (info.maxTotalTimeout && totalElapsed >= info.maxTotalTimeout) { + // Disarm the still-armed per-leg timer BEFORE dropping the map + // entry: once the entry is gone, `_cleanupTimeout` (the funnel's + // `.finally()` cleanup) can no longer reach the timer, and an + // orphaned leg timer would fire cancel() again after this + // settlement. + clearTimeout(info.timeoutId); this._timeoutInfo.delete(messageId); throw new SdkError(SdkErrorCode.RequestTimeout, 'Maximum total timeout exceeded', { maxTotalTimeout: info.maxTotalTimeout, @@ -1466,12 +1479,19 @@ export abstract class Protocol { // built above. const outbound = this._envelopeOutbound(jsonrpcRequest); - let responseReceived = false; + // `true` once the request has settled through EITHER channel — + // the response handler or cancel(). Guarding cancel() on it (and + // having cancel() set it) makes cancellation idempotent: a late + // per-leg timer or any future duplicate cancel path becomes a + // no-op instead of re-running the body and POSTing a second + // `notifications/cancelled` for an already-settled request. + let settled = false; const cancel = (reason: unknown) => { - if (responseReceived) { + if (settled) { return; } + settled = true; this._progressHandlers.delete(messageId); if (!streamCloseCancels) { @@ -1518,7 +1538,7 @@ export abstract class Protocol { if (options?.signal?.aborted) { return; } - responseReceived = true; + settled = true; if (response instanceof Error) { return reject(response); diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index dfa513565b..22b9605fba 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -972,7 +972,7 @@ describe('protocol tests', () => { setNegotiatedProtocolVersion(proto, era); const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { - timeout: 1000, + timeout: 200, maxTotalTimeout: 1, resetTimeoutOnProgress: true, onprogress: () => {} @@ -1008,6 +1008,16 @@ describe('protocol tests', () => { } else { expect(cancelled).toHaveLength(0); } + + // The settlement must fully disarm the per-leg timer: cross + // the leg `timeout` (200ms) and re-assert nothing else went on + // the wire. Before the fix the over-budget branch orphaned the + // armed leg timer (deleted the map entry without clearTimeout, + // and cancel() never marked the request settled), so the late + // timer re-ran cancel() and POSTed a SECOND + // notifications/cancelled for the same requestId on legacy. + await new Promise(resolve => setTimeout(resolve, 250)); + expect(cancelledSent(tx.sent)).toHaveLength(era === '2025-11-25' ? 1 : 0); } ); From 3b486e83e43e601ae723069f674c0688ce8370c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 08:04:48 +0000 Subject: [PATCH 08/17] fix(core,client): Gecko-safe timeout timers; keep resume token across empty legs; honor cancel for id 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 6: - The widened timeout handler regressed Firefox: Gecko invokes setTimeout callbacks with an extra lateness Number, which `??` passed through as the cancel reason — every per-leg timeout rejected with that number as its message (losing the timeout data) and legacy sessions POSTed notifications/cancelled with a numeric reason. Both timer arm sites now wrap the callback so the handler is invoked with no arguments, and the handler itself takes `instanceof Error` instead of `??`. Regression test simulates a Gecko-style timer. - Both reconnect-rebuild sites in _handleSseStream passed `resumptionToken: lastEventId` with no fallback, so a resume leg that dropped before delivering its first event rescheduled with an undefined token — degrading the resume into a token-less standalone GET (dead-ends as non-resumable on 405 servers, loses the replay position otherwise). Fall back to the token the leg was opened with. Regression test: a zero-event resume leg rebuilds with the original Last-Event-ID. - _oncancel's falsy requestId guard swallowed id 0 — every peer's FIRST outbound request id (_requestMessageId starts at 0) — so a notifications/cancelled for a connection's first request was silently dropped. Explicit `=== undefined` check, with a regression test that a cancellation for request id 0 aborts the stored handler controller. - Changeset: add @modelcontextprotocol/server (the maxTotalTimeout cancel reroute lives in the shared Protocol base, so server-initiated createMessage/elicitInput requests gain the same signal), scope the modern-era stream-close cancel to per-request-stream connections, and document the onerror-contract changes (report-once reconnect legs, no AbortError on deliberate teardown, onRequestStreamEnd on resume open failure). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 3 +- packages/client/src/client/streamableHttp.ts | 13 ++++- .../client/test/client/streamableHttp.test.ts | 48 +++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 22 ++++++-- .../test/shared/protocol.test.ts | 54 +++++++++++++++++++ 5 files changed, 133 insertions(+), 7 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index 0002e5255a..4bc7ce3c58 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -1,5 +1,6 @@ --- '@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, the stream-close cancel on modern ones) while the caller still sees the original maxTotalTimeout error. 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.) +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`). diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index e907c2514c..a045248a95 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -804,7 +804,13 @@ export class StreamableHTTPClientTransport implements Transport { if (needsReconnect && this._abortController && !isIntentionalAbort()) { this._scheduleReconnection( { - resumptionToken: lastEventId, + // Fall back to the token this leg was opened with: + // a resume leg that drops before delivering its + // first event has no `lastEventId`, and rebuilding + // without a token would degrade the resume into a + // token-less standalone GET (dead-ends on 405 + // servers; loses the replay position otherwise). + resumptionToken: lastEventId ?? options.resumptionToken, onresumptiontoken, replayMessageId, requestSignal, @@ -837,7 +843,10 @@ export class StreamableHTTPClientTransport implements Transport { try { this._scheduleReconnection( { - resumptionToken: lastEventId, + // Same fallback as the graceful-close path + // above: never rebuild a resume without its + // token. + resumptionToken: lastEventId ?? options.resumptionToken, onresumptiontoken, replayMessageId, requestSignal, diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index f5e61a9d4c..f74591adc1 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1585,6 +1585,54 @@ describe('StreamableHTTPClientTransport', () => { expect(onStreamEnd).toHaveBeenCalledTimes(1); }); + it('resume leg that drops before its first event reschedules with the ORIGINAL resumption token', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 2, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + // GET#1 (the resume): closes gracefully with ZERO events, so the + // leg produces no lastEventId of its own. + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + controller.close(); + } + }) + }); + // GET#2 (the rebuilt leg): hangs, keeping the count deterministic. + fetchMock.mockImplementation(() => new Promise(() => {})); + + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { resumptionToken: 'evt-42' } + ); + await vi.advanceTimersByTimeAsync(5); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // Fire the scheduled rebuild. + await vi.advanceTimersByTimeAsync(15); + expect(fetchMock).toHaveBeenCalledTimes(2); + + // The rebuilt GET must still carry the token this resume was + // opened with — without the fallback it degrades into a + // token-less standalone GET and loses the replay position. + const secondInit = fetchMock.mock.calls[1]?.[1] as RequestInit; + expect((secondInit.headers as Headers).get('last-event-id')).toBe('evt-42'); + expect(errorSpy).not.toHaveBeenCalled(); + }); + it('resumptionToken send: forwards onresumptiontoken so the resumed stream reports newer event IDs', async () => { transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp')); const errorSpy = vi.fn(); diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index a3473344ac..391bc1eebf 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -738,7 +738,11 @@ export abstract class Protocol { } private async _oncancel(notification: CancelledNotification): Promise { - if (!notification.params.requestId) { + // Explicit undefined check: `0` is a legitimate JSON-RPC request id — + // it is every peer's FIRST outbound request id (`_requestMessageId` + // starts at 0) — and a falsy guard would silently drop its + // cancellation. + if (notification.params.requestId === undefined) { return; } // Handle request cancellation @@ -754,7 +758,11 @@ export abstract class Protocol { resetTimeoutOnProgress: boolean = false ) { this._timeoutInfo.set(messageId, { - timeoutId: setTimeout(onTimeout, timeout), + // Wrapped so the timer fires the handler with NO arguments: + // Gecko (Firefox) invokes setTimeout callbacks with an extra + // "lateness" Number argument, which must not be mistaken for the + // handler's optional error parameter. + timeoutId: setTimeout(() => onTimeout(), timeout), startTime: Date.now(), timeout, maxTotalTimeout, @@ -783,7 +791,9 @@ export abstract class Protocol { } clearTimeout(info.timeoutId); - info.timeoutId = setTimeout(info.onTimeout, info.timeout); + // Wrapped for the same reason as `_setupTimeout`: Gecko passes a + // lateness Number to setTimeout callbacks. + info.timeoutId = setTimeout(() => info.onTimeout(), info.timeout); return true; } @@ -1608,8 +1618,12 @@ export abstract class Protocol { options?.signal?.addEventListener('abort', onAbort, { once: true }); const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC; + // `instanceof Error` rather than `??`: a timer implementation that + // invokes its callback with a non-Error argument (Gecko passes a + // lateness Number) must still produce the plain timeout error, not + // reject the request with that stray value as its message. const timeoutHandler = (error?: Error) => - cancel(error ?? new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout })); + cancel(error instanceof Error ? error : new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout })); this._setupTimeout(messageId, timeout, options?.maxTotalTimeout, timeoutHandler, options?.resetTimeoutOnProgress ?? false); diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 22b9605fba..3161b456ba 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -819,6 +819,32 @@ describe('protocol tests', () => { // Verify the request was aborted expect(wasAborted).toBe(true); }); + + test("aborts the request handler for request id 0 (falsy but legitimate — every peer's FIRST request id)", async () => { + // `_requestMessageId` starts at 0, so a peer cancelling its first + // outbound request sends `requestId: 0`. A falsy guard in + // _oncancel silently dropped exactly that cancellation. + await protocol.connect(transport); + + let wasAborted = false; + protocol.setRequestHandler('ping', async (_request, ctx) => { + await new Promise(resolve => setTimeout(resolve, 100)); + wasAborted = ctx.mcpReq.signal.aborted; + return {}; + }); + + transport.onmessage?.({ jsonrpc: '2.0', id: 0, method: 'ping', params: {} }); + await new Promise(resolve => setTimeout(resolve, 10)); + + transport.onmessage?.({ + jsonrpc: '2.0', + method: 'notifications/cancelled', + params: { requestId: 0, reason: 'User cancelled' } + }); + + await new Promise(resolve => setTimeout(resolve, 150)); + expect(wasAborted).toBe(true); + }); }); // Spec basic/patterns/cancellation §Transport-Specific (2026-07-28): on a @@ -1021,6 +1047,34 @@ describe('protocol tests', () => { } ); + test('per-leg timeout under a Gecko-style timer (callback invoked with a lateness argument): plain timeout error, not "0.42"', async () => { + // Firefox/Gecko invokes setTimeout callbacks with an extra Number + // ("lateness") argument. It must never be mistaken for the timeout + // handler's optional error parameter — that would reject the + // request with the stray number as its message and POST a + // notifications/cancelled whose reason is that number. + const realSetTimeout = globalThis.setTimeout.bind(globalThis); + const setTimeoutSpy = vi + .spyOn(globalThis, 'setTimeout') + .mockImplementation(((fn: (...args: unknown[]) => void, ms?: number) => + realSetTimeout(() => fn(0.42), ms)) as unknown as typeof setTimeout); + try { + const tx = new PerRequestStreamTransport(); + const proto = createTestProtocol(); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { timeout: 1 }); + await expect(pending).rejects.toThrow('Request timed out'); + + const cancelled = cancelledSent(tx.sent); + expect(cancelled).toHaveLength(1); + expect((cancelled[0] as { params?: { reason?: string } }).params?.reason).toContain('Request timed out'); + } finally { + setTimeoutSpy.mockRestore(); + } + }); + test('per-request-stream transport: successful completion releases (aborts) the request-scoped signal', async () => { const tx = new PerRequestStreamTransport(); const proto = createTestProtocol(); From d3bc0a3e74f95cdf3f707cbeb6ce920f615aca40 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 08:29:32 +0000 Subject: [PATCH 09/17] fix(core,client): guard the cancellation-send and SSE POST catches; document protocol-owned send options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 7: - cancel()'s cancellation-send catch reported unconditionally, so an ordinary close() right after a timeout rejection on a legacy session surfaced a spurious "Failed to send cancellation: AbortError" through onerror (the transport's own catch stays silent on the intentional abort and rethrows; _onclose has already cleared _transport by the time the rejection lands). Guard on transport liveness — report only failures on a live connection — with regression tests for both the suppressed spurious report and the preserved genuine-failure report. - SSEClientTransport's POST catch had the same unguarded onerror+throw shape on the transport-lifetime signal alone; close() mid-POST now reads as a clean shutdown (rethrow kept), with a mirroring test. - RequestOptions absorbs TransportSendOptions, so requestSignal and onRequestStreamEnd type-check on the request() path but are protocol-owned there: requestSignal is overwritten with the request-scoped signal and onRequestStreamEnd is not forwarded. Documented on the RequestOptions JSDoc (with the direct transport.send() alternative) rather than narrowed with Omit — a type removal would be compile-breaking in a patch. Having the funnel supply its own onRequestStreamEnd to settle a dead-ended resumed request promptly is a real behavior change (settlement semantics for every per-request-stream request) better taken as a follow-up. - Rescope the migration guide's "Also unchanged: SSE reconnection exhaustion" bullet to the exhaustion message only, and add a Behavioral-changes entry for the new onerror contract (one raw-error report per failed reconnect leg — the "Failed to reconnect SSE stream:" wrapper is gone; intentional aborts no longer surface through onerror). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- docs/migration/upgrade-to-v2.md | 17 ++++-- packages/client/src/client/sse.ts | 8 ++- packages/client/test/client/sse.test.ts | 41 ++++++++++++++ packages/core-internal/src/shared/protocol.ts | 25 ++++++++- .../test/shared/protocol.test.ts | 56 +++++++++++++++++++ 5 files changed, 140 insertions(+), 7 deletions(-) diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index e6cffb3560..5016a40eb1 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1507,11 +1507,18 @@ rewrite required unless noted. 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. +- **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: …`), diff --git a/packages/client/src/client/sse.ts b/packages/client/src/client/sse.ts index 0cc77d8f4f..b3f8381a15 100644 --- a/packages/client/src/client/sse.ts +++ b/packages/client/src/client/sse.ts @@ -407,7 +407,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; } } diff --git a/packages/client/test/client/sse.test.ts b/packages/client/test/client/sse.test.ts index a0d4e7b6f9..49ee5abe8e 100644 --- a/packages/client/test/client/sse.test.ts +++ b/packages/client/test/client/sse.test.ts @@ -246,6 +246,47 @@ describe('SSEClientTransport', () => { await expect(transport.send(testMessage)).rejects.toThrow(/500/); }); + + it('close() while a POST is in flight surfaces no spurious onerror', async () => { + // Create a server whose POST endpoint never responds, so the send + // stays in flight until the transport-lifetime signal aborts it. + await resourceServer.close(); + + resourceServer = createServer((req, res) => { + if (req.method === 'GET') { + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache, no-transform', + Connection: 'keep-alive' + }); + res.write('event: endpoint\n'); + res.write(`data: ${resourceBaseUrl.href}\n\n`); + } + // POST: never respond. + }); + + resourceBaseUrl = await listenOnRandomPort(resourceServer); + + transport = new SSEClientTransport(resourceBaseUrl); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + await transport.start(); + + let sendError: unknown; + const pending = transport.send({ jsonrpc: '2.0', id: 'test-1', method: 'test', params: {} }).catch(error => { + sendError = error; + }); + await new Promise(resolve => setTimeout(resolve, 50)); + + // ACT — deliberate shutdown while the POST is in flight. + await transport.close(); + await pending; + + // ASSERT — the rethrow is kept (send() rejects so callers + // settle), but no onerror fires for a deliberate shutdown. + expect((sendError as Error).name).toBe('AbortError'); + expect(errorSpy).not.toHaveBeenCalled(); + }); }); describe('header handling', () => { diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 391bc1eebf..ada07344f8 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -97,6 +97,18 @@ export const DEFAULT_REQUEST_TIMEOUT_MSEC = 60_000; /** * Options that can be given per request. + * + * The {@linkcode TransportSendOptions} members this type absorbs are not all + * caller-controllable on the `request()` path — the protocol layer owns the + * per-request stream lifecycle there: + * + * - `requestSignal` is OVERWRITTEN with the protocol layer's request-scoped + * signal (aborted when the request settles); a caller-supplied value is + * ignored. Cancel via {@linkcode RequestOptions.signal | signal} instead. + * - `onRequestStreamEnd` is NOT forwarded to the transport. To observe the + * per-request stream's lifecycle directly, call `transport.send()` yourself. + * - `resumptionToken` / `onresumptiontoken` / `relatedRequestId` / `headers` + * are forwarded as documented. */ export type RequestOptions = { /** @@ -1525,7 +1537,18 @@ export abstract class Protocol { // place. { relatedRequestId } ) - .catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`))); + .catch(error => { + // A deliberate transport close aborts an in-flight + // cancellation POST: the transport's own catch stays + // silent (intentional-abort guard) and rethrows, and + // by the time the rejection lands here `_onclose` has + // already cleared `_transport`. Re-reporting would + // resurface an AbortError for a clean shutdown — + // report only failures on a live connection. + if (this._transport !== undefined) { + this._onerror(new Error(`Failed to send cancellation: ${error}`)); + } + }); } // Aborting the request-scoped signal is either the spec cancel // signal itself (modern era: closing the per-request stream IS diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 3161b456ba..4e55e38adc 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -1075,6 +1075,62 @@ describe('protocol tests', () => { } }); + test('close() landing while the cancellation POST is in flight: no spurious "Failed to send cancellation" onerror', async () => { + let rejectCancelSend: ((error: unknown) => void) | undefined; + const tx = new PerRequestStreamTransport(); + const baseSend = tx.send.bind(tx); + tx.send = async (message: JSONRPCMessage, opts?: TransportSendOptions) => { + await baseSend(message, opts); + if ('method' in message && message.method === 'notifications/cancelled') { + // The cancellation POST stays in flight until close() + // aborts it (the transport rethrows the AbortError after + // its own intentional-abort guard stays silent). + return new Promise((_resolve, reject) => { + rejectCancelSend = reject; + }); + } + }; + const proto = createTestProtocol(); + const errors: Error[] = []; + proto.onerror = error => void errors.push(error); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { timeout: 0 }); + await expect(pending).rejects.toThrow('Request timed out'); + expect(rejectCancelSend).toBeDefined(); + + // Deliberate shutdown; the aborted POST's rejection lands after + // _onclose has already cleared the transport. + await proto.close(); + rejectCancelSend?.(new DOMException('The operation was aborted', 'AbortError')); + await new Promise(resolve => setTimeout(resolve, 10)); + + expect(errors.map(e => e.message).filter(m => m.includes('Failed to send cancellation'))).toHaveLength(0); + }); + + test('genuine cancellation-send failure on a live connection still reports through onerror', async () => { + const tx = new PerRequestStreamTransport(); + const baseSend = tx.send.bind(tx); + tx.send = async (message: JSONRPCMessage, opts?: TransportSendOptions) => { + await baseSend(message, opts); + if ('method' in message && message.method === 'notifications/cancelled') { + throw new TypeError('fetch failed'); + } + }; + const proto = createTestProtocol(); + const errors: Error[] = []; + proto.onerror = error => void errors.push(error); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { timeout: 0 }); + await expect(pending).rejects.toThrow('Request timed out'); + await new Promise(resolve => setTimeout(resolve, 10)); + + expect(errors.map(e => e.message).filter(m => m.includes('Failed to send cancellation'))).toHaveLength(1); + }); + test('per-request-stream transport: successful completion releases (aborts) the request-scoped signal', async () => { const tx = new PerRequestStreamTransport(); const proto = createTestProtocol(); From adefd71de420fa07ac809619324df990766b22df Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 08:50:41 +0000 Subject: [PATCH 10/17] fix(core,client): honor relatedRequestId 0 in the debounce gate; disarm every pending reconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 8: - The notification debounce gate used `!options?.relatedRequestId`, so a notification related to request id 0 — every peer's FIRST outbound request id — was wrongly debounced: synchronous sends coalesced and the send options (the request association) were dropped. Same falsy-id pattern as the _oncancel fix; now `=== undefined`, with a regression test mirroring the existing relatedRequestId debounce test at id 0. - _cancelReconnection was a single per-transport slot, but the transport owns multiple concurrent reconnect chains (the standalone notification GET plus one per in-flight legacy request): each schedule overwrote the slot, close() disarmed only the last-written chain (breaking the ReconnectionScheduler JSDoc contract for every other chain), surviving timers pinned short-lived Node processes for up to maxReconnectionDelay, and per-request settlement relied on the fire-time bail alone. Replaced with a Set of per-chain cancels: each scheduled attempt registers its cancel and removes it on fire/disarm, close() invokes and clears all (a throwing cancel still propagates; abort/onclose still run), and a {once} listener on requestSignal disarms the chain's pending attempt the moment its request settles. Tests: close() releases BOTH of two pending chains (timer count 0); a settled request's armed timer is released immediately. - Rescope the migration guide's "Unchanged, for re-baselining relief" bullet to per-leg timeouts and caller aborts, and add a "Changed: maxTotalTimeout settlements now emit the cancel signal" bullet (v1 put nothing on the wire on that settlement). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- docs/migration/upgrade-to-v2.md | 14 ++- packages/client/src/client/streamableHttp.ts | 61 ++++++++++-- .../client/test/client/streamableHttp.test.ts | 94 ++++++++++++++++++- packages/core-internal/src/shared/protocol.ts | 7 +- .../test/shared/protocol.test.ts | 18 ++++ 5 files changed, 181 insertions(+), 13 deletions(-) diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 5016a40eb1..453dfdfcdc 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1503,10 +1503,18 @@ 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)). +- **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. - **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 diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index a045248a95..da0512a1d0 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -332,7 +332,16 @@ export class StreamableHTTPClientTransport implements Transport { private _maxStepUpRetries: number; private _serverRetryMs?: number; // Server-provided retry delay from SSE retry field private readonly _reconnectionScheduler?: ReconnectionScheduler; - private _cancelReconnection?: () => void; + /** + * Cancel functions for EVERY pending scheduled reconnection attempt. The + * transport can own several concurrent reconnect chains at once — the + * standalone notification GET plus one per in-flight request on a legacy + * session — so this must be a set, not a single slot: a slot would be + * overwritten by each schedule and `close()` could disarm only the + * last-written chain, leaving armed timers (or un-cancelled custom + * scheduler tasks) behind. + */ + private readonly _pendingReconnectCancels = new Set<() => void>(); onclose?: () => void; onerror?: (error: Error) => void; @@ -687,8 +696,24 @@ export class StreamableHTTPClientTransport implements Transport { // Calculate next delay based on current attempt count const delay = this._getNextReconnectionDelay(attemptCount); + // Per-chain cancel bookkeeping: each scheduled attempt registers its + // own cancel in `_pendingReconnectCancels` and removes it when the + // attempt fires or is disarmed. `listenerCleanup` releases the + // settlement listener below in the same motion. + let entry: (() => void) | undefined; + let fired = false; + const listenerCleanup = new AbortController(); + const disarmBookkeeping = (): void => { + if (entry !== undefined) { + this._pendingReconnectCancels.delete(entry); + entry = undefined; + } + listenerCleanup.abort(); + }; + const reconnect = (): void => { - this._cancelReconnection = undefined; + fired = true; + disarmBookkeeping(); // Honour BOTH the transport-wide abort and the per-request abort // (a listen subscription closed during the backoff delay): do not // resurrect a stream the caller already tore down. @@ -708,12 +733,30 @@ export class StreamableHTTPClientTransport implements Transport { }); }; + let cancelPending: () => void; if (this._reconnectionScheduler) { const cancel = this._reconnectionScheduler(reconnect, delay, attemptCount); - this._cancelReconnection = typeof cancel === 'function' ? cancel : undefined; + cancelPending = typeof cancel === 'function' ? cancel : () => {}; } else { const handle = setTimeout(reconnect, delay); - this._cancelReconnection = () => clearTimeout(handle); + cancelPending = () => clearTimeout(handle); + } + // A custom scheduler may invoke `reconnect` synchronously; do not + // register bookkeeping for an attempt that already fired. + if (!fired) { + entry = cancelPending; + this._pendingReconnectCancels.add(entry); + // A settled request disarms its own pending attempt immediately + // instead of leaving an armed timer to bail at fire time (the + // request-scoped signal is aborted on every settlement path). + options.requestSignal?.addEventListener( + 'abort', + () => { + cancelPending(); + disarmBookkeeping(); + }, + { once: true, signal: listenerCleanup.signal } + ); } } @@ -932,9 +975,15 @@ export class StreamableHTTPClientTransport implements Transport { async close(): Promise { try { - this._cancelReconnection?.(); + // Disarm EVERY pending scheduled reconnection — the transport can + // own several concurrent chains, each with its own pending + // attempt. (A throwing cancel still propagates to the caller; the + // finally block guarantees the abort and onclose regardless.) + for (const cancel of this._pendingReconnectCancels) { + cancel(); + } } finally { - this._cancelReconnection = undefined; + this._pendingReconnectCancels.clear(); this._abortController?.abort(); this.onclose?.(); } diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index f74591adc1..70fb9137b7 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1585,6 +1585,92 @@ describe('StreamableHTTPClientTransport', () => { expect(onStreamEnd).toHaveBeenCalledTimes(1); }); + it('close() disarms EVERY pending scheduled reconnect, not just the last-scheduled chain', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10_000, + maxRetries: 2, + maxReconnectionDelay: 30_000, + reconnectionDelayGrowFactor: 1 + } + }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + // Every POST returns a primed SSE stream that closes gracefully + // without a response — each send spawns its own reconnect chain. + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementation(async () => ({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('id: ev-1\ndata: \n\n')); + controller.close(); + } + }) + })); + + await transport.start(); + await transport.send({ jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }); + await transport.send({ jsonrpc: '2.0', method: 'long_running_tool', id: 'request-2', params: {} }); + await vi.advanceTimersByTimeAsync(5); + + // Two chains, two pending scheduled attempts. + expect(vi.getTimerCount()).toBe(2); + + // ACT — close must disarm BOTH, not only the last-written one. + await transport.close(); + + expect(vi.getTimerCount()).toBe(0); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('a settled request disarms its own pending scheduled reconnect immediately', async () => { + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10_000, + maxRetries: 2, + maxReconnectionDelay: 30_000, + reconnectionDelayGrowFactor: 1 + } + }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('id: ev-1\ndata: \n\n')); + controller.close(); + } + }) + }); + + const requestAbort = new AbortController(); + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { requestSignal: requestAbort.signal } + ); + await vi.advanceTimersByTimeAsync(5); + expect(vi.getTimerCount()).toBe(1); + + // ACT — the request settles during the backoff window: the armed + // timer is released immediately, not left to bail at fire time. + requestAbort.abort(); + expect(vi.getTimerCount()).toBe(0); + + await vi.advanceTimersByTimeAsync(60_000); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(errorSpy).not.toHaveBeenCalled(); + }); + it('resume leg that drops before its first event reschedules with the ORIGINAL resumption token', async () => { transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { reconnectionOptions: { @@ -2900,7 +2986,7 @@ describe('StreamableHTTPClientTransport', () => { ); // Verify no reconnection was scheduled - expect(transport['_cancelReconnection']).toBeUndefined(); + expect(transport['_pendingReconnectCancels'].size).toBe(0); }); it('should schedule reconnection when maxRetries is greater than 0', async () => { @@ -2922,10 +3008,12 @@ describe('StreamableHTTPClientTransport', () => { // ASSERT - should schedule a reconnection, not report error yet expect(errorSpy).not.toHaveBeenCalled(); - expect(transport['_cancelReconnection']).toBeDefined(); + expect(transport['_pendingReconnectCancels'].size).toBe(1); // Clean up the pending reconnection to avoid test pollution - transport['_cancelReconnection']?.(); + for (const cancel of transport['_pendingReconnectCancels']) { + cancel(); + } }); }); diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index ada07344f8..25510e8835 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -1718,7 +1718,12 @@ export abstract class Protocol { const debouncedMethods = this._options?.debouncedNotificationMethods ?? []; // A notification can only be debounced if it's in the list AND it's "simple" // (i.e., has no parameters and no related request ID that could be lost). - const canDebounce = debouncedMethods.includes(notification.method) && !notification.params && !options?.relatedRequestId; + // `=== undefined` rather than falsiness on the id: `0` is a + // legitimate relatedRequestId (every peer's FIRST outbound request + // id), and a related notification must never be debounced/coalesced + // away from its request association. + const canDebounce = + debouncedMethods.includes(notification.method) && !notification.params && options?.relatedRequestId === undefined; if (canDebounce) { // If a notification of this type is already scheduled, do nothing. diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 4e55e38adc..8b899419ad 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -656,6 +656,24 @@ describe('protocol tests', () => { expect(sendSpy).toHaveBeenCalledWith(expect.any(Object), { relatedRequestId: 'req-2' }); }); + it('should NOT debounce a notification whose relatedRequestId is 0 (falsy but legitimate first-request id)', async () => { + // `_requestMessageId` starts at 0, so a notification related to a + // peer's first request carries relatedRequestId 0. A falsy gate + // wrongly debounced it — coalescing synchronous sends away and + // dropping the request association from the send options. + protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced_with_options'] }); + await protocol.connect(transport); + + // ACT — synchronous back-to-back sends, like the coalescing tests. + protocol.notification({ method: 'test/debounced_with_options' }, { relatedRequestId: 0 }); + protocol.notification({ method: 'test/debounced_with_options' }, { relatedRequestId: 0 }); + await flushMicrotasks(); + + // ASSERT — both sends hit the wire, each keeping its association. + expect(sendSpy).toHaveBeenCalledTimes(2); + expect(sendSpy).toHaveBeenCalledWith(expect.any(Object), { relatedRequestId: 0 }); + }); + it('should clear pending debounced notifications on connection close', async () => { // ARRANGE protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced'] }); From 3f66066df16860fddcb393208f05fe82d213e6b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 08:58:38 +0000 Subject: [PATCH 11/17] fix(core,client): SSE transport lifecycle hardening; identity-keyed cancel guard; settle on scheduler throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2616, round 9: - SSEClientTransport.close() ran its teardown unguarded: a throwing eventSource.close() skipped onclose — the ONLY trigger for Protocol._onclose — stranding every pending request. Now try/finally, mirroring StreamableHTTPClientTransport.close(). And _startOrAuth replaced _abortController on every invocation (including the mid-session 401 recovery path) without aborting the predecessor, orphaning the signal captured by in-flight POSTs and making _send's new intentional-abort guard consult the wrong controller; the class now holds a single transport-lifetime controller (`??=`). Tests for both. - The cancellation-send catch guarded on `this._transport !== undefined`, which re-arms after a close-then-reconnect and would resurface the deliberately-aborted POST's AbortError on the brand-new connection. Now capture-and-compare identity (same idiom as _onrequest's capturedTransport): report only when the POST's own connection is still the live one. Regression test: close, immediately reconnect, then land the AbortError — silent. - The reconnect retry closure's scheduleError catch reported through onerror but never fired options.onRequestStreamEnd, unlike every sibling terminal path — a synchronously-throwing custom ReconnectionScheduler left the listen driver hanging. Fire the callback after the report (no double-fire: the maxRetries branch returns before the scheduler runs). Test with a scheduler that throws on the reschedule after a failed leg. - Correct the RequestOptions.maxTotalTimeout JSDoc to the real event-gated contract: the budget is checked only when a progress notification arrives with resetTimeoutOnProgress + onprogress set (inert otherwise, and can overshoot by up to `timeout` ms if progress stops near the boundary) — the previous "regardless of progress notifications" promised timer-enforced behavior the implementation never had. Arming a real budget timer is a behavior change for existing configs, left to maintainers as a follow-up. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- packages/client/src/client/sse.ts | 19 +++++-- packages/client/src/client/streamableHttp.ts | 6 ++ packages/client/test/client/sse.test.ts | 36 ++++++++++++ .../client/test/client/streamableHttp.test.ts | 56 +++++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 34 +++++++++-- .../test/shared/protocol.test.ts | 36 ++++++++++++ 6 files changed, 178 insertions(+), 9 deletions(-) diff --git a/packages/client/src/client/sse.ts b/packages/client/src/client/sse.ts index b3f8381a15..dadcbf9b07 100644 --- a/packages/client/src/client/sse.ts +++ b/packages/client/src/client/sse.ts @@ -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) { @@ -338,9 +343,15 @@ export class SSEClientTransport implements Transport { } async close(): Promise { - 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 { diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index da0512a1d0..fd684e3c2e 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -729,6 +729,12 @@ export class StreamableHTTPClientTransport implements Transport { this._scheduleReconnection(options, attemptCount + 1); } catch (scheduleError) { this.onerror?.(scheduleError instanceof Error ? scheduleError : new Error(String(scheduleError))); + // The chain is terminally dead (no timer was armed) — + // settle the caller, mirroring the maxRetries-exhaustion + // branch. No double-fire is possible: that branch returns + // before the scheduler runs, so _scheduleReconnection can + // never both fire the callback and throw. + options.onRequestStreamEnd?.(); } }); }; diff --git a/packages/client/test/client/sse.test.ts b/packages/client/test/client/sse.test.ts index 49ee5abe8e..a3742368b9 100644 --- a/packages/client/test/client/sse.test.ts +++ b/packages/client/test/client/sse.test.ts @@ -287,6 +287,42 @@ describe('SSEClientTransport', () => { expect((sendError as Error).name).toBe('AbortError'); expect(errorSpy).not.toHaveBeenCalled(); }); + + it('close() still fires onclose when the EventSource teardown throws', async () => { + // onclose is the ONLY trigger for Protocol._onclose (which settles + // every pending request with ConnectionClosed) — a throwing + // eventSource.close() must not strand the protocol layer. + transport = new SSEClientTransport(resourceBaseUrl); + const onclose = vi.fn(); + transport.onclose = onclose; + transport['_eventSource'] = { + close: () => { + throw new Error('es close failed'); + } + } as unknown as (typeof transport)['_eventSource']; + + await expect(transport.close()).rejects.toThrow('es close failed'); + expect(onclose).toHaveBeenCalledTimes(1); + + // Reset the fake so the afterEach close() doesn't throw again. + transport['_eventSource'] = undefined; + }); + + it('keeps a single transport-lifetime AbortController across _startOrAuth invocations', async () => { + // _startOrAuth also runs on the mid-session 401 recovery path; + // replacing the controller there would orphan the signal captured + // by any POST already in flight — close() could no longer cancel + // it, and _send's intentional-abort guard would consult the wrong + // controller. + transport = new SSEClientTransport(resourceBaseUrl); + await transport.start(); + const controller = transport['_abortController']; + expect(controller).toBeDefined(); + + // Simulate the 401 recovery path re-invoking _startOrAuth. + await transport['_startOrAuth'](); + expect(transport['_abortController']).toBe(controller); + }); }); describe('header handling', () => { diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 70fb9137b7..4c67dc3eed 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1585,6 +1585,62 @@ describe('StreamableHTTPClientTransport', () => { expect(onStreamEnd).toHaveBeenCalledTimes(1); }); + it('a ReconnectionScheduler that throws on reschedule still settles the caller via onRequestStreamEnd', async () => { + let schedulerCalls = 0; + const scheduler: ReconnectionScheduler = reconnect => { + schedulerCalls++; + if (schedulerCalls === 1) { + const handle = setTimeout(reconnect, 1); + return () => clearTimeout(handle); + } + // The reschedule after a failed leg: platform denies the task. + throw new Error('platform denied background task'); + }; + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 5, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + }, + reconnectionScheduler: scheduler + }); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const fetchMock = globalThis.fetch as Mock; + // POST: primed SSE stream, graceful close without a response. + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('id: ev-1\ndata: \n\n')); + controller.close(); + } + }) + }); + // The reconnect leg fails genuinely, forcing a reschedule. + fetchMock.mockRejectedValueOnce(new TypeError('fetch failed')); + + const onStreamEnd = vi.fn(); + await transport.start(); + await transport.send( + { jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} }, + { onRequestStreamEnd: onStreamEnd } + ); + await vi.advanceTimersByTimeAsync(10); + + // The failed leg reported once, the scheduler throw reported once, + // and — because no further attempt could be armed — the chain is + // terminally dead, so the caller was settled. + expect(schedulerCalls).toBe(2); + expect(errorSpy).toHaveBeenCalledWith(new TypeError('fetch failed')); + expect(errorSpy).toHaveBeenCalledWith(new Error('platform denied background task')); + expect(onStreamEnd).toHaveBeenCalledTimes(1); + }); + it('close() disarms EVERY pending scheduled reconnect, not just the last-scheduled chain', async () => { transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { reconnectionOptions: { diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 25510e8835..868b9fd24e 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -143,8 +143,23 @@ export type RequestOptions = { resetTimeoutOnProgress?: boolean; /** - * Maximum total time (in milliseconds) to wait for a response. - * If exceeded, an {@linkcode SdkError} with code {@linkcode SdkErrorCode.RequestTimeout} will be raised, regardless of progress notifications. + * Maximum total time (in milliseconds) to wait for a response across + * progress-driven timeout resets. + * + * The budget is event-gated, not timer-enforced: it is checked when a + * progress notification arrives, and only when the request was issued + * with {@linkcode RequestOptions.resetTimeoutOnProgress | resetTimeoutOnProgress} + * `: true` and an {@linkcode RequestOptions.onprogress | onprogress} + * handler — the combination that makes per-leg timeout resets possible + * (without resets, the per-leg {@linkcode RequestOptions.timeout | timeout} + * already bounds the request on its own, and this option has no effect). + * When a progress notification lands after the budget has elapsed, the + * request rejects with an {@linkcode SdkError} with code + * {@linkcode SdkErrorCode.RequestTimeout} (`Maximum total timeout + * exceeded`). If the remote side stops sending progress near the budget + * boundary, settlement falls to the current per-leg timer instead, so the + * total wait can exceed the budget by up to `timeout` ms. + * * If not specified, there is no maximum total timeout. * * For multi-round-trip requests fulfilled by the auto-fulfilment driver @@ -1517,7 +1532,15 @@ export abstract class Protocol { this._progressHandlers.delete(messageId); if (!streamCloseCancels) { - this._transport + // Capture the transport identity at send time: the catch + // below must key on whether THIS connection is still the + // live one when the rejection lands. A bare + // `this._transport !== undefined` re-arms the report after + // a close-then-reconnect and would resurface the + // deliberately-aborted POST's AbortError on the brand-new + // connection (same idiom as _onrequest's capturedTransport). + const sendTransport = this._transport; + sendTransport ?.send( this._envelopeOutbound({ jsonrpc: '2.0', @@ -1544,8 +1567,9 @@ export abstract class Protocol { // by the time the rejection lands here `_onclose` has // already cleared `_transport`. Re-reporting would // resurface an AbortError for a clean shutdown — - // report only failures on a live connection. - if (this._transport !== undefined) { + // report only failures on the connection the POST + // was actually sent on. + if (this._transport === sendTransport) { this._onerror(new Error(`Failed to send cancellation: ${error}`)); } }); diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 8b899419ad..ffd2c13fce 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -1127,6 +1127,42 @@ describe('protocol tests', () => { expect(errors.map(e => e.message).filter(m => m.includes('Failed to send cancellation'))).toHaveLength(0); }); + test('cancellation POST aborted by close(): stays silent even after an immediate reconnect', async () => { + // The close-then-reconnect recovery pattern re-attaches a + // transport before the aborted POST's rejection lands — the guard + // must key on the connection the POST was SENT on, not on whether + // any transport happens to be attached at rejection time. + let rejectCancelSend: ((error: unknown) => void) | undefined; + const tx = new PerRequestStreamTransport(); + const baseSend = tx.send.bind(tx); + tx.send = async (message: JSONRPCMessage, opts?: TransportSendOptions) => { + await baseSend(message, opts); + if ('method' in message && message.method === 'notifications/cancelled') { + return new Promise((_resolve, reject) => { + rejectCancelSend = reject; + }); + } + }; + const proto = createTestProtocol(); + const errors: Error[] = []; + proto.onerror = error => void errors.push(error); + await proto.connect(tx); + setNegotiatedProtocolVersion(proto, '2025-11-25'); + + const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { timeout: 0 }); + await expect(pending).rejects.toThrow('Request timed out'); + expect(rejectCancelSend).toBeDefined(); + + // Deliberate shutdown, then an IMMEDIATE reconnect on the same + // instance — before the aborted POST's rejection lands. + await proto.close(); + await proto.connect(new PerRequestStreamTransport()); + rejectCancelSend?.(new DOMException('The operation was aborted', 'AbortError')); + await new Promise(resolve => setTimeout(resolve, 10)); + + expect(errors.map(e => e.message).filter(m => m.includes('Failed to send cancellation'))).toHaveLength(0); + }); + test('genuine cancellation-send failure on a live connection still reports through onerror', async () => { const tx = new PerRequestStreamTransport(); const baseSend = tx.send.bind(tx); From e0ab79e4f197b4a37216ef773ac39e19181cb2b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 09:17:19 +0000 Subject: [PATCH 12/17] fix(client): disarm-once reconnect bookkeeping; one composed abort signal per request chain Two lifecycle gaps in the reconnect teardown machinery, both from review: - close() now invokes each pending chain's cancel with per-entry try/finally, so one throwing custom ReconnectionScheduler cancel no longer skips disarming sibling chains (the first error still propagates after every chain is disarmed). Each Set entry now also carries the chain's settlement-listener release, so close() drops the listener and the request's own settlement (onclose -> Protocol._onclose -> .finally() requestSignal abort) can no longer invoke the user's cancel a second time. The settlement listener itself runs cancel-then-disarm under try/finally so a throwing cancel cannot strand a stale Set entry. - SSE reconnect legs now reuse ONE composed transport+request abort signal per request chain (threaded via internal SseLegOptions) instead of composing a fresh anySignal per leg. On Node 20.0-20.2 (no AbortSignal.any) the fallback removes its listener pair only when an input fires, so per-leg composition stranded one closure pair per gracefully-completed resume leg on both input signals until settlement (MaxListenersExceededWarning after ~11 polling cycles). Regression tests: throwing cancel doesn't skip siblings on close(); cancel runs at most once across close()+settlement; listener count stays flat across resume legs with AbortSignal.any absent. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 + packages/client/src/client/streamableHttp.ts | 124 +++++++++++---- .../client/test/client/streamableHttp.test.ts | 144 +++++++++++++++++- 3 files changed, 239 insertions(+), 31 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index 4bc7ce3c58..aa71d1897e 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -4,3 +4,5 @@ --- 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). diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index fd684e3c2e..08727fd884 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -99,6 +99,23 @@ export interface StartSSEOptions { onRequestStreamEnd?: () => void; } +/** + * Internal extension of {@linkcode StartSSEOptions}: the composed + * transport+request abort signal for a request chain, built once by the + * chain's first leg and reused verbatim by every rebuilt reconnect leg. + * Reuse matters on Node 20.0–20.2, where the `anySignal` fallback removes + * its listener pair only when one of its inputs fires — a fresh composite + * per leg would strand one closure pair per gracefully-completed leg on + * BOTH the transport signal and the request signal until the request + * settles (`MaxListenersExceededWarning` after ~11 polling cycles). The + * native `AbortSignal.any` path benefits too: one composite allocation per + * chain instead of one per leg. + */ +type SseLegOptions = StartSSEOptions & { + /** The composed fetch signal shared by every leg of this request chain. */ + fetchSignal?: AbortSignal; +}; + /** * Configuration options for reconnection behavior of the {@linkcode StreamableHTTPClientTransport}. */ @@ -139,9 +156,10 @@ export interface StreamableHTTPReconnectionOptions { * @param reconnect - Call this to perform the reconnection attempt. * @param delay - Suggested delay in milliseconds (from backoff calculation). * @param attemptCount - Zero-indexed retry attempt number. - * @returns An optional cancel function. If returned, it will be called on - * {@linkcode StreamableHTTPClientTransport.close | transport.close()} to abort the - * pending reconnection. + * @returns An optional cancel function. If returned, it is called AT MOST once — + * when the pending reconnection is disarmed early, either by + * {@linkcode StreamableHTTPClientTransport.close | transport.close()} or by the + * originating request settling first. Never called after the attempt fires. * * @example * ```ts source="./streamableHttp.examples.ts#ReconnectionScheduler_basicUsage" @@ -333,15 +351,23 @@ export class StreamableHTTPClientTransport implements Transport { private _serverRetryMs?: number; // Server-provided retry delay from SSE retry field private readonly _reconnectionScheduler?: ReconnectionScheduler; /** - * Cancel functions for EVERY pending scheduled reconnection attempt. The - * transport can own several concurrent reconnect chains at once — the + * Disarm bookkeeping for EVERY pending scheduled reconnection attempt. + * The transport can own several concurrent reconnect chains at once — the * standalone notification GET plus one per in-flight request on a legacy * session — so this must be a set, not a single slot: a slot would be * overwritten by each schedule and `close()` could disarm only the * last-written chain, leaving armed timers (or un-cancelled custom * scheduler tasks) behind. + * + * Each entry carries BOTH halves of a chain's teardown: `cancel` (the + * user-supplied or default timer cancel) and `release` (drops the chain's + * settlement listener from its `requestSignal`). `close()` must invoke + * both — releasing the listener is what keeps the request's own + * settlement (onclose → `Protocol._onclose` → the request funnel's + * `.finally()` aborting `requestSignal`) from invoking the user's cancel + * a second time. */ - private readonly _pendingReconnectCancels = new Set<() => void>(); + private readonly _pendingReconnectCancels = new Set<{ cancel: () => void; release: () => void }>(); onclose?: () => void; onerror?: (error: Error) => void; @@ -535,7 +561,7 @@ export class StreamableHTTPClientTransport implements Transport { return typeof v === 'string' && isModernProtocolVersion(v); } - private async _startOrAuthSse(options: StartSSEOptions, isAuthRetry = false, stepUpRetries = 0): Promise { + private async _startOrAuthSse(options: SseLegOptions, isAuthRetry = false, stepUpRetries = 0): Promise { const { resumptionToken, requestSignal } = options; // Same guard as `_handleSseStream`: a resurrected listen stream (the // POST-SSE → GET reconnect path threads `requestSignal` through @@ -544,6 +570,19 @@ export class StreamableHTTPClientTransport implements Transport { // onerror" gate. const isIntentionalAbort = (): boolean => this._abortController?.signal.aborted === true || requestSignal?.aborted === true; + // Compose the fetch signal ONCE per request chain: the first leg + // builds it, every rebuilt leg reuses it via `fetchSignal` (see + // `SseLegOptions` — on the Node 20.0-20.2 `anySignal` fallback a + // fresh composite per leg would leak one listener pair per completed + // leg on both input signals). + const transportSignal = this._abortController?.signal; + const signal = + options.fetchSignal ?? + (requestSignal !== undefined && transportSignal !== undefined + ? anySignal(transportSignal, requestSignal) + : (requestSignal ?? transportSignal)); + const legOptions: SseLegOptions = options.fetchSignal === signal ? options : { ...options, fetchSignal: signal }; + try { // Try to open an initial SSE stream with GET to listen for server messages // This is optional according to the spec - server may not support it @@ -557,11 +596,6 @@ export class StreamableHTTPClientTransport implements Transport { headers.set('last-event-id', resumptionToken); } - const transportSignal = this._abortController?.signal; - const signal = - requestSignal !== undefined && transportSignal !== undefined - ? anySignal(transportSignal, requestSignal) - : (requestSignal ?? transportSignal); const response = await (this._fetch ?? fetch)(this._url, { ...this._requestInit, method: 'GET', @@ -593,7 +627,7 @@ export class StreamableHTTPClientTransport implements Transport { } await response.text?.().catch(() => {}); // Purposely _not_ awaited, so we don't call onerror twice - return this._startOrAuthSse(options, true, stepUpRetries); + return this._startOrAuthSse(legOptions, true, stepUpRetries); } await response.text?.().catch(() => {}); if (isAuthRetry) { @@ -618,7 +652,7 @@ export class StreamableHTTPClientTransport implements Transport { if (result !== 'AUTHORIZED') { throw markAuthSeamEscape(new UnauthorizedError()); } - return this._startOrAuthSse(options, isAuthRetry, stepUpRetries + 1); + return this._startOrAuthSse(legOptions, isAuthRetry, stepUpRetries + 1); } } @@ -645,7 +679,7 @@ export class StreamableHTTPClientTransport implements Transport { }); } - this._handleSseStream(response.body, options, true); + this._handleSseStream(response.body, legOptions, true); } catch (error) { if (!isIntentionalAbort()) { this.onerror?.(error as Error); @@ -681,7 +715,7 @@ export class StreamableHTTPClientTransport implements Transport { * @param lastEventId The ID of the last received event for resumability * @param attemptCount Current reconnection attempt count for this specific stream */ - private _scheduleReconnection(options: StartSSEOptions, attemptCount = 0): void { + private _scheduleReconnection(options: SseLegOptions, attemptCount = 0): void { // Use provided options or default options const maxRetries = this._reconnectionOptions.maxRetries; @@ -700,7 +734,7 @@ export class StreamableHTTPClientTransport implements Transport { // own cancel in `_pendingReconnectCancels` and removes it when the // attempt fires or is disarmed. `listenerCleanup` releases the // settlement listener below in the same motion. - let entry: (() => void) | undefined; + let entry: { cancel: () => void; release: () => void } | undefined; let fired = false; const listenerCleanup = new AbortController(); const disarmBookkeeping = (): void => { @@ -750,7 +784,7 @@ export class StreamableHTTPClientTransport implements Transport { // A custom scheduler may invoke `reconnect` synchronously; do not // register bookkeeping for an attempt that already fired. if (!fired) { - entry = cancelPending; + entry = { cancel: cancelPending, release: () => listenerCleanup.abort() }; this._pendingReconnectCancels.add(entry); // A settled request disarms its own pending attempt immediately // instead of leaving an armed timer to bail at fire time (the @@ -758,15 +792,21 @@ export class StreamableHTTPClientTransport implements Transport { options.requestSignal?.addEventListener( 'abort', () => { - cancelPending(); - disarmBookkeeping(); + // try/finally: a throwing user-supplied cancel must not + // leave a stale Set entry (which close() would re-invoke) + // or an armed settlement listener behind. + try { + cancelPending(); + } finally { + disarmBookkeeping(); + } }, { once: true, signal: listenerCleanup.signal } ); } } - private _handleSseStream(stream: ReadableStream | null, options: StartSSEOptions, isReconnectable: boolean): void { + private _handleSseStream(stream: ReadableStream | null, options: SseLegOptions, isReconnectable: boolean): void { if (!stream) { // A null body on a per-request stream (or its GET resume) is the // same terminal non-resumable outcome as a 405 — fire the @@ -775,7 +815,7 @@ export class StreamableHTTPClientTransport implements Transport { options.onRequestStreamEnd?.(); return; } - const { onresumptiontoken, replayMessageId, requestSignal, onRequestStreamEnd } = options; + const { onresumptiontoken, replayMessageId, requestSignal, onRequestStreamEnd, fetchSignal } = options; // An intentional abort — transport-wide close OR a per-request abort // (McpSubscription.close() aborting its `requestSignal`) — must read as // a clean shutdown: no misleading "SSE stream disconnected" onerror, @@ -863,7 +903,8 @@ export class StreamableHTTPClientTransport implements Transport { onresumptiontoken, replayMessageId, requestSignal, - onRequestStreamEnd + onRequestStreamEnd, + fetchSignal }, 0 ); @@ -899,7 +940,8 @@ export class StreamableHTTPClientTransport implements Transport { onresumptiontoken, replayMessageId, requestSignal, - onRequestStreamEnd + onRequestStreamEnd, + fetchSignal }, 0 ); @@ -983,10 +1025,30 @@ export class StreamableHTTPClientTransport implements Transport { try { // Disarm EVERY pending scheduled reconnection — the transport can // own several concurrent chains, each with its own pending - // attempt. (A throwing cancel still propagates to the caller; the - // finally block guarantees the abort and onclose regardless.) - for (const cancel of this._pendingReconnectCancels) { - cancel(); + // attempt. Per-entry try/finally: one throwing cancel must not + // skip the remaining chains' cancels, and each chain's settlement + // listener is released here so the request's own settlement + // (onclose → `Protocol._onclose` → the request funnel's + // `.finally()` aborting `requestSignal`) cannot invoke the user's + // cancel a second time. The FIRST cancel error still propagates + // to the caller — after every chain is disarmed; the outer + // finally guarantees the abort and onclose regardless. + let firstError: unknown; + let hasError = false; + for (const entry of this._pendingReconnectCancels) { + try { + entry.cancel(); + } catch (error) { + if (!hasError) { + hasError = true; + firstError = error; + } + } finally { + entry.release(); + } + } + if (hasError) { + throw firstError; } } finally { this._pendingReconnectCancels.clear(); @@ -1253,7 +1315,11 @@ export class StreamableHTTPClientTransport implements Transport { { onresumptiontoken, requestSignal: options?.requestSignal, - onRequestStreamEnd: options?.onRequestStreamEnd + onRequestStreamEnd: options?.onRequestStreamEnd, + // Reuse the POST's composed signal for every + // reconnect leg of this response stream (see + // `SseLegOptions`). + fetchSignal: signal }, false ); diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 4c67dc3eed..ffa3f45f18 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -1,3 +1,5 @@ +import { getEventListeners } from 'node:events'; + import type { JSONRPCMessage, JSONRPCRequest, OAuthTokens } from '@modelcontextprotocol/core-internal'; import { OAuthError, OAuthErrorCode, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core-internal'; import type { Mock, Mocked } from 'vitest'; @@ -3067,8 +3069,8 @@ describe('StreamableHTTPClientTransport', () => { expect(transport['_pendingReconnectCancels'].size).toBe(1); // Clean up the pending reconnection to avoid test pollution - for (const cancel of transport['_pendingReconnectCancels']) { - cancel(); + for (const entry of transport['_pendingReconnectCancels']) { + entry.cancel(); } }); }); @@ -3282,6 +3284,65 @@ describe('StreamableHTTPClientTransport', () => { expect(abortController?.signal.aborted).toBe(true); expect(onclose).toHaveBeenCalledTimes(1); }); + + it('a throwing cancel does not skip cancelling sibling chains on close()', async () => { + const cancel1 = vi.fn(() => { + throw new Error('cancel 1 failed'); + }); + const cancel2 = vi.fn(); + const cancels = [cancel1, cancel2]; + let scheduleCount = 0; + const scheduler: ReconnectionScheduler = vi.fn(() => cancels[scheduleCount++]); + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions, + reconnectionScheduler: scheduler + }); + const onclose = vi.fn(); + transport.onclose = onclose; + + await transport.start(); + // Two independent pending chains, each with its own cancel. + triggerReconnection(transport); + triggerReconnection(transport); + const abortController = transport['_abortController']; + + // The FIRST cancel error still propagates, but only after every + // sibling chain has been disarmed too. + await expect(transport.close()).rejects.toThrow('cancel 1 failed'); + expect(cancel1).toHaveBeenCalledTimes(1); + expect(cancel2).toHaveBeenCalledTimes(1); + expect(abortController?.signal.aborted).toBe(true); + expect(onclose).toHaveBeenCalledTimes(1); + expect(transport['_pendingReconnectCancels'].size).toBe(0); + }); + + it("runs a pending chain's cancel at most once across close() and request settlement", async () => { + const cancel = vi.fn(); + const scheduler: ReconnectionScheduler = vi.fn(() => cancel); + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions, + reconnectionScheduler: scheduler + }); + + await transport.start(); + // A per-request chain in its backoff window: the settlement + // listener is armed on the request-scoped signal. + const requestAbort = new AbortController(); + (transport as unknown as { _scheduleReconnection(opts: StartSSEOptions, attempt?: number): void })._scheduleReconnection( + { requestSignal: requestAbort.signal }, + 0 + ); + + await transport.close(); + expect(cancel).toHaveBeenCalledTimes(1); + + // close() -> onclose -> Protocol._onclose rejects the pending + // request -> its .finally() aborts the request-scoped signal. + // The chain's settlement listener must have been released by + // close(), so the cancel does NOT run a second time. + requestAbort.abort(); + expect(cancel).toHaveBeenCalledTimes(1); + }); }); }); @@ -3546,3 +3607,82 @@ describe('legacy era (2025-11-25): request timeout stops the SSE reconnect chain await client.close(); }); }); + +/** + * Regression for the Node 20.0-20.2 `anySignal` fallback: it removes its + * listener pair only when one of its input signals fires, so composing a + * FRESH transport+request signal per SSE reconnect leg strands one closure + * pair per gracefully-completed leg on BOTH input signals until the request + * settles (MaxListenersExceededWarning after ~11 polling cycles). The + * composite must be built once per request chain and reused by every rebuilt + * leg. + */ +describe('anySignal fallback (Node 20.0-20.2): reconnect legs reuse one composite per request chain', () => { + const originalAny = AbortSignal.any; + + beforeEach(() => { + vi.useFakeTimers(); + vi.spyOn(globalThis, 'fetch'); + // Simulate Node 20.0-20.2, where `AbortSignal.any` is unavailable and + // the manual fallback combinator must be used. + (AbortSignal as { any?: typeof AbortSignal.any }).any = undefined; + }); + + afterEach(() => { + (AbortSignal as { any?: typeof AbortSignal.any }).any = originalAny; + vi.useRealTimers(); + vi.clearAllMocks(); + }); + + it('does not accrue abort listeners on the request or transport signal across resume legs', async () => { + const encoder = new TextEncoder(); + let eventSeq = 0; + const fetchMock = globalThis.fetch as Mock; + fetchMock.mockImplementation(async () => ({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: new ReadableStream({ + start(controller) { + // A priming event id, then a graceful close without a + // response: the documented SSE polling pattern — each leg + // completes with NEITHER input signal aborting. + controller.enqueue(encoder.encode(`id: evt-${++eventSeq}\ndata: \n\n`)); + controller.close(); + } + }) + })); + + const transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 2, + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + await transport.start(); + + const requestAbort = new AbortController(); + await transport['_startOrAuthSse']({ resumptionToken: 'evt-0', requestSignal: requestAbort.signal }); + + // Let the chain run several full polling cycles (stream close + + // backoff + resume GET). + for (let cycle = 0; cycle < 5; cycle++) { + await vi.advanceTimersByTimeAsync(20); + } + expect(fetchMock.mock.calls.length).toBeGreaterThanOrEqual(4); + + const transportSignal = transport['_abortController']!.signal; + // One composite per request chain: exactly one fallback listener pair + // total, no matter how many legs have completed. (Between legs the + // pending attempt's settlement listener is also disarmed, so the + // fallback's listener is the only one left on the request signal.) + const requestListeners = getEventListeners(requestAbort.signal, 'abort'); + const transportListeners = getEventListeners(transportSignal, 'abort'); + expect(requestListeners.length).toBeLessThanOrEqual(2); + expect(transportListeners.length).toBeLessThanOrEqual(2); + + await transport.close(); + }); +}); From 2728d04e2a148dbd86d06cbd4c10e146eae7b9fb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 09:33:23 +0000 Subject: [PATCH 13/17] fix(core,client): honor maxTotalTimeout 0; no EventSource resurrection after close() mid-refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings: - protocol.ts _resetTimeout gated the total-budget check on truthiness, so maxTotalTimeout: 0 — the strictest possible budget — silently disabled the check and left the request indefinitely progress-extendable, while 1 rejected after 1ms. Now gated on !== undefined, mirroring the falsy-vs-legitimate-zero sweep already applied to _oncancel's requestId and the debounce relatedRequestId gate. Boundary test: maxTotalTimeout 0 rejects on the first progress check. - sse.ts's mid-session 401 recovery ran its success continuation (_startOrAuth) after an arbitrarily long onUnauthorized await with no closed-state check: a close() landing during the pending token refresh let the continuation resurrect a live EventSource nothing could tear down (the ES wrapper fetch never carries the transport-lifetime signal, and close() had already run against the old instance). The continuation now bails when the transport-lifetime controller is aborted, rejecting start() with UnauthorizedError('Transport closed during re-authentication'). Regression test: close() during a pending onUnauthorized leaves the GET-attempt count flat and start() rejects. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- .../legacy-sse-reconnect-after-timeout.md | 2 +- packages/client/src/client/sse.ts | 16 +++++- packages/client/test/client/sse.test.ts | 53 +++++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 5 +- .../test/shared/protocol.test.ts | 36 +++++++++++++ 5 files changed, 109 insertions(+), 3 deletions(-) diff --git a/.changeset/legacy-sse-reconnect-after-timeout.md b/.changeset/legacy-sse-reconnect-after-timeout.md index aa71d1897e..1992c14225 100644 --- a/.changeset/legacy-sse-reconnect-after-timeout.md +++ b/.changeset/legacy-sse-reconnect-after-timeout.md @@ -5,4 +5,4 @@ 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 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')`. diff --git a/packages/client/src/client/sse.ts b/packages/client/src/client/sse.ts index dadcbf9b07..034c166c2c 100644 --- a/packages/client/src/client/sse.ts +++ b/packages/client/src/client/sse.ts @@ -224,7 +224,21 @@ 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. diff --git a/packages/client/test/client/sse.test.ts b/packages/client/test/client/sse.test.ts index a3742368b9..9289202267 100644 --- a/packages/client/test/client/sse.test.ts +++ b/packages/client/test/client/sse.test.ts @@ -1762,6 +1762,59 @@ describe('SSEClientTransport', () => { expect(getAttempt).toBe(3); }); + it('close() during a pending onUnauthorized does not resurrect the EventSource', async () => { + // Regression: the 401-recovery success continuation ran + // _startOrAuth() after an arbitrarily long onUnauthorized await + // with no closed-state check. A close() landing while the token + // refresh was pending let _startOrAuth open a brand-new + // EventSource that nothing could ever tear down — the ES wrapper + // fetch never carries the transport-lifetime signal, and close() + // had already run against the old instance. + await resourceServer.close(); + + let getAttempts = 0; + resourceServer = createServer((req, res) => { + if (req.method === 'GET') { + getAttempts++; + res.writeHead(401).end(); + } + }); + resourceBaseUrl = await listenOnRandomPort(resourceServer); + + let releaseRefresh!: () => void; + const refreshPending = new Promise(resolve => { + releaseRefresh = resolve; + }); + let refreshStarted!: () => void; + const refreshStartedPromise = new Promise(resolve => { + refreshStarted = resolve; + }); + const authProvider: AuthProvider = { + token: vi.fn(async () => 'token'), + onUnauthorized: vi.fn(async () => { + refreshStarted(); + await refreshPending; + }) + }; + transport = new SSEClientTransport(resourceBaseUrl, { authProvider }); + + const startPromise = transport.start(); + const startRejection = expect(startPromise).rejects.toThrow('Transport closed during re-authentication'); + // Wait until the 401 recovery is mid-refresh, then close the + // transport while onUnauthorized is still pending. + await refreshStartedPromise; + const attemptsAtClose = getAttempts; + await transport.close(); + + // The refresh resolves AFTER close(): the continuation must bail + // instead of opening a new EventSource. + releaseRefresh(); + await startRejection; + // Give a resurrected EventSource ample time to hit the server. + await new Promise(resolve => setTimeout(resolve, 100)); + expect(getAttempts).toBe(attemptsAtClose); + }); + it('retry failure during SSE connect fires onerror exactly once', async () => { // Regression: when the retry EventSource rejected, its onerror fired inside, then // the outer .then() rejection handler fired onerror AGAIN for the same error. diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 868b9fd24e..0c6941134d 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -803,7 +803,10 @@ export abstract class Protocol { if (!info) return false; const totalElapsed = Date.now() - info.startTime; - if (info.maxTotalTimeout && totalElapsed >= info.maxTotalTimeout) { + // `!== undefined`, not truthiness: `maxTotalTimeout: 0` is the + // STRICTEST budget (rejects on the first check) — a falsy gate would + // silently disable it instead. + if (info.maxTotalTimeout !== undefined && totalElapsed >= info.maxTotalTimeout) { // Disarm the still-armed per-leg timer BEFORE dropping the map // entry: once the entry is gone, `_cleanupTimeout` (the funnel's // `.finally()` cleanup) can no longer reach the timer, and an diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index ffd2c13fce..b4348c7286 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -492,6 +492,42 @@ describe('protocol tests', () => { expect(onProgressMock).toHaveBeenCalledTimes(1); }); + test('maxTotalTimeout: 0 is the strictest budget — rejects on the first progress check, not disabled', async () => { + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + const onProgressMock = vi.fn(); + const requestPromise = testRequest(protocol, request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 0, + resetTimeoutOnProgress: true, + onprogress: onProgressMock + }); + + // The budget is already exhausted, so the FIRST progress + // notification's reset check must reject. Before the fix the + // falsy gate (`info.maxTotalTimeout && ...`) silently disabled a + // 0 budget entirely, leaving the request indefinitely + // progress-extendable — the strictest value was the only one that + // never rejected. + vi.advanceTimersByTime(10); + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { + progressToken: 0, + progress: 25, + total: 100 + } + }); + } + await expect(requestPromise).rejects.toThrow('Maximum total timeout exceeded'); + expect(onProgressMock).not.toHaveBeenCalled(); + }); + test('should timeout if no progress received within timeout period', async () => { await protocol.connect(transport); const request = { method: 'example', params: {} }; From 0e65433af8ebf6c9fb3f77355b000a6f9d1fb7fd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 09:41:50 +0000 Subject: [PATCH 14/17] fix(client): contain throwing scheduler cancels in the settlement listener; fix maxTotalTimeout doc claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The settlement listener in _scheduleReconnection invoked the user-supplied ReconnectionScheduler cancel with try/finally but no catch. An exception thrown inside an AbortSignal 'abort' listener is not delivered to the abort() caller — abort() returns normally and Node reports the exception as an uncaughtException, terminating the process by default. Since the protocol funnel aborts requestSignal on every settlement path, a throwing custom-scheduler cancel would kill the process on the common path. The listener now routes the error through onerror (close()'s sibling loop keeps rethrowing — it has a caller to reject; the listener does not) while the finally disarm still runs. Failing-first regression test: throwing cancel at settlement fires onerror once with the original error, leaves no stale Set entry, and nothing escapes the dispatch. - docs/clients/calling.md still called maxTotalTimeout 'the absolute cap' — the claim the corrected RequestOptions JSDoc retracts. Now states the event-gated contract: checked as each progress update arrives, effective only alongside resetTimeoutOnProgress + onprogress, and can overrun by up to one timeout leg when progress stops near the boundary. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- docs/clients/calling.md | 2 +- packages/client/src/client/streamableHttp.ts | 16 ++++++-- .../client/test/client/streamableHttp.test.ts | 39 +++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/docs/clients/calling.md b/docs/clients/calling.md index 581abe4aea..f3ac43ee52 100644 --- a/docs/clients/calling.md +++ b/docs/clients/calling.md @@ -147,7 +147,7 @@ The server matches `f` against the values it accepts for `tone`: ## Track progress on a long call -Every verb takes request options as a second argument. `onprogress` receives each `notifications/progress` the server emits for this call; `resetTimeoutOnProgress` restarts the request timeout on every update and `maxTotalTimeout` is the absolute cap. +Every verb takes request options as a second argument. `onprogress` receives each `notifications/progress` the server emits for this call; `resetTimeoutOnProgress` restarts the request timeout on every update and `maxTotalTimeout` caps the total wait across those resets — the budget is checked as each progress update arrives (it takes effect only alongside `resetTimeoutOnProgress` and `onprogress`, as here), so if the server stops sending progress near the boundary the call can overrun the budget by up to one `timeout` leg. ```ts source="../../examples/guides/clients/calling.examples.ts#callTool_progress" const exported = await client.callTool( diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index 08727fd884..04c43e295f 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -792,11 +792,21 @@ export class StreamableHTTPClientTransport implements Transport { options.requestSignal?.addEventListener( 'abort', () => { - // try/finally: a throwing user-supplied cancel must not - // leave a stale Set entry (which close() would re-invoke) - // or an armed settlement listener behind. + // catch: an exception thrown inside an AbortSignal 'abort' + // listener is not delivered to the abort() caller — Node + // reports it as an uncaughtException (process exit by + // default), and the funnel aborts requestSignal on EVERY + // settlement, so a throwing user-supplied cancel would + // kill the process on the common path. Route it through + // onerror instead (close() has a caller to reject, so its + // sibling loop rethrows; this listener does not). + // finally: the throw must not leave a stale Set entry + // (which close() would re-invoke) or an armed settlement + // listener behind. try { cancelPending(); + } catch (error) { + this.onerror?.(error instanceof Error ? error : new Error(String(error))); } finally { disarmBookkeeping(); } diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index ffa3f45f18..64242ee873 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -3343,6 +3343,45 @@ describe('StreamableHTTPClientTransport', () => { requestAbort.abort(); expect(cancel).toHaveBeenCalledTimes(1); }); + + it('a throwing cancel at request settlement reports through onerror instead of escaping the abort dispatch', async () => { + // Regression: the settlement listener invoked the user-supplied + // cancel with try/finally but no catch. An exception thrown in an + // AbortSignal 'abort' listener is NOT delivered to the abort() + // caller — abort() returns normally and Node reports the + // exception as an uncaughtException, terminating the process by + // default. The protocol funnel aborts requestSignal on EVERY + // settlement (success, timeout, caller abort, maxTotalTimeout), + // so a throwing custom-scheduler cancel would kill the process on + // the common path. + const cancelError = new Error('cancel failed at settlement'); + const scheduler: ReconnectionScheduler = vi.fn(() => () => { + throw cancelError; + }); + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions, + reconnectionScheduler: scheduler + }); + const onerror = vi.fn(); + transport.onerror = onerror; + + await transport.start(); + const requestAbort = new AbortController(); + (transport as unknown as { _scheduleReconnection(opts: StartSSEOptions, attempt?: number): void })._scheduleReconnection( + { requestSignal: requestAbort.signal }, + 0 + ); + + // The request settles: the funnel aborts its request-scoped + // signal. The throwing cancel must be routed to onerror, not + // escape the EventTarget dispatch. + requestAbort.abort(); + + expect(onerror).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledWith(cancelError); + // The finally disarm still ran: no stale Set entry survives. + expect(transport['_pendingReconnectCancels'].size).toBe(0); + }); }); }); From 9636485b2de7e52a7bf123e435021988ad10c7c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 10:02:35 +0000 Subject: [PATCH 15/17] fix(core,client): settle the graceful SSE tail outside the read try; guard two post-close onerror paths; era-scope the abort doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings: - streamableHttp.ts: the graceful-close settlement tail (needsReconnect check, first _scheduleReconnection, else-branch stream-end callback) ran inside the same try as the stream read loop, so a sync throw from a user-supplied scheduler or onRequestStreamEnd landed in the generic catch, was mislabeled 'SSE stream disconnected', and re-drove the tail: a first-schedule scheduler throw produced two onerror reports (violating the exactly-once contract), and a throwing stream-end callback was invoked twice with the second throw escaping the fire-and-forget processStream() as an unhandledRejection. The tail now runs after the try/catch (catch returns), the first-schedule call is guarded like its error-path twin (raw error once + stream-end), and all processStream stream-end invocations go through a guarded fireStreamEnd that routes callback throws to onerror. - protocol.ts: the debounced-notification fire-and-forget send caught with an unconditional _onerror, so a close() landing while the coalesced POST was in flight resurfaced the deliberate-teardown AbortError at the protocol layer. Now uses the same capture-and-compare guard as cancel()'s notifications/cancelled POST: report only failures on the connection the POST was sent on. - sse.ts: the FAILURE arm of the 401-recovery continuation still called onerror unconditionally after the success arm gained a closed-state guard — a refresh rejecting after close() surfaced a spurious auth error post-shutdown. Mirrors the sibling guard; reject(error) kept so a pending start() settles. - upgrade-to-v2.md: the typed-verbs bullet's era-unqualified 'aborting still sends notifications/cancelled' claim is now era-scoped (cancelled POST on legacy-era + single-channel at any era; stream close IS the cancel on 2026-era Streamable HTTP), matching the corrected wording in the file's own cancellation bullets. All four failing-test-first (docs excepted): first-schedule throw -> one raw onerror + stream-end; throwing onRequestStreamEnd -> invoked once, no unhandledRejection; close-mid-debounced-POST -> no onerror (live-connection twin still reports); refresh rejects after close() -> no onerror, start() still rejects. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- docs/migration/upgrade-to-v2.md | 7 +- packages/client/src/client/sse.ts | 11 ++- packages/client/src/client/streamableHttp.ts | 94 ++++++++++++------- packages/client/test/client/sse.test.ts | 47 ++++++++++ .../client/test/client/streamableHttp.test.ts | 69 ++++++++++++++ packages/core-internal/src/shared/protocol.ts | 17 +++- .../test/shared/protocol.test.ts | 61 ++++++++++++ 7 files changed, 267 insertions(+), 39 deletions(-) diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 453dfdfcdc..e9f2e554a3 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1568,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), diff --git a/packages/client/src/client/sse.ts b/packages/client/src/client/sse.ts index 034c166c2c..d4241b4f59 100644 --- a/packages/client/src/client/sse.ts +++ b/packages/client/src/client/sse.ts @@ -244,7 +244,16 @@ export class SSEClientTransport implements Transport { // callbacks alike. (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); } ); diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index 04c43e295f..ebc5101a0e 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -840,6 +840,16 @@ export class StreamableHTTPClientTransport implements Transport { // Track whether we've received a response - if so, no need to reconnect // Reconnection is for when server disconnects BEFORE sending response let receivedResponse = false; + // A throwing caller-supplied stream-end callback must not escape the + // fire-and-forget processStream() promise as an unhandledRejection — + // route it through onerror instead. + const fireStreamEnd = (): void => { + try { + onRequestStreamEnd?.(); + } catch (callbackError) { + this.onerror?.(callbackError instanceof Error ? callbackError : new Error(String(callbackError))); + } + }; const processStream = async () => { // this is the closest we can get to trying to catch network errors // if something happens reader will throw @@ -893,37 +903,6 @@ export class StreamableHTTPClientTransport implements Transport { } } } - - // Handle graceful server-side disconnect - // Server may close connection after sending event ID and retry field - // Reconnect if: already reconnectable (GET stream) OR received a priming event (POST stream with event ID) - // BUT don't reconnect if we already received a response - the request is complete - const canResume = isReconnectable || hasPrimingEvent; - const needsReconnect = canResume && !receivedResponse; - if (needsReconnect && this._abortController && !isIntentionalAbort()) { - this._scheduleReconnection( - { - // Fall back to the token this leg was opened with: - // a resume leg that drops before delivering its - // first event has no `lastEventId`, and rebuilding - // without a token would degrade the resume into a - // token-less standalone GET (dead-ends on 405 - // servers; loses the replay position otherwise). - resumptionToken: lastEventId ?? options.resumptionToken, - onresumptiontoken, - replayMessageId, - requestSignal, - onRequestStreamEnd, - fetchSignal - }, - 0 - ); - } else if (!isIntentionalAbort()) { - // The per-request stream ended without reconnecting (no - // priming event for a POST stream, or response already - // received). Not a deliberate abort — notify the caller. - onRequestStreamEnd?.(); - } } catch (error) { if (isIntentionalAbort()) { // The reader threw because we aborted it. Not an error; do @@ -944,7 +923,7 @@ export class StreamableHTTPClientTransport implements Transport { this._scheduleReconnection( { // Same fallback as the graceful-close path - // above: never rebuild a resume without its + // below: never rebuild a resume without its // token. resumptionToken: lastEventId ?? options.resumptionToken, onresumptiontoken, @@ -957,13 +936,60 @@ export class StreamableHTTPClientTransport implements Transport { ); } catch (error) { this.onerror?.(new Error(`Failed to reconnect: ${error instanceof Error ? error.message : String(error)}`)); - onRequestStreamEnd?.(); + fireStreamEnd(); } } else { // Non-deliberate stream error without reconnection: the // per-request stream is gone — notify the caller. - onRequestStreamEnd?.(); + fireStreamEnd(); + } + return; + } + + // Handle graceful server-side disconnect. The settlement tail + // runs OUTSIDE the read-loop try: a synchronous throw from a + // user-supplied scheduler or stream-end callback must not land in + // the catch above, where it would be mislabeled "SSE stream + // disconnected" and re-drive this tail a second time (two onerror + // reports for one scheduler failure; a throwing callback invoked + // twice, with the second throw escaping the fire-and-forget + // processStream() as an unhandledRejection). + // Server may close connection after sending event ID and retry field + // Reconnect if: already reconnectable (GET stream) OR received a priming event (POST stream with event ID) + // BUT don't reconnect if we already received a response - the request is complete + const canResume = isReconnectable || hasPrimingEvent; + const needsReconnect = canResume && !receivedResponse; + if (needsReconnect && this._abortController && !isIntentionalAbort()) { + try { + this._scheduleReconnection( + { + // Fall back to the token this leg was opened with: + // a resume leg that drops before delivering its + // first event has no `lastEventId`, and rebuilding + // without a token would degrade the resume into a + // token-less standalone GET (dead-ends on 405 + // servers; loses the replay position otherwise). + resumptionToken: lastEventId ?? options.resumptionToken, + onresumptiontoken, + replayMessageId, + requestSignal, + onRequestStreamEnd, + fetchSignal + }, + 0 + ); + } catch (scheduleError) { + // First-schedule scheduler throw: report the raw error + // exactly once, then settle the caller — mirroring the + // reschedule-throw handling inside _scheduleReconnection. + this.onerror?.(scheduleError instanceof Error ? scheduleError : new Error(String(scheduleError))); + fireStreamEnd(); } + } else if (!isIntentionalAbort()) { + // The per-request stream ended without reconnecting (no + // priming event for a POST stream, or response already + // received). Not a deliberate abort — notify the caller. + fireStreamEnd(); } }; processStream(); diff --git a/packages/client/test/client/sse.test.ts b/packages/client/test/client/sse.test.ts index 9289202267..2be88d4db7 100644 --- a/packages/client/test/client/sse.test.ts +++ b/packages/client/test/client/sse.test.ts @@ -1815,6 +1815,53 @@ describe('SSEClientTransport', () => { expect(getAttempts).toBe(attemptsAtClose); }); + it('a refresh that rejects after close() does not surface onerror (deliberate teardown)', async () => { + // Regression: the FAILURE arm of the 401-recovery continuation + // called this.onerror unconditionally, while the success arm got + // a closed-state guard — so a token refresh that rejects after a + // clean close() (endpoint unreachable at shutdown, abandoned + // interactive flow) surfaced a spurious auth error through + // onerror arbitrarily long after the transport was torn down. + await resourceServer.close(); + + resourceServer = createServer((req, res) => { + if (req.method === 'GET') { + res.writeHead(401).end(); + } + }); + resourceBaseUrl = await listenOnRandomPort(resourceServer); + + let rejectRefresh!: (error: Error) => void; + const refreshPending = new Promise((_resolve, reject) => { + rejectRefresh = reject; + }); + let refreshStarted!: () => void; + const refreshStartedPromise = new Promise(resolve => { + refreshStarted = resolve; + }); + const authProvider: AuthProvider = { + token: vi.fn(async () => 'token'), + onUnauthorized: vi.fn(async () => { + refreshStarted(); + await refreshPending; + }) + }; + transport = new SSEClientTransport(resourceBaseUrl, { authProvider }); + const onerror = vi.fn(); + transport.onerror = onerror; + + const startPromise = transport.start(); + const startRejection = expect(startPromise).rejects.toThrow('refresh failed'); + await refreshStartedPromise; + await transport.close(); + + // The refresh rejects AFTER close(): still settles start(), but + // must not report post-shutdown noise through onerror. + rejectRefresh(new Error('refresh failed')); + await startRejection; + expect(onerror).not.toHaveBeenCalled(); + }); + it('retry failure during SSE connect fires onerror exactly once', async () => { // Regression: when the retry EventSource rejected, its onerror fired inside, then // the outer .then() rejection handler fired onerror AGAIN for the same error. diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 64242ee873..0098b2656f 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -3382,6 +3382,75 @@ describe('StreamableHTTPClientTransport', () => { // The finally disarm still ran: no stale Set entry survives. expect(transport['_pendingReconnectCancels'].size).toBe(0); }); + + it('a first-schedule scheduler throw on graceful close reports the raw error exactly once', async () => { + // Regression: the graceful-close settlement tail ran inside the + // same try as the stream read loop. A scheduler that throws on + // the FIRST schedule of a gap landed in the generic catch, got + // mislabeled "SSE stream disconnected", and the catch re-drove + // the tail: two onerror reports for one scheduler failure. + const scheduleError = new Error('platform denied background task'); + const scheduler: ReconnectionScheduler = vi.fn(() => { + throw scheduleError; + }); + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions, + reconnectionScheduler: scheduler + }); + const onerror = vi.fn(); + transport.onerror = onerror; + const onRequestStreamEnd = vi.fn(); + await transport.start(); + + // A primed POST stream that closes gracefully without a response: + // the graceful tail schedules the first reconnect attempt. + const encoder = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode('id: evt-1\ndata: \n\n')); + controller.close(); + } + }); + transport['_handleSseStream'](stream, { onRequestStreamEnd }, false); + await vi.advanceTimersByTimeAsync(50); + + expect(scheduler).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledWith(scheduleError); + expect(onRequestStreamEnd).toHaveBeenCalledTimes(1); + }); + + it('a throwing onRequestStreamEnd on graceful close is invoked once and never escapes processStream', async () => { + // Regression: with the settlement tail inside the read-loop try, + // a throwing caller-supplied onRequestStreamEnd was caught, the + // catch re-invoked the SAME callback, and its second throw + // escaped the fire-and-forget processStream() promise as an + // unhandledRejection. + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions + }); + const onerror = vi.fn(); + transport.onerror = onerror; + const callbackError = new Error('stream end failed'); + const onRequestStreamEnd = vi.fn(() => { + throw callbackError; + }); + await transport.start(); + + // A POST stream with no priming event ends gracefully: the tail + // takes the no-reconnect branch and fires the stream-end callback. + const stream = new ReadableStream({ + start(controller) { + controller.close(); + } + }); + transport['_handleSseStream'](stream, { onRequestStreamEnd }, false); + await vi.advanceTimersByTimeAsync(50); + + expect(onRequestStreamEnd).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledWith(callbackError); + }); }); }); diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 0c6941134d..36f00327ba 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -1773,8 +1773,21 @@ export abstract class Protocol { } // Send the notification, but don't await it here to avoid blocking. - // Handle potential errors with a .catch(). - this._transport?.send(jsonrpcNotification, options).catch(error => this._onerror(error)); + // Handle potential errors with a .catch(). Capture the + // transport identity at send time (same idiom as cancel()'s + // notifications/cancelled POST): a deliberate close() aborts + // the in-flight coalesced POST — the transport's own catch + // stays silent (intentional-abort guard) and rethrows, and by + // the time the rejection lands here `_onclose` has already + // cleared `_transport`. Re-reporting would resurface an + // AbortError through onerror for a clean shutdown — report + // only failures on the connection the POST was sent on. + const sendTransport = this._transport; + sendTransport.send(jsonrpcNotification, options).catch(error => { + if (this._transport === sendTransport) { + this._onerror(error); + } + }); }); // Return immediately. diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index b4348c7286..39767b404e 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -730,6 +730,67 @@ describe('protocol tests', () => { expect(sendSpy).not.toHaveBeenCalled(); }); + it('debounced send rejection after close() does not surface onerror (deliberate teardown)', async () => { + // Regression: the coalesced POST's fire-and-forget catch reported + // unconditionally. A close() landing while the POST was in flight + // resurfaced the deliberate-teardown AbortError through + // Protocol.onerror — the transport-level guards suppress their + // own report and rethrow, so the protocol layer must key the + // report on whether THIS connection is still the live one (same + // capture-and-compare idiom as cancel()'s notifications/cancelled + // POST). + protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced'] }); + await protocol.connect(transport); + const onerror = vi.fn(); + protocol.onerror = onerror; + + let rejectSend!: (error: Error) => void; + sendSpy.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectSend = reject; + }) + ); + + protocol.notification({ method: 'test/debounced' }); + await flushMicrotasks(); + expect(sendSpy).toHaveBeenCalledTimes(1); + + // close() clears _transport; the aborted POST's rejection lands + // afterwards. + await protocol.close(); + rejectSend(new Error('This operation was aborted')); + await flushMicrotasks(); + + expect(onerror).not.toHaveBeenCalled(); + }); + + it('debounced send rejection on a live connection still reports through onerror', async () => { + protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced'] }); + await protocol.connect(transport); + const onerror = vi.fn(); + protocol.onerror = onerror; + + const sendError = new Error('network down'); + let rejectSend!: (error: Error) => void; + sendSpy.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectSend = reject; + }) + ); + + protocol.notification({ method: 'test/debounced' }); + await flushMicrotasks(); + expect(sendSpy).toHaveBeenCalledTimes(1); + + rejectSend(sendError); + await flushMicrotasks(); + + expect(onerror).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledWith(sendError); + }); + it('should debounce multiple synchronous calls when params property is omitted', async () => { // ARRANGE protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced'] }); From 6bf58a350cf2ba42b6f3649ffb024314e5062dab Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 10:36:21 +0000 Subject: [PATCH 16/17] fix(client): contain every user callback in the reconnect machinery; abort stale probe exchanges on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings: - onRequestStreamEnd was invoked bare at sites reached through floating promise chains (the reconnect closure's scheduleError branch, the resumptionToken short-circuit's catch) and other uncontained contexts (the maxRetries-exhaustion branch, the 405 and null-body terminal outcomes): a throwing caller callback became an unhandledRejection (process exit by default), or landed in an unrelated catch and re-drove settlement. Every invocation in the transport now routes through a guarded _fireRequestStreamEnd helper that reports callback failures via onerror — the callback is a completion signal, not a caller to reject; processStream's fireStreamEnd delegates to it. - onresumptiontoken was invoked bare in the SSE read loop: a throw (e.g. QuotaExceededError from the storage write it is documented for) exited into the generic catch — misattributed 'SSE stream disconnected' onerror, the event's payload (including a response-bearing event) lost unreplayably, and reconnection re-entered at attempt 0 on every resume. Now guarded in place: the failure reports via onerror and the event still dispatches. - ProbeWindow.exchange sent the version-negotiation probe with no send options, so a timed-out probe's settle() tore down nothing on the wire — with probe.maxRetries >= 1 over Streamable HTTP the stale exchange's POST and any primed reconnect chain stayed alive and its late response leaked into the live session (the #2615 shape, one layer down). Each exchange now carries its own AbortController as requestSignal, aborted (idempotently) in settle(); a no-op on transports that ignore requestSignal (stdio). All failing-test-first: throwing onRequestStreamEnd in the reschedule-throw branch and the short-circuit catch -> contained, reported via onerror (pre-fix: unhandledRejection); throwing onresumptiontoken -> reported once, response still dispatches, no bogus reconnect; timed-out probe -> its requestSignal aborted, retry carries a fresh one. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- packages/client/src/client/streamableHttp.ts | 61 +++++++--- .../client/src/client/versionNegotiation.ts | 16 ++- .../client/test/client/streamableHttp.test.ts | 112 ++++++++++++++++++ .../test/client/versionNegotiation.test.ts | 53 +++++++++ 4 files changed, 226 insertions(+), 16 deletions(-) diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index ebc5101a0e..13c00ee51f 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -668,8 +668,11 @@ export class StreamableHTTPClientTransport implements Transport { // stream-end callback so the caller can settle (otherwise // a resumed listen subscription dead-ends silently). The // standalone-GET callers never pass `onRequestStreamEnd`, - // so this is a no-op for them. - options.onRequestStreamEnd?.(); + // so this is a no-op for them. Guarded: a throwing + // callback would otherwise land in this function's catch + // (misattributed onerror + rethrow) and be re-invoked by + // the resumptionToken short-circuit's own catch. + this._fireRequestStreamEnd(options.onRequestStreamEnd); return; } @@ -709,6 +712,24 @@ export class StreamableHTTPClientTransport implements Transport { return Math.min(initialDelay * Math.pow(growFactor, attempt), maxDelay); } + /** + * Invoke a caller-supplied `onRequestStreamEnd` under a guard. The + * callback fires from floating promise chains (the reconnect closure, the + * resumptionToken short-circuit's catch) and fire-and-forget stream + * processors — contexts where a synchronous throw would either become an + * unhandledRejection (process exit by default) or land in an unrelated + * catch and corrupt its settlement logic. Failures are routed through + * `onerror` instead; the callback is a completion signal, not a caller to + * reject. + */ + private _fireRequestStreamEnd(onRequestStreamEnd?: () => void): void { + try { + onRequestStreamEnd?.(); + } catch (error) { + this.onerror?.(error instanceof Error ? error : new Error(String(error))); + } + } + /** * Schedule a reconnection attempt using server-provided retry interval or backoff * @@ -723,7 +744,7 @@ export class StreamableHTTPClientTransport implements Transport { if (attemptCount >= maxRetries) { this.onerror?.(new Error(`Maximum reconnection attempts (${maxRetries}) exceeded.`)); // The per-request stream is now definitively gone. - options.onRequestStreamEnd?.(); + this._fireRequestStreamEnd(options.onRequestStreamEnd); return; } @@ -767,8 +788,10 @@ export class StreamableHTTPClientTransport implements Transport { // settle the caller, mirroring the maxRetries-exhaustion // branch. No double-fire is possible: that branch returns // before the scheduler runs, so _scheduleReconnection can - // never both fire the callback and throw. - options.onRequestStreamEnd?.(); + // never both fire the callback and throw. Guarded: this + // runs inside a floating promise chain, so a throwing + // callback would otherwise become an unhandledRejection. + this._fireRequestStreamEnd(options.onRequestStreamEnd); } }); }; @@ -822,7 +845,7 @@ export class StreamableHTTPClientTransport implements Transport { // same terminal non-resumable outcome as a 405 — fire the // stream-end callback so the caller can settle. No-op for // standalone-GET callers (they never pass `onRequestStreamEnd`). - options.onRequestStreamEnd?.(); + this._fireRequestStreamEnd(options.onRequestStreamEnd); return; } const { onresumptiontoken, replayMessageId, requestSignal, onRequestStreamEnd, fetchSignal } = options; @@ -843,13 +866,7 @@ export class StreamableHTTPClientTransport implements Transport { // A throwing caller-supplied stream-end callback must not escape the // fire-and-forget processStream() promise as an unhandledRejection — // route it through onerror instead. - const fireStreamEnd = (): void => { - try { - onRequestStreamEnd?.(); - } catch (callbackError) { - this.onerror?.(callbackError instanceof Error ? callbackError : new Error(String(callbackError))); - } - }; + const fireStreamEnd = (): void => this._fireRequestStreamEnd(onRequestStreamEnd); const processStream = async () => { // this is the closest we can get to trying to catch network errors // if something happens reader will throw @@ -878,7 +895,18 @@ export class StreamableHTTPClientTransport implements Transport { lastEventId = event.id; // Mark that we've received a priming event - stream is now resumable hasPrimingEvent = true; - onresumptiontoken?.(event.id); + // Guard the caller's persistence hook: a throw (e.g. + // QuotaExceededError from the storage write this + // callback is documented for) must not exit the read + // loop — that would misattribute the failure as "SSE + // stream disconnected", lose this event's payload + // unreplayably (its id was already consumed), and + // re-enter reconnection at attempt 0 on every resume. + try { + onresumptiontoken?.(event.id); + } catch (callbackError) { + this.onerror?.(callbackError instanceof Error ? callbackError : new Error(String(callbackError))); + } } // Skip events with no data (priming events, keep-alives) @@ -1165,7 +1193,10 @@ export class StreamableHTTPClientTransport implements Transport { // maxRetries-exhaustion branch. Not on intentional aborts: // the contract excludes deliberate teardown. if (options?.requestSignal?.aborted !== true && this._abortController?.signal.aborted !== true) { - options?.onRequestStreamEnd?.(); + // Guarded: this catch is a floating promise chain, so + // a throwing callback would otherwise become an + // unhandledRejection. + this._fireRequestStreamEnd(options?.onRequestStreamEnd); } }); return; diff --git a/packages/client/src/client/versionNegotiation.ts b/packages/client/src/client/versionNegotiation.ts index 9b89d962ba..b4687a253f 100644 --- a/packages/client/src/client/versionNegotiation.ts +++ b/packages/client/src/client/versionNegotiation.ts @@ -260,6 +260,17 @@ class ProbeWindow { const id = `server-discover-probe-${++this._probeCounter}`; return new Promise(resolve => { let settled = false; + // Request-scoped abort for this probe exchange, mirroring the + // signal the protocol layer threads for every real request: + // settling the probe — the timeout settle included — must tear + // its POST (and, over Streamable HTTP, any primed per-request + // SSE reconnect chain) down on the wire, not merely stop + // listening. Otherwise a timed-out probe under + // `probe.maxRetries >= 1` leaves an unabortable stale exchange + // whose late response leaks into the live session — the #2615 + // failure shape, one layer down. No-op on transports that ignore + // `requestSignal` (stdio). + const exchangeAbort = new AbortController(); const settle = (reply: RawProbeReply) => { if (settled) return; settled = true; @@ -267,11 +278,14 @@ class ProbeWindow { if (this._pending?.id === id) { this._pending = undefined; } + exchangeAbort.abort(); resolve(reply); }; const timer = setTimeout(() => settle({ kind: 'timeout' }), timeoutMs); this._pending = { id, resolve: settle }; - this._transport.send(buildRequest(id)).catch((error: unknown) => settle({ kind: 'send-error', error })); + this._transport + .send(buildRequest(id), { requestSignal: exchangeAbort.signal }) + .catch((error: unknown) => settle({ kind: 'send-error', error })); }); } diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 0098b2656f..28961f3c80 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -3451,6 +3451,118 @@ describe('StreamableHTTPClientTransport', () => { expect(onerror).toHaveBeenCalledTimes(1); expect(onerror).toHaveBeenCalledWith(callbackError); }); + + it('a throwing onRequestStreamEnd in the reschedule-throw branch routes to onerror, not an unhandledRejection', async () => { + // Regression: the reconnect closure's scheduleError branch fires + // the caller's stream-end callback inside a floating promise + // chain (`_startOrAuthSse(options).catch(...)`) — a throwing + // callback rejected that chain with nothing attached, + // terminating the process by default. + const scheduleError = new Error('platform denied background task'); + let scheduleCalls = 0; + let capturedReconnect: (() => void) | undefined; + const scheduler: ReconnectionScheduler = vi.fn(reconnect => { + scheduleCalls++; + if (scheduleCalls > 1) { + throw scheduleError; + } + capturedReconnect = reconnect; + return () => {}; + }); + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions, + reconnectionScheduler: scheduler + }); + const onerror = vi.fn(); + transport.onerror = onerror; + const callbackError = new Error('stream end failed'); + const onRequestStreamEnd = vi.fn(() => { + throw callbackError; + }); + await transport.start(); + + // First schedule arms; the leg's fetch then fails, so the catch + // reschedules — and the second schedule throws. + (globalThis.fetch as Mock).mockRejectedValue(new Error('network down')); + (transport as unknown as { _scheduleReconnection(opts: StartSSEOptions, attempt?: number): void })._scheduleReconnection( + { onRequestStreamEnd }, + 0 + ); + capturedReconnect!(); + await vi.advanceTimersByTimeAsync(50); + + expect(onRequestStreamEnd).toHaveBeenCalledTimes(1); + // onerror: the leg's open failure, the scheduler error, and the + // contained callback error — nothing escaped the floating chain. + expect(onerror.mock.calls.map(call => call[0])).toEqual(expect.arrayContaining([scheduleError, callbackError])); + }); + + it('a throwing onRequestStreamEnd in the resumptionToken short-circuit routes to onerror, not an unhandledRejection', async () => { + // Regression: the short-circuit's catch fires the stream-end + // callback inside a floating promise chain; a throwing callback + // became an unhandledRejection. + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions + }); + const onerror = vi.fn(); + transport.onerror = onerror; + const callbackError = new Error('stream end failed'); + const onRequestStreamEnd = vi.fn(() => { + throw callbackError; + }); + await transport.start(); + + // A genuine open failure (not an abort) on the resumed GET fires + // the stream-end callback from the short-circuit's catch. + (globalThis.fetch as Mock).mockRejectedValue(new Error('network down')); + await transport.send({ jsonrpc: '2.0', method: 'ping', id: 'r-1' }, { resumptionToken: 'evt-0', onRequestStreamEnd }); + await vi.advanceTimersByTimeAsync(50); + + expect(onRequestStreamEnd).toHaveBeenCalledTimes(1); + expect(onerror.mock.calls.map(call => call[0])).toEqual(expect.arrayContaining([callbackError])); + }); + + it('a throwing onresumptiontoken does not lose the event or misattribute a stream disconnect', async () => { + // Regression: the caller's persistence hook was invoked bare in + // the read loop. A throw (e.g. QuotaExceededError from the + // documented storage write) exited into the generic catch: a + // misattributed "SSE stream disconnected" onerror, the + // response-bearing event's payload lost unreplayably, and a + // reconnect chain re-entered at attempt 0 with the request never + // marked complete. + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions + }); + const onerror = vi.fn(); + transport.onerror = onerror; + const onmessage = vi.fn(); + transport.onmessage = onmessage; + const quotaError = new Error('QuotaExceededError: storage full'); + const onresumptiontoken = vi.fn(() => { + throw quotaError; + }); + await transport.start(); + + // One event carrying BOTH the id (hook throws) and the response. + const encoder = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode('id: evt-1\ndata: {"jsonrpc":"2.0","id":"r-1","result":{}}\n\n')); + controller.close(); + } + }); + transport['_handleSseStream'](stream, { onresumptiontoken }, false); + await vi.advanceTimersByTimeAsync(50); + + // The hook's failure is reported once, correctly attributed… + expect(onerror).toHaveBeenCalledTimes(1); + expect(onerror).toHaveBeenCalledWith(quotaError); + // …the response still dispatches… + expect(onmessage).toHaveBeenCalledTimes(1); + // …and no bogus reconnect chain starts (response received). + expect(transport['_pendingReconnectCancels'].size).toBe(0); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/client/test/client/versionNegotiation.test.ts b/packages/client/test/client/versionNegotiation.test.ts index 60c9a2aaba..7bb1b59c11 100644 --- a/packages/client/test/client/versionNegotiation.test.ts +++ b/packages/client/test/client/versionNegotiation.test.ts @@ -159,6 +159,59 @@ const requests = (sent: JSONRPCMessage[]): JSONRPCRequest[] => sent.filter(isJSO * Probe mechanics (T9) + modern resolution. * ------------------------------------------------------------------------- */ +describe('probe exchange request-scoped abort (#2615 shape, one layer down)', () => { + test('a timed-out probe aborts its requestSignal; the retry sends with a fresh one', async () => { + // Regression: the probe was sent with no send options, so a timed-out + // exchange's settle() tore down nothing on the wire — over Streamable + // HTTP with probe.maxRetries >= 1, the stale probe's POST (and any + // primed GET+Last-Event-ID reconnect chain) stayed alive and its late + // response leaked into the live session. + const sendSignals: Array = []; + let probeSeq = 0; + class CapturingTransport implements Transport { + onclose?: () => void; + onerror?: (error: Error) => void; + onmessage?: (message: JSONRPCMessage) => void; + sessionId?: string; + async start(): Promise {} + async close(): Promise { + this.onclose?.(); + } + setProtocolVersion(): void {} + async send(message: JSONRPCMessage, options?: { requestSignal?: AbortSignal }): Promise { + if (isJSONRPCRequest(message) && message.method === 'server/discover') { + sendSignals.push(options?.requestSignal); + probeSeq++; + if (probeSeq >= 2) { + const id = message.id; + queueMicrotask(() => this.onmessage?.({ jsonrpc: '2.0', id, result: discoverResult([MODERN]) })); + } + // First probe: never answered — times out, then retries. + } + } + } + + const transport = new CapturingTransport(); + const client = new Client( + { name: 'c', version: '0' }, + { versionNegotiation: { mode: 'auto', probe: { timeoutMs: 30, maxRetries: 1 } } } + ); + await client.connect(transport); + + expect(sendSignals).toHaveLength(2); + const [first, second] = sendSignals; + // Every probe exchange carries its own request-scoped signal… + expect(first).toBeDefined(); + expect(second).toBeDefined(); + expect(second).not.toBe(first); + // …and the timeout settle aborted the stale exchange on the wire, so + // its transport-level guards (fetch abort, reconnect disarm) engage. + expect(first!.aborted).toBe(true); + + await client.close(); + }); +}); + describe('auto mode against a modern server', () => { test('probe-first with a string id, no initialize, setProtocolVersion exactly once after era resolution', async () => { const transport = new ScriptedTransport(modernServerScript()); From 87ebece142f8b2dd861c9868abc5a2470f6556c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 10:49:51 +0000 Subject: [PATCH 17/17] fix(client): report raw scheduler errors on the error-path first-schedule catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error-path first-schedule catch in _handleSseStream wrapped a scheduler throw as 'Failed to reconnect: ', discarding the original error object, while its two siblings for the identical failure class — the graceful-close tail's schedule catch and _scheduleReconnection's reschedule catch — report the raw error. Monitors and error-identity checks now see one consistent shape regardless of whether the previous leg errored or closed gracefully. Failing-first identity test mirrors the graceful-path one: a first-schedule scheduler throw after a mid-read stream error surfaces the raw error object through onerror, and the stream-end callback still fires exactly once. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7 --- packages/client/src/client/streamableHttp.ts | 9 +++-- .../client/test/client/streamableHttp.test.ts | 36 +++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index 13c00ee51f..cfb3a9c4ef 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -962,8 +962,13 @@ export class StreamableHTTPClientTransport implements Transport { }, 0 ); - } catch (error) { - this.onerror?.(new Error(`Failed to reconnect: ${error instanceof Error ? error.message : String(error)}`)); + } catch (scheduleError) { + // Report the raw scheduler error — same shape as the + // graceful-close tail's schedule catch and the + // reschedule catch, so monitors see one consistent + // identity for this failure class regardless of how + // the previous leg ended. + this.onerror?.(scheduleError instanceof Error ? scheduleError : new Error(String(scheduleError))); fireStreamEnd(); } } else { diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 28961f3c80..bf6fb0b298 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -3420,6 +3420,42 @@ describe('StreamableHTTPClientTransport', () => { expect(onRequestStreamEnd).toHaveBeenCalledTimes(1); }); + it('a first-schedule scheduler throw on the ERROR path reports the raw error, matching the graceful path', async () => { + // Regression: the error-path first-schedule catch wrapped the + // scheduler throw as `Failed to reconnect: `, discarding + // the original error object, while the graceful-close tail and + // the reschedule catch both report the raw error — inconsistent + // shape for the same failure class. + const scheduleError = new Error('platform denied background task'); + const scheduler: ReconnectionScheduler = vi.fn(() => { + throw scheduleError; + }); + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions, + reconnectionScheduler: scheduler + }); + const onerror = vi.fn(); + transport.onerror = onerror; + const onRequestStreamEnd = vi.fn(); + await transport.start(); + + // A reconnectable stream that ERRORS mid-read: the catch path + // schedules the first reconnect attempt, and the scheduler throws. + const stream = new ReadableStream({ + start(controller) { + controller.error(new Error('network dropped')); + } + }); + transport['_handleSseStream'](stream, { onRequestStreamEnd }, true); + await vi.advanceTimersByTimeAsync(50); + + expect(scheduler).toHaveBeenCalledTimes(1); + // The stream error reports once ("SSE stream disconnected"), and + // the scheduler failure reports RAW — same identity, no wrapper. + expect(onerror.mock.calls.map(call => call[0])).toEqual(expect.arrayContaining([scheduleError])); + expect(onRequestStreamEnd).toHaveBeenCalledTimes(1); + }); + it('a throwing onRequestStreamEnd on graceful close is invoked once and never escapes processStream', async () => { // Regression: with the settlement tail inside the read-loop try, // a throwing caller-supplied onRequestStreamEnd was caught, the