From ea17e976cd43cc74c153a1babf70dc4579e14d07 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:07:20 -0700 Subject: [PATCH] fix(mcp): retry a never-connected server through the existing reconcile seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- gateway/run_profile_reconcile.py | 57 +++++----------- .../test_gateway_mcp_oauth_unattended.py | 67 ++++--------------- tests/tools/test_mcp_reconcile_with_config.py | 43 ++++++++++++ tools/mcp_tool_discovery.py | 28 +++----- 4 files changed, 82 insertions(+), 113 deletions(-) diff --git a/gateway/run_profile_reconcile.py b/gateway/run_profile_reconcile.py index f5a890bc11..d0da7826ee 100644 --- a/gateway/run_profile_reconcile.py +++ b/gateway/run_profile_reconcile.py @@ -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 diff --git a/tests/gateway/test_gateway_mcp_oauth_unattended.py b/tests/gateway/test_gateway_mcp_oauth_unattended.py index 81d7ae1ef2..1b94b4b463 100644 --- a/tests/gateway/test_gateway_mcp_oauth_unattended.py +++ b/tests/gateway/test_gateway_mcp_oauth_unattended.py @@ -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] diff --git a/tests/tools/test_mcp_reconcile_with_config.py b/tests/tools/test_mcp_reconcile_with_config.py index 50b5bf334b..0a382b8a5c 100644 --- a/tests/tools/test_mcp_reconcile_with_config.py +++ b/tests/tools/test_mcp_reconcile_with_config.py @@ -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") diff --git a/tools/mcp_tool_discovery.py b/tools/mcp_tool_discovery.py index 572efb1894..1a50013996 100644 --- a/tools/mcp_tool_discovery.py +++ b/tools/mcp_tool_discovery.py @@ -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."""