fix(mcp): breaker opened by tool errors says "rejected", not "unreachable"
Application errors (isError payloads) keep counting as breaker strikes: that is #10447's point (a server answering errors made the model hammer it 8x in 10s) and #109180 just reasserted it. What #11113 actually hit is the open-breaker MESSAGE: after three rejected fetches the model was told the server was "unreachable" and went to the user instead of fixing its URL. Track whether the streak was all application errors and word the pause accordingly; one transport strike restores the unreachable text.
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user