diff --git a/api/config.py b/api/config.py index 979ce3f7..87c1ce06 100644 --- a/api/config.py +++ b/api/config.py @@ -8156,7 +8156,7 @@ def _get_fresh_memory_models_cache(now: float) -> dict | None: return None -def invalidate_models_cache(): +def invalidate_models_cache(*, delete_disk: bool = True): """Force the TTL cache for get_available_models() to be cleared. Call this after modifying config.cfg in-memory (e.g. in tests) so @@ -8169,6 +8169,14 @@ def invalidate_models_cache(): that call invalidate_models_cache() still get back the previous test's result from the disk cache because the disk hit is checked before the memory cache rebuild runs. + + ``delete_disk=False`` drops only the in-memory snapshot and leaves the + per-profile disk cache in place. Use it when the *sources* have not + changed and the caller merely needs the next request to re-resolve which + profile's catalog to serve (e.g. a per-client profile switch): the disk + cache is already keyed per profile and guarded by + _models_cache_source_fingerprint(), so a stale snapshot is rejected on + read without paying for a full rebuild (live provider fetches). """ global _cache_build_in_progress, _available_models_cache, _available_models_cache_ts global _available_models_live_rebuild_ts, _available_models_cache_source_fingerprint, _cache_build_cv @@ -8187,7 +8195,8 @@ def invalidate_models_cache(): _CREDENTIAL_POOL_CACHE.clear() # Also delete the disk cache so the next cold build starts fresh. # Disk delete is outside the lock — file I/O shouldn't block other readers. - _delete_models_cache_on_disk() + if delete_disk: + _delete_models_cache_on_disk() try: from api.plugin_providers import invalidate_plugin_model_provider_cache diff --git a/api/routes.py b/api/routes.py index 0e159784..fec6cc3d 100644 --- a/api/routes.py +++ b/api/routes.py @@ -16858,11 +16858,14 @@ def handle_post(handler, parsed) -> bool: # process_wide=False: don't mutate the process-global _active_profile. # Per-client profile is managed via cookie + thread-local (#798). result = switch_profile(name, process_wide=False) - # Invalidate the models cache so the very next /api/models request - # rebuilds from the new profile's config.yaml rather than returning + # Drop the in-memory catalog so the very next /api/models request + # re-resolves the new profile's config.yaml rather than returning # the old profile's cached model list (#1200 — profile-switch model bug). + # Keep the per-profile disk cache: it is fingerprint-guarded, and + # deleting it forced a full cold rebuild (live provider fetches, + # several seconds) on every switch even though nothing changed. from api.config import invalidate_models_cache - invalidate_models_cache() + invalidate_models_cache(delete_disk=False) try: from api.gateway_watcher import restart_watcher_for_profile restart_watcher_for_profile(name) diff --git a/tests/test_gateway_watcher_profile.py b/tests/test_gateway_watcher_profile.py index 5b4a4200..fd34d7b8 100644 --- a/tests/test_gateway_watcher_profile.py +++ b/tests/test_gateway_watcher_profile.py @@ -316,7 +316,7 @@ def test_profile_switch_restarts_watcher_best_effort(monkeypatch): monkeypatch.setattr(routes, "read_body", lambda handler: {"name": "demo"}) monkeypatch.setattr(profiles, "_validate_profile_name", lambda name: None) monkeypatch.setattr(profiles, "switch_profile", lambda name, process_wide=False: {"ok": True, "name": name}) - monkeypatch.setattr(config, "invalidate_models_cache", lambda: calls.append("cache")) + monkeypatch.setattr(config, "invalidate_models_cache", lambda *_a, **_kw: calls.append("cache")) monkeypatch.setattr(gateway_watcher, "restart_watcher_for_profile", lambda name: calls.append(("watcher", name))) handler = _FakeHandler() @@ -335,7 +335,7 @@ def test_profile_switch_response_survives_watcher_restart_failure(monkeypatch): monkeypatch.setattr(routes, "read_body", lambda handler: {"name": "demo"}) monkeypatch.setattr(profiles, "_validate_profile_name", lambda name: None) monkeypatch.setattr(profiles, "switch_profile", lambda name, process_wide=False: {"ok": True, "name": name}) - monkeypatch.setattr(config, "invalidate_models_cache", lambda: None) + monkeypatch.setattr(config, "invalidate_models_cache", lambda *_a, **_kw: None) monkeypatch.setattr( gateway_watcher, "restart_watcher_for_profile", diff --git a/tests/test_profile_switch_models_disk_cache.py b/tests/test_profile_switch_models_disk_cache.py new file mode 100644 index 00000000..0146d163 --- /dev/null +++ b/tests/test_profile_switch_models_disk_cache.py @@ -0,0 +1,126 @@ +"""Profile switch must not discard the per-profile models disk cache. + +``POST /api/profile/switch`` calls ``invalidate_models_cache()`` so the next +``/api/models`` re-resolves the new profile's catalog (#1200). That helper also +deleted the on-disk ``models_cache..json`` — a test-isolation +behaviour — which forced a full cold rebuild (live provider ``fetch_models`` +HTTPS calls, ~490k deepcopy calls) on every switch even though no source had +changed. The disk cache is already keyed per profile and rejected on read when +``_models_cache_source_fingerprint()`` differs, so deleting it buys no +correctness on the switch path and only costs seconds of latency. + +The switch route now uses ``invalidate_models_cache(delete_disk=False)``. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent + + +def _catalog(label: str) -> dict: + return { + "active_provider": "openai", + "default_model": label, + "configured_model_badges": {}, + "groups": [ + { + "provider": "OpenAI", + "provider_id": "openai", + "models": [{"id": label, "label": label, "supports_fast_tier": False}], + } + ], + "aliases": {}, + } + + +def _prime_memory_cache(monkeypatch, catalog: dict): + import api.config as cfg + + monkeypatch.setattr(cfg, "_available_models_cache", catalog, raising=False) + monkeypatch.setattr(cfg, "_available_models_cache_ts", 1.0, raising=False) + monkeypatch.setattr(cfg, "_available_models_cache_source_fingerprint", {"p": "x"}, raising=False) + monkeypatch.setattr(cfg, "_cache_build_in_progress", False, raising=False) + + +def test_invalidate_keeps_disk_cache_when_delete_disk_false(tmp_path, monkeypatch): + import api.config as cfg + + cache_path = tmp_path / "models_cache.profile.json" + cache_path.write_text("{}", encoding="utf-8") + monkeypatch.setattr(cfg, "_get_models_cache_path", lambda: cache_path) + _prime_memory_cache(monkeypatch, _catalog("old-profile-model")) + + cfg.invalidate_models_cache(delete_disk=False) + + # In-memory snapshot dropped so the next request re-resolves the profile... + assert cfg._available_models_cache is None + assert cfg._available_models_cache_source_fingerprint is None + assert cfg._models_cache_provenance is None + # ...but the fingerprint-guarded disk cache survives. + assert cache_path.exists(), "delete_disk=False must not unlink the per-profile disk cache" + + +def test_invalidate_default_still_deletes_disk_cache(tmp_path, monkeypatch): + """Test-isolation contract is unchanged for every existing caller.""" + import api.config as cfg + + cache_path = tmp_path / "models_cache.profile.json" + cache_path.write_text("{}", encoding="utf-8") + monkeypatch.setattr(cfg, "_get_models_cache_path", lambda: cache_path) + _prime_memory_cache(monkeypatch, _catalog("old-profile-model")) + + cfg.invalidate_models_cache() + + assert cfg._available_models_cache is None + assert not cache_path.exists() + + +def test_memory_only_invalidate_serves_next_request_from_disk_without_rebuild(tmp_path, monkeypatch): + """After a memory-only drop, get_available_models() must reload the disk + snapshot instead of running the live rebuild.""" + import api.config as cfg + + cache_path = tmp_path / "models_cache.profile.json" + cache_path.write_text("{}", encoding="utf-8") + disk_catalog = _catalog("disk-model") + monkeypatch.setattr(cfg, "_get_models_cache_path", lambda: cache_path) + monkeypatch.setattr(cfg, "_load_models_cache_from_disk", lambda: disk_catalog if cache_path.exists() else None) + monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) + monkeypatch.setattr(cfg, "_models_cache_source_fingerprint", lambda: {"profile": "demo"}) + # Pin config mtime tracking so an unrelated config change (or leaked + # state from an earlier test) cannot force the reload branch. + cfg_file = tmp_path / "config.yaml" + cfg_file.write_text("model: {}\n", encoding="utf-8") + monkeypatch.setattr(cfg, "_get_config_path", lambda: cfg_file) + monkeypatch.setattr(cfg, "_cfg_path", cfg_file, raising=False) + monkeypatch.setattr(cfg, "_cfg_mtime", cfg_file.stat().st_mtime, raising=False) + + def _unexpected_rebuild(*_a, **_kw): + raise AssertionError("profile switch must not trigger a live models rebuild") + + monkeypatch.setattr(cfg, "_invoke_models_rebuild", _unexpected_rebuild) + _prime_memory_cache(monkeypatch, _catalog("stale-memory-model")) + + cfg.invalidate_models_cache(delete_disk=False) + result = cfg.get_available_models() + + assert result["default_model"] == "disk-model" + + +def test_profile_switch_route_preserves_disk_cache(): + """The switch handler must opt out of the disk delete.""" + src = (REPO / "api" / "routes.py").read_text(encoding="utf-8") + start = src.index('if parsed.path == "/api/profile/switch":') + end = src.index('if parsed.path == "/api/profile/create":', start) + block = src[start:end] + + calls = re.findall(r"invalidate_models_cache\(([^)]*)\)", block) + assert calls, "switch route must still invalidate the in-memory models cache (#1200)" + for args in calls: + assert "delete_disk=False" in args, ( + "/api/profile/switch must call invalidate_models_cache(delete_disk=False); " + "deleting the disk cache forces a multi-second cold rebuild on every switch" + ) diff --git a/tests/test_trusted_header_auth.py b/tests/test_trusted_header_auth.py index baf35692..814cef15 100644 --- a/tests/test_trusted_header_auth.py +++ b/tests/test_trusted_header_auth.py @@ -438,7 +438,7 @@ def test_profile_switch_accepts_bound_profile(monkeypatch): monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) monkeypatch.setattr(routes, "read_body", lambda _handler: {"name": "devops"}) monkeypatch.setattr("api.profiles.switch_profile", lambda name, process_wide=False: {"ok": True, "profile": name}) - monkeypatch.setattr("api.config.invalidate_models_cache", lambda: None) + monkeypatch.setattr("api.config.invalidate_models_cache", lambda *_a, **_kw: None) monkeypatch.setattr("api.gateway_watcher.restart_watcher_for_profile", lambda _name: None) routes.handle_post(handler, SimpleNamespace(path="/api/profile/switch", query="")) @@ -851,7 +851,7 @@ def test_consumers_route_through_auth_owner(monkeypatch): monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) monkeypatch.setattr(routes, "read_body", lambda _handler: {"name": "devops"}) monkeypatch.setattr("api.profiles.switch_profile", lambda name, process_wide=False: {"ok": True, "profile": name}) - monkeypatch.setattr("api.config.invalidate_models_cache", lambda: None) + monkeypatch.setattr("api.config.invalidate_models_cache", lambda *_a, **_kw: None) monkeypatch.setattr("api.gateway_watcher.restart_watcher_for_profile", lambda _name: None) routes.handle_get(handler, SimpleNamespace(path="/api/auth/status", query=""))