diff --git a/api/config.py b/api/config.py index 25fb5a14e..63ea4f39d 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. diff --git a/static/ui.js b/static/ui.js index b471a472a..5bb379cd9 100644 --- a/static/ui.js +++ b/static/ui.js @@ -3069,13 +3069,33 @@ 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; + function fetchReasoningChip(){ + _lastReasoningFetchKey=_reasoningEffortQuery(); api('/api/reasoning'+_reasoningEffortQuery()).then(function(st){ _applyReasoningChip((st&&st.reasoning_effort)||'', st||{}); }).catch(function(){_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(); + if(_currentReasoningEffort!==null && _lastReasoningFetchKey===key){ + _applyReasoningChip(_currentReasoningEffort); + return; + } fetchReasoningChip(); } 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..370173063 --- /dev/null +++ b/tests/test_issue4650_reasoning_chip_no_storm.py @@ -0,0 +1,165 @@ +"""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; + +eval(extractFunc('_normalizeReasoningEffort')); +eval(extractFunc('_formatReasoningEffortLabel')); +eval(extractFunc('_reasoningEffortContext')); +eval(extractFunc('_reasoningEffortQuery')); +eval(extractFunc('_applyReasoningChip')); +eval(extractFunc('fetchReasoningChip')); +eval(extractFunc('syncReasoningChip')); + +// ── Scenario ─────────────────────────────────────────────────────────────── +const result = {}; + +// 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; + +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_first_sync_fetches_once(outcome): + assert outcome["after_first_sync"] == 1, ( + f"first sync (cold cache) must fetch exactly once: {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']}" + ) diff --git a/tests/test_issue4650_yaml_config_cache.py b/tests/test_issue4650_yaml_config_cache.py new file mode 100644 index 000000000..31c886045 --- /dev/null +++ b/tests/test_issue4650_yaml_config_cache.py @@ -0,0 +1,111 @@ +"""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") == {}