From 3e616ad4557219590eba474ef6f762f8e8912b5c Mon Sep 17 00:00:00 2001 From: n Date: Wed, 9 Sep 2026 22:56:51 +0000 Subject: [PATCH] fix: harden bounded sidebar cache projection (#7489) Preserve nested request isolation, make the response allowlist single-source, and strip heavy fields from existing index rows after reconciliation. Co-authored-by: Martin Dell --- api/models.py | 33 ++++- api/route_session_list_cache.py | 156 +++++++++++++++----- api/routes.py | 97 ++++-------- tests/test_465_session_branching.py | 4 +- tests/test_issue1855_request_diagnostics.py | 4 +- tests/test_session_list_cache_bounded.py | 128 ++++++++++++++++ 6 files changed, 305 insertions(+), 117 deletions(-) diff --git a/api/models.py b/api/models.py index bc1efcc6..775124dc 100644 --- a/api/models.py +++ b/api/models.py @@ -1191,6 +1191,25 @@ def model_explicit_pick_signature(model, model_provider) -> str: return f"{_m}\x1f{_p}" +_SIDEBAR_HEAVY_METADATA_FIELDS = ( + 'compression_anchor_summary', + 'compression_anchor_details', + 'context_engine_state', + 'compression_recovery', + 'gateway_routing_history', + 'composer_draft', + 'process_wakeup_pause', + 'share_token', +) + + +def _strip_sidebar_heavy_metadata(row: dict) -> dict: + """Remove detail-only values after sidebar reconciliation is complete.""" + for key in _SIDEBAR_HEAVY_METADATA_FIELDS: + row.pop(key, None) + return row + + class Session: def __init__(self, session_id: str=None, title: str='Untitled', workspace=str(DEFAULT_WORKSPACE), created_workspace=None, @@ -1815,13 +1834,7 @@ class Session: ) if include_runtime else False, } if sidebar_metadata_only: - for key in ( - 'compression_anchor_summary', 'compression_anchor_details', - 'context_engine_state', 'compression_recovery', - 'gateway_routing_history', 'composer_draft', - 'process_wakeup_pause', 'share_token', - ): - compact.pop(key, None) + _strip_sidebar_heavy_metadata(compact) return compact @@ -6450,6 +6463,9 @@ def all_sessions( for s in result: if not s.get('profile'): s['profile'] = 'default' + if sidebar_metadata_only: + for s in result: + _strip_sidebar_heavy_metadata(s) return result except Exception: logger.debug("Failed to load session index, falling back to full scan") @@ -6507,6 +6523,9 @@ def all_sessions( for s in result: if not s.get('profile'): s['profile'] = 'default' + if sidebar_metadata_only: + for s in result: + _strip_sidebar_heavy_metadata(s) return result diff --git a/api/route_session_list_cache.py b/api/route_session_list_cache.py index b0bae89a..b91ba7b8 100644 --- a/api/route_session_list_cache.py +++ b/api/route_session_list_cache.py @@ -1,5 +1,6 @@ """Session-list cache helpers extracted from api.routes.""" +import copy import os import re import threading @@ -43,29 +44,86 @@ _SESSIONS_CACHE_ALL_PROFILES_INVALIDATION_VERSION = 0 _SESSIONS_CACHE_PROFILE_INVALIDATION_VERSION: dict[str, int] = {} +_SIDEBAR_SESSION_RESPONSE_FIELDS = { + "session_id", + "title", + "display_title", + "_state_db_title", + "workspace", + "model", + "model_provider", + "message_count", + "user_message_count", + "created_at", + "updated_at", + "last_message_at", + "pinned", + "archived", + "project_id", + "profile", + "input_tokens", + "output_tokens", + "estimated_cost", + "cache_read_tokens", + "cache_write_tokens", + "cache_hit_percent", + "personality", + "context_length", + "config_context_length", + "window_usage_percent", + "source_tag", + "raw_source", + "session_source", + "source_label", + "is_cli_session", + "is_messaging_session", + "is_streaming", + "cron_running", + "active_stream_id", + "has_pending_user_message", + "pending_started_at", + "default_hidden", + "worktree_path", + "worktree_branch", + "parent_session_id", + "parent_title", + "parent_source", + "relationship_type", + "pre_compression_snapshot", + "_lineage_root_id", + "_lineage_tip_id", + "_compression_segment_count", + "_lineage_collapsed_count", + "_parent_lineage_root_id", + "_parent_lineage_tip_id", + "_cross_surface_child_session", + "match_type", + "match_preview", + # Preserved so the sidebar can suppress rename / action-menu / swipe on + # read-only sessions and render the detailed gateway model label. Only the + # latest bounded routing object is included; routing history stays excluded. + "read_only", + "is_read_only", + "gateway_routing", +} + + def _session_list_cache_sidebar_fields() -> set[str]: """Return the canonical bounded field set used by the list response.""" - try: - import api.routes as _routes + return _SIDEBAR_SESSION_RESPONSE_FIELDS - fields = getattr(_routes, "_SIDEBAR_SESSION_RESPONSE_FIELDS", None) - if isinstance(fields, set): - return fields - except Exception: - pass - # The fallback is intentionally small and contains no transcript-bearing - # fields. Normal runtime calls resolve the canonical set above after - # api.routes has finished importing. + +def _session_list_cache_copy_value(value): + """Copy only mutable values after the transcript-bearing projection.""" + if isinstance(value, (dict, list, set)): + return copy.deepcopy(value) + return value + + +def _session_list_cache_copy_row(row: dict) -> dict: return { - "session_id", "title", "workspace", "model", "model_provider", - "message_count", "user_message_count", "created_at", "updated_at", - "last_message_at", "pinned", "archived", "project_id", "profile", - "source_tag", "session_source", "is_streaming", "active_stream_id", - "has_pending_user_message", "pending_started_at", "default_hidden", - "worktree_path", "worktree_branch", "parent_session_id", - "parent_title", "parent_source", "relationship_type", - "pre_compression_snapshot", "read_only", "is_read_only", - "gateway_routing", + key: _session_list_cache_copy_value(value) + for key, value in row.items() } @@ -86,34 +144,40 @@ def _session_list_cache_bounded_payload(payload: dict) -> dict: projected.append({}) continue item = {key: row[key] for key in fields if key in row} - if isinstance(item.get("gateway_routing"), dict): - item["gateway_routing"] = dict(item["gateway_routing"]) - projected.append(item) + projected.append(_session_list_cache_copy_row(item)) return projected bounded = { - key: value + key: _session_list_cache_copy_value(value) for key, value in payload.items() if key not in {"sessions", "sidebar_reference_sessions"} } - bounded["sessions"] = project_rows(payload.get("sessions")) - bounded["sidebar_reference_sessions"] = project_rows( - payload.get("sidebar_reference_sessions") - ) + if "sessions" in payload: + bounded["sessions"] = project_rows(payload.get("sessions")) + if "sidebar_reference_sessions" in payload: + bounded["sidebar_reference_sessions"] = project_rows( + payload.get("sidebar_reference_sessions") + ) return bounded def _session_list_cache_copy_payload(payload: dict) -> dict: """Copy only the already-bounded cache shape for request-local mutation.""" - copied = dict(payload) - copied["sessions"] = [dict(row) for row in payload.get("sessions", [])] - copied["sidebar_reference_sessions"] = [ - dict(row) for row in payload.get("sidebar_reference_sessions", []) - ] - for rows in (copied["sessions"], copied["sidebar_reference_sessions"]): - for row in rows: - if isinstance(row.get("gateway_routing"), dict): - row["gateway_routing"] = dict(row["gateway_routing"]) + copied = { + key: _session_list_cache_copy_value(value) + for key, value in payload.items() + if key not in {"sessions", "sidebar_reference_sessions"} + } + if "sessions" in payload: + copied["sessions"] = [ + _session_list_cache_copy_row(row) + for row in payload.get("sessions", []) + ] + if "sidebar_reference_sessions" in payload: + copied["sidebar_reference_sessions"] = [ + _session_list_cache_copy_row(row) + for row in payload.get("sidebar_reference_sessions", []) + ] return copied @@ -351,16 +415,32 @@ def _session_list_cache_stale_reason(key: tuple) -> str | None: return None -def _session_list_cache_set(key: tuple, payload: dict) -> None: +def _session_list_cache_set( + key: tuple, + payload: dict, + *, + expected_invalidation_stamp: tuple[int, int] | None = None, +) -> bool: if not isinstance(payload, dict): - return + return False stamp = _session_list_cache_resolved_source_stamp(key) bounded = _session_list_cache_bounded_payload(payload) with _SESSIONS_CACHE_LOCK: + # Projection intentionally happens outside the lock so large source rows + # cannot block cache hits. Re-check the caller's pre-build generation + # atomically before insertion, otherwise a rename/archive/delete clear + # that lands during projection can be undone by this stale write. + if ( + expected_invalidation_stamp is not None + and _session_list_cache_invalidation_stamp(key) + != expected_invalidation_stamp + ): + return False _SESSIONS_CACHE[key] = (time.monotonic(), stamp, bounded) _SESSIONS_CACHE.move_to_end(key) while len(_SESSIONS_CACHE) > _SESSIONS_CACHE_MAX_ENTRIES: _SESSIONS_CACHE.popitem(last=False) + return True def _session_list_cache_clear(profile: str | None = None) -> None: diff --git a/api/routes.py b/api/routes.py index d50841ee..305b1539 100644 --- a/api/routes.py +++ b/api/routes.py @@ -2771,8 +2771,14 @@ def _get_cached_session_list_payload( "session list stale-cache background rebuild failed" ) return - if _session_list_cache_invalidation_stamp(key) == invalidation_stamp: - _session_list_cache_set(key, payload) + if ( + _session_list_cache_invalidation_stamp(key) == invalidation_stamp + and _session_list_cache_set( + key, + payload, + expected_invalidation_stamp=invalidation_stamp, + ) + ): return rebuild_attempts += 1 if rebuild_attempts >= 3: @@ -2808,8 +2814,14 @@ def _get_cached_session_list_payload( while True: invalidation_stamp = _session_list_cache_invalidation_stamp(key) payload = builder() - if _session_list_cache_invalidation_stamp(key) == invalidation_stamp: - _session_list_cache_set(key, payload) + if ( + _session_list_cache_invalidation_stamp(key) == invalidation_stamp + and _session_list_cache_set( + key, + payload, + expected_invalidation_stamp=invalidation_stamp, + ) + ): if diag is not None: try: diag.stage("session_list_cache_stored") @@ -2868,7 +2880,11 @@ def _get_cached_session_list_payload( invalidation_stamp = _session_list_cache_invalidation_stamp(key) payload = builder() if _session_list_cache_invalidation_stamp(key) == invalidation_stamp: - _session_list_cache_set(key, payload) + _session_list_cache_set( + key, + payload, + expected_invalidation_stamp=invalidation_stamp, + ) return payload from api.config import ( @@ -10680,71 +10696,12 @@ def _session_attention_summary(session_id: str) -> dict | None: return None -_SIDEBAR_SESSION_RESPONSE_FIELDS = { - "session_id", - "title", - "display_title", - "_state_db_title", - "workspace", - "model", - "model_provider", - "message_count", - "user_message_count", - "created_at", - "updated_at", - "last_message_at", - "pinned", - "archived", - "project_id", - "profile", - "input_tokens", - "output_tokens", - "estimated_cost", - "cache_read_tokens", - "cache_write_tokens", - "cache_hit_percent", - "personality", - "context_length", - "config_context_length", - "window_usage_percent", - "source_tag", - "raw_source", - "session_source", - "source_label", - "is_cli_session", - "is_messaging_session", - "is_streaming", - "cron_running", - "active_stream_id", - "has_pending_user_message", - "pending_started_at", - "default_hidden", - "worktree_path", - "worktree_branch", - "parent_session_id", - "parent_title", - "parent_source", - "relationship_type", - "pre_compression_snapshot", - "_lineage_root_id", - "_lineage_tip_id", - "_compression_segment_count", - "_lineage_collapsed_count", - "_parent_lineage_root_id", - "_parent_lineage_tip_id", - "_cross_surface_child_session", - "match_type", - "match_preview", - # Preserved so the sidebar can suppress rename / action-menu / swipe on - # read-only (imported CLI + Claude Code) sessions, and render the detailed - # gateway model label. Dropping these silently regressed both surfaces. - # Only the latest `gateway_routing` is included (the sidebar label reader - # prefers it); the unbounded `gateway_routing_history` is intentionally NOT - # sent in the list payload to avoid per-row bloat. - "read_only", - "is_read_only", - "gateway_routing", -} +# One canonical allowlist owns both cache projection and final serialization. +# Keeping it in the cache module avoids a circular-import fallback that could +# silently truncate otherwise valid sidebar fields. +_SIDEBAR_SESSION_RESPONSE_FIELDS = ( + _route_session_list_cache._SIDEBAR_SESSION_RESPONSE_FIELDS +) def _sidebar_session_response_item(session: dict, *, redact_enabled: bool | None = None) -> dict: diff --git a/tests/test_465_session_branching.py b/tests/test_465_session_branching.py index e8a89e34..e792c0c4 100644 --- a/tests/test_465_session_branching.py +++ b/tests/test_465_session_branching.py @@ -606,7 +606,9 @@ def test_session_compact_includes_parent(): # the top of compact() which pushed the parent_session_id field beyond a # 1500-char window — widen the scan to 3000 chars to cover the full # return-dict body without re-tightening every time compact() grows. - compact_def_match = re.search(r"def compact\(self", src) + # compact() accepts optional projection flags on separate lines; match the + # method name and first parameter without pinning formatting. + compact_def_match = re.search(r"def compact\(\s*self", src) assert compact_def_match, "Could not find compact() method" snippet = src[compact_def_match.start():compact_def_match.start() + 3000] assert "'parent_session_id'" in snippet, \ diff --git a/tests/test_issue1855_request_diagnostics.py b/tests/test_issue1855_request_diagnostics.py index 6843120d..26ca0666 100644 --- a/tests/test_issue1855_request_diagnostics.py +++ b/tests/test_issue1855_request_diagnostics.py @@ -95,7 +95,9 @@ def test_issue1855_target_routes_are_wired_to_diagnostics(): src = Path("api/routes.py").read_text(encoding="utf-8") assert 'RequestDiagnostics.maybe_start("GET", parsed.path' in src - assert "all_sessions(diag=diag, include_lineage_metadata=False)" in src + assert 'kwargs = {"diag": diag, "include_lineage_metadata": False}' in src + assert 'kwargs["sidebar_metadata_only"] = True' in src + assert "return all_sessions(**kwargs)" in src assert 'RequestDiagnostics.maybe_start("POST", parsed.path' in src assert "_handle_chat_start(handler, body, diag=diag)" in src for stage in ( diff --git a/tests/test_session_list_cache_bounded.py b/tests/test_session_list_cache_bounded.py index 9572430e..f10ff947 100644 --- a/tests/test_session_list_cache_bounded.py +++ b/tests/test_session_list_cache_bounded.py @@ -1,6 +1,11 @@ """Regression tests for bounded sidebar session-list caching.""" +import json +import threading + from api import route_session_list_cache as cache +from api import models +from api import routes from api.models import Session @@ -59,6 +64,89 @@ def test_cache_read_returns_independent_bounded_rows(monkeypatch): assert second["sessions"][0]["title"] == "One" +def test_cache_read_isolates_nested_rows_and_settings(monkeypatch): + cache._session_list_cache_clear() + monkeypatch.setattr(cache, "_session_list_cache_resolved_source_stamp", lambda _key: ("stable",)) + key = _key() + cache._session_list_cache_set(key, { + "sessions": [{ + "session_id": "s1", + "gateway_routing": { + "routing": [{"provider": "first", "model": "model-a"}], + }, + }], + "settings": {"show_cli_sessions": False}, + }) + + first, _ = cache._session_list_cache_get(key) + assert first is not None + first["sessions"][0]["gateway_routing"]["routing"][0]["provider"] = "mutated" + first["settings"]["show_cli_sessions"] = True + second, _ = cache._session_list_cache_get(key) + assert second is not None + + assert second["sessions"][0]["gateway_routing"]["routing"][0]["provider"] == "first" + assert second["settings"]["show_cli_sessions"] is False + + +def test_invalidation_during_projection_cannot_reinsert_stale_payload(monkeypatch): + cache._session_list_cache_clear() + key = routes._session_list_cache_key( + active_profile="default", + all_profiles=False, + show_cli_sessions=False, + show_previous_messaging_sessions=False, + show_cron_sessions=False, + ) + stale_payload = {"sessions": [{"session_id": "before-clear", "title": "stale"}]} + fresh_payload = {"sessions": [{"session_id": "after-clear", "title": "fresh"}]} + projection_started = threading.Event() + release_projection = threading.Event() + real_project = cache._session_list_cache_bounded_payload + + def blocked_project(value): + if value == stale_payload: + projection_started.set() + assert release_projection.wait(5), "projection release timed out" + return real_project(value) + + monkeypatch.setattr(cache, "_session_list_cache_bounded_payload", blocked_project) + result = {} + build_count = 0 + + def builder(): + nonlocal build_count + build_count += 1 + return stale_payload if build_count == 1 else fresh_payload + + def owner(): + result["payload"] = routes._get_cached_session_list_payload( + key=key, + builder=builder, + ) + + thread = threading.Thread(target=owner) + thread.start() + assert projection_started.wait(5), "cache set never reached projection" + cache._session_list_cache_clear() + release_projection.set() + thread.join(5) + + assert not thread.is_alive() + assert build_count == 2 + assert result["payload"] == fresh_payload + cached, fresh = cache._session_list_cache_get(key) + assert fresh is True + assert cached == fresh_payload + + +def test_sidebar_field_allowlist_has_one_authority(): + fields = getattr(cache, "_SIDEBAR_SESSION_RESPONSE_FIELDS", None) + assert fields is not None + assert routes._SIDEBAR_SESSION_RESPONSE_FIELDS is fields + assert cache._session_list_cache_sidebar_fields() is fields + + def test_sidebar_compact_omits_heavy_session_metadata(): session = Session( session_id="heavy", @@ -86,3 +174,43 @@ def test_sidebar_compact_omits_heavy_session_metadata(): "process_wakeup_pause", "share_token", ): assert field not in sidebar + + +def test_sidebar_metadata_only_projects_existing_index_rows(monkeypatch, tmp_path): + heavy_fields = { + "compression_anchor_summary": "s" * 1000000, + "compression_anchor_details": {"detail": "d" * 1000000}, + "context_engine_state": {"state": "x" * 1000000}, + "compression_recovery": {"recovery": "r" * 1000000}, + "gateway_routing_history": [{"provider": "old"}] * 1000, + "composer_draft": {"draft": "c" * 1000000}, + "process_wakeup_pause": {"pause": "p" * 1000000}, + "share_token": "token-value", + } + index_path = tmp_path / "_index.json" + index_path.write_text(json.dumps([{ + "session_id": "indexed", + "title": "Indexed", + "message_count": 1, + "user_message_count": 1, + "created_at": 1.0, + "updated_at": 2.0, + "last_message_at": 2.0, + "profile": "default", + **heavy_fields, + }])) + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", index_path) + monkeypatch.setattr(models, "SESSIONS", {}) + monkeypatch.setattr(models, "_persisted_session_ids_snapshot", lambda: {"indexed"}) + monkeypatch.setattr(models, "_active_stream_ids", lambda: set()) + monkeypatch.setattr(models, "_apply_sidebar_state_db_overrides", lambda _rows: None) + + rows = models.all_sessions( + include_lineage_metadata=False, + sidebar_metadata_only=True, + ) + + assert [row["session_id"] for row in rows] == ["indexed"] + for field in heavy_fields: + assert field not in rows[0]