Skip to content

Commit 295267e

Browse files
committed
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.
1 parent cc4b416 commit 295267e

4 files changed

Lines changed: 291 additions & 13 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@modelcontextprotocol/client': patch
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 alongside the `notifications/cancelled` POST when the request settles — the wire-visible cancellation behavior is unchanged for every (era × transport) combination.

packages/client/test/client/streamableHttp.test.ts

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type { Mock, Mocked } from 'vitest';
44

55
import type { OAuthClientProvider } from '../../src/client/auth';
66
import { UnauthorizedError } from '../../src/client/auth';
7+
import { Client } from '../../src/client/client';
78
import type { ReconnectionScheduler, StartSSEOptions, StreamableHTTPReconnectionOptions } from '../../src/client/streamableHttp';
89
import { StreamableHTTPClientTransport } from '../../src/client/streamableHttp';
910

@@ -1464,6 +1465,57 @@ describe('StreamableHTTPClientTransport', () => {
14641465
expect(fetchMock).toHaveBeenCalledTimes(1);
14651466
});
14661467

1468+
it('per-request requestSignal abort while a reconnect is scheduled: the pending reconnect never fires (#2615)', async () => {
1469+
// ARRANGE — a POST stream that is primed (SSE event id) and then
1470+
// closes gracefully WITHOUT delivering the response, so the
1471+
// transport schedules a GET+Last-Event-ID reconnect. The abort
1472+
// lands in the window between "reconnect scheduled" and "reconnect
1473+
// fires" — the shape a request timeout produces.
1474+
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
1475+
reconnectionOptions: {
1476+
initialReconnectionDelay: 10,
1477+
maxRetries: 5,
1478+
maxReconnectionDelay: 1000,
1479+
reconnectionDelayGrowFactor: 1
1480+
}
1481+
});
1482+
const errorSpy = vi.fn();
1483+
transport.onerror = errorSpy;
1484+
1485+
const primedClosingStream = new ReadableStream<Uint8Array>({
1486+
start(controller) {
1487+
controller.enqueue(new TextEncoder().encode('id: ev-1\ndata: \n\n'));
1488+
controller.close();
1489+
}
1490+
});
1491+
const fetchMock = globalThis.fetch as Mock;
1492+
fetchMock.mockResolvedValueOnce({
1493+
ok: true,
1494+
status: 200,
1495+
headers: new Headers({ 'content-type': 'text/event-stream' }),
1496+
body: primedClosingStream
1497+
});
1498+
1499+
const requestAbort = new AbortController();
1500+
await transport.start();
1501+
await transport.send(
1502+
{ jsonrpc: '2.0', method: 'long_running_tool', id: 'request-1', params: {} },
1503+
{ requestSignal: requestAbort.signal }
1504+
);
1505+
// Let the stream close and the reconnect get scheduled (delay 10ms).
1506+
await vi.advanceTimersByTimeAsync(5);
1507+
expect(fetchMock).toHaveBeenCalledTimes(1);
1508+
1509+
// ACT — the request settles (timeout/cancel) before the reconnect fires.
1510+
requestAbort.abort();
1511+
await vi.advanceTimersByTimeAsync(100);
1512+
1513+
// ASSERT — the scheduled reconnect saw the aborted requestSignal
1514+
// and bailed: no GET resume, no onerror.
1515+
expect(fetchMock).toHaveBeenCalledTimes(1);
1516+
expect(errorSpy).not.toHaveBeenCalled();
1517+
});
1518+
14671519
it('onRequestStreamEnd fires when the per-request POST stream ends gracefully without reconnecting', async () => {
14681520
// ARRANGE — a POST stream with NO priming event id (so the
14691521
// graceful-close path does NOT schedule a reconnect): the
@@ -2737,3 +2789,160 @@ describe('StreamableHTTPClientTransport', () => {
27372789
});
27382790
});
27392791
});
2792+
2793+
/**
2794+
* End-to-end regression for #2615: on a legacy (2025-11-25) session, the
2795+
* transport's request-scoped SSE reconnect chain (GET + Last-Event-ID
2796+
* resumption) must stop once the originating request settles via timeout.
2797+
* Before the fix, the chain kept resuming forever (every successful resume
2798+
* resets the retry counter), and a late resumed GET carrying the original
2799+
* JSON-RPC response surfaced as "Received a response for an unknown message
2800+
* ID".
2801+
*/
2802+
describe('legacy era (2025-11-25): request timeout stops the SSE reconnect chain (#2615)', () => {
2803+
beforeEach(() => {
2804+
vi.useFakeTimers();
2805+
vi.spyOn(globalThis, 'fetch');
2806+
});
2807+
2808+
afterEach(() => {
2809+
vi.useRealTimers();
2810+
vi.clearAllMocks();
2811+
});
2812+
2813+
const encoder = new TextEncoder();
2814+
const sseResponse = (chunks: string[]) => ({
2815+
ok: true,
2816+
status: 200,
2817+
headers: new Headers({ 'content-type': 'text/event-stream' }),
2818+
body: new ReadableStream<Uint8Array>({
2819+
start(controller) {
2820+
for (const chunk of chunks) {
2821+
controller.enqueue(encoder.encode(chunk));
2822+
}
2823+
controller.close();
2824+
}
2825+
})
2826+
});
2827+
const jsonResponse = (message: JSONRPCMessage) => ({
2828+
ok: true,
2829+
status: 200,
2830+
headers: new Headers({ 'content-type': 'application/json' }),
2831+
json: async () => message,
2832+
text: async () => JSON.stringify(message)
2833+
});
2834+
const accepted = () => ({ ok: true, status: 202, headers: new Headers(), text: async () => '' });
2835+
const methodNotAllowed = () => ({
2836+
ok: false,
2837+
status: 405,
2838+
statusText: 'Method Not Allowed',
2839+
headers: new Headers(),
2840+
text: async () => ''
2841+
});
2842+
2843+
it('stops resuming once the request times out; the late response never surfaces as an unknown message ID', async () => {
2844+
let pingId: string | number | undefined;
2845+
let eventSeq = 0;
2846+
let settled = false;
2847+
let resumesAfterSettle = 0;
2848+
const cancelledPosts: JSONRPCMessage[] = [];
2849+
2850+
const fetchMock = globalThis.fetch as Mock;
2851+
fetchMock.mockImplementation(async (_url, init: RequestInit) => {
2852+
if (init.method === 'GET') {
2853+
const lastEventId = (init.headers as Headers).get('last-event-id');
2854+
// Standalone notification stream: not offered by this server.
2855+
if (lastEventId === null) {
2856+
return methodNotAllowed();
2857+
}
2858+
// Request-scoped resume. Once the request has settled, hand
2859+
// back the late original response — before the fix this is
2860+
// the resumed GET that surfaced "unknown message ID".
2861+
if (settled) {
2862+
resumesAfterSettle++;
2863+
return sseResponse([`id: evt-${++eventSeq}\ndata: {"jsonrpc":"2.0","id":${JSON.stringify(pingId)},"result":{}}\n\n`]);
2864+
}
2865+
// Keep the chain alive: a priming event id, then a graceful
2866+
// close without the response (the server expects the client
2867+
// to resume via GET + Last-Event-ID).
2868+
return sseResponse([`id: evt-${++eventSeq}\ndata: \n\n`]);
2869+
}
2870+
const message = JSON.parse(init.body as string) as JSONRPCMessage;
2871+
if ('method' in message) {
2872+
if (message.method === 'initialize' && 'id' in message) {
2873+
return jsonResponse({
2874+
jsonrpc: '2.0',
2875+
id: message.id,
2876+
result: {
2877+
protocolVersion: '2025-11-25',
2878+
capabilities: {},
2879+
serverInfo: { name: 'legacy-server', version: '1.0.0' }
2880+
}
2881+
});
2882+
}
2883+
if (message.method === 'notifications/cancelled') {
2884+
cancelledPosts.push(message);
2885+
return accepted();
2886+
}
2887+
if (message.method === 'ping' && 'id' in message) {
2888+
pingId = message.id;
2889+
// SSE response: retry hint + priming event id, then a
2890+
// graceful close without the response.
2891+
return sseResponse([`retry: 10\nid: evt-${++eventSeq}\ndata: \n\n`]);
2892+
}
2893+
}
2894+
return accepted();
2895+
});
2896+
2897+
const transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
2898+
reconnectionOptions: {
2899+
initialReconnectionDelay: 10,
2900+
maxRetries: 2,
2901+
maxReconnectionDelay: 1000,
2902+
reconnectionDelayGrowFactor: 1
2903+
}
2904+
});
2905+
const client = new Client({ name: 'test-client', version: '1.0.0' });
2906+
const errors: Error[] = [];
2907+
client.onerror = error => errors.push(error);
2908+
2909+
await client.connect(transport);
2910+
2911+
const resumeGetCount = () =>
2912+
fetchMock.mock.calls.filter(call => call[1]?.method === 'GET' && (call[1].headers as Headers).get('last-event-id') !== null)
2913+
.length;
2914+
2915+
let settledError: unknown;
2916+
const pending = client.ping({ timeout: 100 }).catch(error => {
2917+
settled = true;
2918+
settledError = error;
2919+
});
2920+
2921+
// Let the reconnect chain run a few resume cycles before the timeout.
2922+
await vi.advanceTimersByTimeAsync(50);
2923+
expect(resumeGetCount()).toBeGreaterThan(0);
2924+
expect(settled).toBe(false);
2925+
2926+
// Cross the request timeout.
2927+
await vi.advanceTimersByTimeAsync(100);
2928+
await pending;
2929+
expect(settled).toBe(true);
2930+
expect(String(settledError)).toContain('Request timed out');
2931+
2932+
// The legacy wire cancel signal is unchanged: exactly one
2933+
// notifications/cancelled POST.
2934+
expect(cancelledPosts).toHaveLength(1);
2935+
2936+
// Give an orphaned chain ample time to keep resuming (before the fix
2937+
// it reconnected forever — each successful resume resets the retry
2938+
// counter, so maxRetries never binds).
2939+
await vi.advanceTimersByTimeAsync(2000);
2940+
2941+
// THE KEY ASSERTIONS: no resumed GET after the request settled, and
2942+
// the late response never surfaced as an unknown message ID.
2943+
expect(resumesAfterSettle).toBe(0);
2944+
expect(errors.map(e => e.message)).not.toContainEqual(expect.stringContaining('unknown message ID'));
2945+
2946+
await client.close();
2947+
});
2948+
});

packages/core-internal/src/shared/protocol.ts

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1413,9 +1413,18 @@ export abstract class Protocol<ContextT extends BaseContext> {
14131413
// POSTing `notifications/cancelled`. Every other (era × transport)
14141414
// combination — legacy era on any transport, modern era on stdio /
14151415
// in-memory — keeps today's `notifications/cancelled` POST path
1416-
// unchanged.
1416+
// unchanged (the legacy era on a per-request-stream transport
1417+
// additionally aborts `requestSignal` locally; see below).
14171418
const streamCloseCancels = codec.era === MODERN_WIRE_REVISION && this._transport.hasPerRequestStream === true;
1418-
const requestAbort = streamCloseCancels ? new AbortController() : undefined;
1419+
// The per-request AbortController exists on EVERY per-request-stream
1420+
// transport, not just when stream-close is the spec cancel signal.
1421+
// On the legacy era the `notifications/cancelled` POST below stays
1422+
// the wire signal, but the transport still owns a per-request SSE
1423+
// reconnect chain (GET + Last-Event-ID resumption) that nothing
1424+
// else tears down: without this signal, a request that settles via
1425+
// timeout or caller abort leaves orphaned reconnects running until
1426+
// the late response surfaces as "unknown message ID" (#2615).
1427+
const requestAbort = this._transport.hasPerRequestStream === true ? new AbortController() : undefined;
14191428

14201429
const messageId = this._requestMessageId++;
14211430
cleanupMessageId = messageId;
@@ -1450,7 +1459,7 @@ export abstract class Protocol<ContextT extends BaseContext> {
14501459
}
14511460
this._progressHandlers.delete(messageId);
14521461

1453-
if (requestAbort === undefined) {
1462+
if (!streamCloseCancels) {
14541463
this._transport
14551464
?.send(
14561465
this._envelopeOutbound({
@@ -1464,14 +1473,18 @@ export abstract class Protocol<ContextT extends BaseContext> {
14641473
{ relatedRequestId, resumptionToken, onresumptiontoken }
14651474
)
14661475
.catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`)));
1467-
} else {
1468-
// Modern-era per-request-stream transport: aborting the
1469-
// request's underlying stream IS the spec cancel signal.
1470-
// The transport already swallows the resulting AbortError
1471-
// (no spurious `onerror`); a post-abort send() rejection
1472-
// re-hits an already-settled promise below and is a no-op.
1473-
requestAbort.abort();
14741476
}
1477+
// Aborting the request-scoped signal is either the spec cancel
1478+
// signal itself (modern era: closing the per-request stream IS
1479+
// the cancellation, so no `notifications/cancelled` above) or a
1480+
// purely local teardown alongside the POST (legacy era: it
1481+
// stops the transport's SSE reconnect chain for this request —
1482+
// #2615). The transport already swallows the resulting
1483+
// AbortError (no spurious `onerror`); a post-abort send()
1484+
// rejection re-hits an already-settled promise below and is a
1485+
// no-op. The cancelled POST above does not carry this signal,
1486+
// so aborting here cannot cut off that send.
1487+
requestAbort?.abort();
14751488

14761489
// Wrap the reason in an SdkError if it isn't already
14771490
const error = reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason));

packages/core-internal/test/shared/protocol.test.ts

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,7 @@ describe('protocol tests', () => {
881881
expect(cancelledSent(sent)).toHaveLength(1);
882882
});
883883

884-
test('legacy era + per-request-stream transport: behavior unchanged — POSTs notifications/cancelled, no requestSignal', async () => {
884+
test('legacy era + per-request-stream transport: POSTs notifications/cancelled AND aborts the requestSignal (#2615)', async () => {
885885
const tx = new PerRequestStreamTransport();
886886
const proto = createTestProtocol();
887887
await proto.connect(tx);
@@ -890,13 +890,64 @@ describe('protocol tests', () => {
890890
const ac = new AbortController();
891891
const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { signal: ac.signal });
892892

893-
// Legacy path is byte-identical to before: no requestSignal threaded.
894-
expect(tx.lastRequestSignal).toBeUndefined();
893+
// The requestSignal is threaded on the legacy era too — it is not
894+
// the spec cancel signal there (the POST below is), but the
895+
// transport needs it to tear down the request's SSE reconnect
896+
// chain (GET + Last-Event-ID resumption) when the request settles.
897+
const requestSignal = tx.lastRequestSignal;
898+
expect(requestSignal).toBeInstanceOf(AbortSignal);
899+
expect(requestSignal?.aborted).toBe(false);
895900

896901
ac.abort('user cancel');
897902
await expect(pending).rejects.toThrow();
898903

904+
// The wire signal is unchanged (spec cancel = notifications/cancelled)…
899905
expect(cancelledSent(tx.sent)).toHaveLength(1);
906+
// …and the request-scoped abort additionally stops any reconnect
907+
// chain the transport still owns for this request (#2615).
908+
expect(requestSignal?.aborted).toBe(true);
909+
});
910+
911+
test('legacy era + per-request-stream transport: timeout POSTs notifications/cancelled AND aborts the requestSignal (#2615)', async () => {
912+
const tx = new PerRequestStreamTransport();
913+
const proto = createTestProtocol();
914+
await proto.connect(tx);
915+
setNegotiatedProtocolVersion(proto, '2025-11-25');
916+
917+
const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { timeout: 0 });
918+
const requestSignal = tx.lastRequestSignal;
919+
expect(requestSignal).toBeInstanceOf(AbortSignal);
920+
921+
await expect(pending).rejects.toThrow('Request timed out');
922+
923+
expect(cancelledSent(tx.sent)).toHaveLength(1);
924+
expect(requestSignal?.aborted).toBe(true);
925+
});
926+
927+
test('legacy era + single-channel transport (no hasPerRequestStream): POSTs notifications/cancelled, no requestSignal', async () => {
928+
// stdio / in-memory shape: hasPerRequestStream is undefined.
929+
const sent: JSONRPCMessage[] = [];
930+
let sawRequestSignal: AbortSignal | undefined;
931+
const tx = new MockTransport();
932+
tx.send = async (m: JSONRPCMessage, opts?: TransportSendOptions) => {
933+
sent.push(m);
934+
if (opts?.requestSignal !== undefined) {
935+
sawRequestSignal = opts.requestSignal;
936+
}
937+
};
938+
const proto = createTestProtocol();
939+
await proto.connect(tx);
940+
setNegotiatedProtocolVersion(proto, '2025-11-25');
941+
942+
const ac = new AbortController();
943+
const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { signal: ac.signal });
944+
ac.abort('user cancel');
945+
await expect(pending).rejects.toThrow();
946+
947+
// No per-request stream to tear down — the legacy single-channel
948+
// path stays byte-identical: cancelled POST only, no requestSignal.
949+
expect(cancelledSent(sent)).toHaveLength(1);
950+
expect(sawRequestSignal).toBeUndefined();
900951
});
901952

902953
test('modern era + per-request-stream transport: timeout aborts the stream, NO notifications/cancelled', async () => {

0 commit comments

Comments
 (0)