Skip to content

Commit 2728d04

Browse files
committed
fix(core,client): honor maxTotalTimeout 0; no EventSource resurrection after close() mid-refresh
Two review findings: - protocol.ts _resetTimeout gated the total-budget check on truthiness, so maxTotalTimeout: 0 — the strictest possible budget — silently disabled the check and left the request indefinitely progress-extendable, while 1 rejected after 1ms. Now gated on !== undefined, mirroring the falsy-vs-legitimate-zero sweep already applied to _oncancel's requestId and the debounce relatedRequestId gate. Boundary test: maxTotalTimeout 0 rejects on the first progress check. - sse.ts's mid-session 401 recovery ran its success continuation (_startOrAuth) after an arbitrarily long onUnauthorized await with no closed-state check: a close() landing during the pending token refresh let the continuation resurrect a live EventSource nothing could tear down (the ES wrapper fetch never carries the transport-lifetime signal, and close() had already run against the old instance). The continuation now bails when the transport-lifetime controller is aborted, rejecting start() with UnauthorizedError('Transport closed during re-authentication'). Regression test: close() during a pending onUnauthorized leaves the GET-attempt count flat and start() rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY68ke5euCq6nJ1kFePRz7
1 parent e0ab79e commit 2728d04

5 files changed

Lines changed: 109 additions & 3 deletions

File tree

.changeset/legacy-sse-reconnect-after-timeout.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,4 +5,4 @@
55

66
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 on EVERY settlement path — cancel/timeout, `maxTotalTimeout` exceeded, and successful completion (which also releases the per-request abort listener otherwise leaked on Node 20.0–20.2) — the wire cancellation mechanism per era is unchanged (legacy connections still POST `notifications/cancelled`; modern 2026-07-28 connections still cancel via per-request stream close), and `maxTotalTimeout` settlements — which previously settled without any wire cancel signal at either era — now route through the request's cancel path and emit the era's signal (the `notifications/cancelled` POST on legacy connections and modern single-channel transports, the stream-close cancel on modern per-request-stream connections) while the caller still sees the original maxTotalTimeout error. This lives in the shared `Protocol` base, so server-initiated requests (`createMessage`, `elicitInput`) gain the same maxTotalTimeout cancellation signal. Additionally, the `notifications/cancelled` send no longer inherits the original request's `resumptionToken`, which on Streamable HTTP short-circuited the send into an SSE resume and silently swallowed the cancellation. For a request re-issued with `resumptionToken` on the SDK's own Streamable HTTP transport that POST is best-effort: the re-issued request's JSON-RPC id never reached the server (the send resumed via GET instead of POSTing), so the server cannot correlate the cancellation and such requests are only torn down locally; custom per-request-stream transports that POST the re-issued body normally give the server a correlatable id. (Same asymmetry inbound, pre-existing: progress notifications replayed on a resumed stream carry the original request's `progressToken`, so `onprogress`/`resetTimeoutOnProgress` do not survive a `resumptionToken` re-issue on this transport.) The client transport's `onerror` contract is also tightened: each failed SSE reconnect leg now reports exactly once with the underlying error (the `"Failed to reconnect SSE stream:"` wrapper message is gone), deliberate teardown (`close()` landing mid-POST/mid-GET/mid-DELETE, or a settled request's signal aborting its resume) no longer surfaces an `AbortError` through `onerror`, and `onRequestStreamEnd` now fires when a `resumptionToken` resume fails to open (a terminal outcome that previously reported only through `onerror`).
77

8-
Two hardening details in the same machinery: `close()` now disarms every pending reconnect chain even when a custom `ReconnectionScheduler` cancel throws (the first error still propagates, after all chains are disarmed) and releases each chain's settlement listener, so a user-supplied cancel runs at most once across `close()` and the request's own settlement; and each request chain's reconnect legs now reuse one composed transport+request abort signal instead of composing a fresh one per leg (on Node 20.0-20.2, where `AbortSignal.any` is unavailable, per-leg composition stranded one abort-listener pair per completed resume leg on both input signals until the request settled).
8+
Two hardening details in the same machinery: `close()` now disarms every pending reconnect chain even when a custom `ReconnectionScheduler` cancel throws (the first error still propagates, after all chains are disarmed) and releases each chain's settlement listener, so a user-supplied cancel runs at most once across `close()` and the request's own settlement; and each request chain's reconnect legs now reuse one composed transport+request abort signal instead of composing a fresh one per leg (on Node 20.0-20.2, where `AbortSignal.any` is unavailable, per-leg composition stranded one abort-listener pair per completed resume leg on both input signals until the request settled). Two adjacent lifecycle/boundary fixes from the same review sweep: `maxTotalTimeout: 0` is now honored as the strictest budget (rejecting on the first progress-driven timeout reset) instead of being silently disabled by a falsy check; and on the legacy HTTP+SSE transport (`SSEClientTransport`), a `close()` that lands while a mid-session 401 token refresh (`onUnauthorized`) is pending no longer lets the recovery continuation open a new EventSource that nothing can tear down — the continuation now rejects with `UnauthorizedError('Transport closed during re-authentication')`.

packages/client/src/client/sse.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,21 @@ export class SSEClientTransport implements Transport {
224224
this._eventSource?.close();
225225
this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then(
226226
// onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject.
227-
() => this._startOrAuth().then(resolve, reject),
227+
() => {
228+
// Deferred continuation after an arbitrarily
229+
// long refresh await: a close() that landed in
230+
// the meantime must not be undone by opening a
231+
// brand-new EventSource — the ES wrapper fetch
232+
// never carries the transport-lifetime signal,
233+
// and close() already ran against the old
234+
// instance, so nothing could ever tear the
235+
// resurrected stream down.
236+
if (this._abortController?.signal.aborted === true) {
237+
reject(new UnauthorizedError('Transport closed during re-authentication'));
238+
return;
239+
}
240+
this._startOrAuth().then(resolve, reject);
241+
},
228242
// onUnauthorized failed → not yet reported. Auth-seam
229243
// stamp: covers the SDK's OAuth flow and custom
230244
// callbacks alike.

packages/client/test/client/sse.test.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1762,6 +1762,59 @@ describe('SSEClientTransport', () => {
17621762
expect(getAttempt).toBe(3);
17631763
});
17641764

1765+
it('close() during a pending onUnauthorized does not resurrect the EventSource', async () => {
1766+
// Regression: the 401-recovery success continuation ran
1767+
// _startOrAuth() after an arbitrarily long onUnauthorized await
1768+
// with no closed-state check. A close() landing while the token
1769+
// refresh was pending let _startOrAuth open a brand-new
1770+
// EventSource that nothing could ever tear down — the ES wrapper
1771+
// fetch never carries the transport-lifetime signal, and close()
1772+
// had already run against the old instance.
1773+
await resourceServer.close();
1774+
1775+
let getAttempts = 0;
1776+
resourceServer = createServer((req, res) => {
1777+
if (req.method === 'GET') {
1778+
getAttempts++;
1779+
res.writeHead(401).end();
1780+
}
1781+
});
1782+
resourceBaseUrl = await listenOnRandomPort(resourceServer);
1783+
1784+
let releaseRefresh!: () => void;
1785+
const refreshPending = new Promise<void>(resolve => {
1786+
releaseRefresh = resolve;
1787+
});
1788+
let refreshStarted!: () => void;
1789+
const refreshStartedPromise = new Promise<void>(resolve => {
1790+
refreshStarted = resolve;
1791+
});
1792+
const authProvider: AuthProvider = {
1793+
token: vi.fn(async () => 'token'),
1794+
onUnauthorized: vi.fn(async () => {
1795+
refreshStarted();
1796+
await refreshPending;
1797+
})
1798+
};
1799+
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
1800+
1801+
const startPromise = transport.start();
1802+
const startRejection = expect(startPromise).rejects.toThrow('Transport closed during re-authentication');
1803+
// Wait until the 401 recovery is mid-refresh, then close the
1804+
// transport while onUnauthorized is still pending.
1805+
await refreshStartedPromise;
1806+
const attemptsAtClose = getAttempts;
1807+
await transport.close();
1808+
1809+
// The refresh resolves AFTER close(): the continuation must bail
1810+
// instead of opening a new EventSource.
1811+
releaseRefresh();
1812+
await startRejection;
1813+
// Give a resurrected EventSource ample time to hit the server.
1814+
await new Promise(resolve => setTimeout(resolve, 100));
1815+
expect(getAttempts).toBe(attemptsAtClose);
1816+
});
1817+
17651818
it('retry failure during SSE connect fires onerror exactly once', async () => {
17661819
// Regression: when the retry EventSource rejected, its onerror fired inside, then
17671820
// the outer .then() rejection handler fired onerror AGAIN for the same error.

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -803,7 +803,10 @@ export abstract class Protocol<ContextT extends BaseContext> {
803803
if (!info) return false;
804804

805805
const totalElapsed = Date.now() - info.startTime;
806-
if (info.maxTotalTimeout && totalElapsed >= info.maxTotalTimeout) {
806+
// `!== undefined`, not truthiness: `maxTotalTimeout: 0` is the
807+
// STRICTEST budget (rejects on the first check) — a falsy gate would
808+
// silently disable it instead.
809+
if (info.maxTotalTimeout !== undefined && totalElapsed >= info.maxTotalTimeout) {
807810
// Disarm the still-armed per-leg timer BEFORE dropping the map
808811
// entry: once the entry is gone, `_cleanupTimeout` (the funnel's
809812
// `.finally()` cleanup) can no longer reach the timer, and an

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,42 @@ describe('protocol tests', () => {
492492
expect(onProgressMock).toHaveBeenCalledTimes(1);
493493
});
494494

495+
test('maxTotalTimeout: 0 is the strictest budget — rejects on the first progress check, not disabled', async () => {
496+
await protocol.connect(transport);
497+
const request = { method: 'example', params: {} };
498+
const mockSchema: ZodType<{ result: string }> = z.object({
499+
result: z.string()
500+
});
501+
const onProgressMock = vi.fn();
502+
const requestPromise = testRequest(protocol, request, mockSchema, {
503+
timeout: 1000,
504+
maxTotalTimeout: 0,
505+
resetTimeoutOnProgress: true,
506+
onprogress: onProgressMock
507+
});
508+
509+
// The budget is already exhausted, so the FIRST progress
510+
// notification's reset check must reject. Before the fix the
511+
// falsy gate (`info.maxTotalTimeout && ...`) silently disabled a
512+
// 0 budget entirely, leaving the request indefinitely
513+
// progress-extendable — the strictest value was the only one that
514+
// never rejected.
515+
vi.advanceTimersByTime(10);
516+
if (transport.onmessage) {
517+
transport.onmessage({
518+
jsonrpc: '2.0',
519+
method: 'notifications/progress',
520+
params: {
521+
progressToken: 0,
522+
progress: 25,
523+
total: 100
524+
}
525+
});
526+
}
527+
await expect(requestPromise).rejects.toThrow('Maximum total timeout exceeded');
528+
expect(onProgressMock).not.toHaveBeenCalled();
529+
});
530+
495531
test('should timeout if no progress received within timeout period', async () => {
496532
await protocol.connect(transport);
497533
const request = { method: 'example', params: {} };

0 commit comments

Comments
 (0)