From 7bb12d0aab62a51d200da4c29c76389c8a94d370 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Fri, 18 Sep 2026 12:23:25 +0000 Subject: [PATCH 01/10] fix: keep per-profile models disk cache across /api/profile/switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/profile/switch called invalidate_models_cache(), whose test-isolation contract also unlinks models_cache..json. On a switch nothing about the sources changed, so the delete only forced a full cold rebuild (live provider fetch_models HTTPS calls, ~490k deepcopy calls) on the very next /api/models — several seconds on every switch. The disk cache is already keyed per profile and rejected on read when _models_cache_source_fingerprint() differs, so it is safe to keep. Add a delete_disk keyword (default True — every existing caller keeps the old behaviour) and have the switch route pass delete_disk=False. Regression tests cover: memory-only drop preserves the disk file, default call still unlinks it, the next get_available_models() serves the disk snapshot without a live rebuild, and the switch route opts out. Two existing zero-arg stubs of invalidate_models_cache accept **kwargs now. --- api/config.py | 13 +- api/routes.py | 9 +- tests/test_gateway_watcher_profile.py | 4 +- .../test_profile_switch_models_disk_cache.py | 126 ++++++++++++++++++ tests/test_trusted_header_auth.py | 4 +- 5 files changed, 147 insertions(+), 9 deletions(-) create mode 100644 tests/test_profile_switch_models_disk_cache.py 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="")) From 217a2fcea8eadc91385de0919086c732f1f6dfd3 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Fri, 18 Sep 2026 18:39:44 +0000 Subject: [PATCH 02/10] review: document delete_disk invalidation modes; assert switch route kwargs via executed handle_post - docs/architecture/models-cache-invalidation.md: add 'Invalidation modes' section describing delete_disk=True/False and why /api/profile/switch uses the memory-only mode (fingerprint-guarded per-profile disk cache). - tests: replace the source-text scan of the switch route with two executed routes.handle_post tests: one captures invalidate_models_cache kwargs and asserts exactly [((), {'delete_disk': False})]; one runs the real invalidate_models_cache and asserts the disk file survives while the in-memory catalog is dropped. Both fail if the route reverts to the default delete_disk=True. --- .../architecture/models-cache-invalidation.md | 25 +++++ .../test_profile_switch_models_disk_cache.py | 96 +++++++++++++++---- 2 files changed, 104 insertions(+), 17 deletions(-) diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index f50d14b0..21f4f2bd 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -66,6 +66,26 @@ the fingerprint captured at publish time. TTL expires. When the runtime version cannot be resolved (early boot), that check is skipped rather than wedging the boot. +## Invalidation modes (`invalidate_models_cache`) + +`invalidate_models_cache(*, delete_disk=True)` is the only entry point that +drops the published snapshot. It has two modes: + +| Mode | What is dropped | When to use | +| --- | --- | --- | +| `delete_disk=True` (default) | in-memory snapshot **and** the per-profile `models_cache.json` on disk | A source may have changed, or test isolation requires a guaranteed cold build. Every pre-existing caller keeps this mode. | +| `delete_disk=False` | in-memory snapshot only; the disk snapshot is left in place | The sources have **not** changed and the caller only needs the next request to re-resolve *which* profile's catalog to serve. | + +`POST /api/profile/switch` uses `delete_disk=False`. The disk cache is already +keyed per profile (`_get_models_cache_path()`), and a stale or wrong-profile +snapshot is rejected on read by `_is_loadable_disk_cache()` via the source +fingerprint above, so deleting it on a switch bought no correctness — it only +forced a full cold rebuild (live provider `fetch_models` calls, several seconds) +on every switch. Because this mode leans entirely on the fingerprint check, it +is safe only while that check stays the single gate for serving a disk snapshot +(change-protocol items 1 and 4). Both modes must still call +`_sync_models_cache_provenance()` so the hot-path tuple cannot tear. + ## Change protocol 1. Add or change a source axis in `_models_cache_source_fingerprint()` only — @@ -90,6 +110,11 @@ timestamp-only churn keeps the fingerprint identical (and a session visit after Codex refresh needs no live rebuild), while genuine changes — a new model, a visibility change, any catalog field, any unknown field — still invalidate. +`tests/test_profile_switch_models_disk_cache.py` covers the invalidation modes: +`delete_disk=False` keeps the disk file and the next `get_available_models()` +reloads it without a live rebuild, the default still unlinks it, and the +executed `/api/profile/switch` route passes `delete_disk=False`. + ## References Issues/PRs: #2443, #7540, #7556, #7558. RCAs: `t_d127953d`, `t_16551f61`. diff --git a/tests/test_profile_switch_models_disk_cache.py b/tests/test_profile_switch_models_disk_cache.py index 0146d163..3f121971 100644 --- a/tests/test_profile_switch_models_disk_cache.py +++ b/tests/test_profile_switch_models_disk_cache.py @@ -14,10 +14,8 @@ 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 +import json +from urllib.parse import urlparse def _catalog(label: str) -> dict: @@ -110,17 +108,81 @@ def test_memory_only_invalidate_serves_next_request_from_disk_without_rebuild(tm 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] +class _FakeHandler: + def __init__(self): + self.status = None + self.sent_headers: list[tuple[str, str]] = [] + self.body = bytearray() + self.wfile = self + self.headers = {} - 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" - ) + def send_response(self, code): + self.status = code + + def send_header(self, key, value): + self.sent_headers.append((key, value)) + + def end_headers(self): + pass + + def write(self, data): + self.body.extend(data if isinstance(data, (bytes, bytearray)) else data.encode("utf-8")) + + def get_json(self): + return json.loads(self.body.decode("utf-8")) + + +def _drive_profile_switch(monkeypatch, name: str = "demo") -> _FakeHandler: + """Execute POST /api/profile/switch through routes.handle_post with the + auth / CSRF / profile-fs / watcher collaborators stubbed out, leaving + ``config.invalidate_models_cache`` to whatever the caller installed.""" + from api import gateway_watcher, profiles, routes + + for mod in ("api.auth", "api.helpers", "api.routes"): + monkeypatch.setattr(f"{mod}.is_auth_enabled", lambda: False, raising=False) + monkeypatch.setattr(routes, "_check_csrf", lambda handler: True) + monkeypatch.setattr(routes, "read_body", lambda handler: {"name": name}) + 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(gateway_watcher, "restart_watcher_for_profile", lambda _name: None) + + handler = _FakeHandler() + routes.handle_post(handler, urlparse("/api/profile/switch")) + return handler + + +def test_profile_switch_route_calls_invalidate_with_delete_disk_false(monkeypatch): + """Executed-route guard: the live switch handler must pass + ``delete_disk=False`` (not rely on the default, which unlinks the disk + cache and forces a cold rebuild).""" + from api import config + + calls: list[tuple[tuple, dict]] = [] + monkeypatch.setattr(config, "invalidate_models_cache", lambda *a, **kw: calls.append((a, kw))) + + handler = _drive_profile_switch(monkeypatch) + + assert handler.status == 200, handler.body.decode("utf-8", "replace") + assert handler.get_json() == {"ok": True, "name": "demo"} + assert calls == [((), {"delete_disk": False})], ( + "/api/profile/switch must call invalidate_models_cache(delete_disk=False) exactly once; " + f"got {calls!r}" + ) + + +def test_profile_switch_route_leaves_disk_cache_file_in_place(tmp_path, monkeypatch): + """End-to-end through the real ``invalidate_models_cache``: after the + executed switch route, the in-memory catalog is dropped but the + per-profile disk snapshot still exists.""" + from api import 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")) + + handler = _drive_profile_switch(monkeypatch) + + assert handler.status == 200, handler.body.decode("utf-8", "replace") + assert cfg._available_models_cache is None, "switch must still drop the in-memory catalog (#1200)" + assert cache_path.exists(), "switch must not unlink the per-profile disk cache" From 0415d07cd0d1ce824657a89e9787fcf0361f459d Mon Sep 17 00:00:00 2001 From: carlotestor Date: Sat, 19 Sep 2026 10:50:37 +0000 Subject: [PATCH 03/10] docs: invalidate_models_cache is the only delete_disk entry point, not the only snapshot-dropping path --- docs/architecture/models-cache-invalidation.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index 21f4f2bd..ce96c968 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -69,7 +69,13 @@ the fingerprint captured at publish time. ## Invalidation modes (`invalidate_models_cache`) `invalidate_models_cache(*, delete_disk=True)` is the only entry point that -drops the published snapshot. It has two modes: +offers the `delete_disk` choice. It is **not** the only path that drops the +published in-memory snapshot: `invalidate_provider_models_cache()`, +`_get_fresh_memory_models_cache()` (on a fingerprint mismatch or an invalid +cached shape) and the config-reload branch inside +`get_available_models()` also reset `_available_models_cache` and call +`_sync_models_cache_provenance()`. None of those paths take a `delete_disk` +argument; they only clear memory. `invalidate_models_cache` has two modes: | Mode | What is dropped | When to use | | --- | --- | --- | From 093191cfb8642fc87d5ccdb37533190d1fef607f Mon Sep 17 00:00:00 2001 From: carlotestor Date: Sat, 19 Sep 2026 11:10:48 +0000 Subject: [PATCH 04/10] docs: state memory and disk behaviour per invalidation path (provider invalidation and config reload also delete the disk snapshot) --- .../architecture/models-cache-invalidation.md | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index ce96c968..5557be43 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -66,21 +66,28 @@ the fingerprint captured at publish time. TTL expires. When the runtime version cannot be resolved (early boot), that check is skipped rather than wedging the boot. -## Invalidation modes (`invalidate_models_cache`) +## Invalidation paths: memory vs. disk -`invalidate_models_cache(*, delete_disk=True)` is the only entry point that -offers the `delete_disk` choice. It is **not** the only path that drops the -published in-memory snapshot: `invalidate_provider_models_cache()`, -`_get_fresh_memory_models_cache()` (on a fingerprint mismatch or an invalid -cached shape) and the config-reload branch inside -`get_available_models()` also reset `_available_models_cache` and call -`_sync_models_cache_provenance()`. None of those paths take a `delete_disk` -argument; they only clear memory. `invalidate_models_cache` has two modes: +Every path that drops the published in-memory snapshot resets +`_available_models_cache` (plus its timestamps and source fingerprint) and +calls `_sync_models_cache_provenance()` so the hot-path tuple cannot tear. They +differ in what they do to the per-profile `models_cache.json` on disk +(`_delete_models_cache_on_disk()`): -| Mode | What is dropped | When to use | +| Path | In-memory snapshot | Disk snapshot | | --- | --- | --- | -| `delete_disk=True` (default) | in-memory snapshot **and** the per-profile `models_cache.json` on disk | A source may have changed, or test isolation requires a guaranteed cold build. Every pre-existing caller keeps this mode. | -| `delete_disk=False` | in-memory snapshot only; the disk snapshot is left in place | The sources have **not** changed and the caller only needs the next request to re-resolve *which* profile's catalog to serve. | +| `invalidate_models_cache(delete_disk=True)` (default) | dropped | **deleted** unconditionally | +| `invalidate_models_cache(delete_disk=False)` | dropped | left in place | +| `invalidate_provider_models_cache(provider_id)` | dropped | **deleted** unconditionally (no `delete_disk` option) | +| `_get_fresh_memory_models_cache()` on a fingerprint mismatch or invalid cached shape | dropped | untouched | +| config-reload branch in `get_available_models()` (`_cfg_changed`) | dropped | deleted by `reload_config()` → `_refresh_config_cache()` **only if** a config was already loaded (`_old_cfg_mtime != 0.0`); a first-ever load (server start, profile switch) keeps it | + +`invalidate_models_cache` is the only entry point that offers the +`delete_disk` choice. `delete_disk=True` is for when a source may have changed, +or test isolation requires a guaranteed cold build; every pre-existing caller +keeps this mode. `delete_disk=False` is for when the sources have **not** +changed and the caller only needs the next request to re-resolve *which* +profile's catalog to serve. `POST /api/profile/switch` uses `delete_disk=False`. The disk cache is already keyed per profile (`_get_models_cache_path()`), and a stale or wrong-profile @@ -89,8 +96,7 @@ fingerprint above, so deleting it on a switch bought no correctness — it only forced a full cold rebuild (live provider `fetch_models` calls, several seconds) on every switch. Because this mode leans entirely on the fingerprint check, it is safe only while that check stays the single gate for serving a disk snapshot -(change-protocol items 1 and 4). Both modes must still call -`_sync_models_cache_provenance()` so the hot-path tuple cannot tear. +(change-protocol items 1 and 4). ## Change protocol From 78bb3665602fa8d1f8b8e71d987f412f59b10554 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Sat, 19 Sep 2026 11:23:31 +0000 Subject: [PATCH 05/10] fix: keep the target profile's models disk cache on the first /api/models after a per-client switch switch_profile(process_wide=False) skips reload_config(), so _cfg_mtime still held the previous profile's nonzero mtime. The first /api/models for the new profile took the config-reload branch and _refresh_config_cache() treated _old_cfg_mtime != 0.0 as a config edit, unlinking the *target* profile's models_cache..json - defeating delete_disk=False on the very next request. Only delete the disk snapshot when the same config.yaml path was already loaded (_old_cfg_path == config_path); a path change (profile switch) keeps it. Executed two-profile end-to-end test (real homes, configs, cache files) plus a guard that a same-profile config edit still deletes the snapshot. Doc table updated. --- api/config.py | 24 ++- .../architecture/models-cache-invalidation.md | 12 +- ...ch_next_models_request_keeps_disk_cache.py | 156 ++++++++++++++++++ 3 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 tests/test_profile_switch_next_models_request_keeps_disk_cache.py diff --git a/api/config.py b/api/config.py index 87c1ce06..46710c35 100644 --- a/api/config.py +++ b/api/config.py @@ -549,9 +549,15 @@ def _refresh_config_cache(config_path: Path | None = None) -> None: if config_path is None: config_path = _get_config_path() _cfg_cache.clear() - # Remember the old mtime so we can tell whether config actually changed - # vs. first-ever load (mtime == 0.0, e.g. server start or profile switch). + # Remember the old mtime AND path so we can tell whether *this* config file + # actually changed vs. a first-ever load (mtime == 0.0, server start) or a + # load of a different profile's config.yaml. _cfg_mtime is process-global, + # so after a per-client profile switch (process_wide=False skips + # reload_config()) it still holds the previous profile's nonzero mtime; + # without the path check the next /api/models on the new profile would + # look like an edit and unlink the new profile's disk snapshot. _old_cfg_mtime = _cfg_mtime + _old_cfg_path = _cfg_path _cfg_path = config_path _cfg_mtime = 0.0 try: @@ -607,11 +613,15 @@ def _refresh_config_cache(config_path: Path | None = None) -> None: _apply_config_defaults(_cfg_cache) _cfg_fingerprint = _fingerprint_config(_cfg_cache) # Bust the models cache so the next request sees fresh config values. - # Only delete the disk cache when config has actually changed -- not on - # first-ever load (when _old_cfg_mtime == 0.0, i.e. server start or - # profile switch) -- preserving the disk cache so the next restart - # still hits the fast path without a cold run. - if _old_cfg_mtime != 0.0: + # Only delete the disk cache when the SAME config file was already loaded + # (a real edit / explicit reload of the active profile's config.yaml) -- + # not on first-ever load (_old_cfg_mtime == 0.0, server start) and not + # when the path changed because the request belongs to another profile + # (per-client profile switch). The per-profile disk snapshot is + # fingerprint-guarded on read, so a path change needs no unlink; deleting + # it here forced a full cold rebuild on the first /api/models after every + # switch. + if _old_cfg_mtime != 0.0 and _old_cfg_path == config_path: _delete_models_cache_on_disk() diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index 5557be43..9fd3a798 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -80,7 +80,17 @@ differ in what they do to the per-profile `models_cache.json` on disk | `invalidate_models_cache(delete_disk=False)` | dropped | left in place | | `invalidate_provider_models_cache(provider_id)` | dropped | **deleted** unconditionally (no `delete_disk` option) | | `_get_fresh_memory_models_cache()` on a fingerprint mismatch or invalid cached shape | dropped | untouched | -| config-reload branch in `get_available_models()` (`_cfg_changed`) | dropped | deleted by `reload_config()` → `_refresh_config_cache()` **only if** a config was already loaded (`_old_cfg_mtime != 0.0`); a first-ever load (server start, profile switch) keeps it | +| config-reload branch in `get_available_models()` (`_cfg_changed`) | dropped | deleted by `reload_config_if_stale()` → `_refresh_config_cache()` **only if** the *same* `config.yaml` path was already loaded and its mtime moved (`_old_cfg_mtime != 0.0 and _old_cfg_path == config_path`, i.e. a real edit of the active profile's config); a first-ever load (server start, `_cfg_mtime == 0.0`) or a path change keeps it | + +The path guard matters for per-client profile switches: `switch_profile(name, +process_wide=False)` deliberately skips `reload_config()`, so the process-global +`_cfg_mtime` / `_cfg_path` still describe the *previous* profile's config after +`POST /api/profile/switch`. The first `/api/models` for the new profile then +takes the config-reload branch (different path, different mtime). Without the +`_old_cfg_path == config_path` check that reload looked like a config edit and +unlinked the *target* profile's `models_cache..json`, defeating the +`delete_disk=False` switch on the very next request +(`tests/test_profile_switch_next_models_request_keeps_disk_cache.py`). `invalidate_models_cache` is the only entry point that offers the `delete_disk` choice. `delete_disk=True` is for when a source may have changed, diff --git a/tests/test_profile_switch_next_models_request_keeps_disk_cache.py b/tests/test_profile_switch_next_models_request_keeps_disk_cache.py new file mode 100644 index 00000000..cf969ddc --- /dev/null +++ b/tests/test_profile_switch_next_models_request_keeps_disk_cache.py @@ -0,0 +1,156 @@ +"""Greptile review thread on #7632 (docs/architecture/models-cache-invalidation.md): + +A per-client ``POST /api/profile/switch`` (``process_wide=False``) skips +``reload_config()``, so the process-global ``_cfg_mtime`` still holds the +*previous* profile's nonzero mtime. The next ``/api/models`` for the new +profile sees a different config mtime, takes the config-reload branch, and +``_refresh_config_cache()`` used to treat ``_old_cfg_mtime != 0.0`` as "config +edited" and unlink the disk snapshot — the *target* profile's +``models_cache..json`` — defeating ``delete_disk=False`` on the very next +request. + +``_refresh_config_cache()`` now only deletes the disk snapshot when the same +config.yaml path was already loaded (a real edit of the active profile's +config). These tests drive real profile homes, real config files and real disk +cache files through ``switch_profile()`` + ``get_available_models()``. +""" + +from __future__ import annotations + +import os +from pathlib import Path + + +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 _setup_two_profiles(tmp_path: Path, monkeypatch): + """Real default + named profile homes, distinct config mtimes, one disk + cache file per profile. Returns (default_cache, demo_cache, demo_config).""" + import api.config as cfg + import api.profiles as profiles + + monkeypatch.delenv("HERMES_CONFIG_PATH", raising=False) + default_home = tmp_path / ".hermes" + default_home.mkdir() + (default_home / "config.yaml").write_text("model:\n default: default-model\n", encoding="utf-8") + demo_home = default_home / "profiles" / "demo" + demo_home.mkdir(parents=True) + demo_config = demo_home / "config.yaml" + demo_config.write_text("model:\n default: demo-model\n", encoding="utf-8") + os.utime(demo_config, (1_000_000, 1_000_000)) # guarantee a different mtime + + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", default_home) + monkeypatch.setattr(profiles, "_active_profile", "default") + monkeypatch.setattr(profiles._tls, "profile", None, raising=False) + + default_cache = tmp_path / "models_cache.json" + demo_cache = tmp_path / "models_cache.demo.json" + default_cache.write_text("{}", encoding="utf-8") + demo_cache.write_text("{}", encoding="utf-8") + monkeypatch.setattr(cfg, "_models_cache_path", default_cache) + + # Serve the disk snapshot; never allow a live rebuild. + monkeypatch.setattr(cfg, "_load_models_cache_from_disk", lambda: _catalog("disk-model")) + monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) + + def _unexpected_rebuild(*_a, **_kw): + raise AssertionError("must not trigger a live models rebuild") + + monkeypatch.setattr(cfg, "_invoke_models_rebuild", _unexpected_rebuild) + + # Snapshot/restore the process-global config state touched by reload_config(). + for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): + monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) + saved_cache = dict(cfg._cfg_cache) + + def _restore(): + cfg._cfg_cache.clear() + cfg._cfg_cache.update(saved_cache) + + monkeypatch.setattr(cfg, "_available_models_cache", None, raising=False) + monkeypatch.setattr(cfg, "_available_models_cache_source_fingerprint", None, raising=False) + monkeypatch.setattr(cfg, "_cache_build_in_progress", False, raising=False) + + # Load the default profile's config as the server would have at boot. + cfg.reload_config() + assert cfg._cfg_path == default_home / "config.yaml" + assert cfg._cfg_mtime != 0.0 + + return default_cache, demo_cache, demo_config, _restore + + +def test_first_models_request_after_per_client_switch_keeps_target_disk_cache(tmp_path, monkeypatch): + import api.config as cfg + import api.profiles as profiles + + default_cache, demo_cache, _demo_config, restore = _setup_two_profiles(tmp_path, monkeypatch) + try: + # Warm the default profile: /api/models with the default cookie. + cfg.get_available_models() + assert default_cache.exists() + + # Per-client switch, exactly as the route does it. + profiles.switch_profile("demo", process_wide=False) + cfg.invalidate_models_cache(delete_disk=False) + assert demo_cache.exists() + prev_mtime = cfg._cfg_mtime + assert prev_mtime != 0.0, "per-client switch leaves the previous profile's mtime in place" + + # First /api/models from that client (cookie → thread-local profile). + profiles.set_request_profile("demo") + try: + result = cfg.get_available_models() + finally: + profiles.clear_request_profile() + + assert result["default_model"] == "disk-model" + assert cfg._cfg_path == _demo_config, "config-reload branch must have re-pointed to the demo config" + assert cfg._cfg_mtime == _demo_config.stat().st_mtime + assert demo_cache.exists(), ( + "first /api/models after a per-client profile switch must not unlink the " + "target profile's models_cache.demo.json" + ) + assert default_cache.exists() + finally: + restore() + + +def test_same_profile_config_edit_still_deletes_disk_cache(tmp_path, monkeypatch): + """The path guard must not weaken the real-edit contract: an mtime change + on the *same* config.yaml still drops the disk snapshot.""" + import api.config as cfg + import api.profiles as profiles + + default_cache, demo_cache, demo_config, restore = _setup_two_profiles(tmp_path, monkeypatch) + try: + profiles.switch_profile("demo", process_wide=False) + cfg.invalidate_models_cache(delete_disk=False) + profiles.set_request_profile("demo") + try: + cfg.get_available_models() + assert demo_cache.exists() + # Edit demo's config.yaml → mtime moves on the same path. + demo_config.write_text("model:\n default: demo-model-2\n", encoding="utf-8") + os.utime(demo_config, (2_000_000, 2_000_000)) + cfg.invalidate_models_cache(delete_disk=False) + cfg.get_available_models() + finally: + profiles.clear_request_profile() + assert not demo_cache.exists(), "editing the active profile's config.yaml must still delete its disk cache" + assert default_cache.exists() + finally: + restore() From 7f2e812280c17daf3d2e30440e80452417b65a02 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Wed, 23 Sep 2026 15:31:44 +0000 Subject: [PATCH 06/10] fix: fingerprint .env key names and provider plugins; drop a profile's models cache on delete/create A retained per-profile snapshot is now rejected after a switch when the target profile's .env credential set or model-provider plugins changed, and a recreated same-name profile no longer inherits the deleted profile's catalog. --- api/config.py | 135 +++++++++++-- api/profiles.py | 12 ++ .../architecture/models-cache-invalidation.md | 36 +++- ...profile_switch_models_cache_source_axes.py | 177 ++++++++++++++++++ .../test_profile_switch_models_disk_cache.py | 6 +- 5 files changed, 334 insertions(+), 32 deletions(-) create mode 100644 tests/test_profile_switch_models_cache_source_axes.py diff --git a/api/config.py b/api/config.py index 46710c35..020b010c 100644 --- a/api/config.py +++ b/api/config.py @@ -7603,25 +7603,36 @@ def _get_models_cache_path() -> Path: ``_models_cache_path`` to an isolated tmp file). """ try: - from api.profiles import get_active_profile_name, _is_root_profile + from api.profiles import get_active_profile_name - name = (get_active_profile_name() or "").strip() - if not name or _is_root_profile(name): - return _models_cache_path - # Defensive filename sanitization: the cookie-derived profile name is - # already validated by _PROFILE_ID_RE at the request boundary, but keep - # the on-disk filename safe regardless of how the name was resolved. - safe = re.sub(r"[^a-z0-9_-]", "_", name.lower())[:64] - if not safe: - return _models_cache_path - # Splice the profile into the default filename: models_cache.json → - # models_cache..json, keeping the default's parent dir + suffix. - base = _models_cache_path - return base.with_name(f"{base.stem}.{safe}{base.suffix}") + return _models_cache_path_for_profile(get_active_profile_name()) except Exception: return _models_cache_path +def _models_cache_path_for_profile(name: str | None) -> Path: + """Return the /api/models disk-cache path owned by profile *name*.""" + from api.profiles import _is_root_profile + + name = (name or "").strip() + if not name or _is_root_profile(name): + return _models_cache_path + # Keep the on-disk filename safe regardless of how the name was resolved. + safe = re.sub(r"[^a-z0-9_-]", "_", name.lower())[:64] + if not safe: + return _models_cache_path + base = _models_cache_path + return base.with_name(f"{base.stem}.{safe}{base.suffix}") + + +def delete_profile_models_cache(name: str) -> None: + """Unlink profile *name*'s models disk snapshot (profile delete/create).""" + try: + os.unlink(str(_models_cache_path_for_profile(name))) + except (OSError, ValueError): + pass + + def _get_auth_store_path() -> Path: """Return the auth.json path for the active Hermes profile.""" try: @@ -7892,6 +7903,88 @@ def _auth_store_semantic_fingerprint(path: Path) -> dict: return fp +def _active_profile_home() -> Path: + try: + from api.profiles import get_active_hermes_home as _gah + + return _gah() + except ImportError: + return _DEFAULT_HERMES_HOME + + +def _models_cache_env_fingerprint(path: Path) -> dict: + """Return the sorted names of non-empty keys in a profile ``.env``. + + Credential presence decides which providers are detected; values are + never recorded, so rotating a key keeps the snapshot. + """ + p = Path(path).expanduser() + fp: dict = {"path": str(p)} + try: + text = p.read_text(encoding="utf-8") + except FileNotFoundError: + fp["missing"] = True + return fp + except OSError: + st = p.stat() + fp["mtime_ns"], fp["size"] = st.st_mtime_ns, st.st_size + return fp + keys = set() + for line in text.splitlines(): + line = line.strip() + if line.startswith("export "): + line = line[len("export "):].lstrip() + if not line or line.startswith("#") or "=" not in line: + continue + k, v = line.split("=", 1) + if v.strip().strip('"').strip("'"): + keys.add(k.strip()) + fp["present_keys"] = sorted(keys) + return fp + + +def _plugin_manifest_fields(plugin_dir: Path) -> dict | None: + for filename in ("plugin.yaml", "plugin.yml"): + manifest = plugin_dir / filename + try: + text = manifest.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + fields = {} + for line in text.splitlines(): + key, sep, value = line.partition(":") + if sep and not line[:1].isspace() and key.strip() in ("name", "kind", "version"): + fields[key.strip()] = value.strip().strip("\"'") + return fields + return None + + +def _models_cache_plugin_fingerprint(home: Path) -> list: + """Return ``[dir, name, version]`` for the profile's model-provider plugins. + + Mirrors providers' discovery: every dir under ``plugins/model-providers/`` + plus flat ``plugins/`` entries whose manifest declares that kind. + """ + found = [] + plugins_root = Path(home).expanduser() / "plugins" + for base, flat in ((plugins_root / "model-providers", False), (plugins_root, True)): + try: + children = sorted(base.iterdir()) + except OSError: + continue + for child in children: + if not child.is_dir() or child.name.startswith(("_", ".")): + continue + if flat and child.name == "model-providers": + continue + fields = _plugin_manifest_fields(child) + if flat and (fields or {}).get("kind") != "model-provider": + continue + fields = fields or {} + found.append([str(child.relative_to(plugins_root)), fields.get("name", ""), fields.get("version", "")]) + return found + + def _models_cache_source_fingerprint() -> dict: """Return the current config/auth/catalog fingerprint for /api/models cache. @@ -7903,9 +7996,12 @@ def _models_cache_source_fingerprint() -> dict: mtime/size fingerprint because it is only rewritten on deliberate user edits (which can change anything) and does not churn on a timer. """ + home = _active_profile_home() return { "config_yaml": _models_cache_file_fingerprint(_get_config_path()), "auth_json": _auth_store_semantic_fingerprint(_get_auth_store_path()), + "env": _models_cache_env_fingerprint(home / ".env"), + "plugins": _models_cache_plugin_fingerprint(home), "catalog": _models_cache_catalog_fingerprint(), } @@ -8181,12 +8277,11 @@ def invalidate_models_cache(*, delete_disk: bool = True): 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). + per-profile disk cache in place. Use it when the caller only needs the + next request to re-resolve which profile's catalog to serve (e.g. a + per-client profile switch). The snapshot is reused only while + _models_cache_source_fingerprint() (config, auth, .env key names, + provider plugins, catalog) still matches. """ 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 diff --git a/api/profiles.py b/api/profiles.py index 78f18735..1250899e 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -2623,6 +2623,7 @@ def create_profile_api(name: str, clone_from: str = None, model_provider=model_provider, ) + _drop_profile_models_cache(name) # Invalidate cached root-profile-name lookup; create_profile may have added # a new profile that flips is_default semantics on the agent side (#1612). _SKILLS_STATS_CACHE.clear() @@ -2651,6 +2652,16 @@ def create_profile_api(name: str, clone_from: str = None, } +def _drop_profile_models_cache(name: str) -> None: + """A deleted or new profile must never inherit a same-name models snapshot.""" + try: + from api.config import delete_profile_models_cache + + delete_profile_models_cache(name) + except Exception: + logger.debug("Failed to drop models cache for profile %s", name, exc_info=True) + + def delete_profile_api(name: str) -> dict: """Delete a profile. Switches to default first if it's the active one. @@ -2684,6 +2695,7 @@ def delete_profile_api(name: str) -> dict: else: raise ValueError(f"Profile '{name}' does not exist.") + _drop_profile_models_cache(name) # Drop cached root-profile-name lookup — list_profiles_api() shape changed. _SKILLS_STATS_CACHE.clear() _invalidate_list_profiles_cache() diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index 9fd3a798..fab7a7d8 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -20,7 +20,7 @@ which the #7556 review flagged as an undocumented runtime contract. `_sync_models_cache_provenance()` must run at every site that publishes or invalidates the snapshot, so the tuple can never tear. -## The three source axes +## The five source axes `_models_cache_source_fingerprint()` is the single chokepoint. A cache is served only when every axis matches the value recorded in the cache — the disk reader @@ -31,6 +31,8 @@ the fingerprint captured at publish time. | --- | --- | --- | | `config_yaml` | stat identity: `mtime_ns` + size (`_models_cache_file_fingerprint`) | The file is rewritten only on deliberate user edits, and any edit can change the provider/model set, so the cheap conservative identity wins. | | `auth_json` | content hash with a volatile-key deny-list (`_auth_store_semantic_fingerprint`, `_AUTH_FINGERPRINT_VOLATILE_KEYS`) | The credential store is rewritten roughly every 14 minutes by credential-pool / OAuth refresh; none of those rotating fields feed `detected_providers` or the returned catalog, and stat identity made the 24h cache churn on every refresh (RCA `t_d127953d` / `t_16551f61`). | +| `env` | sorted **names** of non-empty keys in the active profile's `.env` (`_models_cache_env_fingerprint`); values are never recorded | Env credentials decide `detected_providers`. Adding or removing a key changes the catalog; rotating a value does not. | +| `plugins` | `[dir, name, version]` for each model-provider plugin in the active profile home (`_models_cache_plugin_fingerprint`): every dir under `plugins/model-providers/`, plus flat `plugins/` entries whose `plugin.yaml` declares `kind: model-provider` | Plugin providers feed the catalog through `api/plugin_providers.py`. Installing, removing, or upgrading one must invalidate the catalog. The discovery rules match `providers._scan_home_layer`. | | `catalog` | baked-in provider catalog sha256 (`_PROVIDER_MODELS` + `_PROVIDER_DISPLAY`) plus the Codex local catalog (`_codex_models_cache_fingerprint`, `_CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS`) | A restart after a catalog change must not keep serving a persisted payload for up to 24h (#2443). Codex rewrites `~/.codex/models_cache.json` on its own timer, bumping `mtime_ns` and size while models, `etag`, and `client_version` stay identical, so the Codex axis hashes **content** with only the refresh timestamps (`fetched_at`, `updated_at`) removed (#7540, #7556). | ## Invariant: deny-lists are one-directional @@ -99,14 +101,21 @@ keeps this mode. `delete_disk=False` is for when the sources have **not** changed and the caller only needs the next request to re-resolve *which* profile's catalog to serve. -`POST /api/profile/switch` uses `delete_disk=False`. The disk cache is already -keyed per profile (`_get_models_cache_path()`), and a stale or wrong-profile -snapshot is rejected on read by `_is_loadable_disk_cache()` via the source -fingerprint above, so deleting it on a switch bought no correctness — it only -forced a full cold rebuild (live provider `fetch_models` calls, several seconds) -on every switch. Because this mode leans entirely on the fingerprint check, it -is safe only while that check stays the single gate for serving a disk snapshot -(change-protocol items 1 and 4). +`POST /api/profile/switch` uses `delete_disk=False`. The disk cache is keyed +per profile (`_get_models_cache_path()`), and `_is_loadable_disk_cache()` +rejects any snapshot whose source fingerprint no longer matches. The fingerprint +covers every profile-local input to the catalog: `config.yaml`, `auth.json`, +the `.env` key names, and the model-provider plugins. A switch therefore reuses +the snapshot only when none of those inputs changed, and skips the full cold +rebuild (live provider `fetch_models` calls). This mode depends on the +fingerprint being the only gate for serving a disk snapshot (change-protocol +items 1 and 4). A new catalog input must be added as an axis before anything +can rely on this mode. + +A profile's snapshot is not stored in its home, so deleting the home leaves the +file behind. `delete_profile_api()` and `create_profile_api()` both unlink +`models_cache..json` (`delete_profile_models_cache()`), so a profile that +is recreated with the same name never inherits the old catalog. ## Change protocol @@ -137,6 +146,15 @@ visibility change, any catalog field, any unknown field — still invalidate. reloads it without a live rebuild, the default still unlinks it, and the executed `/api/profile/switch` route passes `delete_disk=False`. +`tests/test_profile_switch_models_cache_source_axes.py` covers the safety side +of that mode. It saves a real snapshot, changes one source in the target +profile, runs the switch, and asserts a fresh rebuild for each of these cases: +adding or removing a `.env` key, installing a plugin (in either location), +removing a plugin, bumping a plugin's version, and deleting then recreating the +profile. Two tests pin the speed-up: with unchanged sources, and after rotating +a `.env` value, the switch still reuses the snapshot, and no secret value is +written to disk. + ## References Issues/PRs: #2443, #7540, #7556, #7558. RCAs: `t_d127953d`, `t_16551f61`. diff --git a/tests/test_profile_switch_models_cache_source_axes.py b/tests/test_profile_switch_models_cache_source_axes.py new file mode 100644 index 00000000..90a86948 --- /dev/null +++ b/tests/test_profile_switch_models_cache_source_axes.py @@ -0,0 +1,177 @@ +"""A retained per-profile models disk snapshot must be rejected after a switch +when a catalog source changed in the target profile: its ``.env`` provider +credentials, its installed model-provider plugins, or the profile itself +(delete + recreate with the same name). + +Each test saves a real snapshot for profile ``demo`` through +``_save_models_cache_to_disk()``, changes one source while ``default`` is +active, performs the per-client switch exactly as ``/api/profile/switch`` does +(``invalidate_models_cache(delete_disk=False)``), and asserts the first +``get_available_models()`` for ``demo`` serves the fresh rebuild. +""" + +from __future__ import annotations + +import os +import shutil +import sys +from types import SimpleNamespace +from pathlib import Path + +import pytest + + +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": {}, + } + + +@pytest.fixture +def two_profiles(tmp_path: Path, monkeypatch): + import api.config as cfg + import api.profiles as profiles + + monkeypatch.delenv("HERMES_CONFIG_PATH", raising=False) + default_home = tmp_path / ".hermes" + default_home.mkdir() + (default_home / "config.yaml").write_text("model:\n default: default-model\n", encoding="utf-8") + demo_home = default_home / "profiles" / "demo" + demo_home.mkdir(parents=True) + (demo_home / "config.yaml").write_text("model:\n default: demo-model\n", encoding="utf-8") + os.utime(demo_home / "config.yaml", (1_000_000, 1_000_000)) + + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", default_home) + monkeypatch.setattr(profiles, "_active_profile", "default") + monkeypatch.setattr(profiles._tls, "profile", None, raising=False) + monkeypatch.setattr(cfg, "_models_cache_path", tmp_path / "models_cache.json") + monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) + monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _builder: _catalog("fresh-model")) + for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): + monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) + saved_cfg = dict(cfg._cfg_cache) + cfg.invalidate_models_cache(delete_disk=False) + cfg.reload_config() + yield SimpleNamespace(cfg=cfg, profiles=profiles, demo_home=demo_home, cache=tmp_path / "models_cache.demo.json") + profiles.clear_request_profile() + cfg.invalidate_models_cache(delete_disk=False) + cfg._cfg_cache.clear() + cfg._cfg_cache.update(saved_cfg) + + +def _save_demo_snapshot(env) -> None: + env.profiles.set_request_profile("demo") + try: + env.cfg._save_models_cache_to_disk(_catalog("snapshot-model")) + finally: + env.profiles.clear_request_profile() + assert env.cache.exists() + + +def _switch_to_demo_and_fetch(env) -> dict: + env.profiles.switch_profile("demo", process_wide=False) + env.cfg.invalidate_models_cache(delete_disk=False) + env.profiles.set_request_profile("demo") + try: + return env.cfg.get_available_models() + finally: + env.profiles.clear_request_profile() + + +def _write_plugin(root: Path, name: str, version: str, *, flat: bool) -> Path: + base = root / "plugins" if flat else root / "plugins" / "model-providers" + d = base / name + d.mkdir(parents=True, exist_ok=True) + (d / "plugin.yaml").write_text( + f"name: {name}\nkind: model-provider\nversion: {version}\n", encoding="utf-8" + ) + return d + + +def test_unchanged_sources_keep_serving_the_retained_snapshot(two_profiles): + """Control: with no source change the switch reuses the snapshot (the speed-up).""" + _save_demo_snapshot(two_profiles) + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "snapshot-model" + + +def test_switch_after_adding_env_key_rejects_snapshot(two_profiles): + _save_demo_snapshot(two_profiles) + (two_profiles.demo_home / ".env").write_text("DEEPSEEK_API_KEY=sk-new\n", encoding="utf-8") + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_switch_after_removing_env_key_rejects_snapshot(two_profiles): + env_file = two_profiles.demo_home / ".env" + env_file.write_text("DEEPSEEK_API_KEY=sk-old\nOTHER=1\n", encoding="utf-8") + _save_demo_snapshot(two_profiles) + env_file.write_text("OTHER=1\n", encoding="utf-8") + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_env_value_rotation_keeps_snapshot_and_fingerprint_has_no_secret(two_profiles): + env_file = two_profiles.demo_home / ".env" + env_file.write_text("DEEPSEEK_API_KEY=sk-secret-one\n", encoding="utf-8") + _save_demo_snapshot(two_profiles) + assert "sk-secret-one" not in two_profiles.cache.read_text(encoding="utf-8") + env_file.write_text("DEEPSEEK_API_KEY=sk-secret-two\n", encoding="utf-8") + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "snapshot-model" + + +@pytest.mark.parametrize("flat", [False, True], ids=["model-providers-dir", "flat-install-dir"]) +def test_switch_after_installing_plugin_rejects_snapshot(two_profiles, flat): + _save_demo_snapshot(two_profiles) + _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=flat) + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_switch_after_removing_plugin_rejects_snapshot(two_profiles): + plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=False) + _save_demo_snapshot(two_profiles) + shutil.rmtree(plugin) + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_switch_after_plugin_version_change_rejects_snapshot(two_profiles): + _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=True) + _save_demo_snapshot(two_profiles) + _write_plugin(two_profiles.demo_home, "acme", "2.0.0", flat=True) + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_delete_profile_removes_its_models_cache(two_profiles, monkeypatch): + import api.profiles as profiles + + monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) + monkeypatch.setitem(sys.modules, "hermes_cli.profiles", None) + _save_demo_snapshot(two_profiles) + profiles.delete_profile_api("demo") + assert not two_profiles.demo_home.exists() + assert not two_profiles.cache.exists() + + +def test_recreate_same_name_profile_does_not_resurrect_old_catalog(two_profiles, monkeypatch): + import api.profiles as profiles + + monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) + monkeypatch.setitem(sys.modules, "hermes_cli.profiles", None) + monkeypatch.setattr(profiles, "list_profiles_api", lambda: []) + _save_demo_snapshot(two_profiles) + stale = two_profiles.cache.read_text(encoding="utf-8") + profiles.delete_profile_api("demo") + # A snapshot left behind by an older build or another process. + two_profiles.cache.write_text(stale, encoding="utf-8") + profiles.create_profile_api("demo") + assert not two_profiles.cache.exists() + (two_profiles.demo_home / "config.yaml").write_text("model:\n default: demo-model\n", encoding="utf-8") + os.utime(two_profiles.demo_home / "config.yaml", (1_000_000, 1_000_000)) + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" diff --git a/tests/test_profile_switch_models_disk_cache.py b/tests/test_profile_switch_models_disk_cache.py index 3f121971..37dc386b 100644 --- a/tests/test_profile_switch_models_disk_cache.py +++ b/tests/test_profile_switch_models_disk_cache.py @@ -5,9 +5,9 @@ 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. +changed. The disk cache is keyed per profile and rejected on read when +``_models_cache_source_fingerprint()`` differs; the source changes that must +still force a rebuild are covered in test_profile_switch_models_cache_source_axes.py. The switch route now uses ``invalidate_models_cache(delete_disk=False)``. """ From 4e173335164c6bcdc2ef8bcc35fc0e93259a7570 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Wed, 23 Sep 2026 15:52:14 +0000 Subject: [PATCH 07/10] fix: fingerprint .env key names with the provider .env loader --- api/config.py | 23 ++++--------------- ...profile_switch_models_cache_source_axes.py | 18 +++++++++++++++ 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/api/config.py b/api/config.py index 020b010c..05094e48 100644 --- a/api/config.py +++ b/api/config.py @@ -7918,28 +7918,15 @@ def _models_cache_env_fingerprint(path: Path) -> dict: Credential presence decides which providers are detected; values are never recorded, so rotating a key keeps the snapshot. """ + # Same parser provider detection uses, so the key names match exactly. + from api.providers import _load_env_file + p = Path(path).expanduser() fp: dict = {"path": str(p)} - try: - text = p.read_text(encoding="utf-8") - except FileNotFoundError: + if not p.exists(): fp["missing"] = True return fp - except OSError: - st = p.stat() - fp["mtime_ns"], fp["size"] = st.st_mtime_ns, st.st_size - return fp - keys = set() - for line in text.splitlines(): - line = line.strip() - if line.startswith("export "): - line = line[len("export "):].lstrip() - if not line or line.startswith("#") or "=" not in line: - continue - k, v = line.split("=", 1) - if v.strip().strip('"').strip("'"): - keys.add(k.strip()) - fp["present_keys"] = sorted(keys) + fp["present_keys"] = sorted(k for k, v in _load_env_file(p).items() if v) return fp diff --git a/tests/test_profile_switch_models_cache_source_axes.py b/tests/test_profile_switch_models_cache_source_axes.py index 90a86948..ffba7d5e 100644 --- a/tests/test_profile_switch_models_cache_source_axes.py +++ b/tests/test_profile_switch_models_cache_source_axes.py @@ -118,6 +118,24 @@ def test_switch_after_removing_env_key_rejects_snapshot(two_profiles): assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" +def test_switch_after_export_prefix_rejects_snapshot(two_profiles): + """`export KEY=` is key `export KEY` to the profile .env loaders, so the provider key is gone.""" + env_file = two_profiles.demo_home / ".env" + env_file.write_text("DEEPSEEK_API_KEY=sk-old\n", encoding="utf-8") + _save_demo_snapshot(two_profiles) + env_file.write_text("export DEEPSEEK_API_KEY=sk-old\n", encoding="utf-8") + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_env_fingerprint_keys_match_provider_env_loader(two_profiles): + from api.providers import _load_env_file + + env_file = two_profiles.demo_home / ".env" + env_file.write_text("export A=1\nB='x'\nC=\n# D=1\n", encoding="utf-8") + loaded = sorted(k for k, v in _load_env_file(env_file).items() if v) + assert two_profiles.cfg._models_cache_env_fingerprint(env_file)["present_keys"] == loaded + + def test_env_value_rotation_keeps_snapshot_and_fingerprint_has_no_secret(two_profiles): env_file = two_profiles.demo_home / ".env" env_file.write_text("DEEPSEEK_API_KEY=sk-secret-one\n", encoding="utf-8") From 35bb952defad13896e705c02300eff255e1cf089 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Wed, 23 Sep 2026 18:00:35 +0000 Subject: [PATCH 08/10] fix: fingerprint model-provider plugin file contents, not just manifest version --- api/config.py | 23 +++++++++++-- .../architecture/models-cache-invalidation.md | 4 +-- ...profile_switch_models_cache_source_axes.py | 32 +++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/api/config.py b/api/config.py index 1650865d..bf55bcb0 100644 --- a/api/config.py +++ b/api/config.py @@ -7978,7 +7978,7 @@ def _plugin_manifest_fields(plugin_dir: Path) -> dict | None: def _models_cache_plugin_fingerprint(home: Path) -> list: - """Return ``[dir, name, version]`` for the profile's model-provider plugins. + """Return ``[dir, file stamps]`` for the profile's model-provider plugins. Mirrors providers' discovery: every dir under ``plugins/model-providers/`` plus flat ``plugins/`` entries whose manifest declares that kind. @@ -7998,11 +7998,28 @@ def _models_cache_plugin_fingerprint(home: Path) -> list: fields = _plugin_manifest_fields(child) if flat and (fields or {}).get("kind") != "model-provider": continue - fields = fields or {} - found.append([str(child.relative_to(plugins_root)), fields.get("name", ""), fields.get("version", "")]) + found.append([str(child.relative_to(plugins_root)), _plugin_tree_stamps(child)]) return found +def _plugin_tree_stamps(plugin_dir: Path) -> list: + # The loader execs __init__.py, which can import sibling modules or read data files, + # so stamp every file in the tree; bytecode caches are derived and skipped. + stamps = [] + for root, dirs, files in os.walk(plugin_dir): + dirs[:] = sorted(d for d in dirs if d != "__pycache__" and not d.startswith(".")) + for name in sorted(files): + if name.endswith((".pyc", ".pyo")): + continue + path = os.path.join(root, name) + try: + st = os.stat(path) + except OSError: + continue + stamps.append([os.path.relpath(path, plugin_dir), st.st_mtime_ns, st.st_size]) + return stamps + + def _models_cache_source_fingerprint() -> dict: """Return the current config/auth/catalog fingerprint for /api/models cache. diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index fab7a7d8..19eb48fc 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -32,7 +32,7 @@ the fingerprint captured at publish time. | `config_yaml` | stat identity: `mtime_ns` + size (`_models_cache_file_fingerprint`) | The file is rewritten only on deliberate user edits, and any edit can change the provider/model set, so the cheap conservative identity wins. | | `auth_json` | content hash with a volatile-key deny-list (`_auth_store_semantic_fingerprint`, `_AUTH_FINGERPRINT_VOLATILE_KEYS`) | The credential store is rewritten roughly every 14 minutes by credential-pool / OAuth refresh; none of those rotating fields feed `detected_providers` or the returned catalog, and stat identity made the 24h cache churn on every refresh (RCA `t_d127953d` / `t_16551f61`). | | `env` | sorted **names** of non-empty keys in the active profile's `.env` (`_models_cache_env_fingerprint`); values are never recorded | Env credentials decide `detected_providers`. Adding or removing a key changes the catalog; rotating a value does not. | -| `plugins` | `[dir, name, version]` for each model-provider plugin in the active profile home (`_models_cache_plugin_fingerprint`): every dir under `plugins/model-providers/`, plus flat `plugins/` entries whose `plugin.yaml` declares `kind: model-provider` | Plugin providers feed the catalog through `api/plugin_providers.py`. Installing, removing, or upgrading one must invalidate the catalog. The discovery rules match `providers._scan_home_layer`. | +| `plugins` | `[dir, [relpath, mtime_ns, size] per file]` for each model-provider plugin in the active profile home (`_models_cache_plugin_fingerprint`, `_plugin_tree_stamps`): every dir under `plugins/model-providers/`, plus flat `plugins/` entries whose `plugin.yaml` declares `kind: model-provider`; `__pycache__`/`.pyc` are skipped | Plugin providers feed the catalog through `api/plugin_providers.py`, and `fallback_models` comes from the plugin code the loader execs (`__init__.py` and anything it imports), so any file edit must invalidate the catalog even without a `version` bump. The discovery rules match `providers._scan_home_layer`. | | `catalog` | baked-in provider catalog sha256 (`_PROVIDER_MODELS` + `_PROVIDER_DISPLAY`) plus the Codex local catalog (`_codex_models_cache_fingerprint`, `_CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS`) | A restart after a catalog change must not keep serving a persisted payload for up to 24h (#2443). Codex rewrites `~/.codex/models_cache.json` on its own timer, bumping `mtime_ns` and size while models, `etag`, and `client_version` stay identical, so the Codex axis hashes **content** with only the refresh timestamps (`fetched_at`, `updated_at`) removed (#7540, #7556). | ## Invariant: deny-lists are one-directional @@ -150,7 +150,7 @@ executed `/api/profile/switch` route passes `delete_disk=False`. of that mode. It saves a real snapshot, changes one source in the target profile, runs the switch, and asserts a fresh rebuild for each of these cases: adding or removing a `.env` key, installing a plugin (in either location), -removing a plugin, bumping a plugin's version, and deleting then recreating the +removing a plugin, bumping a plugin's version, editing plugin code without a version bump, and deleting then recreating the profile. Two tests pin the speed-up: with unchanged sources, and after rotating a `.env` value, the switch still reuses the snapshot, and no secret value is written to disk. diff --git a/tests/test_profile_switch_models_cache_source_axes.py b/tests/test_profile_switch_models_cache_source_axes.py index ffba7d5e..dd7c4b09 100644 --- a/tests/test_profile_switch_models_cache_source_axes.py +++ b/tests/test_profile_switch_models_cache_source_axes.py @@ -193,3 +193,35 @@ def test_recreate_same_name_profile_does_not_resurrect_old_catalog(two_profiles, (two_profiles.demo_home / "config.yaml").write_text("model:\n default: demo-model\n", encoding="utf-8") os.utime(two_profiles.demo_home / "config.yaml", (1_000_000, 1_000_000)) assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +@pytest.mark.parametrize("flat", [False, True], ids=["model-providers-dir", "flat-install-dir"]) +def test_switch_after_plugin_code_change_without_version_bump_rejects_snapshot(two_profiles, flat): + """fallback_models lives in the plugin's code, which the loader imports; the version may not move.""" + plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=flat) + init = plugin / "__init__.py" + init.write_text("FALLBACK_MODELS = ('acme-1',)\n", encoding="utf-8") + os.utime(init, (1_000_000, 1_000_000)) + _save_demo_snapshot(two_profiles) + init.write_text("FALLBACK_MODELS = ('acme-1', 'acme-2')\n", encoding="utf-8") + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_switch_after_plugin_submodule_change_rejects_snapshot(two_profiles): + plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=False) + (plugin / "__init__.py").write_text("from .models import FALLBACK_MODELS\n", encoding="utf-8") + sub = plugin / "models.py" + sub.write_text("FALLBACK_MODELS = ('acme-1',)\n", encoding="utf-8") + os.utime(sub, (1_000_000, 1_000_000)) + _save_demo_snapshot(two_profiles) + sub.write_text("FALLBACK_MODELS = ('acme-1', 'acme-2')\n", encoding="utf-8") + assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" + + +def test_plugin_bytecode_cache_does_not_churn_fingerprint(two_profiles): + plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=False) + (plugin / "__init__.py").write_text("X = 1\n", encoding="utf-8") + before = two_profiles.cfg._models_cache_plugin_fingerprint(two_profiles.demo_home) + (plugin / "__pycache__").mkdir() + (plugin / "__pycache__" / "__init__.cpython-311.pyc").write_bytes(b"x") + assert two_profiles.cfg._models_cache_plugin_fingerprint(two_profiles.demo_home) == before From 7344c8df77cfec747d2230ed1b4721e740b8538d Mon Sep 17 00:00:00 2001 From: carlotestor Date: Wed, 23 Sep 2026 19:11:19 +0000 Subject: [PATCH 09/10] refactor: shrink profile-switch models cache diff Reuse _get_models_cache_path(profile) for profile delete/create instead of a second path helper, derive the fingerprint home from the auth store path, fold the three switch test files into one parametrized file, and trim comments/docs. Behaviour, source axes and switch fast path are unchanged. --- api/config.py | 121 ++----- api/profiles.py | 8 +- api/routes.py | 8 +- .../architecture/models-cache-invalidation.md | 74 +--- tests/test_gateway_watcher_profile.py | 4 +- ...profile_switch_models_cache_source_axes.py | 227 ------------- .../test_profile_switch_models_disk_cache.py | 320 +++++++++--------- ...ch_next_models_request_keeps_disk_cache.py | 156 --------- 8 files changed, 215 insertions(+), 703 deletions(-) delete mode 100644 tests/test_profile_switch_models_cache_source_axes.py delete mode 100644 tests/test_profile_switch_next_models_request_keeps_disk_cache.py diff --git a/api/config.py b/api/config.py index bf55bcb0..5c9294f9 100644 --- a/api/config.py +++ b/api/config.py @@ -564,13 +564,8 @@ def _refresh_config_cache(config_path: Path | None = None) -> None: if config_path is None: config_path = _get_config_path() _cfg_cache.clear() - # Remember the old mtime AND path so we can tell whether *this* config file - # actually changed vs. a first-ever load (mtime == 0.0, server start) or a - # load of a different profile's config.yaml. _cfg_mtime is process-global, - # so after a per-client profile switch (process_wide=False skips - # reload_config()) it still holds the previous profile's nonzero mtime; - # without the path check the next /api/models on the new profile would - # look like an edit and unlink the new profile's disk snapshot. + # Remember the old mtime so we can tell whether config actually changed + # vs. first-ever load (mtime == 0.0, e.g. server start or profile switch). _old_cfg_mtime = _cfg_mtime _old_cfg_path = _cfg_path _cfg_path = config_path @@ -628,14 +623,11 @@ def _refresh_config_cache(config_path: Path | None = None) -> None: _apply_config_defaults(_cfg_cache) _cfg_fingerprint = _fingerprint_config(_cfg_cache) # Bust the models cache so the next request sees fresh config values. - # Only delete the disk cache when the SAME config file was already loaded - # (a real edit / explicit reload of the active profile's config.yaml) -- - # not on first-ever load (_old_cfg_mtime == 0.0, server start) and not - # when the path changed because the request belongs to another profile - # (per-client profile switch). The per-profile disk snapshot is - # fingerprint-guarded on read, so a path change needs no unlink; deleting - # it here forced a full cold rebuild on the first /api/models after every - # switch. + # Only delete the disk cache when config has actually changed -- not on + # first-ever load (when _old_cfg_mtime == 0.0, i.e. server start) and not + # on a path change (per-client profile switch leaves the old profile's + # mtime) -- preserving the disk cache so the next restart + # still hits the fast path without a cold run. if _old_cfg_mtime != 0.0 and _old_cfg_path == config_path: _delete_models_cache_on_disk() @@ -7607,7 +7599,7 @@ _MODELS_CACHE_SCHEMA_VERSION = 3 _models_cache_path = STATE_DIR / "models_cache.json" -def _get_models_cache_path() -> Path: +def _get_models_cache_path(profile: str | None = None) -> Path: """Return the /api/models disk-cache path for the *active* profile (#3957). WebUI profile switching is per-client/cookie scoped (issue #798), but the @@ -7631,39 +7623,29 @@ def _get_models_cache_path() -> Path: The named-profile path is derived from ``_models_cache_path`` (the module-level default), not from ``STATE_DIR`` directly, so the path stays correct if the default is repointed (e.g. tests monkeypatch - ``_models_cache_path`` to an isolated tmp file). + ``_models_cache_path`` to an isolated tmp file). Pass *profile* to get + another profile's path (profile delete/create). """ try: - from api.profiles import get_active_profile_name + from api.profiles import get_active_profile_name, _is_root_profile - return _models_cache_path_for_profile(get_active_profile_name()) + name = (profile or get_active_profile_name() or "").strip() + if not name or _is_root_profile(name): + return _models_cache_path + # Defensive filename sanitization: the cookie-derived profile name is + # already validated by _PROFILE_ID_RE at the request boundary, but keep + # the on-disk filename safe regardless of how the name was resolved. + safe = re.sub(r"[^a-z0-9_-]", "_", name.lower())[:64] + if not safe: + return _models_cache_path + # Splice the profile into the default filename: models_cache.json → + # models_cache..json, keeping the default's parent dir + suffix. + base = _models_cache_path + return base.with_name(f"{base.stem}.{safe}{base.suffix}") except Exception: return _models_cache_path -def _models_cache_path_for_profile(name: str | None) -> Path: - """Return the /api/models disk-cache path owned by profile *name*.""" - from api.profiles import _is_root_profile - - name = (name or "").strip() - if not name or _is_root_profile(name): - return _models_cache_path - # Keep the on-disk filename safe regardless of how the name was resolved. - safe = re.sub(r"[^a-z0-9_-]", "_", name.lower())[:64] - if not safe: - return _models_cache_path - base = _models_cache_path - return base.with_name(f"{base.stem}.{safe}{base.suffix}") - - -def delete_profile_models_cache(name: str) -> None: - """Unlink profile *name*'s models disk snapshot (profile delete/create).""" - try: - os.unlink(str(_models_cache_path_for_profile(name))) - except (OSError, ValueError): - pass - - def _get_auth_store_path() -> Path: """Return the auth.json path for the active Hermes profile.""" try: @@ -7943,46 +7925,26 @@ def _active_profile_home() -> Path: return _DEFAULT_HERMES_HOME -def _models_cache_env_fingerprint(path: Path) -> dict: - """Return the sorted names of non-empty keys in a profile ``.env``. - - Credential presence decides which providers are detected; values are - never recorded, so rotating a key keeps the snapshot. - """ - # Same parser provider detection uses, so the key names match exactly. +def _models_cache_env_fingerprint(path: Path) -> list: + """Sorted non-empty ``.env`` key names, parsed like provider detection; values never recorded.""" from api.providers import _load_env_file - p = Path(path).expanduser() - fp: dict = {"path": str(p)} - if not p.exists(): - fp["missing"] = True - return fp - fp["present_keys"] = sorted(k for k, v in _load_env_file(p).items() if v) - return fp + return sorted(k for k, v in _load_env_file(Path(path).expanduser()).items() if v) -def _plugin_manifest_fields(plugin_dir: Path) -> dict | None: - for filename in ("plugin.yaml", "plugin.yml"): - manifest = plugin_dir / filename +def _declares_model_provider_kind(plugin_dir: Path) -> bool: + for manifest in (plugin_dir / "plugin.yaml", plugin_dir / "plugin.yml"): try: - text = manifest.read_text(encoding="utf-8", errors="replace") + lines = manifest.read_text(encoding="utf-8", errors="replace").splitlines() except OSError: continue - fields = {} - for line in text.splitlines(): - key, sep, value = line.partition(":") - if sep and not line[:1].isspace() and key.strip() in ("name", "kind", "version"): - fields[key.strip()] = value.strip().strip("\"'") - return fields - return None + pairs = (line.partition(":") for line in lines if not line[:1].isspace()) + return any(k.strip() == "kind" and v.strip().strip("\"'") == "model-provider" for k, _, v in pairs) + return False def _models_cache_plugin_fingerprint(home: Path) -> list: - """Return ``[dir, file stamps]`` for the profile's model-provider plugins. - - Mirrors providers' discovery: every dir under ``plugins/model-providers/`` - plus flat ``plugins/`` entries whose manifest declares that kind. - """ + """``[dir, file stamps]`` per model-provider plugin, discovered like providers._scan_home_layer.""" found = [] plugins_root = Path(home).expanduser() / "plugins" for base, flat in ((plugins_root / "model-providers", False), (plugins_root, True)): @@ -7993,18 +7955,14 @@ def _models_cache_plugin_fingerprint(home: Path) -> list: for child in children: if not child.is_dir() or child.name.startswith(("_", ".")): continue - if flat and child.name == "model-providers": - continue - fields = _plugin_manifest_fields(child) - if flat and (fields or {}).get("kind") != "model-provider": + if flat and (child.name == "model-providers" or not _declares_model_provider_kind(child)): continue found.append([str(child.relative_to(plugins_root)), _plugin_tree_stamps(child)]) return found def _plugin_tree_stamps(plugin_dir: Path) -> list: - # The loader execs __init__.py, which can import sibling modules or read data files, - # so stamp every file in the tree; bytecode caches are derived and skipped. + # The loader execs __init__.py, which may import siblings or read data files; skip bytecode. stamps = [] for root, dirs, files in os.walk(plugin_dir): dirs[:] = sorted(d for d in dirs if d != "__pycache__" and not d.startswith(".")) @@ -8311,12 +8269,7 @@ def invalidate_models_cache(*, delete_disk: bool = True): 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 caller only needs the - next request to re-resolve which profile's catalog to serve (e.g. a - per-client profile switch). The snapshot is reused only while - _models_cache_source_fingerprint() (config, auth, .env key names, - provider plugins, catalog) still matches. + ``delete_disk=False`` keeps the fingerprint-guarded disk snapshot (profile switch). """ 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 diff --git a/api/profiles.py b/api/profiles.py index b7fd9297..e94e0b84 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -2685,11 +2685,11 @@ def create_profile_api(name: str, clone_from: str = None, def _drop_profile_models_cache(name: str) -> None: """A deleted or new profile must never inherit a same-name models snapshot.""" - try: - from api.config import delete_profile_models_cache + from api.config import _get_models_cache_path - delete_profile_models_cache(name) - except Exception: + try: + _get_models_cache_path(name).unlink(missing_ok=True) + except OSError: logger.debug("Failed to drop models cache for profile %s", name, exc_info=True) diff --git a/api/routes.py b/api/routes.py index 1b459479..4547318d 100644 --- a/api/routes.py +++ b/api/routes.py @@ -17044,12 +17044,10 @@ 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) - # Drop the in-memory catalog so the very next /api/models request - # re-resolves the new profile's config.yaml rather than returning + # Invalidate the models cache so the very next /api/models request + # rebuilds from 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. + # The per-profile disk snapshot is fingerprint-guarded, so keep it. from api.config import invalidate_models_cache invalidate_models_cache(delete_disk=False) try: diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index 19eb48fc..6f0398b1 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -31,8 +31,8 @@ the fingerprint captured at publish time. | --- | --- | --- | | `config_yaml` | stat identity: `mtime_ns` + size (`_models_cache_file_fingerprint`) | The file is rewritten only on deliberate user edits, and any edit can change the provider/model set, so the cheap conservative identity wins. | | `auth_json` | content hash with a volatile-key deny-list (`_auth_store_semantic_fingerprint`, `_AUTH_FINGERPRINT_VOLATILE_KEYS`) | The credential store is rewritten roughly every 14 minutes by credential-pool / OAuth refresh; none of those rotating fields feed `detected_providers` or the returned catalog, and stat identity made the 24h cache churn on every refresh (RCA `t_d127953d` / `t_16551f61`). | -| `env` | sorted **names** of non-empty keys in the active profile's `.env` (`_models_cache_env_fingerprint`); values are never recorded | Env credentials decide `detected_providers`. Adding or removing a key changes the catalog; rotating a value does not. | -| `plugins` | `[dir, [relpath, mtime_ns, size] per file]` for each model-provider plugin in the active profile home (`_models_cache_plugin_fingerprint`, `_plugin_tree_stamps`): every dir under `plugins/model-providers/`, plus flat `plugins/` entries whose `plugin.yaml` declares `kind: model-provider`; `__pycache__`/`.pyc` are skipped | Plugin providers feed the catalog through `api/plugin_providers.py`, and `fallback_models` comes from the plugin code the loader execs (`__init__.py` and anything it imports), so any file edit must invalidate the catalog even without a `version` bump. The discovery rules match `providers._scan_home_layer`. | +| `env` | sorted **names** of non-empty `.env` keys, parsed by `providers._load_env_file` (`_models_cache_env_fingerprint`); values are never recorded | Env credentials decide `detected_providers`; rotating a value does not. | +| `plugins` | `[relpath, mtime_ns, size]` of every non-bytecode file of each model-provider plugin, discovered like `providers._scan_home_layer` (`_models_cache_plugin_fingerprint`) | The loader execs plugin code that builds `fallback_models`, so any file edit must invalidate, even without a `version` bump. | | `catalog` | baked-in provider catalog sha256 (`_PROVIDER_MODELS` + `_PROVIDER_DISPLAY`) plus the Codex local catalog (`_codex_models_cache_fingerprint`, `_CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS`) | A restart after a catalog change must not keep serving a persisted payload for up to 24h (#2443). Codex rewrites `~/.codex/models_cache.json` on its own timer, bumping `mtime_ns` and size while models, `etag`, and `client_version` stay identical, so the Codex axis hashes **content** with only the refresh timestamps (`fetched_at`, `updated_at`) removed (#7540, #7556). | ## Invariant: deny-lists are one-directional @@ -70,52 +70,19 @@ the fingerprint captured at publish time. ## Invalidation paths: memory vs. disk -Every path that drops the published in-memory snapshot resets -`_available_models_cache` (plus its timestamps and source fingerprint) and -calls `_sync_models_cache_provenance()` so the hot-path tuple cannot tear. They -differ in what they do to the per-profile `models_cache.json` on disk -(`_delete_models_cache_on_disk()`): - | Path | In-memory snapshot | Disk snapshot | | --- | --- | --- | -| `invalidate_models_cache(delete_disk=True)` (default) | dropped | **deleted** unconditionally | -| `invalidate_models_cache(delete_disk=False)` | dropped | left in place | -| `invalidate_provider_models_cache(provider_id)` | dropped | **deleted** unconditionally (no `delete_disk` option) | -| `_get_fresh_memory_models_cache()` on a fingerprint mismatch or invalid cached shape | dropped | untouched | -| config-reload branch in `get_available_models()` (`_cfg_changed`) | dropped | deleted by `reload_config_if_stale()` → `_refresh_config_cache()` **only if** the *same* `config.yaml` path was already loaded and its mtime moved (`_old_cfg_mtime != 0.0 and _old_cfg_path == config_path`, i.e. a real edit of the active profile's config); a first-ever load (server start, `_cfg_mtime == 0.0`) or a path change keeps it | +| `invalidate_models_cache()` (default `delete_disk=True`) | dropped | **deleted** | +| `invalidate_models_cache(delete_disk=False)` (`POST /api/profile/switch`) | dropped | kept | +| `invalidate_provider_models_cache(provider_id)` | dropped | **deleted** | +| `_get_fresh_memory_models_cache()` on fingerprint mismatch / invalid shape | dropped | untouched | +| config-reload branch in `get_available_models()` | dropped | deleted by `_refresh_config_cache()` only when the *same* `config.yaml` path was already loaded and changed; a first load or path change (per-client switch) keeps it | -The path guard matters for per-client profile switches: `switch_profile(name, -process_wide=False)` deliberately skips `reload_config()`, so the process-global -`_cfg_mtime` / `_cfg_path` still describe the *previous* profile's config after -`POST /api/profile/switch`. The first `/api/models` for the new profile then -takes the config-reload branch (different path, different mtime). Without the -`_old_cfg_path == config_path` check that reload looked like a config edit and -unlinked the *target* profile's `models_cache..json`, defeating the -`delete_disk=False` switch on the very next request -(`tests/test_profile_switch_next_models_request_keeps_disk_cache.py`). - -`invalidate_models_cache` is the only entry point that offers the -`delete_disk` choice. `delete_disk=True` is for when a source may have changed, -or test isolation requires a guaranteed cold build; every pre-existing caller -keeps this mode. `delete_disk=False` is for when the sources have **not** -changed and the caller only needs the next request to re-resolve *which* -profile's catalog to serve. - -`POST /api/profile/switch` uses `delete_disk=False`. The disk cache is keyed -per profile (`_get_models_cache_path()`), and `_is_loadable_disk_cache()` -rejects any snapshot whose source fingerprint no longer matches. The fingerprint -covers every profile-local input to the catalog: `config.yaml`, `auth.json`, -the `.env` key names, and the model-provider plugins. A switch therefore reuses -the snapshot only when none of those inputs changed, and skips the full cold -rebuild (live provider `fetch_models` calls). This mode depends on the -fingerprint being the only gate for serving a disk snapshot (change-protocol -items 1 and 4). A new catalog input must be added as an axis before anything -can rely on this mode. - -A profile's snapshot is not stored in its home, so deleting the home leaves the -file behind. `delete_profile_api()` and `create_profile_api()` both unlink -`models_cache..json` (`delete_profile_models_cache()`), so a profile that -is recreated with the same name never inherits the old catalog. +The switch keeps the disk snapshot because it is keyed per profile and +`_is_loadable_disk_cache()` rejects it unless every axis above matches, so any +new catalog input must become an axis first. `delete_profile_api()` and +`create_profile_api()` unlink `models_cache..json`, so a recreated profile +never inherits the old catalog. ## Change protocol @@ -141,19 +108,10 @@ timestamp-only churn keeps the fingerprint identical (and a session visit after Codex refresh needs no live rebuild), while genuine changes — a new model, a visibility change, any catalog field, any unknown field — still invalidate. -`tests/test_profile_switch_models_disk_cache.py` covers the invalidation modes: -`delete_disk=False` keeps the disk file and the next `get_available_models()` -reloads it without a live rebuild, the default still unlinks it, and the -executed `/api/profile/switch` route passes `delete_disk=False`. - -`tests/test_profile_switch_models_cache_source_axes.py` covers the safety side -of that mode. It saves a real snapshot, changes one source in the target -profile, runs the switch, and asserts a fresh rebuild for each of these cases: -adding or removing a `.env` key, installing a plugin (in either location), -removing a plugin, bumping a plugin's version, editing plugin code without a version bump, and deleting then recreating the -profile. Two tests pin the speed-up: with unchanged sources, and after rotating -a `.env` value, the switch still reuses the snapshot, and no secret value is -written to disk. +`tests/test_profile_switch_models_disk_cache.py` covers the switch: the disk +snapshot survives it and is served without a live rebuild, each source-axis +change or a delete/recreate forces a fresh rebuild, and a same-path config +edit still deletes the snapshot. ## References diff --git a/tests/test_gateway_watcher_profile.py b/tests/test_gateway_watcher_profile.py index fd34d7b8..d8f0c24e 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 *_a, **_kw: calls.append("cache")) + monkeypatch.setattr(config, "invalidate_models_cache", lambda **kw: calls.append(("cache", kw))) monkeypatch.setattr(gateway_watcher, "restart_watcher_for_profile", lambda name: calls.append(("watcher", name))) handler = _FakeHandler() @@ -324,7 +324,7 @@ def test_profile_switch_restarts_watcher_best_effort(monkeypatch): assert handler.status == 200 assert handler.get_json() == {"ok": True, "name": "demo"} - assert calls == ["cache", ("watcher", "demo")] + assert calls == [("cache", {"delete_disk": False}), ("watcher", "demo")] assert any(k == "Set-Cookie" for k, _v in handler.sent_headers) diff --git a/tests/test_profile_switch_models_cache_source_axes.py b/tests/test_profile_switch_models_cache_source_axes.py deleted file mode 100644 index dd7c4b09..00000000 --- a/tests/test_profile_switch_models_cache_source_axes.py +++ /dev/null @@ -1,227 +0,0 @@ -"""A retained per-profile models disk snapshot must be rejected after a switch -when a catalog source changed in the target profile: its ``.env`` provider -credentials, its installed model-provider plugins, or the profile itself -(delete + recreate with the same name). - -Each test saves a real snapshot for profile ``demo`` through -``_save_models_cache_to_disk()``, changes one source while ``default`` is -active, performs the per-client switch exactly as ``/api/profile/switch`` does -(``invalidate_models_cache(delete_disk=False)``), and asserts the first -``get_available_models()`` for ``demo`` serves the fresh rebuild. -""" - -from __future__ import annotations - -import os -import shutil -import sys -from types import SimpleNamespace -from pathlib import Path - -import pytest - - -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": {}, - } - - -@pytest.fixture -def two_profiles(tmp_path: Path, monkeypatch): - import api.config as cfg - import api.profiles as profiles - - monkeypatch.delenv("HERMES_CONFIG_PATH", raising=False) - default_home = tmp_path / ".hermes" - default_home.mkdir() - (default_home / "config.yaml").write_text("model:\n default: default-model\n", encoding="utf-8") - demo_home = default_home / "profiles" / "demo" - demo_home.mkdir(parents=True) - (demo_home / "config.yaml").write_text("model:\n default: demo-model\n", encoding="utf-8") - os.utime(demo_home / "config.yaml", (1_000_000, 1_000_000)) - - monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", default_home) - monkeypatch.setattr(profiles, "_active_profile", "default") - monkeypatch.setattr(profiles._tls, "profile", None, raising=False) - monkeypatch.setattr(cfg, "_models_cache_path", tmp_path / "models_cache.json") - monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) - monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _builder: _catalog("fresh-model")) - for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): - monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) - saved_cfg = dict(cfg._cfg_cache) - cfg.invalidate_models_cache(delete_disk=False) - cfg.reload_config() - yield SimpleNamespace(cfg=cfg, profiles=profiles, demo_home=demo_home, cache=tmp_path / "models_cache.demo.json") - profiles.clear_request_profile() - cfg.invalidate_models_cache(delete_disk=False) - cfg._cfg_cache.clear() - cfg._cfg_cache.update(saved_cfg) - - -def _save_demo_snapshot(env) -> None: - env.profiles.set_request_profile("demo") - try: - env.cfg._save_models_cache_to_disk(_catalog("snapshot-model")) - finally: - env.profiles.clear_request_profile() - assert env.cache.exists() - - -def _switch_to_demo_and_fetch(env) -> dict: - env.profiles.switch_profile("demo", process_wide=False) - env.cfg.invalidate_models_cache(delete_disk=False) - env.profiles.set_request_profile("demo") - try: - return env.cfg.get_available_models() - finally: - env.profiles.clear_request_profile() - - -def _write_plugin(root: Path, name: str, version: str, *, flat: bool) -> Path: - base = root / "plugins" if flat else root / "plugins" / "model-providers" - d = base / name - d.mkdir(parents=True, exist_ok=True) - (d / "plugin.yaml").write_text( - f"name: {name}\nkind: model-provider\nversion: {version}\n", encoding="utf-8" - ) - return d - - -def test_unchanged_sources_keep_serving_the_retained_snapshot(two_profiles): - """Control: with no source change the switch reuses the snapshot (the speed-up).""" - _save_demo_snapshot(two_profiles) - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "snapshot-model" - - -def test_switch_after_adding_env_key_rejects_snapshot(two_profiles): - _save_demo_snapshot(two_profiles) - (two_profiles.demo_home / ".env").write_text("DEEPSEEK_API_KEY=sk-new\n", encoding="utf-8") - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_switch_after_removing_env_key_rejects_snapshot(two_profiles): - env_file = two_profiles.demo_home / ".env" - env_file.write_text("DEEPSEEK_API_KEY=sk-old\nOTHER=1\n", encoding="utf-8") - _save_demo_snapshot(two_profiles) - env_file.write_text("OTHER=1\n", encoding="utf-8") - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_switch_after_export_prefix_rejects_snapshot(two_profiles): - """`export KEY=` is key `export KEY` to the profile .env loaders, so the provider key is gone.""" - env_file = two_profiles.demo_home / ".env" - env_file.write_text("DEEPSEEK_API_KEY=sk-old\n", encoding="utf-8") - _save_demo_snapshot(two_profiles) - env_file.write_text("export DEEPSEEK_API_KEY=sk-old\n", encoding="utf-8") - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_env_fingerprint_keys_match_provider_env_loader(two_profiles): - from api.providers import _load_env_file - - env_file = two_profiles.demo_home / ".env" - env_file.write_text("export A=1\nB='x'\nC=\n# D=1\n", encoding="utf-8") - loaded = sorted(k for k, v in _load_env_file(env_file).items() if v) - assert two_profiles.cfg._models_cache_env_fingerprint(env_file)["present_keys"] == loaded - - -def test_env_value_rotation_keeps_snapshot_and_fingerprint_has_no_secret(two_profiles): - env_file = two_profiles.demo_home / ".env" - env_file.write_text("DEEPSEEK_API_KEY=sk-secret-one\n", encoding="utf-8") - _save_demo_snapshot(two_profiles) - assert "sk-secret-one" not in two_profiles.cache.read_text(encoding="utf-8") - env_file.write_text("DEEPSEEK_API_KEY=sk-secret-two\n", encoding="utf-8") - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "snapshot-model" - - -@pytest.mark.parametrize("flat", [False, True], ids=["model-providers-dir", "flat-install-dir"]) -def test_switch_after_installing_plugin_rejects_snapshot(two_profiles, flat): - _save_demo_snapshot(two_profiles) - _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=flat) - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_switch_after_removing_plugin_rejects_snapshot(two_profiles): - plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=False) - _save_demo_snapshot(two_profiles) - shutil.rmtree(plugin) - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_switch_after_plugin_version_change_rejects_snapshot(two_profiles): - _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=True) - _save_demo_snapshot(two_profiles) - _write_plugin(two_profiles.demo_home, "acme", "2.0.0", flat=True) - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_delete_profile_removes_its_models_cache(two_profiles, monkeypatch): - import api.profiles as profiles - - monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) - monkeypatch.setitem(sys.modules, "hermes_cli.profiles", None) - _save_demo_snapshot(two_profiles) - profiles.delete_profile_api("demo") - assert not two_profiles.demo_home.exists() - assert not two_profiles.cache.exists() - - -def test_recreate_same_name_profile_does_not_resurrect_old_catalog(two_profiles, monkeypatch): - import api.profiles as profiles - - monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) - monkeypatch.setitem(sys.modules, "hermes_cli.profiles", None) - monkeypatch.setattr(profiles, "list_profiles_api", lambda: []) - _save_demo_snapshot(two_profiles) - stale = two_profiles.cache.read_text(encoding="utf-8") - profiles.delete_profile_api("demo") - # A snapshot left behind by an older build or another process. - two_profiles.cache.write_text(stale, encoding="utf-8") - profiles.create_profile_api("demo") - assert not two_profiles.cache.exists() - (two_profiles.demo_home / "config.yaml").write_text("model:\n default: demo-model\n", encoding="utf-8") - os.utime(two_profiles.demo_home / "config.yaml", (1_000_000, 1_000_000)) - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -@pytest.mark.parametrize("flat", [False, True], ids=["model-providers-dir", "flat-install-dir"]) -def test_switch_after_plugin_code_change_without_version_bump_rejects_snapshot(two_profiles, flat): - """fallback_models lives in the plugin's code, which the loader imports; the version may not move.""" - plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=flat) - init = plugin / "__init__.py" - init.write_text("FALLBACK_MODELS = ('acme-1',)\n", encoding="utf-8") - os.utime(init, (1_000_000, 1_000_000)) - _save_demo_snapshot(two_profiles) - init.write_text("FALLBACK_MODELS = ('acme-1', 'acme-2')\n", encoding="utf-8") - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_switch_after_plugin_submodule_change_rejects_snapshot(two_profiles): - plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=False) - (plugin / "__init__.py").write_text("from .models import FALLBACK_MODELS\n", encoding="utf-8") - sub = plugin / "models.py" - sub.write_text("FALLBACK_MODELS = ('acme-1',)\n", encoding="utf-8") - os.utime(sub, (1_000_000, 1_000_000)) - _save_demo_snapshot(two_profiles) - sub.write_text("FALLBACK_MODELS = ('acme-1', 'acme-2')\n", encoding="utf-8") - assert _switch_to_demo_and_fetch(two_profiles)["default_model"] == "fresh-model" - - -def test_plugin_bytecode_cache_does_not_churn_fingerprint(two_profiles): - plugin = _write_plugin(two_profiles.demo_home, "acme", "1.0.0", flat=False) - (plugin / "__init__.py").write_text("X = 1\n", encoding="utf-8") - before = two_profiles.cfg._models_cache_plugin_fingerprint(two_profiles.demo_home) - (plugin / "__pycache__").mkdir() - (plugin / "__pycache__" / "__init__.cpython-311.pyc").write_bytes(b"x") - assert two_profiles.cfg._models_cache_plugin_fingerprint(two_profiles.demo_home) == before diff --git a/tests/test_profile_switch_models_disk_cache.py b/tests/test_profile_switch_models_disk_cache.py index 37dc386b..abfe0821 100644 --- a/tests/test_profile_switch_models_disk_cache.py +++ b/tests/test_profile_switch_models_disk_cache.py @@ -1,188 +1,174 @@ -"""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 keyed per profile and rejected on read when -``_models_cache_source_fingerprint()`` differs; the source changes that must -still force a rebuild are covered in test_profile_switch_models_cache_source_axes.py. - -The switch route now uses ``invalidate_models_cache(delete_disk=False)``. -""" +"""/api/profile/switch keeps the target profile's models disk snapshot only while its sources are unchanged.""" from __future__ import annotations -import json -from urllib.parse import urlparse +import os +import shutil +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +import api.config as cfg +import api.profiles as profiles 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": {}, - } + group = {"provider": "OpenAI", "provider_id": "openai", "models": [{"id": label, "label": label, "supports_fast_tier": False}]} + return {"active_provider": "openai", "default_model": label, "configured_model_badges": {}, "groups": [group], "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) +@pytest.fixture +def env(tmp_path: Path, monkeypatch): + monkeypatch.delenv("HERMES_CONFIG_PATH", raising=False) + default_home = tmp_path / ".hermes" + demo_home = default_home / "profiles" / "demo" + demo_home.mkdir(parents=True) + (default_home / "config.yaml").write_text("model:\n default: default-model\n", encoding="utf-8") + _write_demo_config(demo_home) + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", default_home) + monkeypatch.setattr(profiles, "_active_profile", "default") + monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) + monkeypatch.setattr(profiles._tls, "profile", None, raising=False) + monkeypatch.setattr(cfg, "_models_cache_path", tmp_path / "models_cache.json") 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")) - + monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _builder: _catalog("fresh-model")) + for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): + monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) + saved_cfg = dict(cfg._cfg_cache) cfg.invalidate_models_cache(delete_disk=False) - result = cfg.get_available_models() - - assert result["default_model"] == "disk-model" + cfg.reload_config() # the default profile's config is loaded, as at server boot + yield SimpleNamespace(home=demo_home, cache=tmp_path / "models_cache.demo.json") + profiles.clear_request_profile() + cfg.invalidate_models_cache(delete_disk=False) + cfg._cfg_cache.clear() + cfg._cfg_cache.update(saved_cfg) -class _FakeHandler: - def __init__(self): - self.status = None - self.sent_headers: list[tuple[str, str]] = [] - self.body = bytearray() - self.wfile = self - self.headers = {} - - def send_response(self, code): - self.status = code - - def send_header(self, key, value): - self.sent_headers.append((key, value)) - - def end_headers(self): - pass - - def write(self, data): - self.body.extend(data if isinstance(data, (bytes, bytearray)) else data.encode("utf-8")) - - def get_json(self): - return json.loads(self.body.decode("utf-8")) +def _write_demo_config(home: Path, default: str = "demo-model", mtime: int = 1_000_000) -> None: + (home / "config.yaml").write_text(f"model:\n default: {default}\n", encoding="utf-8") + os.utime(home / "config.yaml", (mtime, mtime)) -def _drive_profile_switch(monkeypatch, name: str = "demo") -> _FakeHandler: - """Execute POST /api/profile/switch through routes.handle_post with the - auth / CSRF / profile-fs / watcher collaborators stubbed out, leaving - ``config.invalidate_models_cache`` to whatever the caller installed.""" - from api import gateway_watcher, profiles, routes - - for mod in ("api.auth", "api.helpers", "api.routes"): - monkeypatch.setattr(f"{mod}.is_auth_enabled", lambda: False, raising=False) - monkeypatch.setattr(routes, "_check_csrf", lambda handler: True) - monkeypatch.setattr(routes, "read_body", lambda handler: {"name": name}) - 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(gateway_watcher, "restart_watcher_for_profile", lambda _name: None) - - handler = _FakeHandler() - routes.handle_post(handler, urlparse("/api/profile/switch")) - return handler +def _save_demo_snapshot(env) -> None: + profiles.set_request_profile("demo") + try: + cfg._save_models_cache_to_disk(_catalog("snapshot-model")) + finally: + profiles.clear_request_profile() + assert env.cache.exists() -def test_profile_switch_route_calls_invalidate_with_delete_disk_false(monkeypatch): - """Executed-route guard: the live switch handler must pass - ``delete_disk=False`` (not rely on the default, which unlinks the disk - cache and forces a cold rebuild).""" - from api import config - - calls: list[tuple[tuple, dict]] = [] - monkeypatch.setattr(config, "invalidate_models_cache", lambda *a, **kw: calls.append((a, kw))) - - handler = _drive_profile_switch(monkeypatch) - - assert handler.status == 200, handler.body.decode("utf-8", "replace") - assert handler.get_json() == {"ok": True, "name": "demo"} - assert calls == [((), {"delete_disk": False})], ( - "/api/profile/switch must call invalidate_models_cache(delete_disk=False) exactly once; " - f"got {calls!r}" - ) +def _fetch_as_demo() -> dict: + profiles.set_request_profile("demo") + try: + return cfg.get_available_models() + finally: + profiles.clear_request_profile() -def test_profile_switch_route_leaves_disk_cache_file_in_place(tmp_path, monkeypatch): - """End-to-end through the real ``invalidate_models_cache``: after the - executed switch route, the in-memory catalog is dropped but the - per-profile disk snapshot still exists.""" - from api import config as cfg +def _switch_to_demo_and_fetch() -> str: + profiles.switch_profile("demo", process_wide=False) + cfg.invalidate_models_cache(delete_disk=False) # as /api/profile/switch does + return _fetch_as_demo()["default_model"] - 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")) - handler = _drive_profile_switch(monkeypatch) +def _write_plugin(home: Path, version: str = "1.0.0", *, flat: bool = False, code: str | None = None, sub: str | None = None) -> Path: + d = home / "plugins" / ("" if flat else "model-providers") / "acme" + d.mkdir(parents=True, exist_ok=True) + (d / "plugin.yaml").write_text(f"name: acme\nkind: model-provider\nversion: {version}\n", encoding="utf-8") + for name, text in (("__init__.py", code), ("models.py", sub)): + if text is not None: + (d / name).write_text(text, encoding="utf-8") + os.utime(d / name, (1_000_000, 1_000_000)) + return d - assert handler.status == 200, handler.body.decode("utf-8", "replace") - assert cfg._available_models_cache is None, "switch must still drop the in-memory catalog (#1200)" - assert cache_path.exists(), "switch must not unlink the per-profile disk cache" + +def _env(text: str): + return lambda home: (home / ".env").write_text(text, encoding="utf-8") + + +# (id, source state before the snapshot is saved, change made after it, model served after the switch) +_CASES = [ + ("unchanged", None, None, "snapshot-model"), + ("env-value-rotated", _env("DEEPSEEK_API_KEY=sk-one\n"), _env("DEEPSEEK_API_KEY=sk-two\n"), "snapshot-model"), + ("env-key-added", None, _env("DEEPSEEK_API_KEY=sk-new\n"), "fresh-model"), + ("env-key-removed", _env("DEEPSEEK_API_KEY=sk-old\nOTHER=1\n"), _env("OTHER=1\n"), "fresh-model"), + # provider detection reads `export KEY=` as key `export KEY`, so the credential is gone + ("env-export-prefix", _env("DEEPSEEK_API_KEY=sk-old\n"), _env("export DEEPSEEK_API_KEY=sk-old\n"), "fresh-model"), + ("plugin-installed", None, lambda h: _write_plugin(h), "fresh-model"), + ("plugin-installed-flat", None, lambda h: _write_plugin(h, flat=True), "fresh-model"), + ("plugin-removed", lambda h: _write_plugin(h), lambda h: shutil.rmtree(h / "plugins"), "fresh-model"), + ("plugin-version-bumped", lambda h: _write_plugin(h, flat=True), lambda h: _write_plugin(h, "2.0.0", flat=True), "fresh-model"), + ("plugin-code-edited", lambda h: _write_plugin(h, code="M = (1,)\n"), lambda h: (h / "plugins/model-providers/acme/__init__.py").write_text("M = (1, 2)\n"), "fresh-model"), + ("plugin-code-edited-flat", lambda h: _write_plugin(h, flat=True, code="M = (1,)\n"), lambda h: (h / "plugins/acme/__init__.py").write_text("M = (1, 2)\n"), "fresh-model"), + ("plugin-submodule-edited", lambda h: _write_plugin(h, code="from .models import M\n", sub="M = (1,)\n"), lambda h: (h / "plugins/model-providers/acme/models.py").write_text("M = (1, 2)\n"), "fresh-model"), +] + + +@pytest.mark.parametrize("before, change, expected", [c[1:] for c in _CASES], ids=[c[0] for c in _CASES]) +def test_switch_serves_snapshot_only_while_sources_are_unchanged(env, before, change, expected): + if before: + before(env.home) + _save_demo_snapshot(env) + if change: + change(env.home) + assert _switch_to_demo_and_fetch() == expected + assert "sk-" not in env.cache.read_text(encoding="utf-8") # the fingerprint never records a secret + + +def test_same_profile_config_edit_still_deletes_disk_cache(env, monkeypatch): + _save_demo_snapshot(env) + assert _switch_to_demo_and_fetch() == "snapshot-model" + _write_demo_config(env.home, "demo-model-2", 2_000_000) + cfg.invalidate_models_cache(delete_disk=False) + cache_at_rebuild = [] + monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _b: cache_at_rebuild.append(env.cache.exists()) or _catalog("x")) + _fetch_as_demo() + assert cache_at_rebuild == [False] + + +@pytest.mark.parametrize("delete_disk", [True, False]) +def test_invalidate_drops_memory_and_deletes_disk_only_by_default(env, delete_disk): + _save_demo_snapshot(env) + _fetch_as_demo() + profiles.set_request_profile("demo") + try: + cfg.invalidate_models_cache(delete_disk=delete_disk) + finally: + profiles.clear_request_profile() + assert cfg._available_models_cache is None and cfg._models_cache_provenance is None + assert env.cache.exists() is not delete_disk + + +def test_env_fingerprint_keys_match_provider_env_loader(env): + from api.providers import _load_env_file + + _env("export A=1\nB='x'\nC=\n# D=1\n")(env.home) + loaded = sorted(k for k, v in _load_env_file(env.home / ".env").items() if v) + assert cfg._models_cache_env_fingerprint(env.home / ".env") == loaded + + +def test_plugin_bytecode_cache_does_not_churn_fingerprint(env): + plugin = _write_plugin(env.home, code="X = 1\n") + before = cfg._models_cache_plugin_fingerprint(env.home) + (plugin / "__pycache__").mkdir() + (plugin / "__pycache__" / "__init__.cpython-311.pyc").write_bytes(b"x") + assert cfg._models_cache_plugin_fingerprint(env.home) == before + + +@pytest.mark.parametrize("recreate", [False, True]) +def test_delete_and_recreate_profile_drop_its_models_cache(env, monkeypatch, recreate): + monkeypatch.setitem(sys.modules, "hermes_cli.profiles", None) + monkeypatch.setattr(profiles, "list_profiles_api", lambda: []) + _save_demo_snapshot(env) + stale = env.cache.read_text(encoding="utf-8") + profiles.delete_profile_api("demo") + assert not env.home.exists() and not env.cache.exists() + if recreate: + env.cache.write_text(stale, encoding="utf-8") # left behind by an older build or another process + profiles.create_profile_api("demo") + assert not env.cache.exists() + _write_demo_config(env.home) + assert _switch_to_demo_and_fetch() == "fresh-model" diff --git a/tests/test_profile_switch_next_models_request_keeps_disk_cache.py b/tests/test_profile_switch_next_models_request_keeps_disk_cache.py deleted file mode 100644 index cf969ddc..00000000 --- a/tests/test_profile_switch_next_models_request_keeps_disk_cache.py +++ /dev/null @@ -1,156 +0,0 @@ -"""Greptile review thread on #7632 (docs/architecture/models-cache-invalidation.md): - -A per-client ``POST /api/profile/switch`` (``process_wide=False``) skips -``reload_config()``, so the process-global ``_cfg_mtime`` still holds the -*previous* profile's nonzero mtime. The next ``/api/models`` for the new -profile sees a different config mtime, takes the config-reload branch, and -``_refresh_config_cache()`` used to treat ``_old_cfg_mtime != 0.0`` as "config -edited" and unlink the disk snapshot — the *target* profile's -``models_cache..json`` — defeating ``delete_disk=False`` on the very next -request. - -``_refresh_config_cache()`` now only deletes the disk snapshot when the same -config.yaml path was already loaded (a real edit of the active profile's -config). These tests drive real profile homes, real config files and real disk -cache files through ``switch_profile()`` + ``get_available_models()``. -""" - -from __future__ import annotations - -import os -from pathlib import Path - - -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 _setup_two_profiles(tmp_path: Path, monkeypatch): - """Real default + named profile homes, distinct config mtimes, one disk - cache file per profile. Returns (default_cache, demo_cache, demo_config).""" - import api.config as cfg - import api.profiles as profiles - - monkeypatch.delenv("HERMES_CONFIG_PATH", raising=False) - default_home = tmp_path / ".hermes" - default_home.mkdir() - (default_home / "config.yaml").write_text("model:\n default: default-model\n", encoding="utf-8") - demo_home = default_home / "profiles" / "demo" - demo_home.mkdir(parents=True) - demo_config = demo_home / "config.yaml" - demo_config.write_text("model:\n default: demo-model\n", encoding="utf-8") - os.utime(demo_config, (1_000_000, 1_000_000)) # guarantee a different mtime - - monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", default_home) - monkeypatch.setattr(profiles, "_active_profile", "default") - monkeypatch.setattr(profiles._tls, "profile", None, raising=False) - - default_cache = tmp_path / "models_cache.json" - demo_cache = tmp_path / "models_cache.demo.json" - default_cache.write_text("{}", encoding="utf-8") - demo_cache.write_text("{}", encoding="utf-8") - monkeypatch.setattr(cfg, "_models_cache_path", default_cache) - - # Serve the disk snapshot; never allow a live rebuild. - monkeypatch.setattr(cfg, "_load_models_cache_from_disk", lambda: _catalog("disk-model")) - monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) - - def _unexpected_rebuild(*_a, **_kw): - raise AssertionError("must not trigger a live models rebuild") - - monkeypatch.setattr(cfg, "_invoke_models_rebuild", _unexpected_rebuild) - - # Snapshot/restore the process-global config state touched by reload_config(). - for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): - monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) - saved_cache = dict(cfg._cfg_cache) - - def _restore(): - cfg._cfg_cache.clear() - cfg._cfg_cache.update(saved_cache) - - monkeypatch.setattr(cfg, "_available_models_cache", None, raising=False) - monkeypatch.setattr(cfg, "_available_models_cache_source_fingerprint", None, raising=False) - monkeypatch.setattr(cfg, "_cache_build_in_progress", False, raising=False) - - # Load the default profile's config as the server would have at boot. - cfg.reload_config() - assert cfg._cfg_path == default_home / "config.yaml" - assert cfg._cfg_mtime != 0.0 - - return default_cache, demo_cache, demo_config, _restore - - -def test_first_models_request_after_per_client_switch_keeps_target_disk_cache(tmp_path, monkeypatch): - import api.config as cfg - import api.profiles as profiles - - default_cache, demo_cache, _demo_config, restore = _setup_two_profiles(tmp_path, monkeypatch) - try: - # Warm the default profile: /api/models with the default cookie. - cfg.get_available_models() - assert default_cache.exists() - - # Per-client switch, exactly as the route does it. - profiles.switch_profile("demo", process_wide=False) - cfg.invalidate_models_cache(delete_disk=False) - assert demo_cache.exists() - prev_mtime = cfg._cfg_mtime - assert prev_mtime != 0.0, "per-client switch leaves the previous profile's mtime in place" - - # First /api/models from that client (cookie → thread-local profile). - profiles.set_request_profile("demo") - try: - result = cfg.get_available_models() - finally: - profiles.clear_request_profile() - - assert result["default_model"] == "disk-model" - assert cfg._cfg_path == _demo_config, "config-reload branch must have re-pointed to the demo config" - assert cfg._cfg_mtime == _demo_config.stat().st_mtime - assert demo_cache.exists(), ( - "first /api/models after a per-client profile switch must not unlink the " - "target profile's models_cache.demo.json" - ) - assert default_cache.exists() - finally: - restore() - - -def test_same_profile_config_edit_still_deletes_disk_cache(tmp_path, monkeypatch): - """The path guard must not weaken the real-edit contract: an mtime change - on the *same* config.yaml still drops the disk snapshot.""" - import api.config as cfg - import api.profiles as profiles - - default_cache, demo_cache, demo_config, restore = _setup_two_profiles(tmp_path, monkeypatch) - try: - profiles.switch_profile("demo", process_wide=False) - cfg.invalidate_models_cache(delete_disk=False) - profiles.set_request_profile("demo") - try: - cfg.get_available_models() - assert demo_cache.exists() - # Edit demo's config.yaml → mtime moves on the same path. - demo_config.write_text("model:\n default: demo-model-2\n", encoding="utf-8") - os.utime(demo_config, (2_000_000, 2_000_000)) - cfg.invalidate_models_cache(delete_disk=False) - cfg.get_available_models() - finally: - profiles.clear_request_profile() - assert not demo_cache.exists(), "editing the active profile's config.yaml must still delete its disk cache" - assert default_cache.exists() - finally: - restore() From 2dd13660337efe12aaa57d7f18b39f282e825f11 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Thu, 24 Sep 2026 15:29:51 +0000 Subject: [PATCH 10/10] fix: fingerprint .env values, parse manifests like the agent, gate stale models fallback on sources --- api/config.py | 54 +++++++++++---- .../architecture/models-cache-invalidation.md | 7 +- .../test_issue3928_models_budget_fallback.py | 25 ++++--- .../test_profile_switch_models_disk_cache.py | 68 +++++++++++++++++-- 4 files changed, 127 insertions(+), 27 deletions(-) diff --git a/api/config.py b/api/config.py index 5c9294f9..9f088229 100644 --- a/api/config.py +++ b/api/config.py @@ -7926,20 +7926,48 @@ def _active_profile_home() -> Path: def _models_cache_env_fingerprint(path: Path) -> list: - """Sorted non-empty ``.env`` key names, parsed like provider detection; values never recorded.""" + """``[key, HMAC(value)]`` per non-empty ``.env`` entry, parsed like provider detection. + + Values are keyed-hashed with the WebUI signing key so the cache file never holds a secret. + """ + import hmac + from api.auth import _signing_key from api.providers import _load_env_file - return sorted(k for k, v in _load_env_file(Path(path).expanduser()).items() if v) + key = _signing_key() + return [ + [k, hmac.new(key, v.encode("utf-8"), hashlib.sha256).hexdigest()] + for k, v in sorted(_load_env_file(Path(path).expanduser()).items()) + if v + ] def _declares_model_provider_kind(plugin_dir: Path) -> bool: - for manifest in (plugin_dir / "plugin.yaml", plugin_dir / "plugin.yml"): - try: - lines = manifest.read_text(encoding="utf-8", errors="replace").splitlines() - except OSError: + # Same parse as the agent's providers._declares_model_provider_kind: PyYAML, then a line scan. + for filename in ("plugin.yaml", "plugin.yml"): + manifest = plugin_dir / filename + if not manifest.is_file(): continue - pairs = (line.partition(":") for line in lines if not line[:1].isspace()) - return any(k.strip() == "kind" and v.strip().strip("\"'") == "model-provider" for k, _, v in pairs) + try: + text = manifest.read_text(encoding="utf-8", errors="replace") + except Exception: + return False + try: + import yaml as _yaml + + data = _yaml.safe_load(text) + if isinstance(data, dict): + return str(data.get("kind", "")).strip() == "model-provider" + except Exception: + pass + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("#") or ":" not in stripped: + continue + key, _, value = stripped.partition(":") + if key.strip() == "kind": + return value.strip().strip("\"'") == "model-provider" + return False return False @@ -8148,10 +8176,10 @@ def _load_stale_models_cache_from_disk() -> dict | None: The main cache loader enforces metadata stamps for a full cold-path cache hit. This helper intentionally does not apply that stricter policy, so we can still recover a useful fallback payload when the strict loader rejected cache because - metadata or fingerprint fields are stale. It DOES still enforce the schema - version: a cross-schema cache can have an incompatible groups/badge shape, so - serving it to the picker could surface a broken catalog — schema mismatch is a - hard reject even on the fallback path. + the WebUI version stamp is stale. It DOES still enforce the schema version (a + cross-schema cache can have an incompatible groups/badge shape) and the source + fingerprint: a snapshot built from other config/auth/.env/plugin sources is a + wrong catalog, not merely an old one, so it is never served. """ try: import json as _j @@ -8165,6 +8193,8 @@ def _load_stale_models_cache_from_disk() -> dict | None: return None if cache.get("_schema_version") != _MODELS_CACHE_SCHEMA_VERSION: return None + if cache.get("_source_fingerprint") != _models_cache_source_fingerprint(): + return None aliases = cache.get("aliases") if not isinstance(aliases, dict): # The disk cache save path does not persist `aliases`, so a cache diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index 6f0398b1..b28e19a1 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -31,10 +31,13 @@ the fingerprint captured at publish time. | --- | --- | --- | | `config_yaml` | stat identity: `mtime_ns` + size (`_models_cache_file_fingerprint`) | The file is rewritten only on deliberate user edits, and any edit can change the provider/model set, so the cheap conservative identity wins. | | `auth_json` | content hash with a volatile-key deny-list (`_auth_store_semantic_fingerprint`, `_AUTH_FINGERPRINT_VOLATILE_KEYS`) | The credential store is rewritten roughly every 14 minutes by credential-pool / OAuth refresh; none of those rotating fields feed `detected_providers` or the returned catalog, and stat identity made the 24h cache churn on every refresh (RCA `t_d127953d` / `t_16551f61`). | -| `env` | sorted **names** of non-empty `.env` keys, parsed by `providers._load_env_file` (`_models_cache_env_fingerprint`); values are never recorded | Env credentials decide `detected_providers`; rotating a value does not. | -| `plugins` | `[relpath, mtime_ns, size]` of every non-bytecode file of each model-provider plugin, discovered like `providers._scan_home_layer` (`_models_cache_plugin_fingerprint`) | The loader execs plugin code that builds `fallback_models`, so any file edit must invalidate, even without a `version` bump. | +| `env` | `[key, HMAC-SHA256(signing key, value)]` per non-empty `.env` entry, parsed by `providers._load_env_file` (`_models_cache_env_fingerprint`); plaintext values are never recorded | Env keys decide `detected_providers` and values such as `LM_BASE_URL` decide which endpoint is probed, so both key and value changes invalidate. | +| `plugins` | `[relpath, mtime_ns, size]` of every non-bytecode file of each model-provider plugin, discovered like `providers._scan_home_layer`, flat manifests parsed with PyYAML like the agent (`_models_cache_plugin_fingerprint`) | The loader execs plugin code that builds `fallback_models`, so any file edit must invalidate, even without a `version` bump. | | `catalog` | baked-in provider catalog sha256 (`_PROVIDER_MODELS` + `_PROVIDER_DISPLAY`) plus the Codex local catalog (`_codex_models_cache_fingerprint`, `_CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS`) | A restart after a catalog change must not keep serving a persisted payload for up to 24h (#2443). Codex rewrites `~/.codex/models_cache.json` on its own timer, bumping `mtime_ns` and size while models, `etag`, and `client_version` stay identical, so the Codex axis hashes **content** with only the refresh timestamps (`fetched_at`, `updated_at`) removed (#7540, #7556). | +The over-budget stale fallback (`_load_stale_models_cache_from_disk`) tolerates a stale +`_webui_version` but never a source-fingerprint mismatch: such a snapshot is a wrong catalog. + ## Invariant: deny-lists are one-directional - Both volatile-key sets are deny-lists, never allow-lists. They may remove only diff --git a/tests/test_issue3928_models_budget_fallback.py b/tests/test_issue3928_models_budget_fallback.py index 28f030e5..bc04d9d6 100644 --- a/tests/test_issue3928_models_budget_fallback.py +++ b/tests/test_issue3928_models_budget_fallback.py @@ -136,16 +136,12 @@ def _build_stale_disk_cache_payload() -> dict: "aliases": { "chat": "ollama-cloud/chat-1", }, - # Current schema (so the stale-cache loader's schema guard accepts it), - # but a deliberately stale _webui_version + fingerprint so the STRICT - # loader rejects it — this is exactly the "recoverable stale cache" case. + # Current schema and sources, but a stale _webui_version so the STRICT loader + # rejects it: the "recoverable stale cache" case. A source-fingerprint + # mismatch is a wrong catalog and is covered by the rejection test below. "_schema_version": cfg._MODELS_CACHE_SCHEMA_VERSION, "_webui_version": "v999", - "_source_fingerprint": { - "catalog": "stale", - "config_yaml": {"size": 42}, - "auth_json": {"size": 7}, - }, + "_source_fingerprint": cfg._models_cache_source_fingerprint(), } @@ -401,6 +397,19 @@ def test_load_stale_models_cache_from_disk_rejects_cross_schema( assert cfg._load_stale_models_cache_from_disk() is None +def test_load_stale_models_cache_from_disk_rejects_source_fingerprint_mismatch( + monkeypatch, + isolate_models_catalog_state, +): + payload = _build_stale_disk_cache_payload() + payload["_source_fingerprint"] = "other-sources" + models_cache_path = isolate_models_catalog_state["models_cache_path"] + monkeypatch.setattr(cfg, "_get_models_cache_path", lambda: models_cache_path) + models_cache_path.write_text(json.dumps(payload), encoding="utf-8") + + assert cfg._load_stale_models_cache_from_disk() is None + + def test_load_stale_models_cache_reconstructs_aliases_from_config( monkeypatch, isolate_models_catalog_state, diff --git a/tests/test_profile_switch_models_disk_cache.py b/tests/test_profile_switch_models_disk_cache.py index abfe0821..478706e0 100644 --- a/tests/test_profile_switch_models_disk_cache.py +++ b/tests/test_profile_switch_models_disk_cache.py @@ -5,6 +5,7 @@ from __future__ import annotations import os import shutil import sys +import threading from pathlib import Path from types import SimpleNamespace @@ -32,7 +33,6 @@ def env(tmp_path: Path, monkeypatch): monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) monkeypatch.setattr(profiles._tls, "profile", None, raising=False) monkeypatch.setattr(cfg, "_models_cache_path", tmp_path / "models_cache.json") - monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _builder: _catalog("fresh-model")) for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) @@ -74,10 +74,10 @@ def _switch_to_demo_and_fetch() -> str: return _fetch_as_demo()["default_model"] -def _write_plugin(home: Path, version: str = "1.0.0", *, flat: bool = False, code: str | None = None, sub: str | None = None) -> Path: +def _write_plugin(home: Path, version: str = "1.0.0", *, flat: bool = False, code: str | None = None, sub: str | None = None, kind: str = "model-provider") -> Path: d = home / "plugins" / ("" if flat else "model-providers") / "acme" d.mkdir(parents=True, exist_ok=True) - (d / "plugin.yaml").write_text(f"name: acme\nkind: model-provider\nversion: {version}\n", encoding="utf-8") + (d / "plugin.yaml").write_text(f"name: acme\nkind: {kind}\nversion: {version}\n", encoding="utf-8") for name, text in (("__init__.py", code), ("models.py", sub)): if text is not None: (d / name).write_text(text, encoding="utf-8") @@ -92,13 +92,15 @@ def _env(text: str): # (id, source state before the snapshot is saved, change made after it, model served after the switch) _CASES = [ ("unchanged", None, None, "snapshot-model"), - ("env-value-rotated", _env("DEEPSEEK_API_KEY=sk-one\n"), _env("DEEPSEEK_API_KEY=sk-two\n"), "snapshot-model"), + ("env-value-rotated", _env("DEEPSEEK_API_KEY=sk-one\n"), _env("DEEPSEEK_API_KEY=sk-two\n"), "fresh-model"), + ("env-base-url-changed", _env("LM_BASE_URL=http://127.0.0.1:1234/v1\n"), _env("LM_BASE_URL=http://127.0.0.1:5678/v1\n"), "fresh-model"), ("env-key-added", None, _env("DEEPSEEK_API_KEY=sk-new\n"), "fresh-model"), ("env-key-removed", _env("DEEPSEEK_API_KEY=sk-old\nOTHER=1\n"), _env("OTHER=1\n"), "fresh-model"), # provider detection reads `export KEY=` as key `export KEY`, so the credential is gone ("env-export-prefix", _env("DEEPSEEK_API_KEY=sk-old\n"), _env("export DEEPSEEK_API_KEY=sk-old\n"), "fresh-model"), ("plugin-installed", None, lambda h: _write_plugin(h), "fresh-model"), ("plugin-installed-flat", None, lambda h: _write_plugin(h, flat=True), "fresh-model"), + ("plugin-installed-flat-yaml-comment", None, lambda h: _write_plugin(h, flat=True, kind="model-provider # valid YAML comment"), "fresh-model"), ("plugin-removed", lambda h: _write_plugin(h), lambda h: shutil.rmtree(h / "plugins"), "fresh-model"), ("plugin-version-bumped", lambda h: _write_plugin(h, flat=True), lambda h: _write_plugin(h, "2.0.0", flat=True), "fresh-model"), ("plugin-code-edited", lambda h: _write_plugin(h, code="M = (1,)\n"), lambda h: (h / "plugins/model-providers/acme/__init__.py").write_text("M = (1, 2)\n"), "fresh-model"), @@ -118,6 +120,52 @@ def test_switch_serves_snapshot_only_while_sources_are_unchanged(env, before, ch assert "sk-" not in env.cache.read_text(encoding="utf-8") # the fingerprint never records a secret +@pytest.mark.parametrize("change", [_env("LM_BASE_URL=http://127.0.0.1:5678/v1\n"), lambda h: _write_plugin(h, flat=True)], ids=["env", "plugin"]) +def test_slow_rebuild_never_serves_fingerprint_mismatched_snapshot_as_stale_fallback(env, monkeypatch, change): + _env("LM_BASE_URL=http://127.0.0.1:1234/v1\n")(env.home) + _save_demo_snapshot(env) + change(env.home) + profiles.set_request_profile("demo") + try: + assert cfg._load_models_cache_from_disk() is None + assert cfg._load_stale_models_cache_from_disk() is None + finally: + profiles.clear_request_profile() + release = threading.Event() + monkeypatch.setattr(cfg, "_LIVE_REBUILD_BUDGET_SECONDS", 0.05, raising=False) + monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _b: release.wait(5) and _catalog("fresh-model")) + try: + assert _switch_to_demo_and_fetch() != "snapshot-model" + finally: + release.set() + + +def test_stale_fallback_still_serves_snapshot_when_only_webui_version_differs(env, monkeypatch): + _save_demo_snapshot(env) + monkeypatch.setattr(cfg, "_current_webui_version", lambda: "v-other") + profiles.set_request_profile("demo") + try: + assert cfg._load_models_cache_from_disk() is None + assert cfg._load_stale_models_cache_from_disk()["default_model"] == "snapshot-model" + finally: + profiles.clear_request_profile() + + +@pytest.mark.parametrize("manifest", [ + "kind: model-provider # valid YAML comment\n", + "kind: model-provider\n", + "kind: 'model-provider'\n", + "name: x\nkind: tool\n", + "meta:\n kind: model-provider\n", + "# kind: model-provider\n", + "kind: [unclosed\nkind: model-provider\n", +]) +def test_manifest_kind_parse_matches_agent(tmp_path, manifest): + agent = pytest.importorskip("providers") + (tmp_path / "plugin.yaml").write_text(manifest, encoding="utf-8") + assert cfg._declares_model_provider_kind(tmp_path) == agent._declares_model_provider_kind(tmp_path) + + def test_same_profile_config_edit_still_deletes_disk_cache(env, monkeypatch): _save_demo_snapshot(env) assert _switch_to_demo_and_fetch() == "snapshot-model" @@ -147,7 +195,17 @@ def test_env_fingerprint_keys_match_provider_env_loader(env): _env("export A=1\nB='x'\nC=\n# D=1\n")(env.home) loaded = sorted(k for k, v in _load_env_file(env.home / ".env").items() if v) - assert cfg._models_cache_env_fingerprint(env.home / ".env") == loaded + fp = cfg._models_cache_env_fingerprint(env.home / ".env") + assert [k for k, _ in fp] == loaded + assert all(len(h) == 64 and h not in ("1", "x") for _, h in fp) + + +def test_env_fingerprint_is_keyed_not_a_plain_value_hash(env): + import hashlib + + _env("DEEPSEEK_API_KEY=sk-secret\n")(env.home) + (_, digest), = cfg._models_cache_env_fingerprint(env.home / ".env") + assert digest != hashlib.sha256(b"sk-secret").hexdigest() def test_plugin_bytecode_cache_does_not_churn_fingerprint(env):