From 001ee0a9864d3d07e521cb67ebeece64f60b4eeb Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 23 Jun 2026 02:52:05 +0000 Subject: [PATCH] fix(#4662): stamp _cfg_mtime for empty-dict config (Opus gate finding) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'and loaded' guard skipped the _cfg_mtime stamp for a config that parses to an empty {} dict, so get_config()'s current_mtime != _cfg_mtime stale check fired on every call and re-entered reload_config() under _cfg_lock forever — a hot-path spin for a freshly created/reset profile's {} config. Split the guard: the no-op _cfg_cache.update is still skipped for {}, but _cfg_mtime is always stamped when the file parsed to a dict, matching master's pre-#4662 behavior. Incidentally fixes the pre-existing empty/None-config spin. Adds a regression test. --- api/config.py | 58 +++++++++++-------- ...est_issue4662_reload_config_mtime_cache.py | 40 +++++++++++++ 2 files changed, 74 insertions(+), 24 deletions(-) diff --git a/api/config.py b/api/config.py index 58aa3e8b..32ebc5ce 100644 --- a/api/config.py +++ b/api/config.py @@ -494,30 +494,40 @@ def reload_config() -> None: # view (below) — never the helper's per-call expansion — for the # #798 TLS reason documented in the pin block. loaded = _load_yaml_config_file_raw(config_path) - if isinstance(loaded, dict) and loaded: - # The process-global _cfg_cache must reflect PROCESS-env - # expansion, never a profile-scoped block_process_env_fallback - # view — otherwise a reload that fires while a readonly/worker - # scope is active (profile alternation resolves _get_config_path - # to the named profile, #798 TLS) would bake under-expanded - # literal ${VAR}s into the shared cache and starve concurrent - # readers of the module-level `cfg` alias. Expansion re-runs - # per-read elsewhere; here we pin the cache to the unscoped view. - _prev_block = getattr(_thread_ctx, "block_process_env_fallback", False) - _prev_env = getattr(_thread_ctx, "env", None) - try: - _thread_ctx.block_process_env_fallback = False - _thread_ctx.env = {} - _cfg_cache.update(_expand_env_vars(loaded)) - finally: - _thread_ctx.block_process_env_fallback = _prev_block - if _prev_env is None: - try: - del _thread_ctx.env - except AttributeError: - pass - else: - _thread_ctx.env = _prev_env + if isinstance(loaded, dict): + if loaded: + # The process-global _cfg_cache must reflect PROCESS-env + # expansion, never a profile-scoped block_process_env_fallback + # view — otherwise a reload that fires while a readonly/worker + # scope is active (profile alternation resolves _get_config_path + # to the named profile, #798 TLS) would bake under-expanded + # literal ${VAR}s into the shared cache and starve concurrent + # readers of the module-level `cfg` alias. Expansion re-runs + # per-read elsewhere; here we pin the cache to the unscoped view. + _prev_block = getattr(_thread_ctx, "block_process_env_fallback", False) + _prev_env = getattr(_thread_ctx, "env", None) + try: + _thread_ctx.block_process_env_fallback = False + _thread_ctx.env = {} + _cfg_cache.update(_expand_env_vars(loaded)) + finally: + _thread_ctx.block_process_env_fallback = _prev_block + if _prev_env is None: + try: + del _thread_ctx.env + except AttributeError: + pass + else: + _thread_ctx.env = _prev_env + # Stamp _cfg_mtime whenever the file parsed to a dict — INCLUDING + # an empty {} config. The cache-update above is skipped for {} (it's + # a no-op), but _cfg_mtime MUST still be set or get_config()'s + # `current_mtime != _cfg_mtime` stale check fires on every call and + # spins reload_config() under _cfg_lock forever (a `{}` config from a + # freshly created/reset profile is reachable on the switch hot path). + # This matches master's pre-#4662 behavior (it entered the block for + # {} and set the mtime); the inner `if loaded:` only gates the no-op + # cache update, not the mtime stamp. try: _cfg_mtime = Path(config_path).stat().st_mtime except OSError: diff --git a/tests/test_issue4662_reload_config_mtime_cache.py b/tests/test_issue4662_reload_config_mtime_cache.py index 4ff67f57..875f80c8 100644 --- a/tests/test_issue4662_reload_config_mtime_cache.py +++ b/tests/test_issue4662_reload_config_mtime_cache.py @@ -76,3 +76,43 @@ def test_reload_config_expands_env_vars(tmp_path, monkeypatch): cfg.reload_config() key = ((cfg.get_config().get("providers") or {}).get("openai") or {}).get("api_key") assert key == "expanded-value-xyz", f"env var not expanded after reload: {key!r}" + + +def test_reload_config_empty_dict_config_does_not_spin(tmp_path, monkeypatch): + """An empty ``{}`` config must still stamp _cfg_mtime, or get_config()'s + `current_mtime != _cfg_mtime` stale check fires forever and re-enters + reload_config() under _cfg_lock on every call. The cache-update is correctly + skipped for {} (no-op), but the mtime stamp must not be. (Opus gate finding — + a {} config is reachable on the profile-switch hot path via a freshly + created/reset profile, and this also fixes the pre-existing empty/None-config + spin on master.) + """ + import api.config as cfg + + config_path = tmp_path / "config.yaml" + config_path.write_text("{}\n", encoding="utf-8") + monkeypatch.setattr(cfg, "_get_config_path", lambda: config_path) + with cfg._yaml_file_cache_lock: + cfg._yaml_file_cache.clear() + + cfg.reload_config() + # _cfg_mtime must equal the file's real mtime, not 0.0. + assert cfg._cfg_mtime == config_path.stat().st_mtime, ( + f"_cfg_mtime not stamped for empty-dict config (got {cfg._cfg_mtime!r}); " + "get_config() would spin reload_config() forever" + ) + + # And get_config() must NOT re-enter reload_config() on subsequent calls. + reloads = {"n": 0} + real_reload = cfg.reload_config + + def _counting_reload(): + reloads["n"] += 1 + return real_reload() + + monkeypatch.setattr(cfg, "reload_config", _counting_reload) + cfg.get_config() + cfg.get_config() + cfg.get_config() + assert reloads["n"] == 0, f"get_config() re-entered reload_config() {reloads['n']}x on a stable {{}} config (spin)" +