Skip to content

fix(mcp): give tools/call its own request budget and stop undercutting long executions - #6741

Open
asto18089 wants to merge 1 commit into
Hmbown:mainfrom
Pinvou:upstream/mcp-tools-call-budget
Open

asto18089 wants to merge 1 commit into
Hmbown:mainfrom
Pinvou:upstream/mcp-tools-call-budget

Conversation

@asto18089

Copy link
Copy Markdown
Contributor

Summary

MCP tool calls were killed by two stacked short budgets: crates/mcp's generic REQUEST_TIMEOUT (120s) applied to all requests including tools/call, and the TUI pool's default_execute_timeout (60s). A legitimate tool call that runs minutes — builds, test suites, scrapes, remote jobs — was killed mid-flight, and the model's retry compounded the cost. The read-wait also undercut both: recv bounded every response wait by the 120s read knob, so even a raised execute_timeout fired first at the inner read and marked the connection Disconnected.

Changes:

  • crates/mcp: tools/call gets a dedicated CALL_TOOL_TIMEOUT (30 min, matching the TUI pool default); every other request (initialize, lists, resources) keeps the 120s generic budget.
  • TUI pool: default_execute_timeout 60s → 30 min; per-server/global execute_timeout overrides still win via the existing effective_execute_timeout seam.
  • Read-wait: the inner read budget for a request is now max(read_timeout, request budget), so the read knob can no longer silently defeat a raised execute budget; requests whose budget doesn't exceed the read knob keep failing fast at it.
  • HTTP: the client ceiling is built from max(read, execute) so HTTP servers get the same semantics.

Known trade-offs, disclosed for review:

  • prompts/get routes through effective_execute_timeout, so it inherits the new default (comment in code notes this; the override knob is the intended way to keep it tight).
  • All MCP calls serialize on one pool mutex held for the call, so a genuinely wedged tools/call now blocks sibling-server calls/status for up to 30 min (previously ≤60s) unless the turn is interrupted — same shape as before, longer tail; the per-server execute_timeout override is the escape hatch. Happy to explore hold-lock-only-for-dispatch as a follow-up.

Coordination: upstream #6711 reworks engine stream-open budgets — different surface, no file overlap.

Adapted from the Pinvou fork's timeout audit (Pinvou/CodeWhale d349f2537).

Testing

  • cargo fmt --all -- --check
  • cargo clippy -p codewhale-mcp -p codewhale-tui --all-features --locked (clean under the CI allow list)
  • cargo test -p codewhale-mcp --lib — 83 passed; cargo test -p codewhale-tui --lib mcp:: — 251 passed. New pins: a slow tools/call outlives an injected generic budget while resources/read still fails at it; a tool response after the read knob still completes within the execute budget (red without the widening); requests under the read knob still fail at it; the HTTP ceiling covers the execute budget

Checklist

  • Updated docs or comments as needed (docs/MCP.md + zh minimal example now shows the new default)
  • Added or updated tests where relevant
  • No CHANGELOG.md changes

@asto18089
asto18089 requested a review from Hmbown as a code owner September 29, 2026 11:13
@github-actions github-actions Bot added the contribution-gate Author not yet in .github/APPROVED_CONTRIBUTORS; a maintainer grants access with /lgtm label Sep 29, 2026
…g long executions

Root cause: every MCP request shared one finite wall-clock cap sized for
cheap metadata calls. The engine-side stdio proxy answered tools/call
under the generic 120s request budget, and the TUI pool defaulted its
execute timeout to 60s; a legitimate tool run that takes minutes — a
build, a test suite, a long script driven through an MCP server — was
killed mid-flight and the model got "timed out" for healthy work, then
retried and compounded the cost. Worse, raising the documented
`execute_timeout` knob did not actually govern: the connection's inner
response read wait stayed at `read_timeout`, fired first, marked the
connection Disconnected, and silently cut the call off at the read knob.

Mechanism:
- crates/mcp stdio proxy: tools/call now uses a dedicated
  CALL_TOOL_TIMEOUT; every other request keeps the generic 120s
  REQUEST_TIMEOUT.
- TUI pool: default execute timeout 60s -> 1800s. The per-server and
  global `execute_timeout` config fields remain the override path
  (`effective_execute_timeout`), so the constants are documented
  defaults only.
- TUI connection: the per-request inner read wait is widened to
  max(read_timeout, that request's own budget), so a server that is
  silent for the whole execution of a long tools/call is not declared
  dead mid-call. Requests whose budget does not exceed the read knob
  (resources/read, discovery) still fail at the configured read budget.
- TUI HTTP transports: the client's total request ceiling is now
  max(read, execute) so a raised execute_timeout governs HTTP servers
  too; the read knob itself stays intact for the connection-level waits.

Numbers: 1800s (30 minutes) covers long-but-bounded tool workloads
(builds, test suites, remote jobs) while staying a real bound — a wedged
server still cannot hang a consumer forever. The stdio constant matches
the TUI pool default so both surfaces behave the same for one server.

Coordination: PR Hmbown#6711 touches stream-open budgets in the engine; this
change deliberately does not touch stream-open or retry logic — it only
re-budgets per-request MCP calls.

Signed-off-by: asto18089 <asto18089@126.com>
@asto18089
asto18089 force-pushed the upstream/mcp-tools-call-budget branch from be27103 to 3ffbb90 Compare September 29, 2026 11:45

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution-gate Author not yet in .github/APPROVED_CONTRIBUTORS; a maintainer grants access with /lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant