improvement(mcp): align tool discovery timeouts and retries with MCP protocol standard#5753
Conversation
…protocol standard - Raise tools/list idle timeout 10s -> 30s (derived from per-server config, clamped to max execution timeout) with a 60s hard cap via maxTotalTimeout, matching the SDK's DEFAULT_REQUEST_TIMEOUT_MSEC. Enable resetTimeoutOnProgress with an onprogress handler so slow-but-alive servers are not spuriously failed. - Retry read-only tools/list on transient transport errors (timeout, connection-closed, 429/5xx, session 404/400, network) with jittered backoff. tools/call stays conservative (session-error retry only) since it is not idempotent. The MCP SDK does not retry POST requests, so the app owns this. - Wire client.onerror so out-of-band transport failures are observed, not silently dropped. - Map timeouts to a user-facing message and surface refresh/remove failures via the standard emcn toast instead of swallowing them.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
The refresh API wraps post-discovery workflow schema sync in try/catch so a sync failure still returns 200 with connected status and zero workflows updated. MCP settings shows toasts on refresh/remove failures. Reviewed by Cursor Bugbot for commit 561a32f. Configure here. |
Greptile SummaryThis PR makes MCP tool discovery more resilient and improves failure reporting. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(mcp): keep tools/list absolute timeo..." | Re-trigger Greptile |
…nc throws Discovery already persists status and caches tools; a syncToolSchemasToWorkflows failure was escaping the refactored try/catch and returning 500 despite the refresh having succeeded. Guard the secondary sync so it degrades to zero workflows updated instead.
A configured per-server timeout above 60s was expanding maxTotalTimeout past the intended absolute discovery ceiling. Clamp the idle timeout to the ceiling too so tools/list can never hang the UI beyond it; connect() and callTool() still honor the full configured/execution timeout.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 561a32f. Configure here.
Summary
tools/listidle timeout from an aggressive 10s → 30s (derived from the per-servertimeout, clamped to the max execution timeout) with a 60s hard cap viamaxTotalTimeout, matching the MCP TypeScript SDK'sDEFAULT_REQUEST_TIMEOUT_MSEC(60s). EnableresetTimeoutOnProgress+ anonprogresshandler so slow-but-alive servers aren't spuriously failed withMcpError -32001.tools/liston transient transport errors (timeout, connection-closed, 429/5xx, session 404/400, network) with jittered backoff.tools/callstays conservative (session-error retry only) since it isn't idempotent. The SDK doesn't retry POST requests — only SSE streams — so the app owns this.client.onerrorso out-of-band transport failures (SSE disconnects, exhausted reconnects) are observed instead of silently dropped, per the SDK'sProtocol.onerrorcontract.toastinstead of swallowing them.Context
A workspace MCP server whose
tools/listintermittently took >10s was hard-failing discovery (-32001) even though connect/OAuth succeeded — surfacing as a stuck "Connecting…" / "Failed to discover MCP tools" with the tool count dropping from 40 → 0. The 10s cap was ~6× tighter than the SDK default and below every documented ecosystem norm; this loosens it toward the protocol standard and adds the resilience the SDK expects apps to implement themselves.Type of Change
Testing
apps/sim/lib/mcpunit tests (client/service/utils) + MCP route/component/hook tests pass (116 tests).tools/listtimeout retries then succeeds; timeout maps to the user-facinglastError;listToolsis bounded with idle timeout + hard cap + progress reset.tsc --noEmitclean; biome clean.Checklist