Skip to content

perf(state): cover role-sensitive message aggregates - #73654

Closed
stefanpieter wants to merge 3 commits into
NousResearch:mainfrom
stefanpieter:perf/messages-covering-index
Closed

perf(state): cover role-sensitive message aggregates#73654
stefanpieter wants to merge 3 commits into
NousResearch:mainfrom
stefanpieter:perf/messages-covering-index

Conversation

@stefanpieter

@stefanpieter stefanpieter commented Jul 28, 2026

Copy link
Copy Markdown

Review remediation: implementation withdrawn

The current branch intentionally contains no feature diff against the reviewed base. The review correctly established that the proposed permanent Agent-side covering index was not justified by an Agent-owned consumer.

  • Current head: d369b64c7f39c72fb16a205d2db127bf2956ad63
  • Reviewed base/tree restored: c55159f185e0c4a18f4fdaacb666f77d39d10623
  • Corrective commit: d369b64c7 (fix(state): remove unneeded role aggregate index)

Resolution

  • Removed idx_messages_session_timestamp_role and preserved the established idx_messages_session ON messages(session_id, timestamp) schema.
  • Removed the dedicated test that required the withdrawn index.
  • Confirmed Hermes Agent has no owned LOWER(m.role) / role-sensitive aggregate consumer.
  • Accepted the linked Hermes WebUI diagnosis that the all-session cost must be fixed by bounding/caching the WebUI query rather than imposing storage, build, and write-maintenance cost on every Agent database.
  • The exact final tree matches the reviewed base byte-for-byte; the PR feature delta is empty.

Verification

  • Neighboring schema/state suite: 154 passed, 0 failed.
  • py_compile, Ruff, git diff --check: passed.
  • Added-line security scan: 0 added lines, 0 findings.
  • Independent staged review: PASS.
  • Independent exact-head review of ab25d2be…d369b64c7 and c55159f…d369b64c7: PASS, no security or logic findings.
  • Normal fast-forward push verified local, fork ref, and PR head at the exact SHA above.
  • GitHub reports the PR MERGEABLE; the exact-head upstream workflow is action_required pending repository-admin approval: https://github.com/NousResearch/hermes-agent/actions/runs/30621960491

No merge or close action was taken. Maintainer/reviewer disposition of the now-empty PR and its unresolved historical thread remains external.

@alt-glitch alt-glitch added type/perf Performance improvement or optimization area/sessions Session lifecycle, resume, persistence, history sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades P3 Low — cosmetic, nice to have labels Jul 28, 2026
@stefanpieter

Copy link
Copy Markdown
Author

Current-main conflict remediation is pushed at ab25d2be51c9d77e4f3e387439d3a661bdf546a5.

  • Merged main@c55159f185e0c4a18f4fdaacb666f77d39d10623 normally (no history rewrite / no force-push).
  • Preserved the covering-index contract after upstream split SessionDB schema constants out of hermes_state.py: idx_messages_session_timestamp_role now lives in hermes_state_common.py with exact columns (session_id, timestamp, role).
  • Retained the fresh-DB, existing-DB reopen, query-plan, and aggregate-result regressions. Feature delta against the merged base remains only hermes_state_common.py plus tests/test_messages_covering_index.py.

Verification on the pushed tree:

  • focused schema/state suite: 139 passed;
  • py_compile, Ruff, feature-delta git diff --check, and added-line security review: clean (the only generic scan match is a test executing a module-constant EXPLAIN QUERY PLAN, with no input path);
  • independent immutable-snapshot review: PASS, no security or logic blockers;
  • comprehensive suite: 57 failures across 23 unrelated files; clean exact base reproduced 56 in the same selected run and reproduced the remaining write-lock timing failure in exact isolation, so no PR-caused regression was found.

The base advanced again only in six non-overlapping paths after validation; git merge-tree --write-tree proves a clean merge against that live base. GitHub now reports this head MERGEABLE. The exact-head CI run is awaiting repository-admin approval: https://github.com/NousResearch/hermes-agent/actions/runs/30526911578

Fresh maintainer review is requested on this exact SHA.

@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 the focused schema and regression coverage. The DDL is correctly placed in the shared bootstrap: hermes_state_schema.py:308 executes SCHEMA_SQL, and the PR's hermes_state_common.py:286 addition would therefore reach fresh and reopened databases.

Problems

  • This is a permanent index on every Hermes state.db, but current-tree searches find no Hermes-owned role-sensitive aggregate consumer. The linked WebUI report identifies the expensive work as aggregating all sessions before slicing and recommends bounding the WebUI query plus improving cache invalidation instead: nesquena/hermes-webui#2628 (comment).

Suggested changes

  • Please establish that this index remains needed after the WebUI query/cache remediation, or document a Hermes-owned caller that requires it, before adding its global storage and write-maintenance cost.

Automated hermes-sweeper review.

Comment thread hermes_state_common.py Outdated
CREATE INDEX IF NOT EXISTS idx_sessions_parent ON sessions(parent_session_id);
CREATE INDEX IF NOT EXISTS idx_sessions_started ON sessions(started_at DESC);
CREATE INDEX IF NOT EXISTS idx_messages_session ON messages(session_id, timestamp);
CREATE INDEX IF NOT EXISTS idx_messages_session_timestamp_role

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.

This adds a permanent index to every state database, but the cited consumer is external WebUI code and no equivalent aggregate exists in this repository. Please establish that the WebUI query/cache remediation cannot remove the need for this global write/storage cost.

@teknium1 teknium1 added the sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit label Jul 30, 2026
@stefanpieter

Copy link
Copy Markdown
Author

Resolved the current review blocker on exact head d369b64c7f39c72fb16a205d2db127bf2956ad63.

  • Unjustified global cost: removed the permanent (session_id, timestamp, role) index; the existing (session_id, timestamp) index is unchanged.
  • No Agent-owned consumer: current-tree search confirms no Hermes-owned role-sensitive aggregate; the linked WebUI diagnosis places the query-bound/cache fix in WebUI.
  • Regression scope: removed the test that only enforced the withdrawn index. The final branch tree is byte-identical to reviewed base c55159f185e0c4a18f4fdaacb666f77d39d10623, so the PR feature delta is empty.

Verification:

  • neighboring schema/state tests: 154 passed, 0 failed
  • py_compile, Ruff, git diff --check: passed
  • added-line security scan: 0 added lines, 0 findings
  • independent staged and exact-head reviews: PASS, no blockers
  • normal fast-forward push and local/fork/PR SHA equality verified
  • exact-head upstream CI: action_required pending repository-admin approval (run 30621960491)

@teknium1 please resolve the historical thread or otherwise disposition this now-empty PR when available. No merge or close action was taken by this remediation run.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Closing this as the branch now contains no diff against the base (0 files changed, 0 additions) — the covering index was intentionally reverted after the review concluded the role-sensitive aggregate is a WebUI-owned concern rather than an Agent-owned one. Thanks for following through on the review outcome and cleanly withdrawing the change. If a future Agent-side consumer materializes, a fresh PR would be welcome.

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-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 type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants