diff --git a/tests/tools/test_mcp_bridge_single_failure.py b/tests/tools/test_mcp_bridge_single_failure.py new file mode 100644 index 000000000000..3be44d416853 --- /dev/null +++ b/tests/tools/test_mcp_bridge_single_failure.py @@ -0,0 +1,132 @@ +"""Regression test for #50394. + +A single failing stdio MCP server must not churn the whole MCP bridge. + +Root cause: a server that fails to connect is never recorded in +``_servers`` (``start()`` raises before the ``_servers[name] = server`` +line in ``_discover_and_register_server``). Without a post-failure +cooldown, every subsequent ``register_mcp_servers`` pass (one per agent +worker session) re-spawns the failing server from scratch -- a restart +storm that destabilises the healthy co-located servers. The fix arms a +per-server exponential backoff so a chronically failing server is retried +on a schedule, isolated from the rest of the bridge. +""" + +from unittest.mock import patch + +import pytest + +import tools.mcp_tool as mcp_mod + + +@pytest.fixture(autouse=True) +def _reset_mcp_state(): + """Snapshot and restore the module-level MCP state around each test.""" + snapshot = ( + dict(mcp_mod._servers), + set(mcp_mod._server_connecting), + dict(mcp_mod._server_connect_errors), + dict(mcp_mod._server_connect_retry_after), + dict(mcp_mod._server_connect_failures), + ) + mcp_mod._servers.clear() + mcp_mod._server_connecting.clear() + mcp_mod._server_connect_errors.clear() + mcp_mod._server_connect_retry_after.clear() + mcp_mod._server_connect_failures.clear() + try: + yield + finally: + (servers, connecting, errs, retry_after, failures) = snapshot + mcp_mod._servers.clear(); mcp_mod._servers.update(servers) + mcp_mod._server_connecting.clear(); mcp_mod._server_connecting.update(connecting) + mcp_mod._server_connect_errors.clear(); mcp_mod._server_connect_errors.update(errs) + mcp_mod._server_connect_retry_after.clear(); mcp_mod._server_connect_retry_after.update(retry_after) + mcp_mod._server_connect_failures.clear(); mcp_mod._server_connect_failures.update(failures) + + +class TestConnectCooldownHelpers: + def test_failure_arms_exponential_backoff(self): + now = 1000.0 + with patch("tools.mcp_tool.time.monotonic", return_value=now): + mcp_mod._record_connect_failure("bad") + d1 = mcp_mod._server_connect_retry_after["bad"] + mcp_mod._record_connect_failure("bad") + d2 = mcp_mod._server_connect_retry_after["bad"] + assert d1 == now + mcp_mod._CONNECT_RETRY_BASE_BACKOFF_SEC + assert d2 == now + mcp_mod._CONNECT_RETRY_BASE_BACKOFF_SEC * 2 + assert mcp_mod._server_connect_failures["bad"] == 2 + + def test_backoff_is_capped(self): + for _ in range(50): + mcp_mod._record_connect_failure("bad") + deadline = mcp_mod._server_connect_retry_after["bad"] + assert deadline <= mcp_mod.time.monotonic() + mcp_mod._CONNECT_RETRY_MAX_BACKOFF_SEC + 1 + + def test_cooldown_active_then_clears(self): + now = 5000.0 + with patch("tools.mcp_tool.time.monotonic", return_value=now): + mcp_mod._record_connect_failure("bad") + assert mcp_mod._connect_cooldown_active("bad") is True + later = now + mcp_mod._CONNECT_RETRY_MAX_BACKOFF_SEC + 1 + with patch("tools.mcp_tool.time.monotonic", return_value=later): + assert mcp_mod._connect_cooldown_active("bad") is False + mcp_mod._clear_connect_failure("bad") + assert "bad" not in mcp_mod._server_connect_retry_after + assert "bad" not in mcp_mod._server_connect_failures + + def test_unknown_server_not_in_cooldown(self): + assert mcp_mod._connect_cooldown_active("never-seen") is False + + +@pytest.mark.skipif(not mcp_mod._MCP_AVAILABLE, reason="mcp SDK not installed") +class TestRegisterMcpServersIsolation: + """register_mcp_servers must not re-spawn a server still in cooldown.""" + + def _run_with_mocked_connect(self, attempts): + async def fake_connect(name, config): + attempts.append(name) + if name == "bad": + raise ConnectionError("exec: bad: not found") + server = mcp_mod.MCPServerTask(name) + server._registered_tool_names = [] + server._tools = [] + return server + + return patch("tools.mcp_tool._connect_server", side_effect=fake_connect) + + def test_failing_server_skipped_on_second_pass(self): + attempts = [] + cfg = { + "good": {"command": "good-cmd"}, + "bad": {"command": "bad-cmd"}, + } + with self._run_with_mocked_connect(attempts), \ + patch("tools.mcp_tool._register_server_tools", return_value=[]), \ + patch("tools.mcp_tool._filter_suspicious_mcp_servers", side_effect=lambda x: x): + mcp_mod.register_mcp_servers(cfg) + assert "good" in mcp_mod._servers + assert "bad" not in mcp_mod._servers + assert mcp_mod._connect_cooldown_active("bad") is True + assert "bad" in attempts + + attempts.clear() + mcp_mod.register_mcp_servers(cfg) + assert "bad" not in attempts, ( + "failing server was re-spawned despite active cooldown -- " + "restart storm not isolated (#50394)" + ) + + def test_cooldown_expiry_allows_retry(self): + attempts = [] + cfg = {"bad": {"command": "bad-cmd"}} + with self._run_with_mocked_connect(attempts), \ + patch("tools.mcp_tool._register_server_tools", return_value=[]), \ + patch("tools.mcp_tool._filter_suspicious_mcp_servers", side_effect=lambda x: x): + mcp_mod.register_mcp_servers(cfg) + assert mcp_mod._connect_cooldown_active("bad") is True + + mcp_mod._server_connect_retry_after["bad"] = mcp_mod.time.monotonic() - 1 + attempts.clear() + mcp_mod.register_mcp_servers(cfg) + assert "bad" in attempts, "elapsed cooldown should permit a retry" diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index a66235040284..07e088533049 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -1518,7 +1518,14 @@ class TestToolsetInjection: broken_fixed = True call_count = 0 - # Second call: should retry broken, skip good + # The failed server is now serving a post-failure backoff + # (#50394: prevents a tight re-spawn storm across the frequent + # per-worker-session discovery passes). Expire that cooldown to + # simulate the retry window having elapsed. + import tools.mcp_tool as _mcp_mod + _mcp_mod._server_connect_retry_after.pop("broken", None) + + # Next call after the cooldown: should retry broken, skip good result2 = discover_mcp_tools() assert "mcp__good__ping" in result2 assert "mcp__broken__ping" in result2 diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 27d5cf278094..af14b122a1d1 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3486,6 +3486,61 @@ _servers: Dict[str, MCPServerTask] = {} _server_connecting: set[str] = set() _server_connect_errors: Dict[str, str] = {} +# Connection-retry cooldown (per-server isolation against restart storms). +# +# A single stdio MCP server that fails to spawn (bad PATH, ``exec: not +# found``, crash-on-start) is never recorded in ``_servers`` -- ``start()`` +# raises and ``_discover_and_register_server`` aborts before the +# ``_servers[name] = server`` line. Without a cooldown, EVERY subsequent +# ``discover_mcp_tools()`` (one per agent worker session, i.e. every few +# seconds) sees the server as "not connected" and re-spawns it from +# scratch. That is the restart storm in #50394: the failing server is +# re-attempted on the shared MCP event loop on every worker session, the +# subprocesses pile up unreaped, and the churn destabilises the healthy +# co-located servers (their tools intermittently surface as +# "Unknown tool"). +# +# Fix: after a failed connection attempt, stamp a monotonic +# ``retry_after`` deadline with exponential backoff. ``register_mcp_servers`` +# skips a server whose cooldown has not elapsed, so a chronically failing +# server is retried on a backoff schedule instead of on every worker +# session -- isolating it from the rest of the bridge. A successful +# connection clears the state. +_server_connect_retry_after: Dict[str, float] = {} # name -> monotonic deadline +_server_connect_failures: Dict[str, int] = {} # name -> consecutive failures +_CONNECT_RETRY_BASE_BACKOFF_SEC = 30.0 +_CONNECT_RETRY_MAX_BACKOFF_SEC = 600.0 + + +def _record_connect_failure(server_name: str) -> None: + """Stamp an exponential-backoff cooldown after a failed connect. + + Called (under ``_lock``) when a server fails its discovery/connect + attempt. The cooldown grows geometrically with the consecutive + failure count and is capped at :data:`_CONNECT_RETRY_MAX_BACKOFF_SEC`, + so a permanently-broken server settles into infrequent retries + rather than a tight respawn loop. + """ + n = _server_connect_failures.get(server_name, 0) + 1 + _server_connect_failures[server_name] = n + backoff = min( + _CONNECT_RETRY_BASE_BACKOFF_SEC * (2 ** (n - 1)), + _CONNECT_RETRY_MAX_BACKOFF_SEC, + ) + _server_connect_retry_after[server_name] = time.monotonic() + backoff + + +def _clear_connect_failure(server_name: str) -> None: + """Clear the connect-cooldown state after a successful connection.""" + _server_connect_failures.pop(server_name, None) + _server_connect_retry_after.pop(server_name, None) + + +def _connect_cooldown_active(server_name: str) -> bool: + """Return True if ``server_name`` is still within its retry cooldown.""" + deadline = _server_connect_retry_after.get(server_name) + return deadline is not None and time.monotonic() < deadline + # Circuit breaker: consecutive error counts per server. After # _CIRCUIT_BREAKER_THRESHOLD consecutive failures, the handler returns # a "server unreachable" message that tells the model to stop retrying, @@ -5508,7 +5563,15 @@ def register_mcp_servers(servers: Dict[str, dict]) -> List[str]: new_servers = { k: v for k, v in servers.items() - if k not in _servers and _parse_boolish(v.get("enabled", True), default=True) + if k not in _servers + and _parse_boolish(v.get("enabled", True), default=True) + # Skip a server still serving its post-failure backoff. Without + # this, a server that fails to connect (and is therefore never + # recorded in ``_servers``) would be re-spawned on every worker + # session's discovery pass -- the #50394 restart storm. The + # cooldown is cleared automatically on the next successful + # connect or by a manual /mcp refresh. + and not _connect_cooldown_active(k) } # Cached entries with no live session are parked or mid-reconnect. # Their tools are deregistered, so nothing else can reach @@ -5557,6 +5620,11 @@ def register_mcp_servers(servers: Dict[str, dict]) -> List[str]: with _lock: _server_connecting.discard(name) _server_connect_errors[name] = message + # Arm the per-server backoff so the next discovery pass + # doesn't immediately re-spawn this failing server + # (#50394). Isolated to this server -- healthy servers + # in the same batch are unaffected. + _record_connect_failure(name) logger.warning( "Failed to connect to MCP server '%s'%s: %s", name, @@ -5567,6 +5635,7 @@ def register_mcp_servers(servers: Dict[str, dict]) -> List[str]: with _lock: _server_connecting.discard(name) _server_connect_errors.pop(name, None) + _clear_connect_failure(name) # Per-server timeouts are handled inside _discover_and_register_server. # The outer timeout is generous: 120s total for parallel discovery. @@ -6050,6 +6119,11 @@ def shutdown_mcp_servers(): ) with _lock: _servers.clear() + # Drop connect-retry cooldowns too: a full shutdown/restart + # should re-attempt every server immediately, not honour a + # stale per-server backoff from before the restart (#50394). + _server_connect_retry_after.clear() + _server_connect_failures.clear() with _lock: loop = _mcp_loop