Merge pull request #81570 from NousResearch/bb/figma-mcp-401
Recover an MCP server that 401s at startup instead of needing a restart
This commit is contained in:
brooklyn!
GitHub
commit
d92bfa0a38
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue