Skip to content

Commit 0826c2d

Browse files
fix(client): listen polish — derived ack-wait signal; envelope on listen-cancel; intentional-abort guards across reconnect path; typed result-during-opening
connect()'s auto-open now derives a one-shot AbortController bound to connect's signal only for the listen() ack-wait duration (listener removed in finally; already-aborted handled), so an aborted connect rejects fast instead of blocking for the ack timeout, while the auto-opened subscription still outlives connect's signal. wireTeardown's notifications/cancelled now spreads _outboundMetaEnvelope() into params._meta — the listen-path cancel was the only modern outbound bypassing the auto-envelope. streamableHttp: the intentional per-request abort guard now also covers _send()'s catch (abort before headers), _startOrAuthSse()'s GET fetch signal + catch, and _scheduleReconnection()'s timer-fire / catch — a closed listen subscription on a flaky network can no longer be resurrected as a GET stream nor surface a misleading onerror. parked.terminated 'response' surfaces a typed SdkErrorCode.InvalidResult ('server answered subscriptions/listen with a result; expected the acknowledged notification') instead of a generic stream-ended message.
1 parent c72a59a commit 0826c2d

4 files changed

Lines changed: 194 additions & 21 deletions

File tree

packages/client/src/client/client.ts

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -927,20 +927,37 @@ export class Client extends Protocol<ClientContext> {
927927
// server may not support it, or refuse on capacity). Surface
928928
// via onerror; the consumer can call listen() later.
929929
//
930-
// Forward ONLY the ack timeout from connect()'s options.
931930
// listen() binds RequestOptions.signal to the SUBSCRIPTION
932-
// lifetime, so a connect-scoped signal (e.g.
933-
// `AbortSignal.timeout(30_000)` for the handshake) would tear
934-
// the auto-opened stream down the moment it fires after
935-
// connect has already resolved. Connect's signal governs the
936-
// handshake only; the auto-opened subscription outlives it.
931+
// lifetime, so connect()'s signal must NOT be forwarded
932+
// verbatim — a connect-scoped `AbortSignal.timeout(30_000)`
933+
// would silently tear the auto-opened stream down the moment
934+
// it fires after connect has resolved. But connect()'s signal
935+
// MUST still cancel the in-connect ack WAIT (otherwise an
936+
// aborted connect blocks here for the full ack timeout).
937+
// Derived one-shot: bound to connect()'s signal only for the
938+
// duration of the listen() await; the listener is removed in
939+
// `finally` so the auto-opened subscription outlives connect's
940+
// signal.
941+
const ackAbort = new AbortController();
942+
const onConnectAbort = (): void => ackAbort.abort(options?.signal?.reason);
943+
// Handle the already-aborted case (aborted between the
944+
// discover leg resolving and now): the listener never fires
945+
// for a past event.
946+
if (options?.signal?.aborted) onConnectAbort();
947+
options?.signal?.addEventListener('abort', onConnectAbort);
937948
try {
938-
this._autoOpenedSubscription = await this.listen(
939-
filter,
940-
options?.timeout === undefined ? undefined : { timeout: options.timeout }
941-
);
949+
this._autoOpenedSubscription = await this.listen(filter, {
950+
timeout: options?.timeout,
951+
signal: ackAbort.signal
952+
});
942953
} catch (error) {
954+
// Connect-signal abort during the ack wait propagates as a
955+
// connect() rejection (caller asked to abort connect); a
956+
// server-side refusal stays a soft onerror.
957+
if (options?.signal?.aborted) throw error;
943958
this.onerror?.(error instanceof Error ? error : new Error(String(error)));
959+
} finally {
960+
options?.signal?.removeEventListener('abort', onConnectAbort);
944961
}
945962
}
946963
}
@@ -1404,8 +1421,16 @@ export class Client extends Protocol<ClientContext> {
14041421
requestAbort.abort();
14051422
const id = listenMessageId;
14061423
if (id !== undefined) {
1424+
// Carry the same modern auto-envelope as every other outbound
1425+
// (request()'s cancel and Protocol.notification() both go via
1426+
// `_envelopeOutbound`); the listen path was the only outbound
1427+
// bypassing it.
14071428
await this.transport
1408-
?.send({ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: id } })
1429+
?.send({
1430+
jsonrpc: '2.0',
1431+
method: 'notifications/cancelled',
1432+
params: { _meta: { ...this._outboundMetaEnvelope() }, requestId: id }
1433+
})
14091434
.catch(() => {});
14101435
}
14111436
};
@@ -1485,9 +1510,22 @@ export class Client extends Protocol<ClientContext> {
14851510
// Pre-ack capacity / params rejection arrives as a JSON-RPC error
14861511
// for the listen id; transport close is delivered the same way.
14871512
// A 'response' (the spec defines listen as never receiving a
1488-
// result) is treated as the server having ended the stream.
1513+
// result) surfaces as a typed protocol-shape error so a server
1514+
// bug — answering listen with a JSON-RPC result instead of the
1515+
// acknowledged notification — is diagnosable, not a 60s ack
1516+
// timeout.
14891517
void parked.terminated.then(({ reason, error }) => {
14901518
if (reason === 'unparked') return;
1519+
if (reason === 'response') {
1520+
settle({
1521+
cause: 'remote',
1522+
error: new SdkError(
1523+
SdkErrorCode.InvalidResult,
1524+
'server answered subscriptions/listen with a result; expected the acknowledged notification'
1525+
)
1526+
});
1527+
return;
1528+
}
14911529
settle({ cause: 'remote', error: error ?? new Error('subscriptions/listen: stream ended') });
14921530
});
14931531
parked.sent.catch(error => {

packages/client/src/client/streamableHttp.ts

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,13 @@ export class StreamableHTTPClientTransport implements Transport {
300300
}
301301

302302
private async _startOrAuthSse(options: StartSSEOptions, isAuthRetry = false): Promise<void> {
303-
const { resumptionToken } = options;
303+
const { resumptionToken, requestSignal } = options;
304+
// Same guard as `_handleSseStream`: a resurrected listen stream (the
305+
// POST-SSE → GET reconnect path threads `requestSignal` through
306+
// `StartSSEOptions`) must honour the per-request abort exactly as the
307+
// original POST did — both as a fetch signal and as a "do not surface
308+
// onerror" gate.
309+
const isIntentionalAbort = (): boolean => this._abortController?.signal.aborted === true || requestSignal?.aborted === true;
304310

305311
try {
306312
// Try to open an initial SSE stream with GET to listen for server messages
@@ -315,11 +321,16 @@ export class StreamableHTTPClientTransport implements Transport {
315321
headers.set('last-event-id', resumptionToken);
316322
}
317323

324+
const transportSignal = this._abortController?.signal;
325+
const signal =
326+
requestSignal !== undefined && transportSignal !== undefined
327+
? anySignal(transportSignal, requestSignal)
328+
: (requestSignal ?? transportSignal);
318329
const response = await (this._fetch ?? fetch)(this._url, {
319330
...this._requestInit,
320331
method: 'GET',
321332
headers,
322-
signal: this._abortController?.signal
333+
signal
323334
});
324335

325336
if (!response.ok) {
@@ -366,7 +377,9 @@ export class StreamableHTTPClientTransport implements Transport {
366377

367378
this._handleSseStream(response.body, options, true);
368379
} catch (error) {
369-
this.onerror?.(error as Error);
380+
if (!isIntentionalAbort()) {
381+
this.onerror?.(error as Error);
382+
}
370383
throw error;
371384
}
372385
}
@@ -413,8 +426,12 @@ export class StreamableHTTPClientTransport implements Transport {
413426

414427
const reconnect = (): void => {
415428
this._cancelReconnection = undefined;
416-
if (this._abortController?.signal.aborted) return;
429+
// Honour BOTH the transport-wide abort and the per-request abort
430+
// (a listen subscription closed during the backoff delay): do not
431+
// resurrect a stream the caller already tore down.
432+
if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return;
417433
this._startOrAuthSse(options).catch(error => {
434+
if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return;
418435
this.onerror?.(new Error(`Failed to reconnect SSE stream: ${error instanceof Error ? error.message : String(error)}`));
419436
try {
420437
this._scheduleReconnection(options, attemptCount + 1);
@@ -780,7 +797,14 @@ export class StreamableHTTPClientTransport implements Transport {
780797
await response.text?.().catch(() => {});
781798
}
782799
} catch (error) {
783-
this.onerror?.(error as Error);
800+
// Intentional per-request abort BEFORE response headers (the
801+
// `subscriptions/listen` driver aborting its `requestSignal`):
802+
// fetch rejects with AbortError. Same guard as
803+
// `_handleSseStream`'s `isIntentionalAbort` — do not surface a
804+
// misleading onerror; still rethrow so `parked.sent` settles.
805+
if (options?.requestSignal?.aborted !== true) {
806+
this.onerror?.(error as Error);
807+
}
784808
throw error;
785809
}
786810
}

packages/client/test/client/listen.test.ts

Lines changed: 81 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,14 @@
77
* connection.
88
*/
99
import type { JSONRPCMessage, JSONRPCNotification } from '@modelcontextprotocol/core';
10-
import { InMemoryTransport, LATEST_PROTOCOL_VERSION, SdkError, SdkErrorCode, SUBSCRIPTION_ID_META_KEY } from '@modelcontextprotocol/core';
10+
import {
11+
InMemoryTransport,
12+
LATEST_PROTOCOL_VERSION,
13+
PROTOCOL_VERSION_META_KEY,
14+
SdkError,
15+
SdkErrorCode,
16+
SUBSCRIPTION_ID_META_KEY
17+
} from '@modelcontextprotocol/core';
1118
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
1219

1320
import { Client } from '../../src/client/client.js';
@@ -143,7 +150,13 @@ describe('Client.listen()', () => {
143150
const listenId = (written.find(m => (m as { method?: string }).method === 'subscriptions/listen') as { id: number | string }).id;
144151
written.length = 0;
145152
await sub.close();
146-
expect(written).toEqual([{ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: listenId } }]);
153+
expect(written).toHaveLength(1);
154+
const cancel = written[0] as unknown as { method: string; params: { requestId: unknown; _meta?: Record<string, unknown> } };
155+
expect(cancel.method).toBe('notifications/cancelled');
156+
expect(cancel.params.requestId).toBe(listenId);
157+
// The listen-path cancel carries the same modern auto-envelope as
158+
// every other outbound (request()'s cancel, Protocol.notification()).
159+
expect(cancel.params._meta?.[PROTOCOL_VERSION_META_KEY]).toBe(MODERN);
147160
// Idempotent.
148161
await sub.close();
149162
expect(written).toHaveLength(1);
@@ -422,7 +435,9 @@ describe('Client.listen()', () => {
422435
written.length = 0;
423436
ac.abort();
424437
await flush();
425-
expect(written).toEqual([{ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: listenId } }]);
438+
expect(written).toHaveLength(1);
439+
expect((written[0] as JSONRPCNotification).method).toBe('notifications/cancelled');
440+
expect((written[0] as unknown as { params: { requestId: unknown } }).params.requestId).toBe(listenId);
426441
// Caller-signal abort is consumer-initiated → 'local'.
427442
await expect(sub.closed).resolves.toBe('local');
428443
// close() after signal-abort is idempotent.
@@ -575,6 +590,66 @@ describe('Client.listen()', () => {
575590
await client.close();
576591
});
577592

593+
it('connect-scoped signal aborted DURING the auto-open ack wait: connect rejects fast (no 60s hang)', async () => {
594+
// Regression: forwarding only {timeout} into the auto-open listen()
595+
// meant connect()'s signal could not cancel the in-connect ack wait —
596+
// an aborted connect blocked here for the full ack timeout.
597+
const { clientTx } = await scriptedModernNoAck();
598+
const onChanged = () => {};
599+
const client = new Client(
600+
{ name: 'c', version: '1' },
601+
{ versionNegotiation: { mode: 'auto' }, listChanged: { tools: { onChanged } } }
602+
);
603+
const connectScoped = new AbortController();
604+
const t0 = Date.now();
605+
const pending = client.connect(clientTx, { signal: connectScoped.signal });
606+
// discover resolves; connect is now awaiting the auto-open ack.
607+
await flush();
608+
connectScoped.abort(new Error('connect-abort'));
609+
const error = await pending.catch(e => e as Error);
610+
expect(error).toBeInstanceOf(Error);
611+
expect(Date.now() - t0).toBeLessThan(1000);
612+
// No leaked per-listen state on the aborted connect.
613+
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
614+
await client.close();
615+
});
616+
617+
it('server answers listen with a JSON-RPC RESULT during opening: rejects with a typed InvalidResult (not 60s)', async () => {
618+
const [clientTx, serverTx] = InMemoryTransport.createLinkedPair();
619+
serverTx.onmessage = m => {
620+
const req = m as { id?: number | string; method?: string };
621+
if (req.method === 'server/discover' && req.id !== undefined) {
622+
void serverTx.send({
623+
jsonrpc: '2.0',
624+
id: req.id,
625+
result: {
626+
resultType: 'complete',
627+
supportedVersions: [MODERN],
628+
capabilities: { tools: { listChanged: true } },
629+
serverInfo: { name: 's', version: '1' }
630+
}
631+
});
632+
}
633+
if (req.method === 'subscriptions/listen' && req.id !== undefined) {
634+
// Buggy server: answers with a result instead of the
635+
// acknowledged notification. Spec defines listen as never
636+
// receiving a result.
637+
void serverTx.send({ jsonrpc: '2.0', id: req.id, result: {} });
638+
}
639+
};
640+
await serverTx.start();
641+
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });
642+
await client.connect(clientTx);
643+
const t0 = Date.now();
644+
const error = await client.listen({ toolsListChanged: true }).catch(e => e as SdkError);
645+
expect(error).toBeInstanceOf(SdkError);
646+
expect((error as SdkError).code).toBe(SdkErrorCode.InvalidResult);
647+
expect((error as SdkError).message).toContain('expected the acknowledged notification');
648+
expect(Date.now() - t0).toBeLessThan(1000);
649+
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
650+
await client.close();
651+
});
652+
578653
it('transport closes BEFORE the ack: listen() rejects fast', async () => {
579654
const { clientTx, serverTx } = await scriptedModernNoAck();
580655
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });
@@ -624,7 +699,9 @@ describe('Client.listen()', () => {
624699
written.length = 0;
625700
await a.close();
626701
// Only `a`'s id is cancelled; `b` stays open.
627-
expect(written).toEqual([{ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: ids[0] } }]);
702+
expect(written).toHaveLength(1);
703+
expect((written[0] as JSONRPCNotification).method).toBe('notifications/cancelled');
704+
expect((written[0] as unknown as { params: { requestId: unknown } }).params.requestId).toBe(ids[0]);
628705
expect(listenState.size).toBe(1);
629706
await b.close();
630707
expect(listenState.size).toBe(0);

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,6 +1155,40 @@ describe('StreamableHTTPClientTransport', () => {
11551155
expect(fetchMock).toHaveBeenCalledTimes(1);
11561156
});
11571157

1158+
it('per-request requestSignal abort BEFORE response headers: no misleading onerror; send() still rejects', async () => {
1159+
// ARRANGE — fetch is in flight (pending promise) when the
1160+
// requestSignal aborts; fetch rejects with AbortError before the
1161+
// SSE stream handler ever runs. _send's catch must apply the same
1162+
// intentional-abort guard as _handleSseStream.
1163+
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'));
1164+
const errorSpy = vi.fn();
1165+
transport.onerror = errorSpy;
1166+
const fetchMock = globalThis.fetch as Mock;
1167+
fetchMock.mockImplementationOnce(
1168+
(_url, init: RequestInit) =>
1169+
new Promise((_resolve, reject) => {
1170+
init.signal?.addEventListener('abort', () => reject(init.signal?.reason), { once: true });
1171+
})
1172+
);
1173+
1174+
const requestAbort = new AbortController();
1175+
await transport.start();
1176+
const sent = transport.send(
1177+
{ jsonrpc: '2.0', method: 'subscriptions/listen', id: 'listen-1', params: {} },
1178+
{ requestSignal: requestAbort.signal }
1179+
);
1180+
// Let _send reach the in-flight fetch.
1181+
await vi.advanceTimersByTimeAsync(0);
1182+
expect(fetchMock).toHaveBeenCalledTimes(1);
1183+
1184+
// ACT — abort before headers.
1185+
requestAbort.abort(new Error('intentional'));
1186+
1187+
// ASSERT — send() rejects (so parked.sent settles), but no onerror.
1188+
await expect(sent).rejects.toThrow();
1189+
expect(errorSpy).not.toHaveBeenCalled();
1190+
});
1191+
11581192
it('anySignal fallback removes the sibling listener (no leak on the transport-lifetime signal)', async () => {
11591193
// ARRANGE — force the manual fallback path (Node 20.0–20.2).
11601194
const nativeAny = AbortSignal.any;

0 commit comments

Comments
 (0)