From 1f4e70a5d42d2c868d72a6455ba02b0e4eab9148 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 21 Jun 2026 21:45:13 -0700 Subject: [PATCH] stage #4652 (nesquena-hermes): stop /api/reasoning storm + memoize YAML config parse (fixes #4650) (#4659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two-layer perf fix for the /api/reasoning storm dropping streaming to ~2 tok/s: - ui.js: syncReasoningChip() serves the cached chip on routine topbar syncs, only re-fetching when the model/provider key (_lastReasoningFetchKey) changes — restores pre-a9ce2889 behavior while keeping its model-switch-refresh intent. - config.py: memoize _load_yaml_config_file keyed on (path, st_mtime_ns, st_size) under a lock; cache the RAW parsed dict and re-run cheap _expand_env_vars per call (cache stays correct vs os.environ and read-modify-save callers get a fresh structure; on-disk edits picked up via mtime/size). Code byte-faithful from PR head f868f10a1ad7. 9 own tests pass. Co-authored-by: nesquena-hermes --- CHANGELOG.md | 6 + api/config.py | 45 +++- static/messages.js | 7 + static/panels.js | 14 ++ static/ui.js | 52 +++- ...est_issue1103_reasoning_chip_visibility.py | 17 +- .../test_issue4650_reasoning_chip_no_storm.py | 227 ++++++++++++++++++ tests/test_issue4650_yaml_config_cache.py | 135 +++++++++++ 8 files changed, 498 insertions(+), 5 deletions(-) create mode 100644 tests/test_issue4650_reasoning_chip_no_storm.py create mode 100644 tests/test_issue4650_yaml_config_cache.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 30b1e8e98..2bdc12281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ ## [Unreleased] +## [v0.51.573] — 2026-06-22 — Release UF (reasoning request-storm fix) + +### Fixed + +- **Fixed a streaming slowdown caused by a `/api/reasoning` request storm.** A recent change made the reasoning-effort chip re-fetch `/api/reasoning` on every routine topbar sync; during streaming those fire at high frequency, so output could drop to ~2 tokens/sec under the request flood. The chip now serves its cached state on routine syncs and only re-fetches when the model/provider identity actually changes (the only input that changes the endpoint's answer), restoring fast streaming. As defense-in-depth, the server also memoizes the `config.yaml` parse (keyed on file mtime+size; ~125 ms per parse on a large config) so a read storm can't turn into a YAML-reparse storm. Fixes #4650. + ## [v0.51.572] — 2026-06-22 — Release UE (in-chat cronjob profile pinning) ### Fixed diff --git a/api/config.py b/api/config.py index 25fb5a14e..9c769b59f 100644 --- a/api/config.py +++ b/api/config.py @@ -467,21 +467,56 @@ def reload_config() -> None: _delete_models_cache_on_disk() +# Memoized parse cache for _load_yaml_config_file, keyed on (resolved path, +# st_mtime_ns, st_size). yaml.safe_load on an ~800-line / 24KB config.yaml costs +# ~125ms of pure-Python parsing, and hot read paths (e.g. GET /api/reasoning -> +# get_reasoning_status) call this on every request. Without a cache, a UI sync +# storm turns into a YAML-reparse storm (#4650). We cache the RAW parsed dict and +# re-run _expand_env_vars() on every call: env expansion is cheap, always returns +# a fresh structure (so callers that read-modify-save the result never corrupt the +# cache), and keeps ${VAR} references live against the current os.environ. The +# (mtime_ns, size) key means any on-disk edit (including by _save_yaml_config_file) +# is picked up on the next read. +_yaml_file_cache: dict[str, tuple] = {} +_yaml_file_cache_lock = threading.Lock() + + def _load_yaml_config_file(config_path: Path) -> dict: try: import yaml as _yaml except ImportError: return {} - if not config_path.exists(): + try: + st = config_path.stat() + except OSError: + # Missing or unstattable file — preserve the original "no config" contract. return {} + + cache_key = str(config_path) + stat_key = (st.st_mtime_ns, st.st_size) + with _yaml_file_cache_lock: + cached = _yaml_file_cache.get(cache_key) + if cached is not None and cached[0] == stat_key: + expanded = _expand_env_vars(cached[1]) + return expanded if isinstance(expanded, dict) else {} + + # Cache miss / stale: parse off disk. Done outside the lock so a slow parse + # doesn't serialize unrelated paths; a concurrent duplicate parse is harmless. try: loaded = _yaml.safe_load(config_path.read_text(encoding="utf-8")) - return _expand_env_vars(loaded) if isinstance(loaded, dict) else {} except Exception: logger.debug("Failed to parse yaml config from %s", config_path) return {} + raw = loaded if isinstance(loaded, dict) else {} + with _yaml_file_cache_lock: + _yaml_file_cache[cache_key] = (stat_key, raw) + if not raw: + return {} + expanded = _expand_env_vars(raw) + return expanded if isinstance(expanded, dict) else {} + def get_config_for_profile_home(profile_home: "Path | str | None") -> dict: """Return the config dict for an explicit profile home directory. @@ -564,6 +599,12 @@ def _save_yaml_config_file(config_path: Path, config_data: dict) -> None: _yaml.safe_dump(_config_for_yaml_save(config_data), sort_keys=False, allow_unicode=True), encoding="utf-8", ) + # Invalidate the memoized parse for this path so the next read re-parses the + # bytes we just wrote. mtime_ns+size keying normally catches edits, but a + # WebUI save that preserves size with a coarse/unchanged mtime could otherwise + # serve a stale dict (#4650 review) — evicting on our own write closes that gap. + with _yaml_file_cache_lock: + _yaml_file_cache.pop(str(config_path), None) # Initial load diff --git a/static/messages.js b/static/messages.js index f341a6d06..d69fa1118 100644 --- a/static/messages.js +++ b/static/messages.js @@ -4300,6 +4300,13 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ if(typeof _messageRenderableMessageCount==='function'&&typeof _messageRenderWindowSize!=='undefined'){ _messageRenderWindowSize=Math.max(typeof _currentMessageRenderWindowSize==='function'?_currentMessageRenderWindowSize():50, _messageRenderableMessageCount()); } + // #4650 review: the agent turn that just completed may have changed + // server-side reasoning config (e.g. a `/reasoning ` slash + // command writes agent.reasoning_effort) WITHOUT changing the model/ + // provider cache key. Invalidate the reasoning-chip cache once at the + // turn boundary so the following syncTopbar() refetches the authoritative + // effort exactly once (not per-token — the storm short-circuit is intact). + if(typeof _lastReasoningFetchKey!=='undefined') _lastReasoningFetchKey=null; syncTopbar();renderMessages({preserveScroll:true}); if(shouldFollowOnDone&&typeof scrollToBottom==='function') scrollToBottom(); if(typeof noteWorkspaceMutationsFromToolCalls==='function') noteWorkspaceMutationsFromToolCalls(S.toolCalls); diff --git a/static/panels.js b/static/panels.js index 107a9bb4e..dc7ab2aa2 100644 --- a/static/panels.js +++ b/static/panels.js @@ -5899,6 +5899,12 @@ async function switchToProfile(name) { if (_switchGen !== _profileSwitchGeneration) return; S.activeProfile = data.active || name; S.activeProfileIsDefault = !!data.is_default; + // #4650 review: a profile switch can change agent.reasoning_effort (and other + // reasoning inputs like base_url) WITHOUT changing the default model/provider, + // which is all the reasoning-chip cache key tracks. Force exactly one reasoning + // refetch for the new profile so the chip reflects the new profile's effort + // (the syncTopbar() calls below route through syncReasoningChip()). + if (typeof _lastReasoningFetchKey !== 'undefined') _lastReasoningFetchKey = null; // Reconnect the gateway SSE to the NEW profile's watcher. The backend watcher // registry is now profile-keyed (#3629), but this tab's existing EventSource is @@ -9226,6 +9232,14 @@ function _openAuxAdvancedOptions(taskKey,cfg){ if(typeof showToast==='function') showToast(isMain?(t('settings_main_advanced_saved')||'Main model options saved'):(t('settings_aux_advanced_saved')||'Auxiliary options saved')); overlay.style.display='none'; _loadAuxiliaryModels(); + // #4650 review: a main-model advanced save can change base_url, which + // /api/reasoning's answer depends on for some providers (e.g. LM Studio), + // WITHOUT changing the model/provider cache key. Invalidate the reasoning + // cache and refresh so the chip reflects the new config (one refetch). + if(isMain){ + if(typeof _lastReasoningFetchKey!=='undefined') _lastReasoningFetchKey=null; + if(typeof fetchReasoningChip==='function') fetchReasoningChip(); + } }catch(e){ if(typeof showToast==='function') showToast(isMain?(t('settings_main_advanced_save_failed')||'Failed to save main model options'):(t('settings_aux_advanced_save_failed')||'Failed to save auxiliary options')); } diff --git a/static/ui.js b/static/ui.js index b471a472a..cd3061849 100644 --- a/static/ui.js +++ b/static/ui.js @@ -3069,13 +3069,61 @@ function _applyReasoningChip(eff){ _highlightReasoningOption(effort); } +// Tracks the model/provider identity of the last reasoning fetch so routine +// topbar syncs can serve the cached chip state instead of re-hitting the +// network. null = never fetched. +let _lastReasoningFetchKey=null; +// Monotonic dispatch counter. Each fetchReasoningChip() increments it and the +// async handlers capture their own value; a response (success OR failure) only +// applies if it is still the most recent dispatch. This defeats out-of-order +// resolution even when two fetches share the same model/provider key (e.g. a +// profile switch that resets the cache and refetches the same default model but +// a different agent.reasoning_effort) — #4650 review. +let _reasoningFetchSeq=0; + function fetchReasoningChip(){ - api('/api/reasoning'+_reasoningEffortQuery()).then(function(st){ + // Set the cache key OPTIMISTICALLY before the request so rapid routine syncs + // while this GET is in flight short-circuit instead of re-dispatching (that + // in-flight window is exactly where the #4650 storm lived). + const key=_reasoningEffortQuery(); + const seq=++_reasoningFetchSeq; + _lastReasoningFetchKey=key; + api('/api/reasoning'+key).then(function(st){ + // Ignore a stale/superseded response: only the most recent dispatch may + // apply, so an older in-flight GET can't poison the current chip (#4650). + if(seq!==_reasoningFetchSeq) return; _applyReasoningChip((st&&st.reasoning_effort)||'', st||{}); - }).catch(function(){_applyReasoningChip('', {supported_efforts:[]});}); + }).catch(function(){ + // Same staleness guard on failure: a stale error must neither hide the chip + // nor clear a newer fetch's key. Only the latest dispatch clears the key so + // routine syncs retry after a genuine transient failure. + if(seq!==_reasoningFetchSeq) return; + _lastReasoningFetchKey=null; + _applyReasoningChip('', {supported_efforts:[]}); + }); } function syncReasoningChip(){ + // #4650: syncTopbar() calls this on every routine UI refresh, and during + // streaming those fire at high frequency. Before a9ce2889 this served the + // cached _currentReasoningEffort after the first load; that commit made it + // refetch unconditionally to refresh supported-efforts after a model switch, + // which turned ordinary syncs into a GET /api/reasoning storm (one per token). + // Restore the cache short-circuit but keep a9ce2889's intent: only hit the + // network when nothing is cached yet OR the model/provider identity changed + // since the last fetch (the only inputs that change /api/reasoning's answer). + // The user-pick and model-switch paths still update the cache directly. + const key=_reasoningEffortQuery(); + // Short-circuit on the KEY alone: if a fetch for this exact model/provider has + // already been dispatched (in-flight) or completed, do not dispatch another — + // this is what stops the #4650 storm, including the COLD-cache window where + // _currentReasoningEffort is still null between the first dispatch and its + // response (10 syncs before the first GET resolves must produce ONE request, + // not ten). Apply the cached chip only once we actually have an effort value. + if(_lastReasoningFetchKey===key){ + if(_currentReasoningEffort!==null) _applyReasoningChip(_currentReasoningEffort); + return; + } fetchReasoningChip(); } diff --git a/tests/test_issue1103_reasoning_chip_visibility.py b/tests/test_issue1103_reasoning_chip_visibility.py index 8c01abd98..5c8ac758d 100644 --- a/tests/test_issue1103_reasoning_chip_visibility.py +++ b/tests/test_issue1103_reasoning_chip_visibility.py @@ -56,7 +56,22 @@ def test_ui_js_passes_model_context_to_reasoning_api(): assert "_reasoningEffortQuery" in src, ( "ui.js must pass the active session model/provider to /api/reasoning" ) - assert "api('/api/reasoning'+_reasoningEffortQuery())" in src + # The /api/reasoning GET must carry the model/provider query. #4650 captures + # the query into a local `key` first (for the in-flight storm + stale-success + # guards), so accept either the inlined form or the captured-key form — both + # pass _reasoningEffortQuery()'s output to the endpoint. + fetch_match = re.search(r"function fetchReasoningChip\(\)\{(.+?)\n\}", src, re.DOTALL) + assert fetch_match, "fetchReasoningChip function must exist" + fetch_body = fetch_match.group(1) + inlined = "api('/api/reasoning'+_reasoningEffortQuery())" in src + captured = ( + re.search(r"const\s+key\s*=\s*_reasoningEffortQuery\(\)", fetch_body) + and "api('/api/reasoning'+key)" in fetch_body + ) + assert inlined or captured, ( + "fetchReasoningChip must pass _reasoningEffortQuery() (model/provider context) " + "to GET /api/reasoning, either inlined or via a captured key" + ) def test_fetchReasoningChip_calls_apply(): diff --git a/tests/test_issue4650_reasoning_chip_no_storm.py b/tests/test_issue4650_reasoning_chip_no_storm.py new file mode 100644 index 000000000..724b5ef49 --- /dev/null +++ b/tests/test_issue4650_reasoning_chip_no_storm.py @@ -0,0 +1,227 @@ +"""Behavioural test for the #4650 reasoning-chip request-storm fix. + +`syncTopbar()` calls `syncReasoningChip()` on every routine UI refresh, and +during streaming those fire at high frequency. Commit a9ce2889 made +`syncReasoningChip()` refetch `GET /api/reasoning` unconditionally, so ordinary +syncs became a network storm (one request per token -> ~2 tok/s). + +The fix restores the pre-a9ce2889 cache short-circuit while keeping that +commit's intent (refresh supported-efforts after a model switch): fetch only +when nothing is cached yet OR the model/provider identity changed since the +last fetch. This test drives the ACTUAL functions from static/ui.js via node +and counts network calls, so the storm cannot silently come back. +""" +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).parent.parent.resolve() +UI_JS_PATH = REPO_ROOT / "static" / "ui.js" + +NODE = shutil.which("node") +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +_DRIVER_SRC = r""" +const fs = require('fs'); +const src = fs.readFileSync(process.argv[2], 'utf8'); + +function makeEl() { + return { + style: {}, dataset: {}, title: '', textContent: '', value: '', + classList: { add(){}, remove(){}, toggle(){}, contains(){return false} }, + querySelectorAll(){return []}, querySelector(){return null}, + getBoundingClientRect(){return {left:0,top:0,width:0,height:0}}, + }; +} + +const els = { + composerReasoningWrap: makeEl(), + composerReasoningLabel: makeEl(), + composerReasoningChip: makeEl(), + composerReasoningDropdown: makeEl(), + modelSelect: makeEl(), +}; + +// Mutable app state the reasoning helpers read from. +global.S = { session: { model: 'gpt-5', model_provider: 'openai' } }; +global.window = {}; +global.document = { createElement: makeEl, addEventListener(){}, querySelectorAll(){return []}, querySelector(){return null} }; +global.$ = id => els[id] || null; + +// Count every network call and remember the URL it hit. +let CALLS = []; +global.api = (url) => { CALLS.push(url); return { then: () => ({ catch: () => {} }), catch: () => {} }; }; + +// Helpers the reasoning code calls. +global._modelStateForSelect = () => ({ model: '', model_provider: null }); +global._highlightReasoningOption = () => {}; +global._applyReasoningOptions = () => {}; + +function extractFunc(name) { + const re = new RegExp('function\\s+' + name + '\\s*\\('); + const start = src.search(re); + if (start < 0) throw new Error(name + ' not found'); + let i = src.indexOf('{', start); let depth = 1; i++; + while (depth > 0 && i < src.length) { + if (src[i] === '{') depth++; else if (src[i] === '}') depth--; i++; + } + return src.slice(start, i); +} + +// `let _currentReasoningEffort` / `_currentReasoningEffortsSupported` / +// `_lastReasoningFetchKey` are module-scope state the functions close over; +// declare them in this eval scope so the extracted functions can see them. +var _currentReasoningEffort = null; +var _currentReasoningEffortsSupported = null; +var _lastReasoningFetchKey = null; +var _reasoningFetchSeq = 0; + +eval(extractFunc('_normalizeReasoningEffort')); +eval(extractFunc('_formatReasoningEffortLabel')); +eval(extractFunc('_reasoningEffortContext')); +eval(extractFunc('_reasoningEffortQuery')); +eval(extractFunc('_applyReasoningChip')); +eval(extractFunc('fetchReasoningChip')); +eval(extractFunc('syncReasoningChip')); + +// ── Scenario ─────────────────────────────────────────────────────────────── +const result = {}; + +// 0. COLD-CACHE in-flight burst: 10 syncs BEFORE the first GET resolves (no +// _applyReasoningChip yet, so _currentReasoningEffort is still null). This is +// the streaming storm's worst case — it must produce exactly ONE request, +// relying on the optimistic key short-circuit, not on a resolved chip. +for (let i = 0; i < 10; i++) syncReasoningChip(); +result.after_cold_inflight_burst = CALLS.length; + +// 1. First sync with nothing cached -> must fetch. +syncReasoningChip(); +result.after_first_sync = CALLS.length; + +// Simulate the fetch resolving (what _applyReasoningChip does on response). +_applyReasoningChip('high', { supported_efforts: ['low','high'] }); + +// 2. Ten routine syncs with the SAME model (the streaming storm) -> no new fetch. +for (let i = 0; i < 10; i++) syncReasoningChip(); +result.after_ten_same_model_syncs = CALLS.length; + +// 3. Model switch -> exactly one more fetch. +global.S.session.model = 'claude-opus-4'; +global.S.session.model_provider = 'anthropic'; +syncReasoningChip(); +result.after_model_switch = CALLS.length; + +// 4. More routine syncs on the new model -> still no new fetch. +_applyReasoningChip('low', { supported_efforts: ['low','high'] }); +for (let i = 0; i < 5; i++) syncReasoningChip(); +result.after_new_model_syncs = CALLS.length; + +// 5. OUT-OF-ORDER staleness guard: capture each fetch's success callback and +// fire an OLDER fetch's response AFTER a newer dispatch. The stale response +// must be ignored (generation guard), even though both share the same key +// after a profile-switch-style cache reset. Swap in a capturing api() mock. +const HANDLERS = []; +global.api = (url) => ({ + then: (onOk) => { HANDLERS.push({ url, onOk }); return { catch: (onErr) => { HANDLERS[HANDLERS.length-1].onErr = onErr; } }; }, + catch: () => {}, +}); +_currentReasoningEffort = null; _lastReasoningFetchKey = null; +fetchReasoningChip(); // dispatch #A (seq N) +fetchReasoningChip(); // dispatch #B (seq N+1) — supersedes A +// Resolve the OLDER one (#A) last with one effort; must be ignored. +HANDLERS[0].onOk({ reasoning_effort: 'low', supported_efforts: ['low','high'] }); +result.effort_after_stale_A = _currentReasoningEffort; // expect null (ignored) +// Now resolve the NEWER one (#B) with a different effort; must apply. +HANDLERS[1].onOk({ reasoning_effort: 'high', supported_efforts: ['low','high'] }); +result.effort_after_fresh_B = _currentReasoningEffort; // expect 'high' +// A late FAILURE from the older dispatch must NOT hide the fresh chip. +if (HANDLERS[0].onErr) HANDLERS[0].onErr(); +result.effort_after_stale_A_fail = _currentReasoningEffort; // expect still 'high' + +result.calls = CALLS; +process.stdout.write(JSON.stringify(result)); +""" + + +@pytest.fixture(scope="module") +def driver_path(tmp_path_factory): + p = tmp_path_factory.mktemp("reasoning_storm_driver") / "driver.js" + p.write_text(_DRIVER_SRC, encoding="utf-8") + return str(p) + + +@pytest.fixture(scope="module") +def outcome(driver_path): + result = subprocess.run( + [NODE, driver_path, str(UI_JS_PATH)], + capture_output=True, text=True, timeout=30, + ) + if result.returncode != 0: + raise RuntimeError(f"node driver failed: {result.stderr}") + return json.loads(result.stdout) + + +def test_cold_inflight_burst_fetches_once(outcome): + """The worst-case streaming storm: 10 syncs BEFORE the first GET resolves + (cold cache, _currentReasoningEffort still null) must produce exactly ONE + request. The optimistic key short-circuit — not a resolved chip — closes + this in-flight window (#4650 review finding 2).""" + assert outcome["after_cold_inflight_burst"] == 1, ( + "a cold-cache burst of syncs before the first response settles must fire " + f"exactly one GET, not one-per-sync: {outcome['calls']}" + ) + + +def test_first_sync_fetches_once(outcome): + assert outcome["after_first_sync"] == 1, ( + f"a further sync on the same in-flight key must not add a request: {outcome['calls']}" + ) + + +def test_repeated_syncs_same_model_do_not_refetch(outcome): + """The core #4650 regression: 10 routine syncs on the same model must NOT + issue 10 network calls — that was the per-token request storm.""" + assert outcome["after_ten_same_model_syncs"] == 1, ( + "routine topbar syncs on an unchanged model must serve the cache, not " + f"refetch — request storm regression: {outcome['calls']}" + ) + + +def test_model_switch_triggers_one_refetch(outcome): + """a9ce2889's intent must survive: switching models refreshes the chip's + supported-efforts, so exactly one fetch fires on the switch.""" + assert outcome["after_model_switch"] == 2, ( + f"a model switch must trigger exactly one refetch: {outcome['calls']}" + ) + + +def test_syncs_after_switch_do_not_refetch(outcome): + assert outcome["after_new_model_syncs"] == 2, ( + "routine syncs after a model switch must serve the cache again: " + f"{outcome['calls']}" + ) + + +def test_stale_out_of_order_success_is_ignored(outcome): + """#4650 review: an older in-flight /api/reasoning success that resolves AFTER + a newer dispatch (same key, e.g. profile switch) must not poison the chip.""" + assert outcome["effort_after_stale_A"] is None, ( + "a superseded older fetch's success must be ignored (generation guard), " + f"got {outcome['effort_after_stale_A']!r}" + ) + assert outcome["effort_after_fresh_B"] == "high", ( + "the newest fetch's success must apply" + ) + + +def test_stale_out_of_order_failure_does_not_hide_fresh_chip(outcome): + """A late failure from a superseded dispatch must not clear/hide the chip + that a newer successful fetch already applied.""" + assert outcome["effort_after_stale_A_fail"] == "high", ( + "a stale failure must not overwrite the fresh chip state: " + f"got {outcome['effort_after_stale_A_fail']!r}" + ) diff --git a/tests/test_issue4650_yaml_config_cache.py b/tests/test_issue4650_yaml_config_cache.py new file mode 100644 index 000000000..29470fac3 --- /dev/null +++ b/tests/test_issue4650_yaml_config_cache.py @@ -0,0 +1,135 @@ +"""Backend half of the #4650 fix: _load_yaml_config_file memoizes the parsed +config keyed on (path, mtime_ns, size). + +GET /api/reasoning -> get_reasoning_status() calls _load_yaml_config_file on +every request, and yaml.safe_load on an ~800-line config costs ~125ms. Under +the (pre-fix) request storm that became a YAML-reparse storm. This test pins +the cache contract: + + * a second read of an unchanged file does NOT re-run yaml.safe_load, + * an on-disk edit (new mtime/size) DOES trigger a fresh parse, + * env-var expansion still runs per call (so ${VAR} stays live and callers + that mutate the returned dict never corrupt the cached copy). +""" +import importlib +import time + +import pytest + +config = importlib.import_module("api.config") + + +@pytest.fixture +def clean_cache(): + config._yaml_file_cache.clear() + yield + config._yaml_file_cache.clear() + + +def _write(p, text): + p.write_text(text, encoding="utf-8") + + +def test_second_read_uses_cache_no_reparse(tmp_path, monkeypatch, clean_cache): + cfg = tmp_path / "config.yaml" + _write(cfg, "agent:\n reasoning_effort: high\n") + + calls = {"n": 0} + import yaml as _yaml + real_load = _yaml.safe_load + + def counting_load(text): + calls["n"] += 1 + return real_load(text) + + monkeypatch.setattr(_yaml, "safe_load", counting_load) + + first = config._load_yaml_config_file(cfg) + second = config._load_yaml_config_file(cfg) + + assert first == {"agent": {"reasoning_effort": "high"}} + assert second == first + assert calls["n"] == 1, "second read of an unchanged file must hit the cache, not reparse" + + +def test_mtime_change_invalidates_cache(tmp_path, monkeypatch, clean_cache): + cfg = tmp_path / "config.yaml" + _write(cfg, "agent:\n reasoning_effort: high\n") + + calls = {"n": 0} + import yaml as _yaml + real_load = _yaml.safe_load + + def counting_load(text): + calls["n"] += 1 + return real_load(text) + + monkeypatch.setattr(_yaml, "safe_load", counting_load) + + assert config._load_yaml_config_file(cfg)["agent"]["reasoning_effort"] == "high" + # Rewrite with different content + bump mtime so (mtime_ns, size) changes. + time.sleep(0.01) + _write(cfg, "agent:\n reasoning_effort: low\n") + import os + st = cfg.stat() + os.utime(cfg, ns=(st.st_atime_ns, st.st_mtime_ns + 1_000_000)) + + reloaded = config._load_yaml_config_file(cfg) + assert reloaded["agent"]["reasoning_effort"] == "low", "an on-disk edit must be picked up" + assert calls["n"] == 2, "changed file must trigger exactly one fresh parse" + + +def test_env_expansion_runs_per_call_even_from_cache(tmp_path, monkeypatch, clean_cache): + cfg = tmp_path / "config.yaml" + _write(cfg, "agent:\n api_key: ${MY_TEST_KEY}\n") + + monkeypatch.setenv("MY_TEST_KEY", "first") + first = config._load_yaml_config_file(cfg) + assert first["agent"]["api_key"] == "first" + + # Cache hit (file unchanged) but env changed -> expansion must reflect it. + monkeypatch.setenv("MY_TEST_KEY", "second") + second = config._load_yaml_config_file(cfg) + assert second["agent"]["api_key"] == "second", "env expansion must run per call, not be frozen in the cache" + + +def test_caller_mutation_does_not_corrupt_cache(tmp_path, clean_cache): + cfg = tmp_path / "config.yaml" + _write(cfg, "display:\n show_reasoning: true\n") + + first = config._load_yaml_config_file(cfg) + # Mirror the read-modify-save callers (set_reasoning_display etc.). + first["display"]["show_reasoning"] = False + first["injected"] = "mutation" + + second = config._load_yaml_config_file(cfg) + assert second["display"]["show_reasoning"] is True, "caller mutation must not leak into the cache" + assert "injected" not in second + + +def test_missing_file_returns_empty(tmp_path, clean_cache): + assert config._load_yaml_config_file(tmp_path / "nope.yaml") == {} + + +def test_save_evicts_cache_so_next_read_is_fresh(tmp_path, clean_cache): + """#4650 review: a WebUI save that preserves (mtime_ns, size) could otherwise + serve a stale cached parse. _save_yaml_config_file must evict the cache entry + for the path it writes, so the next read reflects the saved content.""" + cfg = tmp_path / "config.yaml" + _write(cfg, "agent:\n reasoning_effort: high\n") + + first = config._load_yaml_config_file(cfg) + assert first["agent"]["reasoning_effort"] == "high" + assert str(cfg) in config._yaml_file_cache, "first read should populate the cache" + + # Save new content through the WebUI writer. + config._save_yaml_config_file(cfg, {"agent": {"reasoning_effort": "low"}}) + assert str(cfg) not in config._yaml_file_cache, ( + "_save_yaml_config_file must evict the memoized parse for the written path" + ) + + # Next read must reflect the saved value even if mtime/size happened to collide. + fresh = config._load_yaml_config_file(cfg) + assert fresh["agent"]["reasoning_effort"] == "low", ( + "read after save must return the freshly-written config, not a stale cache hit" + )