diff --git a/api/config.py b/api/config.py index 3a23bb7ec..2c60945e2 100644 --- a/api/config.py +++ b/api/config.py @@ -440,6 +440,19 @@ def _apply_config_defaults(config_data: dict) -> None: for key, value in _DEFAULT_EXPERIMENTAL_CONFIG.items(): experimental.setdefault(key, value) + +def reload_config_if_stale() -> None: + """Refresh config.yaml once for concurrent stale read paths.""" + with _cfg_lock: + try: + config_path = _get_config_path() + current_mtime = config_path.stat().st_mtime + except OSError: + current_mtime = 0.0 + cache_stale = current_mtime != _cfg_mtime or _cfg_path != config_path + if not _cfg_cache or (cache_stale and not _cfg_has_in_memory_overrides()): + _refresh_config_cache(config_path) + def get_config() -> dict: """Return the cached config dict, loading from disk if needed.""" @@ -450,15 +463,7 @@ def get_config() -> dict: current_mtime = 0.0 cache_stale = current_mtime != _cfg_mtime or _cfg_path != config_path if not _cfg_cache or (cache_stale and not _cfg_has_in_memory_overrides()): - with _cfg_lock: - try: - config_path = _get_config_path() - current_mtime = config_path.stat().st_mtime - except OSError: - current_mtime = 0.0 - cache_stale = current_mtime != _cfg_mtime or _cfg_path != config_path - if not _cfg_cache or (cache_stale and not _cfg_has_in_memory_overrides()): - _refresh_config_cache(config_path) + reload_config_if_stale() # When a test (or runtime caller) has rebound ``cfg`` to a different dict # via monkeypatch.setattr(config, "cfg", ...), return that override rather # than the underlying _cfg_cache. Without this branch, get_config() would @@ -5561,7 +5566,7 @@ def get_available_models(*, prefer_cache: bool = False, force_refresh: bool = Fa (_current_mtime != _cfg_mtime or _current_path != _cfg_path) and not _cfg_has_in_memory_overrides() ): - reload_config() + reload_config_if_stale() # ── COLD PATH helper ───────────────────────────────────────────────────── # Extracted so it runs inside _available_models_cache_lock (RLock) to # prevent thundering-herd: only one thread rebuilds while others wait. diff --git a/tests/test_issue5220_config_reload_stampede.py b/tests/test_issue5220_config_reload_stampede.py index e8efd7d54..2db335f1d 100644 --- a/tests/test_issue5220_config_reload_stampede.py +++ b/tests/test_issue5220_config_reload_stampede.py @@ -1,7 +1,9 @@ """Regression coverage for #5220 config reload stampede collapse.""" +import sys import threading import time +import types from concurrent.futures import ThreadPoolExecutor @@ -22,17 +24,23 @@ def test_stale_readers_collapse_to_single_reload_when_config_is_hot_reload(tmp_p calls = {"n": 0} calls_lock = threading.Lock() - real_load = config._load_yaml_config_file_raw + real_refresh = config._refresh_config_cache - def _counted_load(path): + def _counted_refresh(path=None): with calls_lock: calls["n"] += 1 call_no = calls["n"] if call_no == 1: time.sleep(0.1) - return real_load(path) + return real_refresh(path) - monkeypatch.setattr(config, "_load_yaml_config_file_raw", _counted_load) + def _unexpected_reload(): + raise AssertionError( + "stale model readers should route through reload_config_if_stale() instead of forced reload_config()" + ) + + monkeypatch.setattr(config, "_refresh_config_cache", _counted_refresh) + monkeypatch.setattr(config, "reload_config", _unexpected_reload) def _get(): cfg = config.get_config() @@ -79,3 +87,72 @@ def test_explicit_reload_config_forces_disk_refresh(tmp_path, monkeypatch): assert calls["n"] == 1, ( "explicit reload_config() should refresh from disk even when caller requests it" ) + + +def test_stale_model_readers_collapse_to_single_reload_when_models_are_requested(tmp_path, monkeypatch): + import api.config as config + + config_path = tmp_path / "config.yaml" + config_path.write_text( + "model:\n provider: openai\n default: cold-model\n", + encoding="utf-8", + ) + monkeypatch.setattr(config, "_get_config_path", lambda: config_path) + with config._yaml_file_cache_lock: + config._yaml_file_cache.clear() + config.invalidate_models_cache() + + fake_pkg = types.ModuleType("hermes_cli") + fake_pkg.__path__ = [] + fake_models = types.ModuleType("hermes_cli.models") + fake_models._PROVIDER_ALIASES = {} + fake_models.list_available_providers = lambda: [] + fake_auth = types.ModuleType("hermes_cli.auth") + fake_auth.get_auth_status = lambda provider_id: {"logged_in": False, "key_source": ""} + monkeypatch.setitem(sys.modules, "hermes_cli", fake_pkg) + monkeypatch.setitem(sys.modules, "hermes_cli.models", fake_models) + monkeypatch.setitem(sys.modules, "hermes_cli.auth", fake_auth) + monkeypatch.setattr(config, "_load_models_cache_from_disk", lambda: None) + monkeypatch.setattr(config, "_load_stale_models_cache_from_disk", lambda: None) + monkeypatch.setattr(config, "_save_models_cache_to_disk", lambda _data: None) + monkeypatch.setattr(config, "_models_cache_source_fingerprint", lambda: {"config": "test"}) + + config.reload_config() + old_mtime = config._cfg_mtime + config_path.write_text( + "model:\n provider: openai\n default: refreshed-model\n", + encoding="utf-8", + ) + config._cfg_mtime = old_mtime - 1.0 + config.invalidate_models_cache() + + calls = {"n": 0} + calls_lock = threading.Lock() + real_refresh = config._refresh_config_cache + + def _counted_refresh(path=None): + with calls_lock: + calls["n"] += 1 + call_no = calls["n"] + if call_no == 1: + time.sleep(0.1) + return real_refresh(path) + + def _unexpected_reload(): + raise AssertionError( + "stale model readers should route through reload_config_if_stale() instead of forced reload_config()" + ) + + monkeypatch.setattr(config, "_refresh_config_cache", _counted_refresh) + monkeypatch.setattr(config, "reload_config", _unexpected_reload) + + with ThreadPoolExecutor(max_workers=6) as executor: + results = list(executor.map(lambda _idx: config.get_available_models(), range(6))) + + assert all(result.get("default_model") == "refreshed-model" for result in results), ( + f"stale model readers should observe refreshed config, got {results!r}" + ) + assert calls["n"] == 1, ( + f"expected one config refresh for concurrent model reads, got {calls['n']} " + "(base would re-enter refresh work per stale reader)" + )