Skip to content

Commit b3d8c77

Browse files
committed
review: thread stream callbacks through resumed send(), guard graceful-close scheduling
- The send()-resume branch now forwards onresumptiontoken and onRequestStreamEnd to the resumed GET, so new event IDs keep reaching the caller's persistence hook and the pending request settles when reconnection attempts are exhausted instead of hanging to timeout. - Guard the graceful-close scheduleNext() call the same way as the error branch: exhaustion now runs user callbacks from graceful close, and a throwing handler must not fall into the outer catch and double-fire or surface a misleading disconnect error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eqc26ABfbhTimyUUszsUxL
1 parent 1d296bd commit b3d8c77

2 files changed

Lines changed: 107 additions & 2 deletions

File tree

packages/client/src/client/streamableHttp.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -858,7 +858,19 @@ export class StreamableHTTPClientTransport implements Transport {
858858
const canResume = isReconnectable || hasPrimingEvent;
859859
const needsReconnect = canResume && !receivedResponse;
860860
if (needsReconnect && this._abortController && !isIntentionalAbort()) {
861-
scheduleNext();
861+
// Same guard as the error path below. With the fruitless
862+
// accounting, exhaustion — which synchronously invokes
863+
// `onerror`/`onRequestStreamEnd` and any custom
864+
// `ReconnectionScheduler` — is reachable from a graceful
865+
// close, and a throwing user callback must not fall into
866+
// the outer catch (which would surface a misleading
867+
// disconnect error and schedule a second time).
868+
try {
869+
scheduleNext();
870+
} catch (error) {
871+
this.onerror?.(new Error(`Failed to reconnect: ${error instanceof Error ? error.message : String(error)}`));
872+
onRequestStreamEnd?.();
873+
}
862874
} else if (!isIntentionalAbort()) {
863875
// The per-request stream ended without reconnecting (no
864876
// priming event for a POST stream, or response already
@@ -1006,10 +1018,18 @@ export class StreamableHTTPClientTransport implements Transport {
10061018
// same per-request abort as the original POST — modern-era
10071019
// cancel-via-stream-close routes through `requestSignal`, and
10081020
// without it a resumed long-running request would not cancel.
1021+
// Thread the caller's stream callbacks through as well:
1022+
// `onresumptiontoken` so new event IDs on the resumed stream
1023+
// keep reaching the caller's persistence hook, and
1024+
// `onRequestStreamEnd` so the pending request settles instead
1025+
// of hanging when the resumed stream ends for good (e.g.
1026+
// reconnection attempts are exhausted).
10091027
this._startOrAuthSse({
10101028
resumptionToken,
1029+
onresumptiontoken,
10111030
replayMessageId: isJSONRPCRequest(message) ? message.id : undefined,
1012-
requestSignal: options?.requestSignal
1031+
requestSignal: options?.requestSignal,
1032+
onRequestStreamEnd: options?.onRequestStreamEnd
10131033
}).catch(error => this.onerror?.(error));
10141034
return;
10151035
}

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2727,6 +2727,91 @@ describe('StreamableHTTPClientTransport', () => {
27272727
// Before the fix this was null: the empty second stream dropped the token.
27282728
expect(thirdCallHeaders.get('last-event-id')).toBe('event-1');
27292729
});
2730+
2731+
it('threads the stream callbacks through a resumed send(), settling the caller on exhaustion', async () => {
2732+
// A send() with a resumptionToken resumes via GET. New event IDs on
2733+
// the resumed stream must reach the caller's onresumptiontoken, and
2734+
// when reconnection attempts are exhausted the caller's
2735+
// onRequestStreamEnd must fire so the pending request settles
2736+
// instead of hanging until its timeout.
2737+
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
2738+
reconnectionOptions: {
2739+
initialReconnectionDelay: 10,
2740+
maxReconnectionDelay: 1000,
2741+
reconnectionDelayGrowFactor: 1,
2742+
maxRetries: 1
2743+
}
2744+
});
2745+
const errorSpy = vi.fn();
2746+
transport.onerror = errorSpy;
2747+
const tokenSpy = vi.fn();
2748+
const streamEndSpy = vi.fn();
2749+
2750+
const fetchMock = globalThis.fetch as Mock;
2751+
// Resumed stream delivers a fresh priming event then idle-closes;
2752+
// every stream after that closes empty until exhaustion.
2753+
const bodies: string[][] = [['id: event-next\ndata: \n\n']];
2754+
fetchMock.mockImplementation(async () => sseResponse(bodies.shift() ?? []));
2755+
2756+
const requestMessage: JSONRPCRequest = {
2757+
jsonrpc: '2.0',
2758+
method: 'long_running_tool',
2759+
id: 'request-1',
2760+
params: {}
2761+
};
2762+
2763+
await transport.start();
2764+
await transport.send(requestMessage, {
2765+
resumptionToken: 'event-0',
2766+
onresumptiontoken: tokenSpy,
2767+
onRequestStreamEnd: streamEndSpy
2768+
});
2769+
await vi.advanceTimersByTimeAsync(200);
2770+
2771+
// The resume goes out as a GET with the caller's token.
2772+
expect(fetchMock.mock.calls[0]![1]?.method).toBe('GET');
2773+
expect((fetchMock.mock.calls[0]![1]?.headers as Headers).get('last-event-id')).toBe('event-0');
2774+
// The fresh priming event reached the caller's persistence hook.
2775+
expect(tokenSpy).toHaveBeenCalledWith('event-next');
2776+
// Exhaustion settled the caller instead of leaving it hanging.
2777+
expect(streamEndSpy).toHaveBeenCalledTimes(1);
2778+
expect(errorSpy).toHaveBeenCalledWith(
2779+
expect.objectContaining({
2780+
message: 'Maximum reconnection attempts (1) exceeded.'
2781+
})
2782+
);
2783+
});
2784+
2785+
it('does not double-fire exhaustion callbacks when a user onerror handler throws on graceful close', async () => {
2786+
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
2787+
reconnectionOptions: {
2788+
initialReconnectionDelay: 10,
2789+
maxReconnectionDelay: 1000,
2790+
reconnectionDelayGrowFactor: 1,
2791+
maxRetries: 0 // exhaustion trips on the first graceful close
2792+
}
2793+
});
2794+
const errorSpy = vi.fn().mockImplementationOnce(() => {
2795+
throw new Error('user handler exploded');
2796+
});
2797+
transport.onerror = errorSpy;
2798+
const streamEndSpy = vi.fn();
2799+
2800+
const fetchMock = globalThis.fetch as Mock;
2801+
fetchMock.mockImplementation(async () => sseResponse([]));
2802+
2803+
await transport.start();
2804+
await transport['_startOrAuthSse']({ onRequestStreamEnd: streamEndSpy });
2805+
await vi.advanceTimersByTimeAsync(100);
2806+
2807+
// The throwing handler is contained by the graceful branch's guard:
2808+
// the caller still settles exactly once, no reconnect is scheduled,
2809+
// and no misleading 'SSE stream disconnected' error is emitted.
2810+
expect(streamEndSpy).toHaveBeenCalledTimes(1);
2811+
expect(fetchMock).toHaveBeenCalledTimes(1);
2812+
const messages = errorSpy.mock.calls.map(args => (args[0] as Error).message);
2813+
expect(messages.some(m => m.includes('SSE stream disconnected'))).toBe(false);
2814+
});
27302815
});
27312816

27322817
describe('prevent infinite recursion when server returns 401 after successful auth', () => {

0 commit comments

Comments
 (0)