fix(#4662): stamp _cfg_mtime for empty-dict config (Opus gate finding)
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.
This commit is contained in:
parent
6ddd519ba9
commit
001ee0a986
2 changed files with 74 additions and 24 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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)"
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue