fix(mcp): isolate a single failing stdio server from the bridge (#50394)

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"

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

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