From 5e065c80f5706e7c383ba714c027347b68ca6273 Mon Sep 17 00:00:00 2001 From: Sanath Vengaldas Date: Fri, 6 Nov 2026 05:54:23 +0000 Subject: [PATCH] fix(core): enforce maxTotalTimeout as a hard cap _setupTimeout armed the pending timer with `timeout` only, so `maxTotalTimeout` was read solely in `_resetTimeout`, which runs only on a progress notification with `resetTimeoutOnProgress`. A request that never sends progress outlived its documented hard cap, and a progress reset re-armed the full `timeout` instead of the remaining total budget. Arm the timer with min(timeout, remaining maxTotalTimeout), re-arm with the remaining budget on reset, and report `Maximum total timeout exceeded` when the cap is what fired. --- .changeset/max-total-timeout-enforced.md | 6 ++ packages/core-internal/src/shared/protocol.ts | 41 ++++++++--- .../test/shared/protocol.test.ts | 71 +++++++++++++++++++ 3 files changed, 110 insertions(+), 8 deletions(-) create mode 100644 .changeset/max-total-timeout-enforced.md diff --git a/.changeset/max-total-timeout-enforced.md b/.changeset/max-total-timeout-enforced.md new file mode 100644 index 0000000000..2ebe2588fa --- /dev/null +++ b/.changeset/max-total-timeout-enforced.md @@ -0,0 +1,6 @@ +--- +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +`maxTotalTimeout` is now enforced as a hard cap. The pending timer is armed with whichever budget runs out first, the per-request `timeout` or what is left of `maxTotalTimeout`, and a progress reset re-arms it with the remaining total budget instead of the full `timeout`. A request that never receives a progress notification now rejects with `Maximum total timeout exceeded` at the cap rather than waiting out `timeout`. diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index ea86863a4f..8dfe858659 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -530,12 +530,12 @@ export type ClientContext = BaseContext; * Information about a request's timeout state */ type TimeoutInfo = { - timeoutId: ReturnType; + timeoutId?: ReturnType; startTime: number; timeout: number; maxTotalTimeout?: number; resetTimeoutOnProgress: boolean; - onTimeout: () => void; + onTimeout: (reason: 'timeout' | 'maxTotalTimeout', elapsed: number) => void; }; /* @@ -771,17 +771,33 @@ export abstract class Protocol { messageId: number, timeout: number, maxTotalTimeout: number | undefined, - onTimeout: () => void, + onTimeout: (reason: 'timeout' | 'maxTotalTimeout', elapsed: number) => void, resetTimeoutOnProgress: boolean = false ) { - this._timeoutInfo.set(messageId, { - timeoutId: setTimeout(onTimeout, timeout), + const info: TimeoutInfo = { startTime: Date.now(), timeout, maxTotalTimeout, resetTimeoutOnProgress, onTimeout - }); + }; + this._armTimeout(info); + this._timeoutInfo.set(messageId, info); + } + + /** + * Arms the pending timer with whichever budget runs out first: the + * per-request `timeout`, or what is left of the `maxTotalTimeout` cap. + */ + private _armTimeout(info: TimeoutInfo) { + const elapsed = Date.now() - info.startTime; + const remainingTotal = info.maxTotalTimeout === undefined ? Number.POSITIVE_INFINITY : info.maxTotalTimeout - elapsed; + const delay = Math.max(0, Math.min(info.timeout, remainingTotal)); + info.timeoutId = setTimeout(() => { + const firedAt = Date.now() - info.startTime; + const reason = info.maxTotalTimeout !== undefined && firedAt >= info.maxTotalTimeout ? 'maxTotalTimeout' : 'timeout'; + info.onTimeout(reason, firedAt); + }, delay); } private _resetTimeout(messageId: number): boolean { @@ -798,7 +814,7 @@ export abstract class Protocol { } clearTimeout(info.timeoutId); - info.timeoutId = setTimeout(info.onTimeout, info.timeout); + this._armTimeout(info); return true; } @@ -1657,7 +1673,16 @@ export abstract class Protocol { options?.signal?.addEventListener('abort', onAbort, { once: true }); const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC; - const timeoutHandler = () => cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout }), 'timeout'); + const timeoutHandler = (reason: 'timeout' | 'maxTotalTimeout', elapsed: number) => + reason === 'maxTotalTimeout' + ? cancel( + new SdkError(SdkErrorCode.RequestTimeout, 'Maximum total timeout exceeded', { + maxTotalTimeout: options?.maxTotalTimeout, + totalElapsed: elapsed + }), + 'timeout' + ) + : cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout }), 'timeout'); this._setupTimeout(messageId, timeout, options?.maxTotalTimeout, timeoutHandler, options?.resetTimeoutOnProgress ?? false); diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 2fb0f64813..87f1183890 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -492,6 +492,77 @@ describe('protocol tests', () => { expect(onProgressMock).toHaveBeenCalledTimes(1); }); + test('should enforce maxTotalTimeout when no progress notification arrives', async () => { + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + let settled: Error | undefined; + void testRequest(protocol, request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 150 + }).catch((error: Error) => { + settled = error; + }); + + await vi.advanceTimersByTimeAsync(151); + + expect(settled?.message).toBe('Maximum total timeout exceeded'); + }); + + test('should re-arm the timeout within the remaining maxTotalTimeout budget', async () => { + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + const onProgressMock = vi.fn(); + let settled: Error | undefined; + void testRequest(protocol, request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 300, + resetTimeoutOnProgress: true, + onprogress: onProgressMock + }).catch((error: Error) => { + settled = error; + }); + + await vi.advanceTimersByTimeAsync(200); + transport.onmessage?.({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { progressToken: 0, progress: 50, total: 100 } + }); + await Promise.resolve(); + expect(onProgressMock).toHaveBeenCalledTimes(1); + + // 200ms elapsed, 100ms of the total budget left: the re-armed timer + // must not outlive the cap. + await vi.advanceTimersByTimeAsync(101); + + expect(settled?.message).toBe('Maximum total timeout exceeded'); + }); + + test('should still report the per-request timeout when it is shorter than maxTotalTimeout', async () => { + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + let settled: Error | undefined; + void testRequest(protocol, request, mockSchema, { + timeout: 100, + maxTotalTimeout: 1000 + }).catch((error: Error) => { + settled = error; + }); + + await vi.advanceTimersByTimeAsync(101); + + expect(settled?.message).toBe('Request timed out'); + }); + test('should timeout if no progress received within timeout period', async () => { await protocol.connect(transport); const request = { method: 'example', params: {} };