hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 28 Packages Projects Releases Wiki Activity

fix(mcp): retry a never-connected server through the existing reconcile seam

Browse Source 
Slim follow-up to the cherry-picked #112447 (@Yongcheng123): same outcome —
an MCP server whose FIRST connect failed is retried by the gateway without a
config edit — with less machinery.

- `_mcp_config_reconciler` now calls `reconcile_mcp_servers_with_config()`
  every housekeeping tick after the baseline one, instead of gating on the
  config.yaml stat signature (plus a `pending` retry set). The reconcile is a
  signature-cached config read plus set compares when nothing moved, so the
  gate bought nothing but the bug: drift (a server enabled on disk, absent
  from `_servers`) never changed the signature.
- Dropped `mcp_servers_missing_from_live()`: it duplicated the `wanted -
  known` compare that `reconcile_mcp_servers_with_config()` already does.
- `reconcile_mcp_servers_with_config()` excludes servers inside their
  per-server connect cooldown from `added`, so a chronically failing server
  neither enters `discover_mcp_tools()` (cross-process discovery lock, up to
  120 s of waiting when another process holds it, a "(1 failed)" log line)
  every minute, nor is reported as "added" when nothing was attempted.
- A discovery-pass timeout now stamps the cooldown for the servers it
  strands: their attempt is still running on the MCP loop and the next
  reconcile tick would otherwise spawn a second one beside it.
- Tests trimmed to two invariants: the chore reconciles each tick after the
  baseline with OAuth suppressed (gateway); the reconcile skips a server in
  cooldown, retries it once the cooldown lapsed, and a timed-out pass stamps
  the cooldown (tools).

Fixes #112445
This commit is contained in:
teknium1
2026-09-16 10:07:20 -07:00
committed by Teknium
parent 31d3b948de
commit ea17e976cd
4 changed files with 82 additions and 113 deletions
Download Patch File Download Diff File Expand all files Collapse all files

57
gateway/run_profile_reconcile.py
View File

@@ -218,52 +218,29 @@ class GatewayProfileReconcileMixin:
def _mcp_config_reconciler(runner=None):
"""Housekeeping chore keeping live MCP servers in step with ``mcp_servers`` on disk: an entry
the user removed (or disabled) after boot must stop — a parked one otherwise self-probes every
``_PARKED_RETRY_INTERVAL`` for the life of the process. One ``stat`` per profile per tick; the
reconcile runs when ``config.yaml``'s signature changed, and again on the next tick while a
dropped server was still mid-connect (``pending``) and could not be torn down yet. Interactive
OAuth is suppressed — this runs on a housekeeping thread nobody is watching."""
from hermes_cli.config import get_config_path
seen: dict = {}
retry: set = set()
def _sig(path) -> tuple:
try:
st = os.stat(path)
return file_signature(st)
except OSError:
return (None, None, None, None)
"""Housekeeping chore keeping live MCP servers in step with ``mcp_servers`` on disk, every tick
after the first (startup discovery owns that one). Reconciling on DRIFT rather than only on a
config EDIT is what brings back a server whose FIRST connect failed (#112445): it never reached
``_servers``, so the parked self-probe — a property of a task that connected once — cannot revive
it, and its config never changes. The reconcile is a cached config read plus set compares when
nothing moved; a server dropped from config is torn down (a parked one otherwise self-probes
every ``_PARKED_RETRY_INTERVAL`` for the life of the process) and a missing one is reconnected
only once its per-server connect cooldown (30s→600s backoff) has lapsed, so a chronically failing
server is retried on that schedule, not every tick. Interactive OAuth is suppressed — this runs
on a housekeeping thread nobody is watching."""
primed: set = set()
def _reconcile_current(label: str) -> None:
from tools.mcp_oauth import suppress_interactive_oauth
from tools.mcp_tool_discovery import mcp_servers_missing_from_live, reconcile_mcp_servers_with_config
sig = _sig(get_config_path())
prev = seen.get(label)
seen[label] = sig
if prev is None:
return # first tick just records the baseline; startup discovery already ran
changed = prev != sig
# Also reconcile on DRIFT, not only on an edit: a server whose first connect failed never
# reached ``_servers``, its config never changes, and nothing else comes back for it — the
# parked self-probe belongs to a task that connected at least once. Without this a transient
# failure at boot (a cold ``npx`` start, a remote server mid-deploy, an OAuth prompt nobody
# can answer on a headless host) costs those tools for the life of the gateway, silently:
# the model simply does not have them. The check is a set compare, and the connect it may
# trigger is already rate-limited per server by the connect cooldown (30s→600s backoff), so
# a chronically failing server is retried on that schedule, not on every tick.
drifted = not changed and bool(mcp_servers_missing_from_live())
if label not in retry and not changed and not drifted:
return
from tools.mcp_tool_discovery import reconcile_mcp_servers_with_config
if label not in primed:
primed.add(label)
return # first tick: startup discovery already reflects this config (or is still running)
with suppress_interactive_oauth():
result = reconcile_mcp_servers_with_config()
retry.discard(label)
if result["pending"]:
retry.add(label)
if result["removed"] or result["added"]:
logger.info("MCP %s (%s): removed=%s added=%s",
"config changed" if changed else "server(s) missing", label,
result["removed"], result["added"])
logger.info("MCP servers reconciled with config (%s): removed=%s added=%s",
label, result["removed"], result["added"])
def _tick() -> None:
from gateway.run import _multiplex_profile_homes, _profile_runtime_scope

67
tests/gateway/test_gateway_mcp_oauth_unattended.py
View File

@@ -23,75 +23,32 @@ async def test_gateway_startup_discovery_suppresses_interactive_oauth(monkeypatc
assert seen == [False]
def test_mcp_config_reconciler_runs_only_when_config_changes(monkeypatch, tmp_path: Path):
def test_mcp_config_reconciler_reconciles_every_tick_after_baseline(monkeypatch, tmp_path: Path):
"""The chore reconciles on DRIFT, not only on a config edit (#112445): a server whose FIRST
connect failed never reached ``_servers`` and its config never changes, so a signature-gated
chore never came back for it. First tick is baseline only (startup discovery owns it); every
later tick reconciles with interactive OAuth suppressed; nothing to report stays silent."""
from gateway.run_profile_reconcile import _mcp_config_reconciler
from tools import mcp_tool_discovery as _mcp_discovery
from tools.mcp_oauth import _is_interactive
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
cfg = tmp_path / "config.yaml"
cfg.write_text("mcp_servers:\n linear:\n url: https://x/mcp\n")
(tmp_path / "config.yaml").write_text("mcp_servers:\n linear:\n url: https://x/mcp\n")
calls: list = []
added: list = ["linear"] # enabled in config, never connected, cooldown lapsed
def fake_reconcile():
calls.append(_is_interactive())
return {"removed": ["linear"], "added": [], "pending": pending.copy()}
return {"removed": [], "added": list(added), "pending": []}
pending: list = []
monkeypatch.setattr(_mcp_discovery, "reconcile_mcp_servers_with_config", fake_reconcile)
# Nothing is missing here: this test covers the config-EDIT trigger. The drift trigger
# (a configured server that never connected) is exercised below.
monkeypatch.setattr(_mcp_discovery, "mcp_servers_missing_from_live", lambda: [])
tick = _mcp_config_reconciler(runner=None)
tick() # baseline only: startup discovery already reflects this file
tick()
assert calls == []
cfg.write_text("model:\n default: x\n") # user removes the entry; size changes -> new signature
tick() # config.yaml untouched -- the missing server is still retried
tick()
assert calls == [False], "reconcile must run once per change, with interactive OAuth suppressed"
assert calls == [False, False], "reconcile runs each tick after the baseline, OAuth suppressed"
added.clear() # it connected: the chore keeps checking, cheaply, and has nothing to report
tick()
assert calls == [False]
cfg.write_text("model:\n default: y\n")
pending.append("linear") # dropped server was still mid-connect: retry next tick, unchanged file
tick()
pending.clear()
tick()
tick()
assert calls == [False, False, False], "one retry after a pending teardown, then quiet again"
def test_mcp_config_reconciler_retries_a_server_that_never_connected(monkeypatch, tmp_path: Path):
"""A server whose FIRST connect failed is retried without the config changing.
It never reached ``_servers``, so the parked self-probe — which belongs to a task that
connected at least once — cannot bring it back, and its config file never changes. Before
this, a transient failure at boot (cold ``npx`` start, remote server mid-deploy, an OAuth
prompt nobody can answer on a headless host) cost those tools for the life of the gateway.
"""
from gateway.run_profile_reconcile import _mcp_config_reconciler
from tools import mcp_tool_discovery as _mcp_discovery
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
cfg = tmp_path / "config.yaml"
cfg.write_text("mcp_servers:\n linear:\n url: https://x/mcp\n")
calls: list = []
missing: list = ["linear"] # enabled in config, never connected
monkeypatch.setattr(_mcp_discovery, "mcp_servers_missing_from_live", lambda: list(missing))
def fake_reconcile():
calls.append(list(missing))
return {"removed": [], "added": list(missing), "pending": []}
monkeypatch.setattr(_mcp_discovery, "reconcile_mcp_servers_with_config", fake_reconcile)
tick = _mcp_config_reconciler(runner=None)
tick() # baseline only, exactly as before: startup discovery is still authoritative
assert calls == []
tick()
assert calls == [["linear"]], "an enabled server that is not live must be retried"
missing.clear() # it connected
tick()
tick()
assert calls == [["linear"]], "and once it is live the chore goes quiet again"
assert calls == [False, False, False]

43
tests/tools/test_mcp_reconcile_with_config.py
View File

@@ -85,3 +85,46 @@ def test_disabled_lazy_and_connecting_entries(monkeypatch, tmp_path):
mcp_tool._server_connecting.discard("notion")
mcp_tool._lazy_server_configs.pop("asana", None)
mcp_tool._lazy_server_tool_names.pop("asana", None)
def test_reconcile_retries_failed_first_connect_only_after_cooldown(monkeypatch, tmp_path):
"""A configured server whose first connect failed is absent from ``_servers`` for good — no
task exists to park and self-probe — so the reconcile is its only reviver (#112445). It must
not enter discovery while the per-server connect cooldown is active (that takes the cross-process
discovery lock and logs a failed pass every tick), and must retry once the cooldown lapsed; a
discovery-pass timeout stamps that cooldown too, else the next tick spawns a second attempt
beside the one still running on the MCP loop."""
from tools import mcp_tool
from tools import mcp_tool_config as _config
from tools import mcp_tool_discovery as disc
from tools import mcp_tool_loop as _loop
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
monkeypatch.setattr(_config, "_load_mcp_config", lambda: {"ghost": {"url": "https://x/mcp"}})
discovered: list = []
monkeypatch.setattr(disc, "discover_mcp_tools", lambda *a, **k: discovered.append(1) or [])
try:
disc._note_connect_failure("ghost", RuntimeError("connection refused"))
assert disc.reconcile_mcp_servers_with_config()["added"] == [] and not discovered, \
"inside the cooldown nothing is attempted"
with mcp_tool._lock:
mcp_tool._server_connect_retry_after.clear()
assert disc.reconcile_mcp_servers_with_config()["added"] == ["ghost"]
assert discovered == [1], "cooldown lapsed: the never-connected server goes through discovery"
def _timeout(*a, **k):
raise TimeoutError("discovery bound")
monkeypatch.setattr(_loop, "_run_on_mcp_loop", _timeout)
with mcp_tool._lock:
mcp_tool._server_connecting.add("ghost")
with pytest.raises(TimeoutError):
disc._run_discovery_pass({"ghost": {"url": "https://x/mcp"}})
assert disc._connect_cooldown_active("ghost"), "a timed-out pass stamps the cooldown"
assert "ghost" not in mcp_tool._server_connecting
finally:
with mcp_tool._lock:
for store in (mcp_tool._server_connect_errors, mcp_tool._server_connect_failures,
mcp_tool._server_connect_retry_after, mcp_tool._server_scope_keys):
store.pop("ghost", None)
mcp_tool._server_connecting.discard("ghost")

28
tools/mcp_tool_discovery.py
View File

@@ -347,6 +347,9 @@ def _run_discovery_pass(new_servers: Dict[str, dict]) -> None:
_core._server_connecting.discard(_server_key(_sn))
_core._server_connect_errors.setdefault(
_server_key(_sn), f"Connection attempt {how} during discovery")
# Its attempt is still running on the MCP loop; without a cooldown the next
# reconcile tick would spawn a second one beside it.
_record_connect_failure(_sn)
raise
finally:
if _was_interrupted:
@@ -493,7 +496,8 @@ def reconcile_mcp_servers_with_config() -> Dict[str, List[str]]:
"""Bring the live server set in step with ``mcp_servers`` as it is on disk NOW: tear down
servers that were removed from config or set ``enabled: false`` (a parked server keeps
self-probing forever otherwise — for hours after the user deleted its entry), then connect
anything newly configured via :func:`discover_mcp_tools`. Scoped to the current registry
anything enabled that is not live via :func:`discover_mcp_tools` — newly configured, or one
whose earlier connect failed and whose cooldown has lapsed. Scoped to the current registry
scope (one multiplexed profile's config prunes only its own connections). A lazily registered
(schema-cache) server loses its cached tools; one still mid-connect cannot be torn down yet and
is reported under ``"pending"`` so the caller retries. Returns
@@ -517,29 +521,17 @@ def reconcile_mcp_servers_with_config() -> Dict[str, List[str]]:
known = {_key_name(key) for key, owner in _core._server_scope_keys.items()
if owner == scope and (key in _core._servers or key in _core._server_connecting)}
known |= {_key_name(key) for key in _core._lazy_server_configs}
added = sorted(wanted - known)
# A configured server that is not live is retried here — this is the only reviver for one whose
# FIRST connect failed (#112445) — but only once its connect cooldown lapsed: ``discover_mcp_tools``
# would skip it anyway, and entering it takes the cross-process discovery lock (up to 120 s of
# waiting when another process holds it) and logs a failed pass, every tick, for nothing.
added = sorted(name for name in wanted - known if not _connect_cooldown_active(name))
if added:
discover_mcp_tools()
return {"removed": stale + sorted(_key_name(k) for k in lazy), "added": added,
"pending": sorted(connecting - wanted)}
def mcp_servers_missing_from_live() -> List[str]:
"""Names this scope's config enables that are neither live nor mid-connect — the set
:func:`reconcile_mcp_servers_with_config` would connect, computed without connecting anything
(the mtime-cached config read plus one locked set compare). Lets a caller reconcile on DRIFT and
not only on a config EDIT: a server whose first connect failed never reached ``_servers``, and
nothing retries it on its own — a parked server self-probes, one that never connected cannot."""
servers = _config._load_mcp_config()
wanted = {name for name, cfg in servers.items() if _enabled(cfg)}
scope = _core._mcp_registry_scope()
with _core._lock:
known = {_key_name(key) for key, owner in _core._server_scope_keys.items()
if owner == scope and (key in _core._servers or key in _core._server_connecting)}
known |= {_key_name(key) for key in _core._lazy_server_configs}
return sorted(wanted - known)
def _forget_lazy_server(key) -> None:
"""Drop a schema-cache (lazy) registration whose config entry is gone: its cached tools would
otherwise stay callable and spawn the server on first use."""