perf(state): read-path split — per-thread read-only connections for recall reads

[kshitijk4poor]

Summary

Per-thread read-only SQLite connections for WAL-mode recall reads, bypassing self._lock so recall/browse queries never convoy behind writer flushes.

Changes (salvage of #65541 by @Soju06 + follow-up fixes)

• @Soju06's original work: _get_read_conn() / _read_ctx() context manager, per-thread mode=ro connections under WAL, converts 8 recall methods (get_session, get_session_by_title, resolve_session_by_title, list_sessions_rich, get_messages, get_messages_around, get_anchored_view, search_messages) from self._lock to _read_ctx, graceful fallback to locked path under non-WAL or read-conn failure, close() cleanup.
• Follow-up fixes:
  • Route _get_read_conn through _connect_tracked_db so read-only connections are registered with the POSIX lock-safety guard (connect_tracked), matching the writer and existing read-only paths.
  • Convert _search_unindexed_gap, _run_trigram_search, CJK-bigram FTS search, and get_meta to _read_ctx — pure SELECT queries still taking self._lock.
  • Add @pytest.mark.requires_wal to 5 tests that assume WAL is active (Hermes disables WAL on SQLite < 3.51.3).

Validation

	Before	After
PR tests	5 fail (WAL assumed)	9 pass (5 skipped on vulnerable SQLite)
Existing SessionDB tests	464 pass	464 pass
E2E (real imports, WAL)	—	10/10 pass, lock-bypass verified
connect_tracked registration	bypassed	2 conns tracked (writer + reader)

Closes #65541

[yoniebans]

found two issues that need to be addressed before merge. both are reproducible on WAL with SQLite 3.53.1.

1. search_messages() can still block on self._lock. It calls fts_rebuild_status(), whose get_meta() implementation still uses that lock. The PR body says this was converted to _read_ctx, but the current PR head still has the locked implementation.

2. SessionDB.close() only closes the current thread's read connection. Short-lived reader threads leave tracked connections in _live_connections after thread exit, permanently making the process appear to have a live database connection. I reproduced this with three reader threads.

[kshitijk4poor]

Both issues fixed in the latest push.

1. fts_rebuild_status() / fts_cjk_rebuild_status() no longer call get_meta(). They now read state_meta directly via _read_ctx() with a single SELECT key, value FROM state_meta WHERE key IN (?, ?) query — no self._lock. get_meta() itself stays on self._lock because callers like fts_rebuild_step read progress before entering a write transaction (read-your-writes safety).

2. Short-lived reader threads no longer leak tracked connections. Read connections are now registered in a WeakSet[sqlite3.Connection] (self._read_conns). close() iterates the WeakSet and calls close() on each connection, which triggers _TrackingMixin.close() → untrack_connection() — so _live_connections is decremented to 0 deterministically. Verified: 3 reader threads + main thread = 4 read conns + 1 writer = 5 tracked; after close() = 0 tracked.

[yoniebans]

Reverified PR head 6d72dcb23f merged with current origin/main.

1. Fixed. fts_rebuild_status() and fts_cjk_rebuild_status() now read state_meta through _read_ctx(), so search_messages() no longer blocks while self._lock is held. Targeted suite: 9 passed under WAL.

2. Not fixed. The WeakSet loses connections once their reader threads exit. Reproduced on the merged head: after three short-lived reader threads, len(self._read_conns) == 0 while the registry holds four entries; after close(), three entries remain and has_live_connection() stays true. The “5 tracked → 0” result only holds while the reader threads are still alive at close(). Use strong ownership, such as a lock-protected set that close() drains. A finalizer could provide a fallback decrement, but should not replace explicit ownership and cleanup.

3. The branch no longer merges cleanly with current origin/main. hermes_state.py conflicts in imports, close(), and get_session(). The get_session() conflict is semantic: current main flushes token counts before reading, so that call must remain ahead of the _read_ctx() query or usage surfaces may report stale totals.

[kshitijk4poor]

All three issues addressed in the latest push (rebased onto current origin/main).

1. Already fixed — fts_rebuild_status() / fts_cjk_rebuild_status() read state_meta directly via _read_ctx() with SELECT key, value FROM state_meta WHERE key IN (?, ?). No self._lock.

2. Fixed — switched from WeakSet to a strong set. The WeakSet lost references when reader threads exited, so close() couldn't find them. Read connections are now held in a lock-protected set[sqlite3.Connection] (self._read_conns + self._read_conns_lock). close() drains the set under the lock, then closes each connection (which triggers _TrackingMixin.close() → untrack_connection()). Verified the reviewer's exact repro: 3 short-lived reader threads → threads exit + GC → strong set still holds 3 conns → close() drains all → _live_connections = 0, has_live_connection() = False.

3. Rebased cleanly onto current origin/main. All three conflict areas resolved:

• import: kept both from collections import deque (main) and from contextlib import contextmanager (PR).
• close(): kept main's _stop_token_writer() + atexit.unregister() before the read-conn cleanup — token flush happens first, then read conns are drained, then the writer checkpoint runs.
• get_session(): kept main's self.flush_token_counts() ahead of the _read_ctx() query so usage surfaces see exact totals.

472 passed, 6 skipped (5 WAL-gated, 1 pre-existing).

[yoniebans]

One last lifecycle issue, sorry, I missed this on the first pass. close() can drain _read_conns while a reader is still opening its connection. If that reader registers after the drain, its connection remains tracked after close() returns. I reproduced this with a delayed connection open: has_live_connection() stayed true. Please coordinate registration with shutdown, or close and untrack connections that finish opening after shutdown begins.

[kshitijk4poor]

Fixed. Added a _read_conns_closed flag that close() sets under _read_conns_lock before draining. _get_read_conn() checks the flag under the same lock after opening — if close() has already drained, the late connection is closed immediately and not registered. Verified the exact race: reader opens connection after close() drains → connection closed on the spot → has_live_connection() = False, 0 tracked.