diff --git a/tests/tools/test_mcp_circuit_breaker.py b/tests/tools/test_mcp_circuit_breaker.py index 3060559ccf..20048bd038 100644 --- a/tests/tools/test_mcp_circuit_breaker.py +++ b/tests/tools/test_mcp_circuit_breaker.py @@ -572,3 +572,41 @@ def test_initial_connect_budget_parks_instead_of_exiting_then_revives(monkeypatc run_task.cancel() asyncio.run(_scenario()) + + +def test_breaker_opened_by_tool_errors_says_rejected_not_unreachable(monkeypatch, tmp_path): + """Three completed calls whose payload is an error still open the breaker (#10447), but the + open-breaker message must not claim the server is unreachable — it answered every time + (#11113); a single transport strike in the streak makes it "unreachable" again.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + from tools import mcp_tool + from tools.mcp_tool_handlers import _make_tool_handler + + async def _call_tool_rejects(*a, **kw): + result = MagicMock() + result.is_error = True + block = MagicMock() + block.text = "DNS lookup failed for https://nope.invalid" + result.content = [block] + result.structured_content = None + return result + + _install_stub_server(mcp_tool, "srv", _call_tool_rejects) + _mcp_loop._ensure_mcp_loop() + try: + handler = _make_tool_handler("srv", "fetch", 10.0) + for _ in range(mcp_tool._CIRCUIT_BREAKER_THRESHOLD): + assert "DNS lookup failed" in json.loads(handler({}))["error"] + tripped = json.loads(handler({}))["error"].lower() + assert "rejected" in tripped and "unreachable" not in tripped, tripped + + mcp_tool._reset_server_error("srv") + mcp_tool._bump_server_error("srv") # transport strike + mcp_tool._bump_server_error("srv", application=True) + mcp_tool._bump_server_error("srv", application=True) + assert "unreachable" in json.loads(handler({}))["error"].lower() + finally: + _cleanup(mcp_tool, "srv") + mcp_tool._server_errors_all_application.pop("srv", None) + diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 98354693bd..ae2da53128 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -456,6 +456,9 @@ _CONNECT_RETRY_BASE_BACKOFF_SEC, _CONNECT_RETRY_MAX_BACKOFF_SEC = 30.0, 600.0 # — they keep the count and timestamp in sync. _server_error_counts: Dict[Any, int] = {} _server_breaker_opened_at: Dict[Any, float] = {} +# True while every strike in the current streak was the tool's own error payload (server reachable, +# call rejected); picks the open-breaker wording, since "unreachable" was false for that case (#11113). +_server_errors_all_application: Dict[Any, bool] = {} _CIRCUIT_BREAKER_THRESHOLD, _CIRCUIT_BREAKER_COOLDOWN_SEC = 3, 60.0 # Trust-tier gating (``trust: full | untrusted``): on an untrusted server every write-capable @@ -470,13 +473,15 @@ _tool_read_only_hints: Dict[Any, Dict[str, bool]] = {} _TRUST_FULL, _TRUST_UNTRUSTED = "full", "untrusted" -def _bump_server_error(server_name: str) -> None: +def _bump_server_error(server_name: str, *, application: bool = False) -> None: """Count a failure; at the threshold (re)stamp the breaker-open time. Keyed by the calling - scope's connection so one profile's failing server never opens another profile's breaker.""" + scope's connection so one profile's failing server never opens another profile's breaker. + *application*: the call completed and the payload was an error (transport is fine).""" from tools.mcp_tool_scope import _resolve_server_key key = _resolve_server_key(server_name) n = _server_error_counts.get(key, 0) + 1 _server_error_counts[key] = n + _server_errors_all_application[key] = application and (n == 1 or _server_errors_all_application.get(key, False)) if n >= _CIRCUIT_BREAKER_THRESHOLD: _server_breaker_opened_at[key] = time.monotonic() @@ -487,6 +492,7 @@ def _reset_server_error(server_name: str) -> None: key = _resolve_server_key(server_name) _server_error_counts[key] = 0 _server_breaker_opened_at.pop(key, None) + _server_errors_all_application.pop(key, None) # Raw server names opted into parallel tool calls (``foo-bar``/``foo_bar`` sanitize alike but diff --git a/tools/mcp_tool_handlers.py b/tools/mcp_tool_handlers.py index c494f03b67..36ff99f4d6 100644 --- a/tools/mcp_tool_handlers.py +++ b/tools/mcp_tool_handlers.py @@ -75,8 +75,15 @@ def _check_circuit_breaker(server_name: str) -> Optional[str]: age = time.monotonic() - _core._server_breaker_opened_at.get(key, 0.0) if failures < _core._CIRCUIT_BREAKER_THRESHOLD or age >= _core._CIRCUIT_BREAKER_COOLDOWN_SEC: return None + retry_in = max(1, int(_core._CIRCUIT_BREAKER_COOLDOWN_SEC - age)) + if _core._server_errors_all_application.get(key): + # The server answered every time; the calls were rejected. Calling it "unreachable" sent the + # model to the user instead of to its own arguments (#11113). + return tool_error(f"MCP server '{server_name}' rejected the last {failures} calls (it is reachable; see the " + f"error text those calls returned). Paused for ~{retry_in}s. Do NOT repeat the same call — " + f"fix the arguments/URL/target or use a different approach.") return tool_error(f"MCP server '{server_name}' is unreachable after {failures} consecutive failures. " - f"Auto-retry available in ~{max(1, int(_core._CIRCUIT_BREAKER_COOLDOWN_SEC - age))}s. Do NOT retry " + f"Auto-retry available in ~{retry_in}s. Do NOT retry " f"this tool yet — use alternative approaches or ask the user to check the MCP server.") @@ -106,8 +113,12 @@ def _result_is_error(result) -> bool: def _record_call_outcome(server_name: str, result) -> Any: - """Breaker bookkeeping: an error payload from the tool itself still counts as a strike.""" - (_core._bump_server_error if _result_is_error(result) else _core._reset_server_error)(server_name) + """Breaker bookkeeping: an error payload from the tool itself still counts as a strike (#10447), + flagged as an application error so the open-breaker message stays truthful.""" + if _result_is_error(result): + _core._bump_server_error(server_name, application=True) + else: + _core._reset_server_error(server_name) return result