From f99d2912477c768a3bc49fd818e79a1bd6ca89db Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 8 Aug 2026 02:21:36 -0500 Subject: [PATCH] fix(mcp): let a server that 401s at startup come back after re-login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An auth failure on the very first connect returned out of the run loop instead of parking. That ended the run task, and the task is the only listener on _reconnect_event — so the server stayed dead for the life of the process. `hermes mcp login`, a /mcp refresh, and the 300s self-probe all had nothing left to wake, and the only cure was a full restart. _classify_mcp_failure already calls 401/403 "permanent" and documents that run() parks those immediately; the early return above it meant auth was the one permanent failure that never got there. Park it with the others and keep the tailored log line, now pointing at `hermes mcp login `. --- .../tools/test_mcp_failure_classification.py | 98 +++++++++++++++++++ .../test_mcp_initial_connect_shutdown.py | 21 +++- tools/mcp_tool.py | 41 +++++--- 3 files changed, 140 insertions(+), 20 deletions(-) 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