Skip to content

Commit 0e5890a

Browse files
refactor(client): listen() driver is transport-level — drop the in-Protocol park primitive
The listen() driver no longer registers in Protocol's _responseHandlers via the bespoke _parkRequest() primitive. Instead it sends directly on the transport with a STRING request id from a Client-owned counter ('listen:N' — JSON-RPC valid, spec subscriptionId is RequestId verbatim, zero collision with Protocol's numeric counter), and demuxes the listen id at the Client layer via three protected overrides: - _onnotification: consumes a matching ack (resolves opening) and a matching string-id notifications/cancelled (settles 'remote'); unmatched ack/cancelled pass through to super. - _onresponse: a string-id response matching a live listen entry is the server's pre-ack JSON-RPC error (settles with the typed ProtocolError) or a buggy result (settles InvalidResult); never reaches Protocol's numeric _responseHandlers map. - _onclose: settles every live per-listen entry 'remote' before Protocol._onclose tears the transport down (the prior implementation got this via _responseHandlers settlement; the redesign no longer registers there). TransportSendOptions gains onRequestStreamEnd, fired by transports that open a per-request stream (Streamable HTTP) when that stream ends or errors for any non-deliberate reason; threaded through the SSE reconnect path so a reconnected stream still carries it. stdio/InMemory ignore it. protocol.ts: _parkRequest, _onParkedNotification, and their dispatch wiring are gone; _onnotification/_onresponse/_onclose are now protected. Net diff vs the integration base is three private→protected keyword changes only. The opening→open→closed state machine, McpSubscription surface, ClientOptions.listChanged auto-open, and the per-request requestSignal mechanism are unchanged; this is internal wiring. Also folds in the bot finding that _setupListChangedHandlers in the modern connect path was outside the soft-fail guard (now surfaces via onerror, connect resolves).
1 parent 0826c2d commit 0e5890a

7 files changed

Lines changed: 380 additions & 274 deletions

File tree

packages/client/src/client/client.ts

Lines changed: 155 additions & 137 deletions
Large diffs are not rendered by default.

packages/client/src/client/streamableHttp.ts

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,15 @@ export interface StartSSEOptions {
5959
* transport-level abort: no `onerror`, no reconnect.
6060
*/
6161
requestSignal?: AbortSignal;
62+
63+
/**
64+
* The per-request stream-end callback supplied via
65+
* `TransportSendOptions.onRequestStreamEnd`. Fired when the SSE response
66+
* stream for the originating POST ends or errors for any non-deliberate
67+
* reason (server closed, network dropped, reconnection exhausted) — never
68+
* when `requestSignal` was aborted.
69+
*/
70+
onRequestStreamEnd?: () => void;
6271
}
6372

6473
/**
@@ -418,6 +427,8 @@ export class StreamableHTTPClientTransport implements Transport {
418427
// Check if we've exceeded maximum retry attempts
419428
if (attemptCount >= maxRetries) {
420429
this.onerror?.(new Error(`Maximum reconnection attempts (${maxRetries}) exceeded.`));
430+
// The per-request stream is now definitively gone.
431+
options.onRequestStreamEnd?.();
421432
return;
422433
}
423434

@@ -454,7 +465,7 @@ export class StreamableHTTPClientTransport implements Transport {
454465
if (!stream) {
455466
return;
456467
}
457-
const { onresumptiontoken, replayMessageId, requestSignal } = options;
468+
const { onresumptiontoken, replayMessageId, requestSignal, onRequestStreamEnd } = options;
458469
// An intentional abort — transport-wide close OR a per-request abort
459470
// (McpSubscription.close() aborting its `requestSignal`) — must read as
460471
// a clean shutdown: no misleading "SSE stream disconnected" onerror,
@@ -535,10 +546,16 @@ export class StreamableHTTPClientTransport implements Transport {
535546
resumptionToken: lastEventId,
536547
onresumptiontoken,
537548
replayMessageId,
538-
requestSignal
549+
requestSignal,
550+
onRequestStreamEnd
539551
},
540552
0
541553
);
554+
} else if (!isIntentionalAbort()) {
555+
// The per-request stream ended without reconnecting (no
556+
// priming event for a POST stream, or response already
557+
// received). Not a deliberate abort — notify the caller.
558+
onRequestStreamEnd?.();
542559
}
543560
} catch (error) {
544561
if (isIntentionalAbort()) {
@@ -562,13 +579,19 @@ export class StreamableHTTPClientTransport implements Transport {
562579
resumptionToken: lastEventId,
563580
onresumptiontoken,
564581
replayMessageId,
565-
requestSignal
582+
requestSignal,
583+
onRequestStreamEnd
566584
},
567585
0
568586
);
569587
} catch (error) {
570588
this.onerror?.(new Error(`Failed to reconnect: ${error instanceof Error ? error.message : String(error)}`));
589+
onRequestStreamEnd?.();
571590
}
591+
} else {
592+
// Non-deliberate stream error without reconnection: the
593+
// per-request stream is gone — notify the caller.
594+
onRequestStreamEnd?.();
572595
}
573596
}
574597
};
@@ -617,14 +640,26 @@ export class StreamableHTTPClientTransport implements Transport {
617640

618641
async send(
619642
message: JSONRPCMessage | JSONRPCMessage[],
620-
options?: { resumptionToken?: string; onresumptiontoken?: (token: string) => void; requestSignal?: AbortSignal }
643+
options?: {
644+
resumptionToken?: string;
645+
onresumptiontoken?: (token: string) => void;
646+
requestSignal?: AbortSignal;
647+
onRequestStreamEnd?: () => void;
648+
}
621649
): Promise<void> {
622650
return this._send(message, options, false);
623651
}
624652

625653
private async _send(
626654
message: JSONRPCMessage | JSONRPCMessage[],
627-
options: { resumptionToken?: string; onresumptiontoken?: (token: string) => void; requestSignal?: AbortSignal } | undefined,
655+
options:
656+
| {
657+
resumptionToken?: string;
658+
onresumptiontoken?: (token: string) => void;
659+
requestSignal?: AbortSignal;
660+
onRequestStreamEnd?: () => void;
661+
}
662+
| undefined,
628663
isAuthRetry: boolean
629664
): Promise<void> {
630665
try {
@@ -775,7 +810,15 @@ export class StreamableHTTPClientTransport implements Transport {
775810
// Handle SSE stream responses for requests
776811
// We use the same handler as standalone streams, which now supports
777812
// reconnection with the last event ID
778-
this._handleSseStream(response.body, { onresumptiontoken, requestSignal: options?.requestSignal }, false);
813+
this._handleSseStream(
814+
response.body,
815+
{
816+
onresumptiontoken,
817+
requestSignal: options?.requestSignal,
818+
onRequestStreamEnd: options?.onRequestStreamEnd
819+
},
820+
false
821+
);
779822
} else if (contentType?.includes('application/json')) {
780823
// For non-streaming servers, we might get direct JSON responses
781824
const data = await response.json();
@@ -801,7 +844,8 @@ export class StreamableHTTPClientTransport implements Transport {
801844
// `subscriptions/listen` driver aborting its `requestSignal`):
802845
// fetch rejects with AbortError. Same guard as
803846
// `_handleSseStream`'s `isIntentionalAbort` — do not surface a
804-
// misleading onerror; still rethrow so `parked.sent` settles.
847+
// misleading onerror; still rethrow so `listen()`'s send-catch
848+
// settles the per-subscription state machine.
805849
if (options?.requestSignal?.aborted !== true) {
806850
this.onerror?.(error as Error);
807851
}

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

Lines changed: 66 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,8 @@ describe('Client.listen()', () => {
296296
expect((error as Error).message).toContain('server cancelled the subscription');
297297
// Rejected promptly (well under the 60s ack timeout).
298298
expect(Date.now() - t0).toBeLessThan(1000);
299-
// No leaked _responseHandlers entry for the listen id.
300-
expect((client as unknown as { _responseHandlers: Map<unknown, unknown> })._responseHandlers.size).toBe(0);
299+
// No leaked per-listen state for the listen id.
300+
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
301301
await client.close();
302302
});
303303

@@ -327,9 +327,8 @@ describe('Client.listen()', () => {
327327

328328
it('a synchronously-delivered server-cancel during send does not leak a _listenState entry', async () => {
329329
// In-process delivery: the server's notifications/cancelled arrives
330-
// inside `_parkRequest`'s send (before `parked` is assigned). settle()
331-
// must still drop the `_listenState` entry it registered via
332-
// onBeforeSend.
330+
// inside `transport.send()` (before the `await opening`). settle()
331+
// must still drop the `_listenState` entry registered before send.
333332
const [clientTx, serverTx] = InMemoryTransport.createLinkedPair();
334333
serverTx.onmessage = m => {
335334
const req = m as { id?: number | string; method?: string };
@@ -361,23 +360,21 @@ describe('Client.listen()', () => {
361360
await client.close();
362361
});
363362

364-
it('a synchronous transport.send throw does not leak a _responseHandlers entry', async () => {
363+
it('a synchronous transport.send throw does not leak a _listenState entry', async () => {
365364
const { clientTx } = await scriptedModern();
366365
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });
367366
await client.connect(clientTx);
368-
const handlers = (client as unknown as { _responseHandlers: Map<unknown, unknown> })._responseHandlers;
369-
const before = handlers.size;
370367
const realSend = clientTx.send.bind(clientTx);
371368
clientTx.send = () => {
372369
throw new Error('send blew up');
373370
};
374371
const error = await client.listen({ toolsListChanged: true }).catch(e => e as Error);
375372
expect((error as Error).message).toContain('send blew up');
376-
// The park primitive unregistered before rethrowing — no leak.
377-
expect(handlers.size).toBe(before);
378-
// settle() in the catch path also dropped the _listenState entry that
379-
// onBeforeSend registered before send threw.
373+
// settle() in the catch path dropped the _listenState entry that was
374+
// registered before send threw; listen() never registers in
375+
// Protocol's `_responseHandlers` so there is nothing to leak there.
380376
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
377+
expect((client as unknown as { _responseHandlers: Map<unknown, unknown> })._responseHandlers.size).toBe(0);
381378
clientTx.send = realSend;
382379
await client.close();
383380
});
@@ -421,7 +418,6 @@ describe('Client.listen()', () => {
421418
expect(cancelled?.params.requestId).toBe(listenId);
422419
// No leaked state.
423420
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
424-
expect((client as unknown as { _responseHandlers: Map<unknown, unknown> })._responseHandlers.size).toBe(0);
425421
await client.close();
426422
});
427423

@@ -657,8 +653,8 @@ describe('Client.listen()', () => {
657653
const t0 = Date.now();
658654
const pending = client.listen({ toolsListChanged: true });
659655
await flush();
660-
// Server-side transport closes before ever acking → Protocol._onclose
661-
// errors every parked _responseHandlers entry → settle({error}).
656+
// Server-side transport closes before ever acking → Client's
657+
// `_onclose` override settles every per-listen state machine.
662658
await serverTx.close();
663659
const error = await pending.catch(e => e as Error);
664660
expect(error).toBeInstanceOf(Error);
@@ -720,8 +716,9 @@ describe('Client.listen()', () => {
720716
const sub = await client.listen({ toolsListChanged: true });
721717
await sub.close();
722718
// The per-listen entry is gone; a late server-side ack and a late
723-
// server-side cancel for this id are NOT consumed by the first-look
724-
// hook (no parked entry matches) and reach the fallback handler.
719+
// server-side cancel for this id are NOT consumed by the
720+
// `_onnotification` override (no entry matches) and reach the
721+
// fallback handler.
725722
const fallback: string[] = [];
726723
client.fallbackNotificationHandler = async n => {
727724
fallback.push(n.method);
@@ -771,7 +768,6 @@ describe('Client.listen()', () => {
771768
await client.connect(clientTx);
772769
const pending = client.listen({ toolsListChanged: true });
773770
await flush();
774-
const handlers = (client as unknown as { _responseHandlers: Map<unknown, unknown> })._responseHandlers;
775771
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(1);
776772
// Fresh connect on a new transport — _resetConnectionState runs.
777773
const { clientTx: clientTx2 } = await scriptedModern();
@@ -780,11 +776,61 @@ describe('Client.listen()', () => {
780776
expect(error).toBeInstanceOf(SdkError);
781777
expect((error as SdkError).code).toBe(SdkErrorCode.ConnectionClosed);
782778
expect((error as SdkError).message).toContain('reconnected or closed');
783-
// No leaked parked handler from the old connection.
784-
expect(handlers.size).toBe(0);
779+
// No leaked per-listen state from the old connection.
780+
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
781+
await client.close();
782+
});
783+
784+
it("the listen request id is a STRING on the wire ('listen:N'); cancel echoes it verbatim", async () => {
785+
const { clientTx, written } = await scriptedModern();
786+
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });
787+
await client.connect(clientTx);
788+
const sub = await client.listen({ toolsListChanged: true });
789+
const wireListen = written.find(m => (m as { method?: string }).method === 'subscriptions/listen') as {
790+
id: unknown;
791+
params: { _meta?: Record<string, unknown> };
792+
};
793+
// String id from a Client-owned counter — JSON-RPC valid; spec
794+
// subscriptionId is the request id verbatim; zero collision with
795+
// Protocol's numeric counter.
796+
expect(typeof wireListen.id).toBe('string');
797+
expect(wireListen.id).toMatch(/^listen:\d+$/);
798+
// The auto-envelope is on the wire too.
799+
expect(wireListen.params._meta?.[PROTOCOL_VERSION_META_KEY]).toBe(MODERN);
800+
written.length = 0;
801+
await sub.close();
802+
const cancel = written[0] as unknown as { method: string; params: { requestId: unknown } };
803+
expect(cancel.params.requestId).toBe(wireListen.id);
785804
await client.close();
786805
});
787806

807+
it("transport-level per-request stream end (onRequestStreamEnd) → closed resolves 'remote'", async () => {
808+
// Mock a transport that captures the per-request `onRequestStreamEnd`
809+
// callback and fires it after the ack — simulating a Streamable HTTP
810+
// server closing the listen request's SSE stream.
811+
const { clientTx, serverTx } = await scriptedModern();
812+
let onStreamEnd: (() => void) | undefined;
813+
const realSend = clientTx.send.bind(clientTx);
814+
clientTx.send = (m, opts) => {
815+
if ((m as { method?: string }).method === 'subscriptions/listen') {
816+
onStreamEnd = (opts as { onRequestStreamEnd?: () => void } | undefined)?.onRequestStreamEnd;
817+
}
818+
return realSend(m, opts);
819+
};
820+
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });
821+
await client.connect(clientTx);
822+
const sub = await client.listen({ toolsListChanged: true });
823+
expect(onStreamEnd).toBeDefined();
824+
// Transport reports the per-request stream ended (server closed the
825+
// SSE response, network dropped it, reconnection exhausted).
826+
onStreamEnd!();
827+
await expect(sub.closed).resolves.toBe('remote');
828+
expect((client as unknown as { _listenState: Map<unknown, unknown> })._listenState.size).toBe(0);
829+
// close() after stream-end is a no-op (state already 'closed').
830+
await sub.close();
831+
await serverTx.close();
832+
});
833+
788834
it('close() resets per-connection state even when transport.close() rejects', async () => {
789835
const { clientTx } = await scriptedModern();
790836
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });

0 commit comments

Comments
 (0)