Skip to content

feat(state): indexed sessions.last_active for recents ordering + legacy trigram opt-out - #70429

Closed
ildunari wants to merge 1 commit into
NousResearch:mainfrom
ildunari:upstream/session-recents-index
Closed

feat(state): indexed sessions.last_active for recents ordering + legacy trigram opt-out#70429
ildunari wants to merge 1 commit into
NousResearch:mainfrom
ildunari:upstream/session-recents-index

Conversation

@ildunari

Copy link
Copy Markdown
Contributor

Part 1 — sessions.last_active (schema v24). list_sessions_rich(order_by_last_active=True) currently computes "most recent activity, walking compression-continuation chains forward" via a recursive CTE over every WHERE-admitted row on every call. This adds a denormalized sessions.last_active REAL column plus idx_sessions_last_active (last_active DESC, started_at DESC, id DESC), so the common unfiltered-recents case can ORDER BY s.last_active directly against the index instead. The column is kept live by a SQL trigger pair (message timestamp updates and session started_at changes propagate up through compression chains) plus two write-path helpers: _touch_session_last_active (monotonic advance — append_message, bulk insert) and _refresh_session_last_active (authoritative recompute — session create, end/reopen across a compression boundary, message replace/clear, single/bulk/pruned/empty-session delete, import), used wherever a naive "touch" could leave a stale ancestor last_active pointing at removed or not-yet-linked activity. A new current_version < 24 migration backfills every existing session on next open; read-only cross-profile DB handles (which never run migrations) detect the missing physical column via _has_sessions_last_active and fall back to the original computed-subquery SELECT, so aggregating another profile's not-yet-migrated DB never raises "no such column".

Also fixed three destructive/import paths that mutated messages without refreshing affected compression ancestors: clear_messages and delete_empty_sessions (capturing ancestor ids before deletion, matching the pattern already used in delete_session/delete_sessions/prune_sessions), and import_sessions — which wires parent_session_id only after all messages are already inserted, so the per-message touch calls ran before there was any chain to climb. The chain is now explicitly settled via _refresh_session_last_active once every parent edge is final, which also gives an empty imported session a real last_active instead of leaving it NULL.

Known limitation, left to maintainer discretion. Every write path this PR controls (the triggers plus the touch/refresh helpers above) keeps last_active correct. But a writer that mutates messages without going through those helpers — an older hermes-agent version sharing this state.db mid-rolling-upgrade, a different language binding, an ad hoc SQL script — will leave that session's last_active stale (behind its true activity) until the next helper-mediated write on that session. This is strictly a recents-ordering staleness, never data loss: the message itself is intact and searchable, only its position in "most recently active" sorting can lag. It self-heals the moment normal traffic resumes on the session (proven by test_stale_write_from_bypassing_writer_self_heals_on_next_write).

We looked at closing this gap and rejected two approaches, in case a maintainer wants to revisit:

  • Mirror AFTER INSERT/AFTER DELETE triggers on messages, matching the existing AFTER UPDATE OF timestamp trigger. Rejected: SQLite fires AFTER INSERT before the Python-level _touch_session_last_active call that follows the same INSERT statement in append_message/_insert_message_rows, so a naive mirror trigger would run its recursive-CTE chain-walk body on every single message write — including the ones the Python helpers already handle correctly with an O(1) monotonic touch — defeating the point of adding an index in the first place.
  • An unconditional on-open reconciliation pass (an earlier revision of this PR shipped exactly this: _find_stale_last_active_session_ids, wired into _init_schema). A focused second review found it insufficient on its own terms: it missed delete-drift and fresh-descendant/stale-ancestor shapes (only compared each session's own message max against its own last_active, not the fuller set of ways a chain can drift); read-only cross-profile handles never call _init_schema at all, so they'd trust stale data indefinitely regardless; the per-open scan is unbounded (~40ms measured at 25k messages, and the web-server paths open a writable SessionDB per request, making this a per-request cost); and — critically — it could move a compression ancestor's last_active backward relative to what a live process had already correctly advanced it to, which is exactly the invariant the monotonic-touch design is built to prevent. Patching all of that felt like the wrong shape for this PR, so it's removed rather than iterated further here.

If maintainers want a stronger, operator-triggered guarantee here, the natural home looks like wiring a reconciliation pass into an existing maintenance entrypoint — hermes sessions optimize-storage already exists as a deliberate, foreground, disk-checked operation for exactly this class of "occasionally necessary, not safe to run silently on every request" repair. We're leaving that choice (and the shape of the fix) to the maintainers rather than including it in this PR.

Part 2 — sessions.disable_fts_trigram. A legacy (pre-v23, inline-FTS) install can set this config key (or HERMES_DISABLE_FTS_TRIGRAM env var, as a process-transport fallback) to skip recreating its old heavyweight trigram index — which, unlike the v23 external-content trigram table, duplicates full message content including large tool-call payloads. The flag is a no-op on v23-shape databases (they always get the compact trigram index); the explicit hermes sessions optimize-storage migration still backfills the compact trigram table for a disabled legacy DB once it's opted in, refreshing _trigram_available after the demote step. Config surface/user-facing docs for this flag are intentionally deferred, not part of this PR.

Testing done: tests/test_hermes_state.py (424 passed, including regression tests for: legacy-version migration backfill parametrized over [17,19,23], two read-only-fallback tests, two compression-chain last_active-refresh tests covering end/reopen and delete/bulk-delete/prune, clear_messages ancestor refresh, import chain settle, and the bypassing-writer self-heal contract) and tests/hermes_state/test_fts_trigram_disable.py (7 passed) both pass in the PR worktree venv. Also ran the broader session-listing/gateway/web-server suites with no regressions.

🤖 Generated with Claude Code

Two related storage improvements to hermes_state.py:

1. sessions.last_active (schema v24): a denormalized, indexed column
   (idx_sessions_last_active) that list_sessions_rich's recents view can
   ORDER BY directly instead of computing "most recent activity per
   compression chain" via a recursive CTE over every candidate row on
   each call. The column is kept current by:
   - a SQL trigger pair (LAST_ACTIVE_REPAIR_TRIGGER_SQL) that propagates
     message timestamp updates and session started_at changes up through
     compression-continuation chains,
   - _touch_session_last_active (append_message / bulk insert) and
     _refresh_session_last_active (session create/end/reopen, message
     replace/clear, delete/prune/delete_empty, import) for the write
     paths that need an authoritative recompute rather than a monotonic
     touch.
   A v24 migration backfills the column for every existing session on
   next open. Read-only cross-profile DB handles (which never migrate)
   detect a missing physical column via _has_sessions_last_active and
   fall back to the original computed-subquery SELECT.

   Fixed three destructive/import paths that mutated messages without
   refreshing affected compression ancestors: clear_messages and
   delete_empty_sessions (capturing ancestor ids before deletion,
   matching the pattern already used in delete_session/delete_sessions/
   prune_sessions), and import_sessions (which wires parent_session_id
   only after all messages are already inserted, so the per-message
   touch calls ran before there was any chain to climb — the chain is
   now explicitly settled via _refresh_session_last_active once every
   parent edge is final, which also gives an empty imported session a
   real last_active instead of leaving it NULL).

   Known, documented limitation (deliberately NOT solved in this PR —
   see PR description for the full writeup and rejected alternatives):
   a writer that mutates messages without going through
   _touch_session_last_active/_refresh_session_last_active — an older
   process version sharing this state.db mid-rolling-upgrade, a
   different language binding, a raw SQL script — leaves last_active
   stale (behind true activity) until the next helper-mediated write on
   that session. This is a recents-ordering staleness only, never data
   loss, and self-heals the moment normal traffic resumes. An earlier
   version of this PR added an unconditional on-open reconciliation
   pass; a second review round found it insufficient (misses delete-
   drift and fresh-descendant/stale-ancestor shapes), unsafe for
   read-only cross-profile handles (which never run it), unbounded on
   large tables, and capable of moving a compression ancestor's
   last_active BACKWARD — so it was removed rather than patched further.
   See the PR description for the full tradeoff and a suggested home
   (`hermes sessions optimize-storage`) if maintainers want an
   operator-triggered repair pass.

2. sessions.disable_fts_trigram (config-backed, env override
   HERMES_DISABLE_FTS_TRIGRAM): lets a legacy (pre-v23 inline-FTS)
   install skip recreating its heavyweight trigram index, which — unlike
   the v23 external-content trigram table — duplicates full message
   content including large tool-call payloads. The flag only affects
   legacy installs still awaiting `hermes sessions optimize-storage`;
   v23-shape databases always keep the compact trigram index regardless
   of the setting, and the explicit optimizer still backfills it for a
   disabled legacy DB once migrated. Config surface/docs for this flag
   are intentionally deferred, not part of this PR.

Testing done (in this worktree, /Users/Kosta/.hermes/hermes-agent/.venv):
- tests/test_hermes_state.py: 424 passed (includes 3 regression tests:
  clear_messages ancestor refresh, import chain settle, and a
  self-heals-on-next-write test proving the documented mixed-writer
  contract — stale after a bypassing raw-SQL insert, corrected by the
  next normal append_message call)
- tests/hermes_state/, tests/state/: all passed
- tests/gateway/test_session_list_allowed_sources.py,
  tests/gateway/test_async_session_db.py,
  tests/test_empty_session_hygiene.py,
  tests/hermes_cli/test_resolve_last_session.py,
  tests/hermes_cli/test_session_export_md.py,
  tests/hermes_cli/test_session_browse.py,
  tests/cli/test_cli_resume_command.py,
  tests/hermes_state/test_session_archiving.py,
  tests/hermes_state/test_resolve_resume_session_id.py: all passed
@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 area/config Config system, migrations, profiles sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state needs-decision Awaiting maintainer decision before any implementation labels Jul 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #68394 proposes a different trigram-FTS configuration surface. This PR also adds the distinct sessions.last_active migration/index work, so the overlap needs a maintainer configuration-contract decision rather than a duplicate closure.

@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 detailed write-path analysis. The indexed-recency premise remains live: current list_sessions_rich() still computes compression-chain activity with a recursive CTE at hermes_state.py:5122-5226.

Problems

  • The PR predates 21c7ae8563, which split SessionDB into hermes_state_schema.py, hermes_state_search.py, and hermes_state_portability.py; current composition is at hermes_state.py:70-72. The monolithic edits need a deliberate port, including current append/delete/import paths.
  • Proposed hermes_state.py:318 adds a user-facing HERMES_DISABLE_FTS_TRIGRAM override and direct YAML parsing. Current session-search settings use hermes_cli/config_defaults.py:2579-2609 plus config-authoritative gateway bridging at gateway/run.py:1729-1736.

Suggested changes

  • Port the last_active migration/index and write-path maintenance to the current mixin boundaries, retaining read-only cross-profile behavior at hermes_state.py:1840-1858.
  • Re-scope the trigram option to the established config contract and reconcile it with the CJK-bigram initialization at hermes_state_schema.py:726-728; #68394 is related configuration work.

Automated hermes-sweeper review.

Comment thread hermes_state.py
trigram from being recreated while a legacy database awaits explicit
``optimize-storage`` migration.
"""
env_value = os.getenv("HERMES_DISABLE_FTS_TRIGRAM")

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 makes a new non-secret HERMES_* variable a user-facing override. Please keep the setting config.yaml-authoritative through DEFAULT_CONFIG and the existing runtime bridge pattern, rather than accepting .env/process environment as a public configuration surface.

@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 Jul 30, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks @ildunari — this was a well-built implementation (triggers + backfill + read-only fallback), but main has since solved the recents-ordering problem with a different architecture: last_active is now computed at query time via a correlated subquery over messages (COALESCE(last_activity_at, MAX(messages.timestamp), started_at) — hermes_state_common._sql_session_last_active), made index-only by the messages(session_id, id)/(session_id, timestamp) indexes (#76877/#76878). A denormalized column now has a semantic gap main's form doesn't: it wouldn't see the mid-turn last_activity_at heartbeat (#72424), so a rebased version would sort stale during active turns. Schema version 24 is also already taken, so the migration gate collides. The trigram opt-out fragment only helps pre-v23 installs and was a no-op on current shapes per your own PR notes. Closing as solved-differently-on-main — the perf goal you identified was real and is now covered.

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

Labels

area/config Config system, migrations, profiles area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation 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