fix(mcp): don't count tool-level errors toward the circuit breaker (#11113) - #67340
fix(mcp): don't count tool-level errors toward the circuit breaker (#11113)#67340Hotragn wants to merge 1 commit into
Conversation
…ousResearch#11113) A completed tools/call round-trip is direct evidence the transport is alive — even when the payload is a tool-level error (result.isError: a DNS failure, HTTP 4xx/5xx, a crashed page). The handler was bumping the consecutive-failure count for any {"error": ...} payload, so three bad URLs branded a healthy Playwright server "unreachable" and short-circuited every other tool on it, steering the model away from a working server with a misleading message. Reset the breaker on any completed round-trip and let only transport- level failures (exceptions out of the call: timeouts, connection loss, dead subprocess) feed it — which is exactly the retry-burn case the breaker was introduced for in NousResearch#10447. The model now always sees the tool's real error text, which it can act on. No error-string classification involved, so there is no infrastructure- error heuristic list to maintain as new MCP servers appear.
alt-glitch
added
type/bug
|
Closing in favor of #61555 — alt-glitch's cross-reference is correct: it has carried the same structural fix (a completed round-trip resets the breaker; only transport exceptions count) since July 9, and the earlier PR should be the one to land. @bradhallett's confirmation-gateway cascade is also a stronger motivating case than my DNS example. Two regression tests on my branch aren't in #61555 and may be worth cherry-picking when it lands: |
2 participants
What does this PR do?
Stops the MCP circuit breaker from counting tool-level errors as server failures.
_make_tool_handler's success path bumped the consecutive-failure count for any{"error": ...}payload — which is exactly what a completedtools/callwithresult.isError(a DNS failure, an HTTP 4xx/5xx, a crashed page) serializes to. Three bad URLs therefore branded a healthy Playwright server "unreachable" and short-circuited every tool on that server, returning a message that actively steers the model away from a working server. Reported with a precise repro in #11113 (P2,tool/mcp).The fix is structural rather than heuristic: a completed RPC round-trip is itself the reachability proof. If
session.call_toolreturned at all — even withisError=True— the transport is alive, so the handler resets the breaker and surfaces the tool's real error text (which the model can act on). Only transport-level failures — exceptions out of the call: timeouts, connection loss, a dead stdio subprocess — feed the breaker, which is precisely the retry-burn case it was introduced for in #10447. The half-open/cooldown machinery (#26892, #16788) is untouched.Relation to #11128 (open, stale since April): same diagnosis, different mechanism — it classifies exception strings into infrastructure-vs-tool via an
_is_infrastructure_error()heuristic, which needs a pattern list maintained as new MCP servers appear, and its base predates the half-open/cooldown rework of this file. This change needs no classification at all.Related Issue
Fixes #11113.
Type of Change
Changes Made
tools/mcp_tool.py— in_make_tool_handler, the completed-call path now always resets the consecutive-failure count instead of parsing the payload for an"error"key; the exception paths (transport failures) still bump. The other{"error": ...}returns in the handler (not-connected, transport-down) already bump explicitly before returning and are unaffected.tests/tools/test_mcp_circuit_breaker.py— 3 new behavior tests: tool-level errors never trip the breaker across threshold+2 calls (the real error text surfaces every time, never "unreachable"); a completed round-trip resets strikes accumulated from earlier transport failures (consecutive-failure semantics); transport exceptions still trip the breaker and short-circuit (locks in the Gateway hangs when Google Workspace MCP skill returns error — infinite retry loop #10447 contract).How to Test
scripts/run_tests.sh tests/tools/test_mcp_circuit_breaker.py— 10 passed (7 existing + 3 new).scripts/run_tests.sh tests/tools/test_mcp_*.py— failing set on native Windows is identical to a pristinemainworktree baseline (6 pre-existing platform failures: tilde expansion, image caching, Windows env vars); zero failures attributable to this change.Checklist
Code
Documentation & Housekeeping
cli-config.yaml.example— N/A (no config keys)CONTRIBUTING.md/AGENTS.md— N/A