Skip to content

Commit 5aec61f

Browse files
Merge pull request #2 from andrea-tomassi/copilot/fix-1
Fix MCP client timeout issue: progress notifications now reset timeout by default
2 parents f657ead + b711289 commit 5aec61f

File tree

2 files changed

+71
-3
lines changed

2 files changed

+71
-3
lines changed

src/shared/protocol.test.ts

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,13 @@ describe("protocol tests", () => {
8282
});
8383

8484
describe("_meta preservation with onprogress", () => {
85+
beforeEach(() => {
86+
jest.useFakeTimers();
87+
});
88+
afterEach(() => {
89+
jest.useRealTimers();
90+
});
91+
8592
test("should preserve existing _meta when adding progressToken", async () => {
8693
await protocol.connect(transport);
8794
const request = {
@@ -101,6 +108,8 @@ describe("protocol tests", () => {
101108

102109
protocol.request(request, mockSchema, {
103110
onprogress: onProgressMock,
111+
resetTimeoutOnProgress: false,
112+
104113
});
105114

106115
expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({
@@ -133,6 +142,8 @@ describe("protocol tests", () => {
133142

134143
protocol.request(request, mockSchema, {
135144
onprogress: onProgressMock,
145+
resetTimeoutOnProgress: false,
146+
136147
});
137148

138149
expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({
@@ -163,7 +174,10 @@ describe("protocol tests", () => {
163174
result: z.string(),
164175
});
165176

166-
protocol.request(request, mockSchema);
177+
protocol.request(request, mockSchema, {
178+
resetTimeoutOnProgress: false,
179+
180+
});
167181

168182
expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({
169183
method: "example",
@@ -190,6 +204,8 @@ describe("protocol tests", () => {
190204

191205
protocol.request(request, mockSchema, {
192206
onprogress: onProgressMock,
207+
resetTimeoutOnProgress: false,
208+
193209
});
194210

195211
expect(sendSpy).toHaveBeenCalledWith(expect.objectContaining({
@@ -465,6 +481,58 @@ describe("protocol tests", () => {
465481
await Promise.resolve();
466482
await expect(requestPromise).resolves.toEqual({ result: "success" });
467483
});
484+
485+
test("should reset timeout by default when progress notification is received", async () => {
486+
await protocol.connect(transport);
487+
const request = { method: "example", params: {} };
488+
const mockSchema: ZodType<{ result: string }> = z.object({
489+
result: z.string(),
490+
});
491+
const onProgressMock = jest.fn();
492+
// Don't specify resetTimeoutOnProgress, should default to true
493+
const requestPromise = protocol.request(request, mockSchema, {
494+
timeout: 1000,
495+
onprogress: onProgressMock,
496+
});
497+
498+
// Advance past most of the timeout period
499+
jest.advanceTimersByTime(900);
500+
501+
// Send progress notification - should reset timeout
502+
if (transport.onmessage) {
503+
transport.onmessage({
504+
jsonrpc: "2.0",
505+
method: "notifications/progress",
506+
params: {
507+
progressToken: 0,
508+
progress: 50,
509+
total: 100,
510+
},
511+
});
512+
}
513+
await Promise.resolve();
514+
515+
expect(onProgressMock).toHaveBeenCalledWith({
516+
progress: 50,
517+
total: 100,
518+
});
519+
520+
// Advance another 900ms (would have timed out without reset)
521+
jest.advanceTimersByTime(900);
522+
523+
// Send final response
524+
if (transport.onmessage) {
525+
transport.onmessage({
526+
jsonrpc: "2.0",
527+
id: 0,
528+
result: { result: "completed" },
529+
});
530+
}
531+
await Promise.resolve();
532+
533+
// Should complete successfully because timeout was reset
534+
await expect(requestPromise).resolves.toEqual({ result: "completed" });
535+
});
468536
});
469537

470538
describe("Debounced Notifications", () => {

src/shared/protocol.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ export type RequestOptions = {
8383
/**
8484
* If true, receiving a progress notification will reset the request timeout.
8585
* This is useful for long-running operations that send periodic progress updates.
86-
* Default: false
86+
* Default: true
8787
*/
8888
resetTimeoutOnProgress?: boolean;
8989

@@ -628,7 +628,7 @@ export abstract class Protocol<
628628
{ timeout }
629629
));
630630

631-
this._setupTimeout(messageId, timeout, options?.maxTotalTimeout, timeoutHandler, options?.resetTimeoutOnProgress ?? false);
631+
this._setupTimeout(messageId, timeout, options?.maxTotalTimeout, timeoutHandler, options?.resetTimeoutOnProgress ?? true);
632632

633633
this._transport.send(jsonrpcRequest, { relatedRequestId, resumptionToken, onresumptiontoken }).catch((error) => {
634634
this._cleanupTimeout(messageId);

0 commit comments

Comments
 (0)