Skip to content

perf(state): add messages(session_id, id) index for window/ordering queries - #76237

Closed
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/messages-session-id-index
Closed

perf(state): add messages(session_id, id) index for window/ordering queries#76237
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/messages-session-id-index

Conversation

@spfcraze

@spfcraze spfcraze commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Every ORDER BY id query on the messages table sorted or scanned the whole session's history: get_messages_around's window seek, latest_message_row_id (LIMIT 1), and get_messages' full-load ordering all paid O(session history) per call — hot mid-turn via session_search and reactions. Add idx_messages_session_id ON messages(session_id, id) to SCHEMA_SQL. messages.id is an original column (INTEGER PRIMARY KEY AUTOINCREMENT), so there is no legacy-column migration hazard (the kanban lesson from #28776 does not apply); CREATE INDEX IF NOT EXISTS keeps existing DBs idempotent.

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/messages-session-id-index — 2 file(s) changed vs base:
    • hermes_state_common.py
    • tests/test_hermes_state.py

hermes_state_common.py: one line in SCHEMA_SQL (idx_messages_session_id), placed next to idx_messages_session. tests/test_hermes_state.py: +2 tests — a VM-step pin (get_messages_around bounded work via SQLite progress handler, calibrated ~12 indexed vs ~855 unindexed handler calls, threshold 300; fails without the index) and window parity with/without the index at probes across the session. No EXPLAIN/plan-text assertions (behavior contracts over snapshots, AGENTS.md).

How to Test

Measured on the real schema (one 20k-message session, median of 30 runs, repo venv): get_messages_around(window=20) 7.08 -> 0.22 ms (32x); latest_message_row_id 3.37 -> 0.011 ms (307x); get_messages full load 111.6 -> 98.6 ms (1.13x — the remaining cost is row deserialization, not the sort). Window results byte-identical at probe points across the session. Insert cost: one extra B-tree entry per message insert, minor next to the 2-3 FTS trigger writes every insert already pays.

Validation completed (recorded by prp):

  1. Sabotage check: pre-fix code fails the regression tests (2 failed), with the fix all pass (142 passed, 0 failed) — target tests/test_hermes_state.py.
  2. Suite tests/test_hermes_state.py: branch 142 passed / 0 failed vs baseline 140 passed / 0 failed — zero branch-only failures.
  3. tests/test_hermes_state.py 142/142 (full state suite). Targeted fullcheck: 142 passed vs 140 baseline, zero branch-only failures. Sabotage revert-verified: both new tests fail pre-fix (VM-step pin over threshold; parity test errors on DROP INDEX of the not-yet-existing index), 142/142 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: 29 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: 140 passed, 2 failed
# head leg (with fix):
#   tests: 142 passed, 0 failed

@alt-glitch alt-glitch added type/perf Performance improvement or optimization P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 1, 2026
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the focused state-store performance improvement. The premise remains present on current main: hermes_state_common.py:285 has only the (session_id, timestamp) message index, while hermes_state.py:6299-6300 and hermes_state.py:6365-6374 filter by session_id and order by id. The placement is appropriate because SessionDB._init_schema() executes SCHEMA_SQL on open (hermes_state_schema.py:428), so CREATE INDEX IF NOT EXISTS covers existing databases.

Problems

  • The checked-in schema excerpts will be stale after this change. website/docs/developer-guide/session-storage.md:112 and website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/developer-guide/session-storage.md:92 list idx_messages_session but omit the proposed (session_id, id) index.

Suggested changes

  • Add the new index declaration to both schema excerpts.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit area/sessions Session lifecycle, resume, persistence, history labels Aug 1, 2026
…ueries

Every ORDER BY id query on the messages table sorted or scanned the
whole session: get_messages_around's window seek, latest_message_row_id
(LIMIT 1), and get_messages' full-load ordering all paid O(session
history) per call — hot mid-turn via session_search and reactions.
messages.id is an original column (INTEGER PRIMARY KEY AUTOINCREMENT),
so the index lives in SCHEMA_SQL next to idx_messages_session — no
legacy-column migration hazard (the kanban lesson from NousResearch#28776 does not
apply).

Measured (real schema, one 20k-message session, median of 30):
get_messages_around 7.08 -> 0.22 ms (32x), latest_message_row_id
3.37 -> 0.011 ms (307x), get_messages full load 111.6 -> 98.6 ms
(1.13x — remaining cost is row deserialization, not the sort).
Window results byte-identical at probe points across the session.

Tests: VM-step pin (get_messages_around bounded work, calibrated
~12 vs ~855 handler calls, threshold 300 — fails without the index)
and window parity with/without the index. No EXPLAIN/plan text
(behavior contracts, AGENTS.md).
@spfcraze

spfcraze commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Both schema excerpts updated in ec0c8d3 (amended, force-pushed) — website/docs/developer-guide/session-storage.md and the zh-Hans translation now list idx_messages_session_id alongside idx_messages_session, matching the SCHEMA_SQL placement. Good catch on the docs drift.

Re-verified after the amend: sabotage (base 2 fail / head 142 pass), zero branch-only failures vs baseline in tests/test_hermes_state.py.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks @spfcraze — verified (EXPLAIN-confirmed index usage, 346→2 VM steps on the probe; mutation-checked) and salvaged clean into #76877 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 comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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