fix(mcp): let a server that 401s at startup come back after re-login

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 <server>`.
This commit is contained in:
Brooklyn Nicholson 2026-08-08 02:21:36 -05:00
parent b9aa9289a8
commit f99d291247
3 changed files with 140 additions and 20 deletions

View File

@ -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()

View File

@ -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)

View File

@ -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