From 0a01b2087d0c0bb11bb7ed650b8de3e6b5205856 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 9 Jul 2026 02:10:41 +0530 Subject: [PATCH] fix(gateway): harden fallback-chain refresh from review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups on the #60987 salvage (review pass): - _refresh_fallback_model: keep last known-good chain on transient config.yaml read/parse failure (user mid-edit, torn write) — only a successful read that lacks the key clears the chain. Previously a refresh error wiped a cached agent's working fallback for the turn. - Move the cached-agent refresh+apply OUTSIDE the agent-cache lock: config.yaml read is disk I/O and the idle-sweep watcher contends on that lock (same reasoning as #52197). Per-session turn serialization keeps the post-lock apply safe. - _apply_fallback_chain_to_agent: clear _unavailable_fallback_keys when chain content actually changes, so an entry re-configured mid-uptime (e.g. credentials added) is retried instead of staying suppressed for the cached agent's lifetime; no-op refreshes keep the memo. - Tests: cwd-independent source pin (Path(__file__) anchor), pin the reuse-path apply call, + regression tests for last-known-good, memo clear-on-change, memo keep-on-unchanged (mutation-verified). --- gateway/run.py | 52 ++++++++++++-- tests/gateway/test_fallback_chain_reload.py | 80 ++++++++++++++++++++- 2 files changed, 122 insertions(+), 10 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index fce858c50ed..54f38d1bb79 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5016,8 +5016,28 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew configured (or changed) after ``hermes gateway`` was running never reached messaging sessions even though the same process's cron jobs fell back correctly. Fixes #60955. + + A TRANSIENT read/parse failure (user mid-edit of config.yaml with a + non-atomic write) keeps the last known-good chain instead of wiping a + cached agent's working fallback for that turn. Only a successful read + that genuinely lacks the key clears the chain. """ - self._fallback_model = self._load_fallback_model() + try: + import yaml as _y + cfg_path = _hermes_home / "config.yaml" + if not cfg_path.exists(): + self._fallback_model = None + return self._fallback_model + with open(cfg_path, encoding="utf-8") as _f: + cfg = _y.safe_load(_f) or {} + except Exception: + # Transient failure — keep last known-good chain. + logger.debug( + "fallback_providers refresh: config.yaml read failed; " + "keeping last known-good chain", exc_info=True, + ) + return self._fallback_model + self._fallback_model = get_fallback_chain(cfg) or None return self._fallback_model @staticmethod @@ -5039,10 +5059,22 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew and rate_limited_until > time.monotonic() ): return + old_chain = list(getattr(agent, "_fallback_chain", []) or []) agent._fallback_chain = new_chain agent._fallback_model = new_chain[0] if new_chain else None if not getattr(agent, "_fallback_activated", False): agent._fallback_index = 0 + # A config edit signals the user changed something — drop the + # session-scoped unavailability memo so re-configured entries + # (e.g. credentials added mid-uptime for a previously-failing + # provider) get retried instead of staying suppressed for the + # cached agent's lifetime. Only on actual content change, so + # the per-message no-op refresh keeps the memo's rate-limiting + # benefit (#60955). + if new_chain != old_chain: + unavailable = getattr(agent, "_unavailable_fallback_keys", None) + if unavailable: + unavailable.clear() def _snapshot_running_agents(self) -> Dict[str, Any]: return { @@ -18099,15 +18131,21 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew # Refresh agent max_iterations from current config # (cached agent may have been created with old config) agent.max_iterations = max_iterations - # Refresh fallback chain the same way — a chain - # configured after this agent was cached (or after - # gateway start) must reach the next turn (#60955). - self._apply_fallback_chain_to_agent( - agent, self._refresh_fallback_model(), - ) logger.debug("Reusing cached agent for session %s", session_key) reused_cached_agent = True + # Lock released — refresh the fallback chain from disk for the + # reused agent OUTSIDE the cache lock (config.yaml read is disk + # I/O; the idle-sweep watcher contends on this lock and stalls + # Discord heartbeats — same reasoning as #52197). A chain + # configured after this agent was cached (or after gateway start) + # must reach the next turn (#60955). Per-session turn + # serialization (_running_agents) keeps this safe post-lock. + if reused_cached_agent and agent is not None: + self._apply_fallback_chain_to_agent( + agent, self._refresh_fallback_model(), + ) + # Lock released — now schedule cleanup of any cross-process-evicted # agent on a daemon thread so memory-provider shutdown / socket # teardown never blocks the gateway event loop or the cache lock diff --git a/tests/gateway/test_fallback_chain_reload.py b/tests/gateway/test_fallback_chain_reload.py index a9d8cf1255a..5547db7e263 100644 --- a/tests/gateway/test_fallback_chain_reload.py +++ b/tests/gateway/test_fallback_chain_reload.py @@ -71,6 +71,34 @@ def test_refresh_fallback_model_clears_when_config_removed(tmp_path, monkeypatch assert runner._fallback_model is None +def test_refresh_fallback_model_keeps_last_known_good_on_read_failure( + tmp_path, monkeypatch, +): + """A transient config.yaml read/parse failure (user mid-edit, non-atomic + write) must NOT wipe the last known-good chain — only a successful read + that genuinely lacks the key clears it.""" + from gateway.run import GatewayRunner + + monkeypatch.setattr("gateway.run._hermes_home", tmp_path) + cfg = tmp_path / "config.yaml" + cfg.write_text( + "fallback_providers:\n" + " - provider: deepseek\n" + " model: deepseek-v4-flash\n" + ) + + runner = SimpleNamespace(_fallback_model=None) + runner._load_fallback_model = GatewayRunner._load_fallback_model + bound = GatewayRunner._refresh_fallback_model.__get__(runner) + good = bound() + assert good == [{"provider": "deepseek", "model": "deepseek-v4-flash"}] + + # Simulate a mid-edit torn write: invalid YAML. + cfg.write_text("fallback_providers:\n - provider: [unclosed\n") + assert bound() == good + assert runner._fallback_model == good + + def test_apply_fallback_chain_updates_primary_agent(): from gateway.run import GatewayRunner @@ -130,16 +158,62 @@ def test_apply_fallback_chain_updates_after_cooldown_expires(): assert agent._fallback_index == 1 +def test_apply_fallback_chain_clears_unavailable_memo_on_content_change(): + """A config edit must drop the session-scoped unavailability memo so a + re-configured entry (credentials added mid-uptime) is retried instead of + staying suppressed for the cached agent's lifetime.""" + from gateway.run import GatewayRunner + + agent = SimpleNamespace( + _fallback_chain=[{"provider": "deepseek", "model": "old"}], + _fallback_model={"provider": "deepseek", "model": "old"}, + _fallback_index=0, + _fallback_activated=False, + _rate_limited_until=0, + _unavailable_fallback_keys={("deepseek", "old", "")}, + ) + new_chain = [{"provider": "deepseek", "model": "deepseek-v4-flash"}] + GatewayRunner._apply_fallback_chain_to_agent(agent, new_chain) + + assert agent._fallback_chain == new_chain + assert agent._unavailable_fallback_keys == set() + + +def test_apply_fallback_chain_keeps_unavailable_memo_when_unchanged(): + """The per-message no-op refresh must NOT clear the memo — it exists to + rate-limit repeated activation attempts against dead entries.""" + from gateway.run import GatewayRunner + + chain = [{"provider": "deepseek", "model": "deepseek-v4-flash"}] + memo = {("deepseek", "deepseek-v4-flash", "")} + agent = SimpleNamespace( + _fallback_chain=list(chain), + _fallback_model=chain[0], + _fallback_index=0, + _fallback_activated=False, + _rate_limited_until=0, + _unavailable_fallback_keys=set(memo), + ) + GatewayRunner._apply_fallback_chain_to_agent(agent, list(chain)) + + assert agent._unavailable_fallback_keys == memo + + def test_background_and_main_agent_paths_call_refresh(): """Both AIAgent construction sites must pass a refreshed chain, not the - startup snapshot. Source-level invariant — mirrors - tests/agent/test_gemini_fast_fallback.py's call-site pin. + startup snapshot, and the cached-agent reuse path must apply the refreshed + chain. Source-level invariant for call sites that resist unit testing. """ from pathlib import Path - source = Path("gateway/run.py").read_text(encoding="utf-8") + source = ( + Path(__file__).resolve().parent.parent.parent / "gateway" / "run.py" + ).read_text(encoding="utf-8") assert "fallback_model=self._refresh_fallback_model()" in source assert source.count("fallback_model=self._refresh_fallback_model()") >= 2 + # The cached-agent reuse path (the load-bearing fix for a long-lived + # session in a running gateway) must apply the refreshed chain. + assert "self._apply_fallback_chain_to_agent(" in source # The stale startup-snapshot form must not remain at create sites. assert "fallback_model=self._fallback_model," not in source