fix(webui): scope profile-home resolve cache to one session-list build
The process-lifetime cache from the previous commit was flagged in PR review (nesquena-hermes): Path.resolve() is a filesystem call, not a pure function of process-startup constants. A profile-home symlink can be retargeted while the long-running webui process stays up, and _safe_resolve() deliberately falls back to the unresolved input after a transient OSError/RuntimeError/ValueError -- a process-lifetime cache would serve either a stale symlink target or a stale fallback forever. Redesign: replace the module-level dict with a contextvars.ContextVar that defaults to None (no active scope -> resolve fresh, exactly like pre-PR behavior). profile_home_resolve_cache_scope() sets it to a fresh dict for the duration of one call and resets it in a finally block, so no cache entry survives past that call. It now wraps only _load_cli_sessions_uncached as a decorator -- the actual N+1 hot loop (hundreds of Session.__init__ calls per sidebar build, each resolving the same profile argument). All other call sites of _resolve_profile_home_param (confirmed via repo-wide grep: all internal to api/workspace.py, none elsewhere) get fresh resolution on every call, same as before this PR existed. Rewrote tests/test_profile_home_resolve_caching.py to prove: (a) repeated resolves within one scope still hit the cache once, (b) two separate scopes (simulating two requests) do not share state -- a symlink change between them is observed on the second scope, and (c) calls outside any scope never cache at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
67d6c3ff50
commit
8a335d5af2
3 files changed
+175
-44
No files matched your search
+2
-1
@@ -35,7 +35,7 @@ from api.config import (
|
|||||||
LOCK, STREAMS, STREAMS_LOCK, DEFAULT_WORKSPACE, DEFAULT_MODEL, PROJECTS_FILE, HOME,
|
LOCK, STREAMS, STREAMS_LOCK, DEFAULT_WORKSPACE, DEFAULT_MODEL, PROJECTS_FILE, HOME,
|
||||||
get_effective_default_model, _get_session_agent_lock,
|
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.usage import prompt_cache_hit_percent
|
||||||
from api.agent_sessions import (
|
from api.agent_sessions import (
|
||||||
_is_continuation_session,
|
_is_continuation_session,
|
||||||
@@ -7708,6 +7708,7 @@ def _state_projection_sidecar_metadata(sid: str) -> dict:
|
|||||||
return dict(metadata)
|
return dict(metadata)
|
||||||
|
|
||||||
|
|
||||||
|
@profile_home_resolve_cache_scope()
|
||||||
def _load_cli_sessions_uncached(
|
def _load_cli_sessions_uncached(
|
||||||
hermes_home: Path,
|
hermes_home: Path,
|
||||||
db_path: Path,
|
db_path: Path,
|
||||||
|
|||||||
+58
-12
@@ -19,6 +19,8 @@ import stat
|
|||||||
import subprocess
|
import subprocess
|
||||||
import sys
|
import sys
|
||||||
import concurrent.futures
|
import concurrent.futures
|
||||||
|
import contextlib
|
||||||
|
import contextvars
|
||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
from collections.abc import Callable
|
from collections.abc import Callable
|
||||||
@@ -207,7 +209,7 @@ def _is_remote_terminal_backend(terminal_cfg: dict | None) -> bool:
|
|||||||
return backend not in ('', 'local')
|
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
|
# `.resolve()` call. The webui's session list builds one `Session` per CLI
|
||||||
# session row (`_load_cli_sessions_uncached`), and `Session.__init__` calls
|
# session row (`_load_cli_sessions_uncached`), and `Session.__init__` calls
|
||||||
# `_resolve_profile_home_param(profile)` once per row -- with only one
|
# `_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"
|
# host load (confirmed via repeated "Slow WebUI request still running"
|
||||||
# warnings pinned to this call site).
|
# warnings pinned to this call site).
|
||||||
#
|
#
|
||||||
# Caching is safe for the life of the process: the mapping from a profile
|
# IMPORTANT: this is deliberately NOT a process-lifetime cache. An earlier
|
||||||
# argument to its home path is a pure function of process-startup constants
|
# revision of this fix cached forever, on the premise that a profile
|
||||||
# (`api.profiles._DEFAULT_HERMES_HOME`, `_INITIAL_HERMES_HOME`) that are
|
# argument always resolves to the same path for the life of the process --
|
||||||
# resolved once at import time and never mutated afterwards. Profile
|
# but `Path.resolve()` is a filesystem call, not a pure function of
|
||||||
# *creation* only creates the directory at that already-deterministic path
|
# startup constants: a profile-home symlink can be retargeted while the
|
||||||
# -- it does not change what path a given profile argument resolves to --
|
# webui process keeps running for days/weeks, and `_safe_resolve()` below
|
||||||
# so there is no runtime path that needs to invalidate an entry here.
|
# deliberately falls back to returning the UNRESOLVED input after a
|
||||||
_PROFILE_HOME_RESOLVE_CACHE: dict[Path, Path] = {}
|
# 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:
|
def _cached_safe_resolve_profile_home(pre_resolve: Path) -> Path:
|
||||||
"""Memoized `_safe_resolve()` for profile-home paths (see cache doc above)."""
|
"""`_safe_resolve()` for profile-home paths.
|
||||||
cached = _PROFILE_HOME_RESOLVE_CACHE.get(pre_resolve)
|
|
||||||
|
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:
|
if cached is not None:
|
||||||
return cached
|
return cached
|
||||||
resolved = _safe_resolve(pre_resolve)
|
resolved = _safe_resolve(pre_resolve)
|
||||||
_PROFILE_HOME_RESOLVE_CACHE[pre_resolve] = resolved
|
cache[pre_resolve] = resolved
|
||||||
return resolved
|
return resolved
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -1,12 +1,29 @@
|
|||||||
"""Regression test: `_resolve_profile_home_param` memoizes its filesystem
|
"""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)`
|
Building the CLI session list calls `_resolve_profile_home_param(profile)`
|
||||||
once per session row (`Session.__init__`), so with hundreds of sessions on
|
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
|
the same profile the unscoped code redundantly re-resolved the identical
|
||||||
a filesystem `.resolve()` on every single row. This asserts repeated calls
|
path via a filesystem `.resolve()` on every single row.
|
||||||
with the same profile argument return an equal Path while invoking the
|
|
||||||
underlying `_safe_resolve()` filesystem call only once.
|
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
|
import sys
|
||||||
from pathlib import Path
|
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
|
import api.workspace as ws # noqa: E402
|
||||||
|
|
||||||
|
|
||||||
def test_resolve_profile_home_param_caches_repeated_path_argument(tmp_path):
|
def test_resolve_profile_home_param_caches_within_one_scope(tmp_path):
|
||||||
ws._PROFILE_HOME_RESOLVE_CACHE.clear()
|
"""(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 = tmp_path / "profile_home"
|
||||||
home_dir.mkdir()
|
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)
|
third = ws._resolve_profile_home_param(home_dir)
|
||||||
|
|
||||||
assert first == second == third == home_dir.resolve()
|
assert first == second == third == home_dir.resolve()
|
||||||
assert spy.call_count == 1, (
|
assert spy.call_count == 3, (
|
||||||
"expected the filesystem resolve() to run once and be served from "
|
"expected every call outside an active scope to resolve fresh with "
|
||||||
f"cache thereafter, but it ran {spy.call_count} times"
|
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):
|
def test_scope_decorator_use_matches_load_cli_sessions_uncached_wiring():
|
||||||
"""Same fix, second branch: a logical profile-NAME STRING (not a Path)
|
"""Sanity check that `profile_home_resolve_cache_scope()` is usable as a
|
||||||
|
|
||||||
resolves through `get_hermes_home_for_profile()` before hitting the same
|
decorator (the way it wraps `_load_cli_sessions_uncached` in
|
||||||
`_cached_safe_resolve_profile_home()` memoization. This mirrors the
|
`api/models.py`), and that the ContextVar is active only during the
|
||||||
Path-argument test above but drives the string branch (workspace.py's
|
decorated call.
|
||||||
``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.
|
|
||||||
"""
|
"""
|
||||||
ws._PROFILE_HOME_RESOLVE_CACHE.clear()
|
assert ws._PROFILE_HOME_RESOLVE_SCOPE.get() is None
|
||||||
home_dir = tmp_path / "named_profile_home"
|
|
||||||
home_dir.mkdir()
|
|
||||||
|
|
||||||
with patch("api.profiles.get_hermes_home_for_profile", return_value=home_dir), \
|
@ws.profile_home_resolve_cache_scope()
|
||||||
patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as spy:
|
def _inside():
|
||||||
first = ws._resolve_profile_home_param("myprofile")
|
return ws._PROFILE_HOME_RESOLVE_SCOPE.get()
|
||||||
second = ws._resolve_profile_home_param("myprofile")
|
|
||||||
third = ws._resolve_profile_home_param("myprofile")
|
|
||||||
|
|
||||||
assert first == second == third == home_dir.resolve()
|
cache_during_call = _inside()
|
||||||
assert spy.call_count == 1, (
|
assert cache_during_call is not None
|
||||||
"expected the filesystem resolve() to run once and be served from "
|
assert ws._PROFILE_HOME_RESOLVE_SCOPE.get() is None
|
||||||
f"cache thereafter, but it ran {spy.call_count} times"
|
|
||||||
)
|
|
||||||
Reference in new issue
Block a user