diff --git a/api/models.py b/api/models.py index 270e50b8..43ae4720 100644 --- a/api/models.py +++ b/api/models.py @@ -8006,7 +8006,7 @@ def _load_cli_sessions_uncached( cli_sessions.append({ 'session_id': sid, 'title': _display_title, - 'workspace': str(get_last_workspace(profile=_cli_profile)), + 'workspace': _cli_workspace(), 'model': row['model'] or None, 'message_count': row['message_count'] or row['actual_message_count'] or 0, 'created_at': row['started_at'], diff --git a/tests/test_profile_home_resolve_caching.py b/tests/test_profile_home_resolve_caching.py index 89836230..b41cc531 100644 --- a/tests/test_profile_home_resolve_caching.py +++ b/tests/test_profile_home_resolve_caching.py @@ -32,6 +32,7 @@ from unittest.mock import patch sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) import api.workspace as ws # noqa: E402 +import api.models as models # noqa: E402 def test_resolve_profile_home_param_caches_within_one_scope(tmp_path): @@ -143,3 +144,206 @@ def test_scope_decorator_use_matches_load_cli_sessions_uncached_wiring(): cache_during_call = _inside() assert cache_during_call is not None assert ws._PROFILE_HOME_RESOLVE_SCOPE.get() is None + + +def _make_cli_sessions_uncached_fixture(tmp_path, n, *, message_count=3): + """state.db + matching MODERN WebUI sidecars (message_count present in the + prefix, literal 'default' profile string -- exactly what real production + sidecars carry, confirmed against a live deployment's webui/sessions/*.json) + for `n` cron rows. + + A "modern" sidecar (one that carries `message_count` in its JSON-prefix) + is required to reach the fast branch of `Session.load_metadata_only()` + (a single `Session(**parsed)` construction) instead of falling back to the + legacy `cls.load()` re-parse path -- see #5854. That keeps this fixture + isolated to the exact N+1 call shape under test: one `Session.__init__` + (and therefore one `_resolve_profile_home_param` call) per sidecar row. + """ + import sqlite3 + import time + + db_path = tmp_path / "state.db" + conn = sqlite3.connect(str(db_path)) + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + source TEXT, + session_source TEXT, + title TEXT, + model TEXT, + started_at REAL NOT NULL, + message_count INTEGER DEFAULT 0, + parent_session_id TEXT, + ended_at REAL, + end_reason TEXT + ); + CREATE INDEX idx_sessions_started ON sessions(started_at); + CREATE TABLE messages ( + id TEXT PRIMARY KEY, + session_id TEXT, + role TEXT, + content TEXT, + timestamp REAL + ); + CREATE INDEX idx_messages_session ON messages(session_id, timestamp); + """ + ) + now = time.time() + sids = [] + for i in range(n): + sid = f"cron_job{i:04d}_{int(now) + i}" + sids.append(sid) + conn.execute( + "INSERT INTO sessions (id, source, session_source, title, model," + " started_at, message_count, parent_session_id, ended_at, end_reason)" + " VALUES (?, 'cron', 'cron', NULL, 'deepseek/deepseek-chat', ?, 1," + " NULL, NULL, NULL)", + (sid, now + i), + ) + conn.execute( + "INSERT INTO messages (id, session_id, role, content, timestamp)" + " VALUES (?, ?, 'user', 'cron task', ?)", + (f"cron_msg_{sid}", sid, now + i), + ) + conn.commit() + conn.close() + + session_dir = tmp_path / "sessions" + session_dir.mkdir() + for sid in sids: + (session_dir / f"{sid}.json").write_text( + '{"session_id": "%s", "title": "Renamed %s", "created_at": 1.0,' + ' "updated_at": 2.0, "archived": false, "message_count": %d,' + ' "profile": "default", "messages": []}' + % (sid, sid, message_count), + encoding="utf-8", + ) + return db_path, sids + + +def _count_profile_home_resolve_calls(call_args_list, expected_profile_home): + """Filter a `_safe_resolve` spy's call list down to just the profile-home + + branch (`pre_resolve == expected_profile_home`), excluding the unrelated + per-row `_safe_resolve()` call that resolves the session's `workspace` + path (same call count either way -- not part of this PR, not what + greptile's comment or the production stack trace was about). + """ + return sum( + 1 for call in call_args_list + if call.args and call.args[0] == expected_profile_home + ) + + +def test_load_cli_sessions_uncached_dedupes_profile_home_resolve_on_sidecar_cache_miss(tmp_path): + """Real N+1 reproduction for PR #7636 review comment (greptile, + api/models.py:7711): on a COLD `_SIDECAR_METADATA_CACHE` (fresh process, + or the first time each sid's sidecar is seen), `_state_projection_sidecar_metadata` + falls through to `Session.load_metadata_only(sid)` for EVERY row that has a + sidecar -- and that constructs a real `Session` via `Session.__init__`, + which resolves the session's `profile` param through + `_resolve_path -> _remote_terminal_workspace_candidate -> + _remote_terminal_cwd -> _resolve_profile_home_param -> _safe_resolve()`, + matching the exact stack trace captured in production + ("Slow WebUI request still running", 5-17s, pinned to this call site). + Real production sidecars carry the literal string "default" for `profile` + (confirmed against a live deployment's webui/sessions/*.json), which is + what this fixture uses too. + + This is a DIFFERENT code path than the direct dict-projection rows + greptile's comment focused on -- it fires from the sidecar-metadata + overlay, not from the row-to-dict projection itself. It IS covered by + `@profile_home_resolve_cache_scope()` on `_load_cli_sessions_uncached`, + because contextvars propagate through ordinary synchronous nested calls + (no thread pool / executor / asyncio task is involved anywhere in this + call chain) -- proven here by counting only the profile-home-resolving + `_safe_resolve()` invocations (filtered by argument), not the session's + unrelated per-row workspace-path resolve, which this PR never touched and + still runs once per row regardless. + """ + from unittest import mock + + import api.profiles as profiles + import api.workspace as ws + + models.clear_sidecar_metadata_cache() + n = 60 + db_path, sids = _make_cli_sessions_uncached_fixture(tmp_path, n) + + with ( + mock.patch("api.models.get_claude_code_sessions", return_value=[]), + mock.patch("api.models.get_last_workspace", return_value=str(tmp_path)), + mock.patch("api.models.ensure_cron_project", return_value="cron-pid"), + mock.patch("api.models.SESSION_DIR", tmp_path / "sessions"), + # Isolate the measurement: `_remote_terminal_cwd()` also calls + # `get_config_for_profile_home()` right after `_resolve_profile_home_param()`, + # and THAT helper does its own two direct, uncached `_safe_resolve()` calls + # on the same profile-home path (api/config.py, a pre-existing, separate + # hot path this PR does not touch and greptile's comment did not raise). + # Left unmocked those calls alias onto the same argument value and would + # make an unrelated inefficiency look like a failure of THIS cache. + mock.patch("api.config.get_config_for_profile_home", return_value={}), + mock.patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as safe_resolve_spy, + ): + result = models._load_cli_sessions_uncached(tmp_path, db_path, None) + + assert len(result) == n + profile_home_resolves = _count_profile_home_resolve_calls( + safe_resolve_spy.call_args_list, profiles._DEFAULT_HERMES_HOME + ) + # Every row's sidecar was a genuine cache miss (fresh clear above), so + # Session.load_metadata_only() ran -- and therefore Session.__init__() ran + # -- once per row: len(sids) separate `_resolve_profile_home_param("default")` + # calls. Without the fix each of those pays its own `_safe_resolve()` + # filesystem call; with the fix active only the FIRST one does, and the + # remaining len(sids) - 1 are served from the call-scoped cache. + assert profile_home_resolves == 1, ( + f"the profile-home _safe_resolve() ran {profile_home_resolves} times across " + f"{n} sidecar-miss rows sharing one profile -- expected exactly 1 (the rest " + "should be served from the call-scoped cache)" + ) + + +def test_load_cli_sessions_uncached_profile_home_resolve_scales_with_fix_disabled(tmp_path): + """Companion to the test above: calling the UNDECORATED function directly + + (bypassing `profile_home_resolve_cache_scope()` via `__wrapped__`, i.e. + reproducing pre-PR behavior) shows the real filesystem resolve DOES scale + linearly with row count -- confirming the dedup measured above is the + scope's doing, not an artifact of some other cache. + """ + from unittest import mock + + import api.workspace as ws + + models.clear_sidecar_metadata_cache() + n = 60 + db_path, sids = _make_cli_sessions_uncached_fixture(tmp_path, n) + + with ( + mock.patch("api.models.get_claude_code_sessions", return_value=[]), + mock.patch("api.models.get_last_workspace", return_value=str(tmp_path)), + mock.patch("api.models.ensure_cron_project", return_value="cron-pid"), + mock.patch("api.models.SESSION_DIR", tmp_path / "sessions"), + # Same isolation as the test above -- see its comment. + mock.patch("api.config.get_config_for_profile_home", return_value={}), + mock.patch.object(ws, "_cached_safe_resolve_profile_home", wraps=ws._cached_safe_resolve_profile_home) as memo_spy, + mock.patch.object(ws, "_safe_resolve", wraps=ws._safe_resolve) as safe_resolve_spy, + ): + undecorated = models._load_cli_sessions_uncached.__wrapped__ + result = undecorated(tmp_path, db_path, None) + + assert len(result) == n + # The memoizing function is still called once per row either way... + assert memo_spy.call_count == n + # ...but with no active scope it never hits its cache branch, so every + # single call pays its own real filesystem resolve. + import api.profiles as profiles + profile_home_resolves = _count_profile_home_resolve_calls( + safe_resolve_spy.call_args_list, profiles._DEFAULT_HERMES_HOME + ) + assert profile_home_resolves == n, ( + f"expected the profile-home _safe_resolve() to run once per row ({n}) with no " + f"active cache scope, but it ran {profile_home_resolves} times" + )