diff --git a/tests/tools/test_mcp_failure_classification.py b/tests/tools/test_mcp_failure_classification.py index bc74aa69cbaae..44fb13137031c 100644 --- a/tests/tools/test_mcp_failure_classification.py +++ b/tests/tools/test_mcp_failure_classification.py @@ -167,3 +167,101 @@ def test_permanent_failure_parks_without_retry_ladder(monkeypatch, tmp_path, cap ] assert len(park_warnings) == 1 assert "FileNotFoundError" in park_warnings[0].getMessage() + + +# ── An initial 401 must stay revivable ─────────────────────────────────────── + +@pytest.mark.no_isolate +def test_initial_auth_failure_parks_and_revives_after_relogin( + monkeypatch, tmp_path, caplog, +): + """A 401 on the FIRST connect must park, not end the run task. + + Ending the task drops the only listener on ``_reconnect_event``, so the + server stayed dead for the life of the process even after the user + re-authenticated. Parking keeps it revivable via the self-probe. + """ + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + httpx = pytest.importorskip("httpx") + + from tools import mcp_tool + + monkeypatch.setattr(mcp_tool, "_PARKED_RETRY_INTERVAL", 0.05) + + _real_sleep = asyncio.sleep + + async def _fast_sleep(_delay, *a, **kw): + await _real_sleep(0) + + monkeypatch.setattr(mcp_tool.asyncio, "sleep", _fast_sleep) + + def _auth_error(): + request = httpx.Request("POST", "https://mcp.example.test/mcp") + response = httpx.Response(401, request=request) + return httpx.HTTPStatusError("401", request=request, response=response) + + state = {"transport_calls": 0, "parked": False, "authenticated": False} + + async def _scenario(): + class _Task(MCPServerTask): + def _is_http(self): + return False + + def _deregister_tools(self): + state["parked"] = True + self._registered_tool_names = [] + + async def _run_stdio(self, config): + state["transport_calls"] += 1 + if not state["authenticated"]: + raise _group(_auth_error()) + self.session = object() + await self._wait_for_lifecycle_event() + + task = _Task("figma") + + with caplog.at_level(logging.DEBUG, logger="tools.mcp_tool"): + run_task = asyncio.ensure_future(task.run({"command": "x"})) + for _ in range(500): + await _real_sleep(0) + if state["parked"]: + break + + assert state["parked"], "auth failure never parked" + assert state["transport_calls"] == 1, ( + f"auth failure burned {state['transport_calls']} attempts" + ) + assert not run_task.done(), ( + "run task exited on a 401 — the server is now unrevivable" + ) + + # The user re-authenticates. Nothing sets _reconnect_event: + # revival must come from the timed self-probe alone. + state["authenticated"] = True + for _ in range(200): + await _real_sleep(0.01) + if task.session is not None: + break + + assert task.session is not None, ( + "parked server never recovered after re-authentication " + f"(transport_calls={state['transport_calls']})" + ) + + task._shutdown_event.set() + task._reconnect_event.set() + try: + await asyncio.wait_for(run_task, timeout=15) + except (asyncio.TimeoutError, asyncio.CancelledError, Exception): + run_task.cancel() + + asyncio.run(_scenario()) + + auth_warnings = [ + r for r in caplog.records + if r.levelno == logging.WARNING + and "failed initial authentication" in r.getMessage() + ] + assert len(auth_warnings) == 1 + assert "hermes mcp login figma" in auth_warnings[0].getMessage() diff --git a/tests/tools/test_mcp_initial_connect_shutdown.py b/tests/tools/test_mcp_initial_connect_shutdown.py index 049e319bef69a..1a74f4f3699c8 100644 --- a/tests/tools/test_mcp_initial_connect_shutdown.py +++ b/tests/tools/test_mcp_initial_connect_shutdown.py @@ -197,8 +197,14 @@ def test_initial_connect_failure_revives_same_registered_server(monkeypatch, tmp _cleanup_mcp_state(mcp_tool, created) -def test_terminal_initial_failure_is_not_retained(monkeypatch, tmp_path): - """A non-recoverable startup error must not leave a dead cache entry.""" +def test_initial_auth_failure_is_retained_and_reaped(monkeypatch, tmp_path): + """An auth failure must stay parked (revivable) and reap on shutdown. + + A 401 used to end the run task outright, which dropped the only listener + on ``_reconnect_event`` — the server could not come back even after the + user re-authenticated. It is now retained like any other parked server, + and must still tear down cleanly. + """ monkeypatch.setenv("HERMES_HOME", str(tmp_path)) from tools import mcp_tool @@ -216,6 +222,7 @@ def test_terminal_initial_failure_is_not_retained(monkeypatch, tmp_path): monkeypatch.setattr(mcp_tool, "MCPServerTask", _AuthFailingServerTask) monkeypatch.setattr(mcp_tool, "_MCP_AVAILABLE", True) + monkeypatch.setattr(mcp_tool, "_PARKED_RETRY_INTERVAL", 3600) monkeypatch.setattr(mcp_tool, "_is_auth_error", lambda exc: True) try: @@ -223,12 +230,18 @@ def test_terminal_initial_failure_is_not_retained(monkeypatch, tmp_path): "auth-failure": {"command": "unused", "connect_timeout": 5} }) == [] assert len(created) == 1 - assert created[0]._task.done() + server = created[0] + assert not server._task.done(), ( + "auth failure ended the run task — the server is unrevivable" + ) with mcp_tool._lock: - assert "auth-failure" not in mcp_tool._servers + assert mcp_tool._servers["auth-failure"] is server assert "terminal authentication failure" in ( mcp_tool._server_connect_errors["auth-failure"] ) + + mcp_tool.shutdown_mcp_servers() + assert server._task.done() finally: _cleanup_mcp_state(mcp_tool, created) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index e55b36b0a60ed..96d9bc4b4fa87 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3341,27 +3341,36 @@ class MCPServerTask: # should not permanently kill the server. # (Ported from Kilo Code's MCP resilience fix.) if not self._ready.is_set(): - if _is_auth_error(root): - logger.warning( - "MCP server '%s' failed initial OAuth authentication, " - "not retrying automatically: %s: %s", - self.name, type(root).__name__, root, - ) - self._error = exc - self._ready.set() - return - if failure_class == "permanent": # Deterministic failure (bad command, non-MCP URL, # 401/403): every retry hits the same wall. Park # immediately instead of burning the retry ladder # and spamming N identical warnings (#65673). - logger.warning( - "MCP server '%s' failed initial connection with a " - "permanent error, parking without retries " - "(state: connecting → parked): %s: %s", - self.name, type(root).__name__, root, - ) + # + # Auth failures park here too rather than returning. + # Returning ends the run task, and with it the only + # listener on ``_reconnect_event`` — so a 401 on the + # very first connect left the server unrevivable for + # the life of the process, even after the user + # re-authenticated with ``hermes mcp login``. Parking + # keeps the task alive so the 300s self-probe (and an + # explicit /mcp refresh) can pick up fresh tokens. + if _is_auth_error(root): + logger.warning( + "MCP server '%s' failed initial authentication, " + "parking until credentials change; re-authenticate " + "with `hermes mcp login %s` " + "(state: connecting → parked): %s: %s", + self.name, self.name, + type(root).__name__, root, + ) + else: + logger.warning( + "MCP server '%s' failed initial connection with a " + "permanent error, parking without retries " + "(state: connecting → parked): %s: %s", + self.name, type(root).__name__, root, + ) self._error = exc self._ready.set() self._was_parked = True