diff --git a/api/models.py b/api/models.py index eca27ab5..270e50b8 100644 --- a/api/models.py +++ b/api/models.py @@ -35,7 +35,7 @@ from api.config import ( LOCK, STREAMS, STREAMS_LOCK, DEFAULT_WORKSPACE, DEFAULT_MODEL, PROJECTS_FILE, HOME, get_effective_default_model, _get_session_agent_lock, ) -from api.workspace import get_last_workspace, _resolve_path +from api.workspace import get_last_workspace, _resolve_path, profile_home_resolve_cache_scope from api.usage import prompt_cache_hit_percent from api.agent_sessions import ( _is_continuation_session, @@ -7708,6 +7708,7 @@ def _state_projection_sidecar_metadata(sid: str) -> dict: return dict(metadata) +@profile_home_resolve_cache_scope() def _load_cli_sessions_uncached( hermes_home: Path, db_path: Path, diff --git a/api/workspace.py b/api/workspace.py index ebeaacaa..511aa9f1 100644 --- a/api/workspace.py +++ b/api/workspace.py @@ -19,6 +19,8 @@ import stat import subprocess import sys import concurrent.futures +import contextlib +import contextvars import threading import time from collections.abc import Callable @@ -207,7 +209,7 @@ def _is_remote_terminal_backend(terminal_cfg: dict | None) -> bool: return backend not in ('', 'local') -# Per-profile-argument cache for `_resolve_profile_home_param`'s filesystem +# Call-scoped cache for `_resolve_profile_home_param`'s filesystem # `.resolve()` call. The webui's session list builds one `Session` per CLI # session row (`_load_cli_sessions_uncached`), and `Session.__init__` calls # `_resolve_profile_home_param(profile)` once per row -- with only one @@ -217,23 +219,67 @@ def _is_remote_terminal_backend(terminal_cfg: dict | None) -> bool: # host load (confirmed via repeated "Slow WebUI request still running" # warnings pinned to this call site). # -# Caching is safe for the life of the process: the mapping from a profile -# argument to its home path is a pure function of process-startup constants -# (`api.profiles._DEFAULT_HERMES_HOME`, `_INITIAL_HERMES_HOME`) that are -# resolved once at import time and never mutated afterwards. Profile -# *creation* only creates the directory at that already-deterministic path -# -- it does not change what path a given profile argument resolves to -- -# so there is no runtime path that needs to invalidate an entry here. -_PROFILE_HOME_RESOLVE_CACHE: dict[Path, Path] = {} +# IMPORTANT: this is deliberately NOT a process-lifetime cache. An earlier +# revision of this fix cached forever, on the premise that a profile +# argument always resolves to the same path for the life of the process -- +# but `Path.resolve()` is a filesystem call, not a pure function of +# startup constants: a profile-home symlink can be retargeted while the +# webui process keeps running for days/weeks, and `_safe_resolve()` below +# deliberately falls back to returning the UNRESOLVED input after a +# transient `OSError`/`RuntimeError`/`ValueError`, which a process-lifetime +# cache would then serve forever. Both are real correctness bugs (caught in +# PR #7636 review), not style nitpicks. +# +# Instead the cache is scoped to exactly ONE invocation of the hot loop +# (`_load_cli_sessions_uncached`, wrapped below in `profile_home_resolve_cache_scope`) +# via a `ContextVar` that holds a fresh dict only while that single +# sidebar-list build is running, and is `None` everywhere else. Every other +# call site -- and every request that isn't actively inside that one +# build -- resolves fresh every time, exactly like pre-PR behavior, so a +# symlink retarget or a transient resolve error is never masked past the +# single call that would have paid for the redundant resolves anyway. +_PROFILE_HOME_RESOLVE_SCOPE: "contextvars.ContextVar[dict[Path, Path] | None]" = ( + contextvars.ContextVar("_PROFILE_HOME_RESOLVE_SCOPE", default=None) +) + + +@contextlib.contextmanager +def profile_home_resolve_cache_scope(): + """Enable memoization of `_resolve_profile_home_param` for one call. + + Use as a decorator (or a `with` block) around the single hot-path + operation that resolves the SAME profile argument hundreds of times in a + tight loop -- currently `_load_cli_sessions_uncached` building the + CLI/cron session list. While active, repeated resolves of the same + profile argument are served from a dict that lives ONLY for the + duration of the call. Outside of it -- including before/after, and any + call site other than the one wrapped -- `_resolve_profile_home_param` + resolves fresh every time, with no caching, matching pre-PR behavior + exactly. Nested/reentrant use restores the previous (outer) value on + exit rather than clobbering it, so this is safe to nest. + """ + token = _PROFILE_HOME_RESOLVE_SCOPE.set({}) + try: + yield + finally: + _PROFILE_HOME_RESOLVE_SCOPE.reset(token) def _cached_safe_resolve_profile_home(pre_resolve: Path) -> Path: - """Memoized `_safe_resolve()` for profile-home paths (see cache doc above).""" - cached = _PROFILE_HOME_RESOLVE_CACHE.get(pre_resolve) + """`_safe_resolve()` for profile-home paths. + + Memoized only while a `profile_home_resolve_cache_scope()` is active + (see module doc above); otherwise resolves fresh every call, matching + pre-PR behavior. + """ + cache = _PROFILE_HOME_RESOLVE_SCOPE.get() + if cache is None: + return _safe_resolve(pre_resolve) + cached = cache.get(pre_resolve) if cached is not None: return cached resolved = _safe_resolve(pre_resolve) - _PROFILE_HOME_RESOLVE_CACHE[pre_resolve] = resolved + cache[pre_resolve] = resolved return resolved diff --git a/tests/test_profile_home_resolve_caching.py b/tests/test_profile_home_resolve_caching.py index ccc8f63f..89836230 100644 --- a/tests/test_profile_home_resolve_caching.py +++ b/tests/test_profile_home_resolve_caching.py @@ -1,12 +1,29 @@ """Regression test: `_resolve_profile_home_param` memoizes its filesystem -`.resolve()` call per profile argument (N+1 sidebar-hang fix). +`.resolve()` call, but ONLY for the duration of a single +`profile_home_resolve_cache_scope()` (currently wrapping +`_load_cli_sessions_uncached`, the sidebar's session-list build). Building the CLI session list calls `_resolve_profile_home_param(profile)` once per session row (`Session.__init__`), so with hundreds of sessions on -the same profile the old code redundantly re-resolved the identical path via -a filesystem `.resolve()` on every single row. This asserts repeated calls -with the same profile argument return an equal Path while invoking the -underlying `_safe_resolve()` filesystem call only once. +the same profile the unscoped code redundantly re-resolved the identical +path via a filesystem `.resolve()` on every single row. + +An earlier revision of this fix cached the result in a plain process-lifetime +module dict. That was flagged in PR #7636 review: `Path.resolve()` is a +filesystem call, not a pure function of process-startup constants -- a +profile-home symlink can be retargeted while the webui process keeps +running, and `_safe_resolve()` deliberately falls back to the UNRESOLVED +input after a transient error, so a process-lifetime cache could serve a +stale or wrong path for the rest of the process's life. + +This file tests the corrected, call-scoped design: + (a) within one scope, repeated resolves of the same argument hit the cache + (single underlying `_safe_resolve()` call) -- same benefit as before. + (b) across two SEPARATE scopes (simulating two different requests/list + builds), the cache does NOT persist -- `_safe_resolve()` runs again + fresh on the second scope, proving no cross-request staleness. + (c) outside any scope, `_resolve_profile_home_param` resolves fresh every + time with no caching at all, matching pre-PR behavior exactly. """ import sys from pathlib import Path @@ -17,8 +34,84 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) import api.workspace as ws # noqa: E402 -def test_resolve_profile_home_param_caches_repeated_path_argument(tmp_path): - ws._PROFILE_HOME_RESOLVE_CACHE.clear() +def test_resolve_profile_home_param_caches_within_one_scope(tmp_path): + """(a) Same as the original test, but the cache now requires an active scope.""" + home_dir = tmp_path / "profile_home" + home_dir.mkdir() + + with patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as spy: + with ws.profile_home_resolve_cache_scope(): + first = ws._resolve_profile_home_param(home_dir) + second = ws._resolve_profile_home_param(home_dir) + third = ws._resolve_profile_home_param(home_dir) + + assert first == second == third == home_dir.resolve() + assert spy.call_count == 1, ( + "expected the filesystem resolve() to run once within one scope and " + f"be served from cache thereafter, but it ran {spy.call_count} times" + ) + + +def test_resolve_profile_home_param_caches_within_one_scope_for_profile_name(tmp_path): + """(a), second branch: a logical profile-NAME STRING (not a Path) resolves + + through `get_hermes_home_for_profile()` before hitting the same + `_cached_safe_resolve_profile_home()` memoization. Confirms the + within-scope cache is genuinely hit for named profiles too, not just + explicit Path callers. + """ + home_dir = tmp_path / "named_profile_home" + home_dir.mkdir() + + with patch("api.profiles.get_hermes_home_for_profile", return_value=home_dir), \ + patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as spy: + with ws.profile_home_resolve_cache_scope(): + first = ws._resolve_profile_home_param("myprofile") + second = ws._resolve_profile_home_param("myprofile") + third = ws._resolve_profile_home_param("myprofile") + + assert first == second == third == home_dir.resolve() + assert spy.call_count == 1, ( + "expected the filesystem resolve() to run once within one scope and " + f"be served from cache thereafter, but it ran {spy.call_count} times" + ) + + +def test_cache_does_not_persist_across_separate_scopes(tmp_path): + """(b) Two separate scoped calls (simulating two different requests) must + + NOT share a cache -- the whole point of the PR #7636 review fix. If a + symlink were retargeted between the two "requests", the second scope + must be free to observe the new target; that's only possible if + `_safe_resolve()` is invoked again fresh on scope #2. + """ + home_dir = tmp_path / "profile_home" + home_dir.mkdir() + + with patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as spy: + with ws.profile_home_resolve_cache_scope(): + ws._resolve_profile_home_param(home_dir) + ws._resolve_profile_home_param(home_dir) + assert spy.call_count == 1 + + # Second, separate scope -- simulates a later, independent request. + with ws.profile_home_resolve_cache_scope(): + ws._resolve_profile_home_param(home_dir) + ws._resolve_profile_home_param(home_dir) + + assert spy.call_count == 2, ( + "expected the second scope to re-resolve fresh (no cross-scope/" + f"cross-request caching), but _safe_resolve ran {spy.call_count} times total" + ) + + +def test_no_caching_outside_any_scope(tmp_path): + """(c) Outside a scope, every call resolves fresh -- matching pre-PR + + behavior exactly, since most call sites of `_resolve_profile_home_param` + are NOT the sidebar list-build hot loop and must never see a cached + (possibly stale) result. + """ home_dir = tmp_path / "profile_home" home_dir.mkdir() @@ -28,34 +121,25 @@ def test_resolve_profile_home_param_caches_repeated_path_argument(tmp_path): third = ws._resolve_profile_home_param(home_dir) assert first == second == third == home_dir.resolve() - assert spy.call_count == 1, ( - "expected the filesystem resolve() to run once and be served from " - f"cache thereafter, but it ran {spy.call_count} times" + assert spy.call_count == 3, ( + "expected every call outside an active scope to resolve fresh with " + f"no caching, but _safe_resolve ran {spy.call_count} times for 3 calls" ) -def test_resolve_profile_home_param_caches_repeated_profile_name_argument(tmp_path): - """Same fix, second branch: a logical profile-NAME STRING (not a Path) +def test_scope_decorator_use_matches_load_cli_sessions_uncached_wiring(): + """Sanity check that `profile_home_resolve_cache_scope()` is usable as a - resolves through `get_hermes_home_for_profile()` before hitting the same - `_cached_safe_resolve_profile_home()` memoization. This mirrors the - Path-argument test above but drives the string branch (workspace.py's - ``isinstance(profile, Path)`` check is False), confirming the cache is - genuinely hit -- not just for explicit Path callers -- and that the - resolved home is correct. + decorator (the way it wraps `_load_cli_sessions_uncached` in + `api/models.py`), and that the ContextVar is active only during the + decorated call. """ - ws._PROFILE_HOME_RESOLVE_CACHE.clear() - home_dir = tmp_path / "named_profile_home" - home_dir.mkdir() + assert ws._PROFILE_HOME_RESOLVE_SCOPE.get() is None - with patch("api.profiles.get_hermes_home_for_profile", return_value=home_dir), \ - patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as spy: - first = ws._resolve_profile_home_param("myprofile") - second = ws._resolve_profile_home_param("myprofile") - third = ws._resolve_profile_home_param("myprofile") + @ws.profile_home_resolve_cache_scope() + def _inside(): + return ws._PROFILE_HOME_RESOLVE_SCOPE.get() - assert first == second == third == home_dir.resolve() - assert spy.call_count == 1, ( - "expected the filesystem resolve() to run once and be served from " - f"cache thereafter, but it ran {spy.call_count} times" - ) + cache_during_call = _inside() + assert cache_during_call is not None + assert ws._PROFILE_HOME_RESOLVE_SCOPE.get() is None