From 7344c8df77cfec747d2230ed1b4721e740b8538d Mon Sep 17 00:00:00 2001 From: carlotestor Date: Wed, 23 Sep 2026 19:11:19 +0000 Subject: [PATCH] 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()