Skip to content

feat(state): denormalize session.list recency (effective_last_active + two-stage query) - #213

Merged
Kyzcreig merged 2 commits into
mainfrom
wt/sl-phase1
Jul 6, 2026
Merged

Kyzcreig merged 2 commits into
mainfrom
wt/sl-phase1

Conversation

@Kyzcreig

@Kyzcreig Kyzcreig commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Phase 1 of docs/desktop/2026-07-05-session-list-loop-starvation-PRD.md (APPROVED, 15 review passes). Denormalized effective_last_active (root-stored, NULL=hidden) + two-stage indexed query: session.list 2263ms->135ms (17x) on the live 3.2GB DB, SEARCH USING COVERING INDEX, byte-identical top-400 ordering to the CTE oracle. Maintenance chokepoint (one _recompute + _set_parent_session_id wrapper, all parent_session_id writers maintenance-adjacent, BEGIN IMMEDIATE, WAL). v2 backfill marker auto-repairs already-v18 stale DBs on open. CTE retained as byte-equivalence oracle. Apollo verified byte-identical ordering on a fresh copy of the real 3.2GB state.db via the open()-path (caught + fixed 2 real backfill-staleness bugs the synthetic suite missed). Reviewed by Apollo.

Kyzcreig added 2 commits July 6, 2026 07:15
Add sessions.effective_last_active with idempotent backfill/indexes, route session.list recency ordering through a two-stage indexed query, and centralize source deny-list/parent-link maintenance.\n\nVerified:\n- scripts/run_tests.sh tests/hermes_state/test_effective_last_active_denorm.py tests/test_hermes_state.py tests/hermes_state/test_resolve_resume_session_id.py tests/gateway/test_session_list_allowed_sources.py tests/gateway/test_async_session_db.py tests/test_tui_gateway_server.py tests/test_empty_session_hygiene.py tests/cli/test_cli_resume_command.py -q\n- scripts/run_tests.sh tests/gateway tests/hermes_state tests/test_hermes_state.py tests/test_tui_gateway_server.py tests/test_empty_session_hygiene.py tests/cli/test_cli_resume_command.py -q\n- effective_last_active writer grep gate + default/source/id-search covering EXPLAIN checks\n- /Users/alexgierczyk/.hermes/hermes-agent/venv/bin/python -m py_compile hermes_state.py tests/hermes_state/test_effective_last_active_denorm.py tests/test_hermes_state.py tui_gateway/server.py cli.py gateway/slash_commands.py
@greptile-apps

greptile-apps Bot commented Jul 6, 2026 •

Copy link
Copy Markdown

Greptile Summary

Introduces a denormalized effective_last_active column (schema v18) on the sessions table, maintained at every write path, and replaces the per-query recursive CTE with a two-stage covering-index scan that cuts session.list latency from ~2263 ms to ~135 ms on the live 3.2 GB DB. A version-stamped backfill marker ensures stale v1 values on already-v18 databases are repaired on first open.

  • hermes_state.py: New column + two covering indexes; full write-path maintenance (append_message, end_session, reopen_session, upsert_session, delete_*, clear_messages); two-stage query replaces CTE for the default list_sessions_rich path; CTE retained as oracle under _force_cte_oracle=True; _LIST_DENY_SOURCES centralised and now unconditionally applied inside list_sessions_rich.
  • tests/hermes_state/test_effective_last_active_denorm.py: 511-line regression suite covering schema migration, idempotent backfill, stale-marker repair, write-path parity vs the CTE oracle, deletion/orphan promotion, and audit drift detection.
  • cli.py / gateway/slash_commands.py / tui_gateway/server.py: Hardcoded \"tool\" string replaced with _LIST_DENY_SOURCES everywhere deny filtering was referenced.

Confidence Score: 3/5

The unconditional deny-list injection silently changes behaviour for any caller that previously relied on source=tool returning results, producing zero rows with no error.

The two-stage query and backfill mechanics are sound and the 511-line test suite is solid. The concern is _list_deny_sources_clause being appended to where_clauses unconditionally: the renamed test confirms this is intentional, but the broader codebase could have callers that pass source=tool expecting results and would silently get an empty list. A targeted audit of all list_sessions_rich call sites would resolve this before merge.

hermes_state.py at the unconditional deny-clause injection and the double-recompute pattern in end_session/reopen_session. tests/hermes_state/test_effective_last_active_denorm.py at the magic-number writer count.

Important Files Changed

Filename Overview
hermes_state.py Core of the PR: adds effective_last_active column, two covering indexes, full write-path maintenance hooks across all session mutations, a two-stage indexed query replacing the CTE for list_sessions_rich, and backfill/migration logic. The deny list is now unconditionally applied to all list_sessions_rich calls, silently breaking any caller that passes source=tool to inspect tool sessions.
tests/hermes_state/test_effective_last_active_denorm.py New 511-line test suite covering schema migration, two-stage vs CTE oracle parity, monotonic bump, visibility changes, deletion/orphan promotions, and audit logging. The test_parent_session_id_writers_are_maintenance_adjacent guard uses a hardcoded count of 8 writers that will silently break for the next contributor adding a new write site.
tests/test_hermes_state.py Two existing tests updated to reflect the new always-deny behaviour for tool sessions; a backfill_effective_last_active() call added to keep a direct-SQL fixture aligned. One new test verifies id-search through a compression chain tip.
cli.py Replaces hardcoded ["tool"] with list(_LIST_DENY_SOURCES) for exclude_sources; now redundant with the built-in deny clause in list_sessions_rich but harmless.
gateway/slash_commands.py Same mechanical change as cli.py: hardcoded tool replaced with _LIST_DENY_SOURCES. No logic changes.
tui_gateway/server.py Two occurrences of frozenset({"tool"}) updated to frozenset(_LIST_DENY_SOURCES) for the manual deny filter in the session.list RPC handler; no behavioral change.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant SessionDB
    participant SQLite

    Note over SessionDB,SQLite: Open / Migration path
    SessionDB->>SQLite: executescript SCHEMA_SQL adds effective_last_active column and 2 covering indexes
    SessionDB->>SQLite: _backfill_effective_last_active CTE UPDATE visible roots
    SessionDB->>SQLite: "UPDATE hidden sessions SET effective_last_active = NULL"
    SessionDB->>SQLite: INSERT state_meta backfill marker v2

    Note over Caller,SQLite: Write-path maintenance every mutation
    Caller->>SessionDB: append_message session_id ts
    SessionDB->>SQLite: INSERT INTO messages
    SessionDB->>SQLite: _bump_effective_last_active_for_message walk root MAX bump

    Caller->>SessionDB: end_session or reopen_session
    SessionDB->>SQLite: UPDATE sessions SET ended_at or end_reason
    SessionDB->>SQLite: _recompute_effective_last_active root_id
    SessionDB->>SQLite: _recompute_effective_last_active_for_session session_id

    Caller->>SessionDB: delete_session or delete_sessions
    SessionDB->>SQLite: _collect_orphan_effective_last_active_targets before delete
    SessionDB->>SQLite: DELETE sessions and messages
    SessionDB->>SQLite: _recompute_effective_last_active_many orphans and roots

    Note over Caller,SQLite: Two-stage indexed read path
    Caller->>SessionDB: list_sessions_rich order_by_last_active True
    SessionDB->>SQLite: Stage 1 LIMIT on idx_sessions_effective_last_active IS NOT NULL
    SQLite-->>SessionDB: top-N session ids and _effective_last_active
    SessionDB->>SQLite: Stage 2 JOIN sessions and messages for preview and last_active
    SQLite-->>SessionDB: enriched rows
    SessionDB-->>Caller: sorted session list
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant SessionDB
    participant SQLite

    Note over SessionDB,SQLite: Open / Migration path
    SessionDB->>SQLite: executescript SCHEMA_SQL adds effective_last_active column and 2 covering indexes
    SessionDB->>SQLite: _backfill_effective_last_active CTE UPDATE visible roots
    SessionDB->>SQLite: "UPDATE hidden sessions SET effective_last_active = NULL"
    SessionDB->>SQLite: INSERT state_meta backfill marker v2

    Note over Caller,SQLite: Write-path maintenance every mutation
    Caller->>SessionDB: append_message session_id ts
    SessionDB->>SQLite: INSERT INTO messages
    SessionDB->>SQLite: _bump_effective_last_active_for_message walk root MAX bump

    Caller->>SessionDB: end_session or reopen_session
    SessionDB->>SQLite: UPDATE sessions SET ended_at or end_reason
    SessionDB->>SQLite: _recompute_effective_last_active root_id
    SessionDB->>SQLite: _recompute_effective_last_active_for_session session_id

    Caller->>SessionDB: delete_session or delete_sessions
    SessionDB->>SQLite: _collect_orphan_effective_last_active_targets before delete
    SessionDB->>SQLite: DELETE sessions and messages
    SessionDB->>SQLite: _recompute_effective_last_active_many orphans and roots

    Note over Caller,SQLite: Two-stage indexed read path
    Caller->>SessionDB: list_sessions_rich order_by_last_active True
    SessionDB->>SQLite: Stage 1 LIMIT on idx_sessions_effective_last_active IS NOT NULL
    SQLite-->>SessionDB: top-N session ids and _effective_last_active
    SessionDB->>SQLite: Stage 2 JOIN sessions and messages for preview and last_active
    SQLite-->>SessionDB: enriched rows
    SessionDB-->>Caller: sorted session list
Loading

Comments Outside Diff (1)

  1. tests/hermes_state/test_effective_last_active_denorm.py, line 1531 (link)

    P2 Magic count of 8 makes this test a maintenance trap

    assert len(writers) == 8 will fail for the next developer who adds any parent_session_id write site (a new migration helper, a new ON CONFLICT clause, etc.), even if they correctly add the adjacent _recompute call. The failure message gives no hint about what changed or what the expected count should be after the fix. Consider replacing the exact count with assert len(writers) >= 8 or splitting into a separate assertion that verifies the coverage rule per match, not the total.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "fix(state): re-backfill effective_last_a..." | Re-trigger Greptile

Comment thread hermes_state.py
Comment on lines +3233 to +3235
deny_clause, deny_params = _list_deny_sources_clause("s")
where_clauses.append(deny_clause)
params.extend(deny_params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Deny list now silently blocks source="tool" even when explicitly requested

_list_deny_sources_clause is appended to where_clauses unconditionally, before the if source: branch. This means list_sessions_rich(source="tool") now always returns zero rows — the test test_list_sessions_rich_source_and_exclude_combined was updated to reflect this, but the behavior change is invisible to callers that previously relied on an explicit source= argument overriding the deny filter. Any call site in the broader codebase that passes source="tool" to inspect tool sessions will silently get an empty list with no error.

Comment thread hermes_state.py
Comment on lines 2069 to 2078
def _do(conn):
root_id = self._resolve_effective_last_active_root(conn, session_id)
conn.execute(
"UPDATE sessions SET ended_at = ?, end_reason = ? "
"WHERE id = ? AND ended_at IS NULL",
(time.time(), end_reason, session_id),
)
self._recompute_effective_last_active(conn, root_id)
self._recompute_effective_last_active_for_session(conn, session_id)
self._execute_write(_do)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Double-recompute of the root in end_session, reopen_session, and _set_parent_session_id

_recompute_effective_last_active(conn, root_id) is called directly and then called again inside _recompute_effective_last_active_for_session (which resolves the same root from session_id and iterates dict.fromkeys([session_id, root_id])). When session_id != root_id — the common case for any mid-chain or tip session — the root's CTE recomputation fires twice per mutation. This is harmless but burns an extra recursive CTE scan on every end_session / reopen_session / _set_parent_session_id call. The direct _recompute_effective_last_active(conn, root_id) call can be removed since _recompute_effective_last_active_for_session already covers it.

@Kyzcreig
Kyzcreig merged commit 5d67b3f into main Jul 6, 2026
35 checks passed
@Kyzcreig
Kyzcreig deleted the wt/sl-phase1 branch July 6, 2026 14:31
Kyzcreig added a commit that referenced this pull request Jul 7, 2026
…_active + two-stage query) (#213)"

This reverts commit 5d67b3f.
Kyzcreig added a commit that referenced this pull request Jul 7, 2026
* Revert "fix(state): force recency backfill v3 (#216)"

This reverts commit cd05a73.

* Revert "feat(tui_gateway): bound heavy session reads (#215)"

This reverts commit 66d98d3.

* Revert "feat(state): denormalize session.list recency (effective_last_active + two-stage query) (#213)"

This reverts commit 5d67b3f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant