Skip to content

fix(state): WAL probe fallback, bare-SELECT locking, writable_schema finally, read-pool self-heal - #87612

Closed
wanliqin wants to merge 4 commits into
NousResearch:mainfrom
wanliqin:fix/state-group
Closed

fix(state): WAL probe fallback, bare-SELECT locking, writable_schema finally, read-pool self-heal#87612
wanliqin wants to merge 4 commits into
NousResearch:mainfrom
wanliqin:fix/state-group

Conversation

@wanliqin

Copy link
Copy Markdown

Summary

Four independent SQLite/state-layer fixes, one commit each:

Tests

359 passed / 19 skipped across 15 related test files (hermes_state, session_db read pool/path split, state_db malformed/notadb self-heal, FTS search), plus 150 passed in tests/hermes_state/. No failures.

…e probe

When _on_disk_journal_mode probing fails (most likely a concurrent
opener holding a lock), _apply_delete_for_wal_reset_bug correctly leaves
the journal mode alone but returned "wal". The caller sets
_wal_active = apply_wal_with_fallback(...) == "wal", so _read_ctx
enabled the lock-free mode=ro read pool. If the DB is actually in
DELETE rollback-journal mode, readers skip the self._lock serialized
path and hit raw SQLITE_BUSY during writes — random read failures for
the instance's lifetime.

Return "delete" for the indeterminate case instead. Claiming "delete"
costs only queuing on the write lock (slow but correct); claiming
"wal" costs unserialized read errors.

Closes #86515
get_compression_lock_holder, the clear_session_activity_labels fast
path, get_handoff_state, and list_pending_handoffs executed bare
self._conn.execute(SELECT ...) on the instance-wide shared write
connection. When another thread is inside _execute_write's BEGIN
IMMEDIATE transaction, these SELECTs join that uncommitted transaction
and can read rows that later roll back; in non-WAL (DELETE) mode they
also collide with writers and surface false "no handoff" negatives.

Route all four through _read_ctx, which degrades to the self._lock
serialized path automatically when WAL is inactive — matching every
other read path in the module.

Closes #86516
…ote leaves no residue

_demote_legacy_fts_to_trash._stage ran PRAGMA writable_schema=ON, the
sqlite_master DELETE, then =RESET as a bare sequence inside
_execute_write. writable_schema is a connection-level switch that does
not roll back with the transaction, so if the DELETE raised (the demote
path exists precisely for pathological DBs where sqlite_master may be
inconsistent) the rollback left writable_schema=ON on the long-lived
self._conn until process exit: subsequent schema parsing skips
integrity checks, masking corruption is_malformed_db_error would
otherwise catch.

Wrap the DELETE in try/finally so RESET always runs.

Closes #86517
… them

_read_ctx's finally unconditionally returned the pooled mode=ro
connection to the LifoPool, whether the query succeeded or raised.
After the backing file is replaced or truncated (the scenario
_reconnect_after_notadb's docstring lists: forked curator inheriting
and closing the write fd, external repair pass), pooled read
connections fail persistently — a truncated file raises 'file is not a
database' on every query, and a POSIX rename-replace keeps the
connection reading the old inode, silently returning stale data. LIFO
order guarantees the next checkout gets the same broken connection,
and _read_open_failed_at backoff only covers open failures, not query
failures. The write connection has a one-shot reconnect self-heal; the
read pool had none, so the fault persisted until process restart.

On sqlite3.DatabaseError, destroy the connection via _close_read_conn
(which also releases its descriptor permit) instead of returning it to
the pool, so the next checkout reopens — the read-side counterpart of
the write-side self-heal.

Closes #86518
@alt-glitch alt-glitch added type/bug Something isn't working 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 16, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

PR: fix(state): WAL probe fallback, bare-SELECT locking, writable_schema finally, read-pool self-heal

  1. self._lock is a plain threading.Lock (hermes_state.py:2543) and _read_ctx() degrades to with self._lock: when WAL is inactive. The migrated helpers (get_compression_lock_holder, clear_session_activity_labels, get_handoff_state, list_pending_handoffs) now serialize on the write lock in rollback-journal mode, and a call from a thread that already holds self._lock would self-deadlock. The compression/lease path runs on worker threads per the new tests — please verify none of these call sites can execute inside an existing with self._lock: block, or switch to RLock if nesting is possible.

  2. _read_ctx marks the connection broken only on sqlite3.DatabaseError. A replaced/truncated backing file can surface as other exception types mid-read; those still return the stale connection to the pool, and the LIFO order hands it to the next checkout. Consider closing on any exception raised from within the yielded read (not just DatabaseError).

  3. The indeterminate WAL probe now reports "delete" instead of "wal". Confirm no consumer or test branches on the literal "wal" return to enable the lock-free read pool — otherwise that path silently changes behavior.

  4. get_compression_lock_holder now goes through the read pool, which changes the timing of reads in the admission path (worker thread vs concurrent appends). The new tests exercise this, but worth a sanity check that this path is not hot enough to make pool checkout a bottleneck during compression.

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:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

3 participants