Skip to content

fix(state): route session-resume reads through the WAL read-only connection - #73539

Closed
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/session-resume-read-path-convoy
Closed

fix(state): route session-resume reads through the WAL read-only connection#73539
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/session-resume-read-path-convoy

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

Summary

  • get_messages_as_conversation, get_resume_conversations, and get_ancestor_display_prefix still acquired self._lock before reading, even though the recent WAL read-path split (per-thread read-only connections via _read_ctx()) was meant to remove exactly this choke point from every recall/browse read. These three are arguably the hottest reads in the file — every session resume across the gateway, CLI, and ACP adapter goes through one of them — so a resume racing a burst of concurrent-session writer flushes still convoys behind them the same way the already-fixed paths used to (measured convoy on the original fix: a 0.23s query stretching to 112s under 6-8 concurrent turns).
  • _session_lineage_root_to_tip — the lineage-walk helper shared by all three functions above, plus get_conversation_root — had its own independent self._lock use. Converting only the three outer functions is not enough: they all call this helper as their first step, so it needed the same conversion or the block would just move one frame down.
  • Both changes mirror the exact with self._lock: self._conn.execute(...)with self._read_ctx() as conn: conn.execute(...) pattern already applied to get_session, get_messages, get_messages_around, get_anchored_view, and search_messages.

Test plan

  • Added test_session_resume_reads_do_not_take_writer_lock to tests/test_session_db_read_path_split.py, mirroring the existing test_reads_do_not_take_writer_lock pattern (holds self._lock in the main thread, asserts the reader thread completes without blocking).
  • This dev venv links SQLite 3.50.4, which is below Hermes's WAL-reset-bug threshold (3.51.3), so the runtime falls back to journal_mode=DELETE and every requires_wal-marked test — including the new one — skips here, same as its siblings. Verified the fix empirically instead: a standalone script that forces journal_mode=WAL + _wal_active=True on a temp SessionDB, holds self._lock in the main thread, and calls all three functions from a second thread.
    • Before the fix: the reader thread blocked for the writer's full lock-hold duration (first on get_messages_as_conversation, then again on get_resume_conversations via the un-converted _session_lineage_root_to_tip).
    • After the fix: all three complete in well under a millisecond while the lock is held.
  • uv run --frozen --extra dev python -m pytest tests/test_hermes_state.py tests/test_session_db_read_path_split.py tests/cli/test_cli_resume_command.py tests/cli/test_resume_display.py tests/tui_gateway/test_protocol.py tests/test_tui_gateway_server.py tests/hermes_cli/test_context_switch_guard.py -q — 1119 passed, 7 skipped (WAL-gated), no regressions.
  • ruff check hermes_state.py tests/test_session_db_read_path_split.py — clean.

…ection

get_messages_as_conversation, get_resume_conversations, and
get_ancestor_display_prefix still took self._lock — the same global
choke point the read-path split (WAL per-thread read-only connections)
was meant to remove from every recall/browse read. These three are the
hottest reads in the file: every session resume across the gateway,
CLI, and ACP adapter goes through one of them, so a resume racing a
burst of concurrent-session writer flushes still convoys behind them
exactly like the fixed paths used to.

_session_lineage_root_to_tip (the lineage walk shared by all three,
plus get_conversation_root) had its own independent self._lock use and
needed the same conversion — without it the outer functions still
blocked on the very first line.

Verified empirically: a reader thread calling all three functions
while another thread holds self._lock blocked for the writer's full
hold duration before the fix, and returned immediately after (SQLite
3.50.4 in this dev venv falls back to journal_mode=DELETE per the
WAL-reset-bug guard, so the requires_wal-marked regression test is
exercised via a local WAL-forced script instead; it still runs and
passes on any runtime where WAL is actually active).
@alt-glitch alt-glitch added type/perf Performance improvement or optimization P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/sessions Session lifecycle, resume, persistence, history labels Jul 28, 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 finding the remaining transcript-read locks. The three converted methods still take self._lock on current main (hermes_state.py:6363, :6544, :6598), and _read_ctx() is the established WAL-safe mechanism (hermes_state.py:2061).

Problems

  • Normal resume still blocks before these reads: resolve_resume_session_id() takes self._lock at hermes_state.py:6287 and calls get_compression_tip(), which locks at hermes_state.py:4950. Current TUI resume calls the resolver at tui_gateway/methods_session.py:364 before get_resume_conversations() at :496; CLI and gateway have the same resolver step.
  • The added test covers the three converted reads but not that resolver/compression-tip preamble, so it cannot establish the stated no-convoy contract for a normal resume.

Suggested changes

  • Convert the read-only queries in resolve_resume_session_id() and get_compression_tip() to _read_ctx() as well, retaining its DELETE-mode fallback.
  • Add a WAL-gated writer-lock regression case for resolving a compression continuation, then cover the full resume sequence.

Automated hermes-sweeper review.


acquired = db._lock.acquire()
try:
done = {}

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 proves the three converted methods no longer take the writer lock, but normal CLI/gateway/TUI resume first calls resolve_resume_session_id(). That helper and its get_compression_tip() preamble still lock on current main, so please add a compression-continuation case that covers the full resume read sequence.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #77803 — thank you @pierrenode. Your commit was cherry-picked, so you remain the author in git history.

Nicely scoped fix: main's _read_ctx() infrastructure existed but the resume paths were still convoying behind writer flushes, and your routing of exactly those four read sites (with the DELETE-journal fallback preserved) was the right-sized change. Your requires_wal skip guard on the new concurrency test was also correct engineering — it skipped cleanly on our SQLite-3.46.0 runner and exercises wherever WAL is active. The only salvage work was re-anchoring one hunk after main centralized the SELECT column list into _CONVERSATION_ROW_COLUMNS. 178 state tests green on the merged tree.

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 P2 Medium — degraded but workaround exists 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