Add timeout reset on progress notifications #152

Ozamatash · 2025-02-17T22:19:26Z

Add timeout reset on progress notifications

Closes #87

Motivation and Context

This change addresses the need for better handling of long-running operations in the MCP protocol. Previously, requests would time out according to a fixed timeout, even if the server was actively sending progress notifications. This could lead to premature timeouts for valid long-running operations.

The implementation allows servers to indicate that an operation is still in flight by sending progress notifications, which will reset the timeout. To prevent indefinite operations, a configurable maximum total timeout is also provided.

How Has This Been Tested?

Basic timeout reset on progress notification
Maximum total timeout enforcement
Multiple sequential progress notifications
Default timeout behavior without progress
Edge cases around timeout boundaries

Breaking Changes

No breaking changes. The implementation is fully backwards compatible:

New options (resetTimeoutOnProgress and maxTotalTimeout) are optional
Default timeout behavior remains unchanged
Existing progress notification handling is preserved

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

jspahrsummers

Thanks for working on this! ✨ Left some comments below, but I'd love to get this merged after they're addressed.

jspahrsummers · 2025-02-19T14:30:57Z

src/shared/protocol.test.ts

@@ -62,6 +62,182 @@ describe("protocol tests", () => {
    await transport.close();
    expect(oncloseMock).toHaveBeenCalled();
  });
+
+  test("should reset timeout when progress notification is received", async () => {
+    jest.useFakeTimers();


Can you please put this into a beforeEach and useRealTimers into an afterEach? (Consider using a describe to group with the below tests too.)

Right now, errors in the test could cause timers not to be restored.

jspahrsummers · 2025-02-19T14:32:21Z

src/shared/protocol.test.ts

+      progress: 50,
+      total: 100,
+    });
+


Presumably we need to advance the timers again here? Otherwise this doesn't seem to really be testing that the timeout got extended.

jspahrsummers · 2025-02-19T14:33:26Z

src/shared/protocol.test.ts

+    // Advance time beyond maxTotalTimeout
+    jest.advanceTimersByTime(150);
+
+    // Send progress notification after maxTotalTimeout
+    if (transport.onmessage) {
+      transport.onmessage({
+        jsonrpc: "2.0",
+        method: "notifications/progress",
+        params: {
+          progressToken: 0,
+          progress: 50,
+          total: 100,
+        },
+      });
+    }


We should test that sending a progress notification doesn't reset the max total timeout, which AFAICT this doesn't exercise—because the timeout already happened by the time the progress notification is sent.

jspahrsummers · 2025-02-19T14:46:37Z

src/shared/protocol.ts

+
+  /**
+   * Maximum total time (in milliseconds) to wait for a response, even if progress notifications are received.
+   * Only used when resetTimeoutOnProgress is true.


It might be a bit confusing that this property only has an effect when another is set. Any reason not to always apply it?

jspahrsummers · 2025-02-19T14:49:13Z

src/shared/protocol.ts

+      cancel(new McpError(
+        ErrorCode.RequestTimeout,
+        "Maximum total timeout exceeded",
+        { maxTotalTimeout: info.maxTotalTimeout, totalElapsed }
+      ));


TIOLI: I think the flow here is a bit confusing, and IMO it'd be cleaner to throw this as an exception here, rather than chain callbacks.

Ozamatash · 2025-02-19T18:43:30Z

Test improvements:
- Moved timer setup/cleanup into beforeEach/afterEach hooks.
- Added timer advancement after progress notification to properly test timeout extension
- Fixed maxTotalTimeout test to verify progress notifications don't reset after timeout
Code changes:
- Made maxTotalTimeout always apply, independent of resetTimeoutOnProgress
- Changed error handling to throw exception directly instead of callbacks.
- Added try-catch in _onprogress to properly handle timeout errors

jspahrsummers

Thanks, looking good! Just one remaining question—sorry for overlooking it the first time.

jspahrsummers · 2025-02-20T17:58:04Z

src/shared/protocol.ts

+        if (!this._resetTimeout(messageId)) {
+          return;
+        }


IIUC this means that not having a timeout handler will skip calling the progress handler. Is that intentional? (Sorry for missing it in the first review!)

Not intentional. Although we only enter the if block if timeoutInfo exists. Changed it to only return early on actual timeout errors.

jspahrsummers

Great, thanks!

GLips · 2025-02-24T23:21:30Z

This looks interesting for an issue I'm working on. Are there updated docs for this feature? I can't tell directly from the PR how I might use it—I don't think I have direct access to the protocol.request function in my implementation.

Ozamatash · 2025-02-25T16:39:03Z

On the server side you need to be sending notification messages and on the client side this feature needs to be enabled in the RequestOptions. This commit wasn't included in the latest release so it's not yet in the official npm package if I understand correctly. Basically mcp clients like Cursor still need to enable this.

GLips · 2025-02-27T21:57:52Z

Appreciate the response @Ozamatash—I'm not clear on how to send notification messages from looking at the typescript SDK documentation or the tests in your PR, but sounds like if I hold tight support (and docs) for this will percolate through the ecosystem. Thanks for the good work!

…ogress-timeout-reset Add timeout reset on progress notifications

Add timeout reset on progress notifications

7f6c046

jspahrsummers requested changes Feb 19, 2025

View reviewed changes

Refactor progress notification timeout handling in protocol

28415d3

Ozamatash requested a review from jspahrsummers February 19, 2025 19:56

jspahrsummers requested changes Feb 20, 2025

View reviewed changes

only return early from progress notification on actual timeout errors

b09240b

Ozamatash requested a review from jspahrsummers February 20, 2025 21:38

jspahrsummers approved these changes Feb 24, 2025

View reviewed changes

jspahrsummers merged commit 5c07636 into modelcontextprotocol:main Feb 24, 2025
2 checks passed

GLips mentioned this pull request Feb 24, 2025

RangeError: Invalid string length when fetching large Figma files GLips/Figma-Context-MCP#3

Closed

Ozamatash deleted the feature/progress-timeout-reset branch February 24, 2025 23:30

MediaInfluences pushed a commit to MediaInfluences/typescript-sdk that referenced this pull request Apr 3, 2025

Merge pull request modelcontextprotocol#152 from Ozamatash/feature/pr…

bff4c7c

…ogress-timeout-reset Add timeout reset on progress notifications

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout reset on progress notifications #152

Add timeout reset on progress notifications #152

Ozamatash commented Feb 17, 2025

jspahrsummers left a comment •

edited

Loading

jspahrsummers Feb 19, 2025

jspahrsummers Feb 19, 2025

jspahrsummers Feb 19, 2025

jspahrsummers Feb 19, 2025

jspahrsummers Feb 19, 2025

Ozamatash commented Feb 19, 2025

jspahrsummers left a comment

jspahrsummers Feb 20, 2025

Ozamatash Feb 20, 2025

jspahrsummers left a comment

GLips commented Feb 24, 2025

Ozamatash commented Feb 25, 2025

GLips commented Feb 27, 2025

Add timeout reset on progress notifications #152

Add timeout reset on progress notifications #152

Conversation

Ozamatash commented Feb 17, 2025

Add timeout reset on progress notifications

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

jspahrsummers left a comment • edited Loading

Choose a reason for hiding this comment

jspahrsummers Feb 19, 2025

Choose a reason for hiding this comment

jspahrsummers Feb 19, 2025

Choose a reason for hiding this comment

jspahrsummers Feb 19, 2025

Choose a reason for hiding this comment

jspahrsummers Feb 19, 2025

Choose a reason for hiding this comment

jspahrsummers Feb 19, 2025

Choose a reason for hiding this comment

Ozamatash commented Feb 19, 2025

jspahrsummers left a comment

Choose a reason for hiding this comment

jspahrsummers Feb 20, 2025

Choose a reason for hiding this comment

Ozamatash Feb 20, 2025

Choose a reason for hiding this comment

jspahrsummers left a comment

Choose a reason for hiding this comment

GLips commented Feb 24, 2025

Ozamatash commented Feb 25, 2025

GLips commented Feb 27, 2025

jspahrsummers left a comment •

edited

Loading