fix: inject progressToken when resetTimeoutOnProgress is set (not only onprogress)#1870
Conversation
Previously, _meta.progressToken was only injected into the outgoing JSON-RPC request when options.onprogress was provided. Setting resetTimeoutOnProgress: true alone had no effect because the SDK's _onprogress handler matches incoming notifications by params.progressToken → messageId — without a token in the request, server progress notifications can never be matched, and _resetTimeout is never called. This change expands the injection condition to also fire when resetTimeoutOnProgress is true, making the flag behave as its name implies: if you want the timeout to reset on progress, the server must be able to send progress notifications that match the request. The onprogress handler registration is unchanged — a no-op progressToken injection when only resetTimeoutOnProgress is set does not register a progress handler (avoiding unnecessary memory use). Fixes: modelcontextprotocol#245
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
travisbreaks
left a comment
There was a problem hiding this comment.
The fix is correct. Without this, resetTimeoutOnProgress alone (without onprogress) never injects the progressToken into the request _meta, so the server has no token to send progress notifications back to, and timeout reset never triggers.
One thing: the options.onprogress check is now evaluated twice (outer || and inner if). Not a real concern at this scale, but worth noting that options is already guaranteed non-null inside the block since the outer condition checked it. Clean fix overall.
Problem
resetTimeoutOnProgress: trueinRequestOptionshas no effect unlessonprogressis also provided, because_meta.progressTokenis only injected whenonprogressexists:The
_onprogresshandler (line ~668) matches incomingnotifications/progressbyparams.progressToken → messageId. Without a token in the request, the server's notifications cannot be matched —_resetTimeoutis never called — and the request times out atDEFAULT_REQUEST_TIMEOUT_MSECregardless of how often the server sends progress.Reproduction: Any tool call where the server sends periodic progress notifications and the client passes
{ resetTimeoutOnProgress: true }withoutonprogresstimes out at 60s. Tracked in issue #245.Fix
Expand the injection condition from
if (options?.onprogress)toif (options?.onprogress || options?.resetTimeoutOnProgress). TheprogressTokenis now injected in both cases, so the server'snotifications/progresscan be matched and_resetTimeoutfires correctly.The
onprogresshandler registration is unchanged — a no-op token injection (when onlyresetTimeoutOnProgressis set) does not register a progress handler, so no memory is wasted on empty callbacks.Why this matters
MCP servers providing long-running HITL tools (human-in-the-loop, waiting for user input) rely on progress notifications to keep calls alive. Clients that set
resetTimeoutOnProgress: trueexpect this to work. Prior to this fix, the flag was silently ignored unless the client also provided a callback, making it effectively unusable without reading the source.Closes #245