diff --git a/api/routes.py b/api/routes.py index 2ab4cba1..ce0099a2 100644 --- a/api/routes.py +++ b/api/routes.py @@ -10488,29 +10488,73 @@ def _cli_visible_session_cap() -> int: return CLI_VISIBLE_SESSION_LIMIT -# Per-project bound on project-assigned CLI rows in the FINAL MERGED payload. -# This is the only place that sees every source of assigned rows at once — -# state.db's own bounded passes plus imported WebUI sidecars from all_sessions(), -# which no model-side cap applies to — so it is the only place that can actually -# bound the assigned set (#6659 review finding 1). +# Bound on project-assigned CLI rows in the FINAL MERGED payload, across EVERY +# project. This is the only place that sees every source of assigned rows at +# once — state.db's own bounded passes plus imported WebUI sidecars from +# all_sessions(), which no model-side cap applies to — so it is the only place +# that can actually bound the assigned set (#6659 review finding 1). +# +# It has to bound the MERGED set, not one project: 200 rows x N projects grows +# with the project count, and 1,000 assigned conversations spread over 5 projects +# still returned all 1,000 — the exact reproduction from that finding. Pinned +# equal to models.PROJECT_ASSIGNED_CLI_LIMIT (itself resolved from the shared +# configurable sidebar window's ceiling on the model side) by +# test_route_merged_assigned_cap_is_the_existing_model_row_cap. CLI_PROJECT_ASSIGNED_CAP = 200 -# Global ceiling on the assigned rows this cap will emit, mirroring -# models.PROJECT_ASSIGNED_CLI_SCAN_CEILING so the route can never hand back more -# assigned conversations than the model pass was allowed to recover. A flat -# per-project bound does NOT bound the payload: 200 rows x N projects grows with -# the project count, and 1,000 assigned rows spread over 5 projects still -# returned all 1,000 — the exact reproduction from #6659 review finding 1. -# Spelled as a literal because api.models is imported further down this module; -# pinned equal to the model constant by -# test_route_project_ceiling_matches_the_model_constant. -CLI_PROJECT_ASSIGNED_SCAN_CEILING = 2000 + + +def _draw_assigned_cli_rows_fairly( + rows_by_project: dict[str, list[int]], budget: int +) -> set[int]: + """Pick ``budget`` assigned row indices, spread fairly across the projects. + + ``rows_by_project`` maps project id -> that project's row indices, newest + first, keyed in order of each project's most recent assigned conversation + (``sessions`` is newest-first, so insertion order already is that order). + + Each round hands one slot to every project that still has history left, so: + + * the drawn set never exceeds ``budget`` — that is the whole point, a + per-project bound does not bound the payload (#6659 review finding 1); + * no single busy project can eat every slot, which a flat ``sessions[:200]`` + truncation would do to whichever project sorts first — the starvation + greptile rejected as P1 on #6659; + * every project keeps at least one row whenever + ``budget >= len(rows_by_project)``. Past that the bound wins: with more + assigned projects than slots, the ``budget`` most recently active projects + get one row each, because the review's number is the hard constraint. + + Within a project the draw is newest-first, so what a chip loses is always the + oldest end of its own history. + """ + drawn: set[int] = set() + if budget <= 0 or not rows_by_project: + return drawn + queues = list(rows_by_project.values()) + offsets = [0] * len(queues) + remaining = budget + while remaining > 0: + progressed = False + for position, project_rows in enumerate(queues): + offset = offsets[position] + if offset >= len(project_rows): + continue + drawn.add(project_rows[offset]) + offsets[position] = offset + 1 + remaining -= 1 + progressed = True + if remaining <= 0: + break + if not progressed: + # Every project is exhausted — the whole assigned set fits. + break + return drawn def _cap_recent_cli_sessions( sessions: list[dict], cli_cap: int | None = None, project_cap: int = CLI_PROJECT_ASSIGNED_CAP, - project_ceiling: int = CLI_PROJECT_ASSIGNED_SCAN_CEILING, ) -> list[dict]: """Cap the default CLI list while retaining project-addressable rows. @@ -10525,43 +10569,39 @@ def _cap_recent_cli_sessions( to a project silently shortens everyone's sidebar to 17 rows. Resolved lazily from the shared configurable window (HERMES_WEBUI_VISIBLE_SESSION_LIMIT), never a second hard-coded 20. - * ``project_cap`` assigned conversations PER PROJECT stay in the payload so - the project chip can reveal them, marked ``default_hidden`` once the recent - window is full. Past that bound they are dropped: keeping assigned rows past - the *recent* cap is the fix, keeping them past *all* bounds just trades a - vanishing session for a stalled sidebar. + * ``project_cap`` assigned conversations IN TOTAL, across every project, stay + in the payload so the project chips can reveal them, marked + ``default_hidden`` once the recent window is full. Past that bound they are + dropped: keeping assigned rows past the *recent* cap is the fix, keeping + them past *all* bounds just trades a vanishing session for a stalled + sidebar. - That per-project budget is an EQUAL share of ``project_ceiling`` - (``ceiling // projects``, never below 1) once enough projects exist to exceed - it, which is what makes the merged payload bounded no matter how many - projects a profile has while keeping a project's allowance independent of how - loud its neighbours are. Same shape as the model-side share of - ``PROJECT_ASSIGNED_CLI_SCAN_CEILING``, and for the same reason a flat global - cap is not usable here: it would leave every project past it permanently - unreachable on every rebuild (greptile P1 on #6659). + That assigned budget is spent by a fair round-robin draw across the projects + (see ``_draw_assigned_cli_rows_fairly``) instead of by truncating the merged + list, so bounding the payload cannot starve a quiet project (greptile P1 on + #6659). ``project_cap <= 0`` disables the assigned bound entirely. """ if cli_cap is None: cli_cap = _cli_visible_session_cap() if cli_cap <= 0: return sessions - # The share denominator has to be known before the first assigned row is - # charged against it, so count the distinct assigned projects up front. - assigned_projects = { - project_id - for session in sessions - if _is_cli_session_for_settings(session) - and (project_id := str(session.get("project_id") or "").strip()) - } - effective_project_cap = project_cap - if project_cap > 0 and project_ceiling > 0 and assigned_projects: - effective_project_cap = min( - project_cap, max(1, project_ceiling // len(assigned_projects)) - ) + # Group the assigned rows per project first: the draw has to weigh the + # projects against each other, which a single forward pass cannot do. + rows_by_project: dict[str, list[int]] = {} + for index, session in enumerate(sessions): + if not _is_cli_session_for_settings(session): + continue + project_id = str(session.get("project_id") or "").strip() + if project_id: + rows_by_project.setdefault(project_id, []).append(index) + drawn = ( + None if project_cap <= 0 + else _draw_assigned_cli_rows_fairly(rows_by_project, project_cap) + ) kept = [] recent_seen = 0 unassigned_seen = 0 - assigned_seen: dict[str, int] = {} - for session in sessions: + for index, session in enumerate(sessions): if _is_cli_session_for_settings(session): project_id = str(session.get("project_id") or "").strip() if not project_id: @@ -10570,9 +10610,7 @@ def _cap_recent_cli_sessions( continue recent_seen += 1 else: - seen = assigned_seen.get(project_id, 0) + 1 - assigned_seen[project_id] = seen - if effective_project_cap > 0 and seen > effective_project_cap: + if drawn is not None and index not in drawn: continue if recent_seen >= cli_cap: session = dict(session) diff --git a/tests/test_project_assigned_cli_session_limit.py b/tests/test_project_assigned_cli_session_limit.py index 8ad9bb50..1436d238 100755 --- a/tests/test_project_assigned_cli_session_limit.py +++ b/tests/test_project_assigned_cli_session_limit.py @@ -12,9 +12,11 @@ swing to the opposite failure, so these tests pin BOTH edges of the contract: is followed by project-scoped, per-project-budgeted queries (greptile P1); * the follow-up is COMPLETE — every starved project is served, with no fixed per-build project cap that would permanently skip the rest (greptile P1); -* the recovered set is BOUNDED — per project, counting logical conversations — - including assigned rows that arrive as imported WebUI sidecars and therefore - never pass through any state.db cap; +* the recovered set is BOUNDED — the FINAL MERGED assigned payload never exceeds + the 200-row assigned cap, counting logical conversations, including assigned + rows that arrive as imported WebUI sidecars and therefore never pass through + any state.db cap — and that bound is spent by a fair per-project draw, so + bounding the payload does not starve a quiet project; * assigned rows do not spend the unassigned sidebar quota: 20 unique unassigned logical conversations survive after lineage/sidecar dedup; * a deleted or cross-profile ``project_id`` cannot hide a session — it resolves @@ -291,11 +293,57 @@ def test_route_cap_bounds_imported_sidecar_assigned_rows(fake_hermes_home): assert not any("default_hidden" in row for row in sidecars) -def test_route_cap_bounds_each_project_independently(fake_hermes_home): +def _assigned_project_counts(rows): + """project_id -> kept row count, for the assigned rows only.""" + counts: dict[str, int] = {} + for row in rows: + project_id = row.get("project_id") + if project_id: + counts[project_id] = counts.get(project_id, 0) + 1 + return counts + + +def test_route_cap_bounds_the_reviewers_five_project_reproduction(fake_hermes_home): + """The review's literal reproduction: 1,000 assigned rows over 5 projects. + + A PER-PROJECT 200 is not a bound on the payload — every one of the five + projects stayed under its own 200, so 1,000 rows went in and 1,000 came out, + which is exactly what review finding 1 reported. The review asked for "the + existing 200-row assigned cap across the final MERGED CLI set", so the bound + is on the whole assigned payload, not on one project. + """ + project_count = 5 + per_project = 200 + rows = [] + for project in range(project_count): + rows.extend( + { + "session_id": f"project-{project}-{index:04d}", + "is_cli_session": True, + "project_id": f"project-{project}", + } + for index in range(per_project) + ) + assert len(rows) == 1000 + + kept = routes._cap_recent_cli_sessions(rows) + + assert len(kept) == routes.CLI_PROJECT_ASSIGNED_CAP == 200 + # Bounded by a FAIR draw, not by truncating the head of the list: the rows + # are grouped per project, so a flat `sessions[:200]` would have handed + # project-0 every slot and left the other four unreachable — the starvation + # greptile rejected as P1. + counts = _assigned_project_counts(kept) + assert len(counts) == project_count + assert set(counts.values()) == {routes.CLI_PROJECT_ASSIGNED_CAP // project_count} + + +def test_route_cap_splits_the_merged_budget_fairly_between_projects(fake_hermes_home): """A busy project must not evict another project's history (greptile P1). - The bound is per project, so a profile with more than - ``CLI_PROJECT_ASSIGNED_CAP`` assigned conversations in TOTAL keeps them all. + The merged budget is drawn round-robin, newest first WITHIN each project, so + two equally deep projects get an equal share and each keeps its newest + conversations rather than an arbitrary slice. """ rows = [] for project in ("project-a", "project-b"): @@ -310,27 +358,26 @@ def test_route_cap_bounds_each_project_independently(fake_hermes_home): kept = routes._cap_recent_cli_sessions(rows) - counts: dict[str, int] = {} - for row in kept: - counts[row["project_id"]] = counts.get(row["project_id"], 0) + 1 - assert counts == { - "project-a": routes.CLI_PROJECT_ASSIGNED_CAP, - "project-b": routes.CLI_PROJECT_ASSIGNED_CAP, - } - assert len(kept) > routes.CLI_PROJECT_ASSIGNED_CAP + half = routes.CLI_PROJECT_ASSIGNED_CAP // 2 + assert _assigned_project_counts(kept) == {"project-a": half, "project-b": half} + assert len(kept) == routes.CLI_PROJECT_ASSIGNED_CAP + # Newest first within each project: the draw keeps the head of each queue. + kept_ids = {row["session_id"] for row in kept} + for project in ("project-a", "project-b"): + assert f"{project}-0000" in kept_ids + assert f"{project}-{half - 1:04d}" in kept_ids + assert f"{project}-{half:04d}" not in kept_ids -def test_route_cap_bounds_the_whole_merged_assigned_set(fake_hermes_home): - """A per-project bound alone is not a bound on the payload (finding 1). +def test_route_cap_never_starves_a_project_with_more_projects_than_slots( + fake_hermes_home, +): + """Every project with an assigned session keeps at least one row. - ``CLI_PROJECT_ASSIGNED_CAP`` rows times N projects grows with the project - count: 1,000 assigned conversations spread over 5 projects reproduced the - review's original "1,000 rows in, 1,000 rows out" exactly, because every - project stayed under its own 200. The equal share of - ``CLI_PROJECT_ASSIGNED_SCAN_CEILING`` is what actually bounds the merged set. + 40 projects x 200 rows still fits the merged cap because 200 // 40 = 5 — the + draw shrinks each project's slice instead of dropping whole projects. """ project_count = 40 - per_project = 200 rows = [] for project in range(project_count): rows.extend( @@ -339,31 +386,23 @@ def test_route_cap_bounds_the_whole_merged_assigned_set(fake_hermes_home): "is_cli_session": True, "project_id": f"project-{project:02d}", } - for index in range(per_project) + for index in range(200) ) - assert len(rows) > routes.CLI_PROJECT_ASSIGNED_SCAN_CEILING kept = routes._cap_recent_cli_sessions(rows) - assert len(kept) <= routes.CLI_PROJECT_ASSIGNED_SCAN_CEILING - # Bounded by an EQUAL share, not by dropping whole projects: every project - # keeps a slice, or this would be the starvation greptile P1 rejected. - counts: dict[str, int] = {} - for row in kept: - counts[row["project_id"]] = counts.get(row["project_id"], 0) + 1 - share = routes.CLI_PROJECT_ASSIGNED_SCAN_CEILING // project_count + counts = _assigned_project_counts(kept) + assert len(kept) == routes.CLI_PROJECT_ASSIGNED_CAP assert len(counts) == project_count - assert set(counts.values()) == {share} - # The share only bites once it is tighter than the per-project cap. - assert share < routes.CLI_PROJECT_ASSIGNED_CAP + assert min(counts.values()) >= 1 + assert set(counts.values()) == {routes.CLI_PROJECT_ASSIGNED_CAP // project_count} -def test_route_cap_share_does_not_shrink_below_the_per_project_cap(fake_hermes_home): - """Few projects must keep the full 200 — the ceiling is a ceiling, not a quota. +def test_route_cap_keeps_every_assigned_row_under_the_merged_cap(fake_hermes_home): + """Negative control: the draw is inert while the assigned set fits. - Negative control for the test above: with the ceiling comfortably wider than - ``project_count * CLI_PROJECT_ASSIGNED_CAP`` the share is inert, so the - bounding above is attributable to the ceiling and not to a blanket shrink. + Without this, "bounded at 200" could just as well be a blanket shrink. Two + projects with 60 conversations each keep all 120. """ rows = [] for project in ("project-a", "project-b"): @@ -373,31 +412,27 @@ def test_route_cap_share_does_not_shrink_below_the_per_project_cap(fake_hermes_h "is_cli_session": True, "project_id": project, } - for index in range(250) + for index in range(60) ) kept = routes._cap_recent_cli_sessions(rows) - counts: dict[str, int] = {} - for row in kept: - counts[row["project_id"]] = counts.get(row["project_id"], 0) + 1 - assert counts == { - "project-a": routes.CLI_PROJECT_ASSIGNED_CAP, - "project-b": routes.CLI_PROJECT_ASSIGNED_CAP, - } + assert _assigned_project_counts(kept) == {"project-a": 60, "project-b": 60} + assert len(kept) == 120 < routes.CLI_PROJECT_ASSIGNED_CAP -def test_route_project_ceiling_matches_the_model_constant(): - """The route literal must not drift from the model-side scan ceiling. +def test_route_merged_assigned_cap_is_the_existing_model_row_cap(): + """The route's merged bound must stay the 200-row cap the review named. - Same reasoning as ``test_background_source_exclusion_literal_matches_the_constant``: - the value is duplicated because ``api.models`` is imported below this cap in - ``api/routes.py``, so only a test can keep the two honest. If they diverge the - route could hand back more assigned rows than the model pass ever recovers. + ``api.models`` is imported below this cap in ``api/routes.py``, so the value + is spelled as a literal there and only a test can keep the two honest. The + merged bound must also never exceed what the model recovery pass can deliver, + or the route would advertise a budget nothing can fill. """ + assert routes.CLI_PROJECT_ASSIGNED_CAP == models.PROJECT_ASSIGNED_CLI_LIMIT == 200 assert ( - routes.CLI_PROJECT_ASSIGNED_SCAN_CEILING - == models.PROJECT_ASSIGNED_CLI_SCAN_CEILING + routes.CLI_PROJECT_ASSIGNED_CAP + <= models.PROJECT_ASSIGNED_CLI_SCAN_CEILING )