diff --git a/api/models.py b/api/models.py index dcb064c1..c89a91ac 100644 --- a/api/models.py +++ b/api/models.py @@ -8222,7 +8222,14 @@ def clear_sidecar_metadata_cache() -> None: def _state_projection_sidecar_metadata(sid: str) -> dict: - """Return UI-owned metadata (title + archived) for a state.db-projected row. + """Return UI-owned metadata (title + archived + project_id) for a state row. + + ``project_id`` is UI-owned in exactly the same way as the other two: + ``/api/session/move`` writes it onto the sidecar and NOTHING writes + ``state.db.sessions.project_id``. Reading it here is what lets the sidebar + projection agree with the assignment ``all_sessions()`` re-surfaces, instead + of counting a moved conversation against the unassigned window (#6659 review + finding 2). Memoized by the sidecar file's (path, mtime_ns, size, ctime_ns) stat signature so the sidebar projection — which calls this once per row in both @@ -8233,11 +8240,11 @@ def _state_projection_sidecar_metadata(sid: str) -> dict: Returns a COPY so callers can't mutate the cached dict. NOTE: this stat-gates on ``SESSION_DIR / f'{sid}.json'`` because that file is - ``Session.load_metadata_only``'s sole source for title+archived. If that ever - stops being true (metadata moves to another store), this gate would short- - circuit before the real source — update both together. + ``Session.load_metadata_only``'s sole source for title+archived+project_id. If + that ever stops being true (metadata moves to another store), this gate would + short-circuit before the real source — update both together. """ - default = {"title": None, "archived": None} + default = {"title": None, "archived": None, "project_id": None} if not is_safe_session_id(sid): return dict(default) p = SESSION_DIR / f'{sid}.json' @@ -8265,6 +8272,8 @@ def _state_projection_sidecar_metadata(sid: str) -> dict: if title: metadata["title"] = title metadata["archived"] = bool(getattr(webui_meta, 'archived', False)) + project_id = str(getattr(webui_meta, 'project_id', None) or '').strip() + metadata["project_id"] = project_id or None with _SIDECAR_METADATA_CACHE_LOCK: # Re-check under lock in case a concurrent build populated it; either @@ -8422,6 +8431,17 @@ def _load_cli_sessions_uncached( return _webhook_pid() return None + # Sidecar-carried assignments, memoized per sid for this scan. + # _state_projection_sidecar_metadata is already stat-cached, but + # _interactive_row_project_id is called several times per row (budget + # seeding, the unassigned count, the projection), and one os.stat per call + # per row is the kind of cold-sidebar I/O #4842 removed. + _sidecar_pid_cache: dict[str, str | None] = {} + def _sidecar_row_project_id(sid: str) -> str | None: + if sid not in _sidecar_pid_cache: + _sidecar_pid_cache[sid] = _state_projection_sidecar_metadata(sid).get('project_id') + return _sidecar_pid_cache[sid] + def _interactive_row_project_id(row: dict) -> str | None: """Project chip for an interactive (CLI/TUI/ACP) state.db row. @@ -8429,6 +8449,16 @@ def _load_cli_sessions_uncached( payload, so the code that budgets a row and the code that renders it can never disagree about which project it belongs to. + The WebUI sidecar is consulted when state.db carries no assignment, + because ``/api/session/move`` writes ``project_id`` onto the sidecar ONLY + — nothing writes ``state.db.sessions.project_id``. Without this the + "``CLI_VISIBLE_SESSION_LIMIT`` unassigned conversations" guarantee counted + a WebUI-side assignment as unassigned while the route (which sees the + sidecar's value through ``all_sessions()``) classified the same + conversation as ASSIGNED, so three moved sessions silently shortened + everyone's sidebar to 17 rows (#6659 review finding 2). state.db wins when + it has an assignment of its own: it is the agent's own record. + Background sources keep the system-chip answer even when a ``source_filter`` routes them through this loop, so one kanban row cannot report a resolved ``project_id`` here and ``None`` from its own bounded @@ -8442,7 +8472,10 @@ def _load_cli_sessions_uncached( or is_webhook_session(sid, source) ): return _state_row_project_id(sid, source) - return _resolved_project_id(row.get('project_id')) + resolved = _resolved_project_id(row.get('project_id')) + if resolved is None: + resolved = _resolved_project_id(_sidecar_row_project_id(sid)) + return resolved profile_value = _cli_profile or 'default' # A deleted WebUI session is tombstoned (see _record_webui_deleted_session_tombstone) @@ -8702,6 +8735,17 @@ def _load_cli_sessions_uncached( unassigned_seen = sum( 1 for row in state_rows if _interactive_row_project_id(row) is None ) + # A sidecar-carried assignment is invisible to the SQL 'unassigned' + # filter (state.db still says NULL), so the refill query would spend + # that many of its own slots re-fetching conversations this pass already + # classified as assigned and already represents. Widen the query by that + # count so it can actually reach the older unassigned conversations the + # moved rows displaced (#6659 review finding 2). + sidecar_only_assigned = sum( + 1 for row in state_rows + if _interactive_row_project_id(row) is not None + and _resolved_project_id(row.get('project_id')) is None + ) if ( unassigned_target and unassigned_seen < unassigned_target @@ -8716,7 +8760,7 @@ def _load_cli_sessions_uncached( try: for row in read_importable_agent_session_rows( db_path, - limit=unassigned_target, + limit=unassigned_target + sidecar_only_assigned, log=logger, exclude_sources=interactive_excluded, project_assignment='unassigned', diff --git a/tests/test_issue4842_cron_projection_perf.py b/tests/test_issue4842_cron_projection_perf.py index 8b46b9a1..953bfe6a 100644 --- a/tests/test_issue4842_cron_projection_perf.py +++ b/tests/test_issue4842_cron_projection_perf.py @@ -201,7 +201,7 @@ def test_missing_sidecar_returns_default_without_caching_growth(tmp_path): meta = models._state_projection_sidecar_metadata("cron_nope_999") # No sidecar means no opinion on archived (None), so the projection falls back # to the state.db row's archived flag (#7548) instead of forcing it to False. - assert meta == {"title": None, "archived": None} + assert meta == {"title": None, "archived": None, "project_id": None} # No file → nothing cached (so the cache can't be poisoned by absent files). assert len(models._SIDECAR_METADATA_CACHE) == 0 diff --git a/tests/test_project_assigned_cli_session_limit.py b/tests/test_project_assigned_cli_session_limit.py index 1436d238..e0d1bf2a 100755 --- a/tests/test_project_assigned_cli_session_limit.py +++ b/tests/test_project_assigned_cli_session_limit.py @@ -1109,6 +1109,97 @@ def test_model_refills_the_unassigned_window_consumed_by_assigned_rows( assert len([s for s in sessions if s["project_id"] == "project-a"]) == 3 +def _write_webui_sidecar(session_dir, sid, *, project_id, updated_at): + """A WebUI session sidecar carrying a project assignment. + + This is what ``/api/session/move`` actually writes: it sets ``s.project_id`` + and saves the sidecar. NOTHING writes ``state.db.sessions.project_id`` — + ``grep 'UPDATE sessions'`` only ever touches title / message_count / + parent_session_id — so the assignment exists only here. + """ + session_dir.mkdir(parents=True, exist_ok=True) + (session_dir / f"{sid}.json").write_text( + json.dumps({ + "session_id": sid, + "title": sid, + "created_at": updated_at, + "updated_at": updated_at, + "message_count": 1, + "project_id": project_id, + "messages": [], + "tool_calls": [], + }), + encoding="utf-8", + ) + models.clear_sidecar_metadata_cache() + models.clear_cli_sessions_cache() + + +def test_sidecar_carried_assignment_does_not_spend_an_unassigned_slot( + fake_hermes_home, tmp_path, monkeypatch +): + """A WebUI-side move must not shorten the unassigned sidebar window. + + ``/api/session/move`` records the assignment on the WebUI sidecar ONLY, so + the model's unassigned counter — which read ``state.db.project_id`` — saw all + 23 conversations as unassigned and stopped at its 20-row window. The route + then reclassified the 3 moved conversations as assigned (``all_sessions()`` + re-surfaces the sidecar's ``project_id`` and the state row is dropped as + already represented), leaving only 17 unassigned CLI rows in the payload and + never fetching cli-00 / cli-01 / cli-02 at all — the reviewer's exact 3 + 20 + -> 17 number, in the guarantee this file's docstring claims to pin. + """ + session_dir = tmp_path / "sessions" + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + _register_projects(tmp_path, "project-a") + + rows = [_session(f"cli-{index:02d}", BASE_TS + index) for index in range(23)] + _write_state_db(fake_hermes_home / "state.db", rows) + + # The three NEWEST conversations were moved into a project from the WebUI. + moved = {"cli-22", "cli-21", "cli-20"} + for sid in sorted(moved): + _write_webui_sidecar( + session_dir, + sid, + project_id="project-a", + updated_at=BASE_TS + int(sid.split("-")[1]), + ) + + sessions = models.get_cli_sessions() + + # The real route pipeline: the moved conversations come back as WebUI sidecar + # rows and their state.db projections are dropped as duplicates. + deduped = routes._dedupe_cli_sidebar_sessions_for_api(sessions, set(moved)) + webui_rows = [ + { + "session_id": sid, + "is_cli_session": True, + "project_id": "project-a", + "updated_at": BASE_TS + int(sid.split("-")[1]), + } + for sid in sorted(moved) + ] + merged = sorted( + webui_rows + deduped, key=lambda s: s.get("updated_at") or 0, reverse=True + ) + kept = routes._cap_recent_cli_sessions(merged) + + unassigned_cli = [ + row for row in kept if row.get("is_cli_session") and not row.get("project_id") + ] + assert len(unassigned_cli) == routes.CLI_VISIBLE_SESSION_CAP == 20 + # The three genuinely unassigned conversations at the bottom of state.db are + # what the lost slots cost: they were never fetched. + kept_ids = {row["session_id"] for row in kept} + assert {"cli-00", "cli-01", "cli-02"} <= kept_ids + + # ...and the model's own notion of "unassigned" agrees with the + # classification the route applied, which is what made the refill reachable. + assert sum(1 for s in sessions if not s["project_id"]) == 20 + assert {s["session_id"] for s in sessions if s["project_id"] == "project-a"} == moved + + def test_unassigned_window_counts_logical_conversations_not_segments( fake_hermes_home, tmp_path ):