fix(sessions): stop a WebUI-side move spending an unassigned slot
/api/session/move writes project_id onto the WebUI sidecar only; nothing writes state.db.sessions.project_id. The model's unassigned counter read state.db exclusively, so it believed all 23 conversations were unassigned and stopped at its 20-row window — while the route, which sees the sidecar's project_id through all_sessions(), classified the 3 moved conversations as ASSIGNED. Net result: 17 unassigned CLI rows in the payload and cli-00/01/02 never fetched at all, the reviewer's exact 3 + 20 -> 17 number. Resolve the assignment from the source of truth the route ends up using: _state_projection_sidecar_metadata (already the UI-owned title/archived reader, already stat-cached) now also returns project_id, and _interactive_row_project_id falls back to it when state.db has none. The unassigned refill query is widened by the number of sidecar-only assignments, because the SQL 'unassigned' filter cannot see them and would otherwise spend those slots re-fetching rows already represented.
This commit is contained in:
1 parent
bc80d71177
commit
27230d4480
3 files changed
+143
-8
No files matched your search
+51
-7
@@ -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',
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
):
|
||||
|
||||
Reference in new issue
Block a user