Skip to content

perf(state): replace search_sessions full-table aggregate with correlated subquery - #76161

Closed
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/search-sessions-subquery
Closed

perf(state): replace search_sessions full-table aggregate with correlated subquery#76161
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/search-sessions-subquery

Conversation

@spfcraze

@spfcraze spfcraze commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

search_sessions() computed last_active via a LEFT JOIN over a derived table aggregating MAX(timestamp) GROUP BY session_id across EVERY message in the DB — O(all messages) per call, reached on every 'hermes -c'/'--resume' launch and ACP session-list. Replace with a correlated scalar subquery that resolves MAX(timestamp) per session row via the messages(session_id, ...) index — the exact pattern list_sessions_rich already uses for its last_active.

Related Issue

No direct issue — discovered via code review and reproduced live (see below).
Related PRs reviewed during the duplicate check (none covers this change):

Changes Made

  • fix/search-sessions-subquery — 2 file(s) changed vs base:
    • hermes_state.py
    • tests/test_hermes_state.py

hermes_state.py: one query rewrite in search_sessions() (derived-table LEFT JOIN -> correlated scalar subquery), same COALESCE fallback and ORDER BY. tests/test_hermes_state.py: +4 tests — MRU ordering with COALESCE-to-started_at fallback for message-less sessions, byte-exact parity vs the pre-fix join form across unfiltered/filtered/paginated calls, and a plan pin that drives the real method and asserts the issued query never materializes a GROUP BY over the messages table.

How to Test

Measured on a scratch DB with the real schema and indexes (2k sessions / 120k messages, median of 15 runs, repo venv): OLD 10.03 ms vs NEW 1.85 ms per LIMIT-20 call (5.4x). EXPLAIN QUERY PLAN: OLD materializes a SCAN of all 120k messages plus an AUTOMATIC COVERING INDEX for the join; NEW does per-row indexed lookups (idx_messages_session). Results byte-identical for both the page query and the full unbounded query. Gap grows with DB size — the aggregate cost is O(total messages), the correlated form is O(sessions).

Validation completed (recorded by prp):

  1. Sabotage check: pre-fix code fails the regression tests (1 failed), with the fix all pass (143 passed, 0 failed) — target tests/test_hermes_state.py.
  2. Suite tests/test_hermes_state.py: branch 143 passed / 0 failed vs baseline 140 passed / 0 failed — zero branch-only failures.
  3. Full state suites: tests/test_hermes_state.py 143/143, tests/hermes_state/ 33/33. Targeted fullcheck: 143 passed vs 140 baseline, zero branch-only failures. Sabotage revert-verified: plan-pin test fails on pre-fix main, 143/143 pass with the fix. Full repo-wide suite NOT run locally for this PR (skipped by decision; CI owns full-suite validation).
  4. Duplicate check: 73 potential matches reviewed — none covers this change.
  5. The full repo-wide suite was not run for this change; GitHub CI owns full-suite validation.

Logs

Sabotage verification output:

# base leg (pre-fix code + branch tests):
#   tests: 142 passed, 1 failed
# head leg (with fix):
#   tests: 143 passed, 0 failed

@alt-glitch alt-glitch added type/perf Performance improvement or optimization P3 Low — cosmetic, nice to have area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 1, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real session-list query cost. Current main still builds the global derived aggregate in search_sessions (hermes_state.py:6966-6972), while the proposed correlated lookup follows the established list_sessions_rich pattern (hermes_state.py:5381-5384).

Problems

  • tests/test_hermes_state.py:763-789 captures internal SQL and asserts EXPLAIN QUERY PLAN/literal GROUP BY and MATERIALIZE details. That freezes this exact implementation rather than the returned-row contract; AGENTS.md:80-83 requires behavior contracts over snapshots.

Suggested changes

  • Drop the plan-pin test and keep the behavioral ordering/fallback test (tests/test_hermes_state.py:731-742) plus result-parity coverage (:744-761).

Automated hermes-sweeper review.

Comment thread tests/test_hermes_state.py Outdated
)
assert "GROUP BY" not in issued[0], (
"search_sessions still aggregates the whole messages table")
assert "MATERIALIZE" not in plan, f"full-table aggregate in plan:\n{plan}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this planner/SQL-shape assertion. It couples the test to one SQLite implementation rather than the observable MRU ordering and started_at fallback contract already covered above.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 1, 2026
…ated subquery

search_sessions() computed last_active via a LEFT JOIN over a derived
table aggregating MAX(timestamp) GROUP BY session_id across EVERY
message in the DB — O(all messages) on every call, reached on each
'hermes -c'/'--resume' launch and ACP session-list. The correlated
scalar subquery resolves MAX(timestamp) per session row via the
messages(session_id, ...) index — the same pattern list_sessions_rich
already uses for its last_active.

Measured (real schema, 2k sessions / 120k messages, median of 15):
10.03 ms -> 1.85 ms per LIMIT-20 call (5.4x), with the OLD plan
materializing a SCAN of all messages and the NEW plan doing indexed
per-row lookups. Results byte-identical (page and full-table).

Tests: MRU ordering + COALESCE fallback behavior, byte-exact parity
against the pre-fix join form (unfiltered/filtered/paginated), and a
plan pin asserting the issued query never materializes a GROUP BY over
the messages table.
@spfcraze

spfcraze commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Replaced the plan-pin in 6ea5a58 (amended, force-pushed). You're right it froze the implementation — EXPLAIN text and a literal GROUP BY assertion are exactly the change-detector pattern AGENTS.md:80-83 bans. Dropped it; the behavioral ordering/fallback test and the parity coverage stay as you suggested.

In its place, the perf contract is now asserted behaviorally: test_search_sessions_bounded_work counts SQLite VM steps (progress handler) executed by the real search_sessions() on a seeded DB — no plan text, no implementation strings, deterministic for a fixed dataset. Calibrated on the test's seed (50 sessions / 5000 messages): pre-fix aggregate = 601 handler calls, correlated subquery = 43; the test threshold is 300, 7x headroom above the fix and 2x below the pre-fix form. Same accepted pattern as the json.loads call-count test in tests/agent/test_canon_args_memo_parity.py (#76098).

Re-verified after the amend: 143/143 in the state suite, sabotage revert-verified (the work-bound test fails on pre-fix main, everything passes with the fix), zero branch-only failures vs baseline.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks @spfcraze — verified including an adversarial many-sessions-few-messages probe (correlated form still wins 2.4x; results byte-identical) and salvaged clean into #76878 with your authorship preserved via cherry-pick. Closing in favor of the salvage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants