diff --git a/.changeset/max-total-timeout-without-progress.md b/.changeset/max-total-timeout-without-progress.md new file mode 100644 index 0000000000..6c7ab297db --- /dev/null +++ b/.changeset/max-total-timeout-without-progress.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +Enforce `RequestOptions.maxTotalTimeout` as a hard cap even when no progress notifications arrive. `_setupTimeout` previously armed only the per-request `timeout`, so a hung call with `{ timeout: 1000, maxTotalTimeout: 150 }` waited the full 1000ms and rejected with `Request timed out`. The pending timer now arms for whichever limit comes first, progress resets re-arm against the remaining budget, and expiry of the cap raises `Maximum total timeout exceeded`. diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 637be389aa..d097bf9f88 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -741,8 +741,11 @@ export abstract class Protocol { onTimeout: () => void, resetTimeoutOnProgress: boolean = false ) { + // Cap the pending timer so maxTotalTimeout is a hard ceiling even + // when no progress notifications arrive. + const delay = maxTotalTimeout ? Math.min(timeout, maxTotalTimeout) : timeout; this._timeoutInfo.set(messageId, { - timeoutId: setTimeout(onTimeout, timeout), + timeoutId: setTimeout(onTimeout, delay), startTime: Date.now(), timeout, maxTotalTimeout, @@ -764,8 +767,11 @@ export abstract class Protocol { }); } + const remainingBudget = info.maxTotalTimeout ? info.maxTotalTimeout - totalElapsed : undefined; + const delay = remainingBudget === undefined ? info.timeout : Math.min(info.timeout, remainingBudget); + clearTimeout(info.timeoutId); - info.timeoutId = setTimeout(info.onTimeout, info.timeout); + info.timeoutId = setTimeout(info.onTimeout, delay); return true; } @@ -1564,7 +1570,22 @@ 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 })); + const timeoutHandler = () => { + const info = this._timeoutInfo.get(messageId); + if (info?.maxTotalTimeout) { + const totalElapsed = Date.now() - info.startTime; + if (totalElapsed >= info.maxTotalTimeout) { + cancel( + new SdkError(SdkErrorCode.RequestTimeout, 'Maximum total timeout exceeded', { + maxTotalTimeout: info.maxTotalTimeout, + totalElapsed + }) + ); + return; + } + } + cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { 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..343ae29ee4 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -492,6 +492,83 @@ describe('protocol tests', () => { expect(onProgressMock).toHaveBeenCalledTimes(1); }); + test('should enforce maxTotalTimeout without progress notifications', async () => { + // Same options an input_required retry leg gets when remaining + // budget is smaller than the per-leg timeout: a silent peer must + // expire at the cap, not at `timeout`. + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + const requestPromise = testRequest(protocol, request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 150 + }); + + vi.advanceTimersByTime(150); + await expect(requestPromise).rejects.toMatchObject({ + code: SdkErrorCode.RequestTimeout, + message: 'Maximum total timeout exceeded', + data: { maxTotalTimeout: 150, totalElapsed: 150 } + }); + }); + + test('should enforce maxTotalTimeout when timeout is left at the default', async () => { + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + const requestPromise = testRequest(protocol, request, mockSchema, { + maxTotalTimeout: 400 + }); + + vi.advanceTimersByTime(400); + await expect(requestPromise).rejects.toMatchObject({ + code: SdkErrorCode.RequestTimeout, + message: 'Maximum total timeout exceeded', + data: { maxTotalTimeout: 400, totalElapsed: 400 } + }); + }); + + test('should re-arm against remaining maxTotalTimeout after progress when remaining is less than timeout', async () => { + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + const onProgressMock = vi.fn(); + const requestPromise = testRequest(protocol, request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 250, + resetTimeoutOnProgress: true, + onprogress: onProgressMock + }); + + vi.advanceTimersByTime(100); + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { + progressToken: 0, + progress: 25, + total: 100 + } + }); + } + await Promise.resolve(); + expect(onProgressMock).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(150); + await expect(requestPromise).rejects.toMatchObject({ + code: SdkErrorCode.RequestTimeout, + message: 'Maximum total timeout exceeded', + data: { maxTotalTimeout: 250, totalElapsed: 250 } + }); + }); + test('should timeout if no progress received within timeout period', async () => { await protocol.connect(transport); const request = { method: 'example', params: {} };