Skip to content

Commit 5840d12

Browse files
committed
fix(core): reset request timeout on progress without an onprogress handler
A progress notification for an in-flight request with no registered onprogress handler was treated as an unknown-token error and returned before the timeout reset ran, so resetTimeoutOnProgress did nothing unless onprogress was also provided (#2076). Reset the timeout for any in-flight request, and only report an unknown token when neither a progress handler nor a response handler is associated with the id. Adds a regression test covering resetTimeoutOnProgress without onprogress.
1 parent 16d13ab commit 5840d12

3 files changed

Lines changed: 66 additions & 5 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@modelcontextprotocol/core': patch
3+
---
4+
5+
Fix `resetTimeoutOnProgress` so it works without an `onprogress` handler. Previously, a progress notification for an in-flight request that had no registered `onprogress` handler was treated as an unknown-token error and returned before the timeout was reset, so `resetTimeoutOnProgress: true` silently did nothing unless `onprogress` was also provided. The request timeout now resets on progress regardless of whether an `onprogress` handler is registered, and a notification is only reported as an unknown token when neither a progress handler nor an in-flight request is associated with it.

packages/core/src/shared/protocol.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -695,14 +695,21 @@ export abstract class Protocol<ContextT extends BaseContext> {
695695
const messageId = Number(progressToken);
696696

697697
const handler = this._progressHandlers.get(messageId);
698-
if (!handler) {
698+
const responseHandler = this._responseHandlers.get(messageId);
699+
const timeoutInfo = this._timeoutInfo.get(messageId);
700+
701+
// A progress notification is only "unknown" when nothing is associated with
702+
// its token — no `onprogress` handler AND no in-flight request. A request
703+
// that is in-flight but registered no `onprogress` handler is still known:
704+
// `resetTimeoutOnProgress` is documented to work on its own (#2076).
705+
if (!handler && !responseHandler) {
699706
this._onerror(new Error(`Received a progress notification for an unknown token: ${JSON.stringify(notification)}`));
700707
return;
701708
}
702709

703-
const responseHandler = this._responseHandlers.get(messageId);
704-
const timeoutInfo = this._timeoutInfo.get(messageId);
705-
710+
// Reset the request timeout on progress when requested. This must run even
711+
// when no `onprogress` handler was registered (#2076) — hence it is no longer
712+
// gated behind the handler lookup above.
706713
if (timeoutInfo && responseHandler && timeoutInfo.resetTimeoutOnProgress) {
707714
try {
708715
this._resetTimeout(messageId);
@@ -716,7 +723,10 @@ export abstract class Protocol<ContextT extends BaseContext> {
716723
}
717724
}
718725

719-
handler(params);
726+
// Deliver to the progress handler if one was registered.
727+
if (handler) {
728+
handler(params);
729+
}
720730
}
721731

722732
private _onresponse(response: JSONRPCResponse | JSONRPCErrorResponse): void {

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,52 @@ describe('protocol tests', () => {
556556
await expect(requestPromise).resolves.toEqual({ result: 'success' });
557557
});
558558

559+
test('should reset timeout on progress when resetTimeoutOnProgress is set without an onprogress handler (#2076)', async () => {
560+
await protocol.connect(transport);
561+
const request = { method: 'example', params: {} };
562+
const mockSchema: ZodType<{ result: string }> = z.object({
563+
result: z.string()
564+
});
565+
const onErrorMock = vi.fn();
566+
protocol.onerror = onErrorMock;
567+
568+
// resetTimeoutOnProgress requested WITHOUT an onprogress callback.
569+
const requestPromise = testRequest(protocol, request, mockSchema, {
570+
timeout: 1000,
571+
resetTimeoutOnProgress: true
572+
});
573+
574+
// Just before the original deadline, a progress notification arrives.
575+
vi.advanceTimersByTime(800);
576+
if (transport.onmessage) {
577+
transport.onmessage({
578+
jsonrpc: '2.0',
579+
method: 'notifications/progress',
580+
params: {
581+
progressToken: 0,
582+
progress: 50,
583+
total: 100
584+
}
585+
});
586+
}
587+
await Promise.resolve();
588+
589+
// A known request with no progress handler must NOT raise "unknown token".
590+
expect(onErrorMock).not.toHaveBeenCalled();
591+
592+
// The timer was reset: advancing past the ORIGINAL deadline must not time out.
593+
vi.advanceTimersByTime(800);
594+
if (transport.onmessage) {
595+
transport.onmessage({
596+
jsonrpc: '2.0',
597+
id: 0,
598+
result: { result: 'success' }
599+
});
600+
}
601+
await Promise.resolve();
602+
await expect(requestPromise).resolves.toEqual({ result: 'success' });
603+
});
604+
559605
test('should respect maxTotalTimeout', async () => {
560606
await protocol.connect(transport);
561607
const request = { method: 'example', params: {} };

0 commit comments

Comments
 (0)