Skip to content

Commit 28ff64b

Browse files
fix(client): listen auto-open inherits only ack timeout; thread requestSignal through SSE reconnect; anySignal fallback removes sibling listener
- auto-open: forwarding connect()'s full RequestOptions into listen() bound a connect-scoped signal (e.g. AbortSignal.timeout for the handshake) to the subscription lifetime, silently tearing the auto-opened stream down when it fired after connect resolved. Forward only the ack timeout. - _handleSseStream: the two _scheduleReconnection call sites rebuilt StartSSEOptions without requestSignal, so after one drop+reconnect of a listen stream the per-request abort guard stopped working. - anySignal fallback (Node 20.0-20.2): {once:true} alone leaked the sibling listener on the transport-lifetime signal per _send() with a requestSignal. Remove both listeners when either input fires.
1 parent 649aff8 commit 28ff64b

4 files changed

Lines changed: 105 additions & 6 deletions

File tree

packages/client/src/client/client.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -909,8 +909,19 @@ export class Client extends Protocol<ClientContext> {
909909
// connection is fully usable without a listen stream (the
910910
// server may not support it, or refuse on capacity). Surface
911911
// via onerror; the consumer can call listen() later.
912+
//
913+
// Forward ONLY the ack timeout from connect()'s options.
914+
// listen() binds RequestOptions.signal to the SUBSCRIPTION
915+
// lifetime, so a connect-scoped signal (e.g.
916+
// `AbortSignal.timeout(30_000)` for the handshake) would tear
917+
// the auto-opened stream down the moment it fires after
918+
// connect has already resolved. Connect's signal governs the
919+
// handshake only; the auto-opened subscription outlives it.
912920
try {
913-
this._autoOpenedSubscription = await this.listen(filter, options);
921+
this._autoOpenedSubscription = await this.listen(
922+
filter,
923+
options?.timeout === undefined ? undefined : { timeout: options.timeout }
924+
);
914925
} catch (error) {
915926
this.onerror?.(error instanceof Error ? error : new Error(String(error)));
916927
}

packages/client/src/client/streamableHttp.ts

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -188,9 +188,25 @@ function anySignal(a: AbortSignal, b: AbortSignal): AbortSignal {
188188
const controller = new AbortController();
189189
if (a.aborted) return (controller.abort(a.reason), controller.signal);
190190
if (b.aborted) return (controller.abort(b.reason), controller.signal);
191-
const forward = (source: AbortSignal) => () => controller.abort(source.reason);
192-
a.addEventListener('abort', forward(a), { once: true });
193-
b.addEventListener('abort', forward(b), { once: true });
191+
// Standard polyfill shape: when EITHER input fires, remove the listener
192+
// registered on the OTHER input too. `{once:true}` alone leaks the
193+
// sibling listener — for `_send()`, `a` is the transport-lifetime signal,
194+
// so every request-scoped `b` that aborts would otherwise leave one
195+
// listener + closure pinned on `a` for the life of the transport.
196+
const cleanup = (): void => {
197+
a.removeEventListener('abort', onA);
198+
b.removeEventListener('abort', onB);
199+
};
200+
function onA(): void {
201+
cleanup();
202+
controller.abort(a.reason);
203+
}
204+
function onB(): void {
205+
cleanup();
206+
controller.abort(b.reason);
207+
}
208+
a.addEventListener('abort', onA, { once: true });
209+
b.addEventListener('abort', onB, { once: true });
194210
return controller.signal;
195211
}
196212

@@ -501,7 +517,8 @@ export class StreamableHTTPClientTransport implements Transport {
501517
{
502518
resumptionToken: lastEventId,
503519
onresumptiontoken,
504-
replayMessageId
520+
replayMessageId,
521+
requestSignal
505522
},
506523
0
507524
);
@@ -527,7 +544,8 @@ export class StreamableHTTPClientTransport implements Transport {
527544
{
528545
resumptionToken: lastEventId,
529546
onresumptiontoken,
530-
replayMessageId
547+
replayMessageId,
548+
requestSignal
531549
},
532550
0
533551
);

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,39 @@ describe('Client.listen()', () => {
483483
await client.close();
484484
});
485485

486+
it('connect-scoped signal does NOT bind to the auto-opened subscription lifetime', async () => {
487+
// Regression: forwarding connect()'s full RequestOptions into the
488+
// auto-open listen() call meant a connect-scoped signal — typically
489+
// `AbortSignal.timeout(30_000)` for the handshake — was bound to the
490+
// SUBSCRIPTION lifetime. When it fired after connect resolved, the
491+
// auto-opened stream was silently torn down.
492+
const { clientTx, written } = await scriptedModern();
493+
const onChanged = () => {};
494+
const client = new Client(
495+
{ name: 'c', version: '1' },
496+
{ versionNegotiation: { mode: 'auto' }, listChanged: { tools: { onChanged } } }
497+
);
498+
const errors: Error[] = [];
499+
client.onerror = e => errors.push(e);
500+
const connectScoped = new AbortController();
501+
await client.connect(clientTx, { signal: connectScoped.signal });
502+
expect(client.autoOpenedSubscription).toBeDefined();
503+
written.length = 0;
504+
505+
// The connect-scoped signal fires AFTER connect resolved (as a
506+
// handshake `AbortSignal.timeout` would).
507+
connectScoped.abort();
508+
await flush();
509+
510+
// The auto-opened subscription is still live: no wire teardown
511+
// (`notifications/cancelled`) was sent, and the per-listen state
512+
// entry is still registered.
513+
expect(written.some(m => (m as JSONRPCNotification).method === 'notifications/cancelled')).toBe(false);
514+
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(1);
515+
expect(errors).toHaveLength(0);
516+
await client.close();
517+
});
518+
486519
it('transport closes BEFORE the ack: listen() rejects fast', async () => {
487520
const { clientTx, serverTx } = await scriptedModernNoAck();
488521
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });

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

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

1158+
it('anySignal fallback removes the sibling listener (no leak on the transport-lifetime signal)', async () => {
1159+
// ARRANGE — force the manual fallback path (Node 20.0–20.2).
1160+
const nativeAny = AbortSignal.any;
1161+
(AbortSignal as { any?: unknown }).any = undefined;
1162+
try {
1163+
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'));
1164+
const fetchMock = globalThis.fetch as Mock;
1165+
fetchMock.mockResolvedValue({ ok: true, status: 202, headers: new Headers() });
1166+
await transport.start();
1167+
1168+
const transportSignal = (transport as unknown as { _abortController: AbortController })._abortController.signal;
1169+
const addSpy = vi.spyOn(transportSignal, 'addEventListener');
1170+
const removeSpy = vi.spyOn(transportSignal, 'removeEventListener');
1171+
1172+
// ACT — N sends each with a fresh request-scoped signal that
1173+
// aborts after the send completes (the McpSubscription.close()
1174+
// pattern). Each send registers one fallback listener on the
1175+
// transport-lifetime signal; aborting the request-scoped
1176+
// signal must remove it.
1177+
for (let i = 0; i < 5; i++) {
1178+
const requestAbort = new AbortController();
1179+
await transport.send(
1180+
{ jsonrpc: '2.0', method: 'subscriptions/listen', id: `listen-${i}`, params: {} },
1181+
{ requestSignal: requestAbort.signal }
1182+
);
1183+
requestAbort.abort();
1184+
}
1185+
1186+
// ASSERT — every listener registered on the transport-lifetime
1187+
// signal was removed; nothing accrues per send().
1188+
expect(addSpy.mock.calls.length).toBeGreaterThan(0);
1189+
expect(removeSpy.mock.calls.length).toBe(addSpy.mock.calls.length);
1190+
} finally {
1191+
(AbortSignal as { any?: unknown }).any = nativeAny;
1192+
}
1193+
});
1194+
11581195
it('should NOT reconnect a POST stream when error response was received', async () => {
11591196
// ARRANGE
11601197
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {

0 commit comments

Comments
 (0)