Fix timeout reset upon progress notification - #2817
lowlyocean wants to merge 1 commit into
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
koriyoshi2041
left a comment
There was a problem hiding this comment.
The startTime reset here changes maxTotalTimeout from an absolute request lifetime cap into another inactivity window. That is why all 12 protocol:timeout:max-total E2E variants time out in Node 20/22/24: recurring progress can now postpone the maximum indefinitely. The inactivity timer already resets via clearTimeout + setTimeout; startTime needs to remain the original request timestamp. Please remove the info.startTime = Date.now() assignment and keep/add a regression where progress extends the ordinary timeout but the request still rejects at the original maxTotalTimeout deadline.
|
@koriyoshi2041 What is the correct fix for this behavior? Are you able to reproduce it? What is the maxTotalTimeout, because currently I am seeing the MCP inspector client always report an error after 60s despite receiving progress notifications. |
|
I can reproduce it: all three The intended split is: The fix here is to remove |
|
Sorry, I didn't mean can you reproduce the e2e test failures introduced by this PR. I meant, can you reproduce the error reported in the linked issue modelcontextprotocol/inspector#2314 ( Inspector receieves progress notifications but times out after 60s). Since the protocol supports resetting the 60s default request timeout on progress notifications , then Inspector should not be using a maxTotalTime of 60s (because it defeats the reset). |
|
Thanks for clarifying. I have not reproduced the original Inspector #2314 report yet. I checked the current Inspector path instead. The useful next reproduction is therefore version/transport-specific: run the #2314 server pattern against Inspector 2.2.0 and current main, capture the actual |
Motivation and Context
Closes #2816
How Has This Been Tested?
Ran the full test suite
Breaking Changes
No breaking changes
Types of changes
Checklist
Additional context
This fixes MCP inspector v2, which claims to automatically reset request timeout when progress notifications are received