fix(state): refuse a fresh read-pool open on a quarantined SessionDB handle

[nftpoetrist]

Summary

_get_read_conn() (the WAL read-pool's miss path, hermes_state.py) opens a brand-new sqlite3 connection to self.db_path with no awareness of self._db_corrupt at all — it is a completely separate connection-opening path from _reopen_after_close_locked(), which already refuses exactly this class of operation on the write side:

"reopening would hand a fresh connection... to a file we already know is structurally damaged"

Opening a fresh read-only connection to a file already known to be structurally corrupt is the same "reopen a damaged image" the write path refuses — _get_read_conn() just does it through a code path the quarantine work (bcc2e65818, #101095/#101224) never wired up.

Empirically confirmed: forced _wal_active = True on a quarantined handle (this sandbox's SQLite build falls back to journal_mode=DELETE, so WAL is never really active here — forcing it exercises the path directly) and called _get_read_conn() — it opened a new connection (the mocked _connect_tracked_db was called) with zero guard.

Fix

Return None — the existing "read path unavailable, fall back to the locked path" contract _get_read_conn() already uses for every other unavailability reason (WAL inactive, read-only handle, pool closed, backoff window, permit exhausted) — when self._db_corrupt is set. The locked fallback in _read_ctx() then either reuses the already-open self._conn (no new open — a no-op change to the existing quarantine contract) or, if self._conn is already None, hits _reopen_after_close_locked's existing quarantine check (already correctly raises StateDbCorruptError, proven by the existing test_reopen_after_close_refused_when_quarantined).

Scope note (deliberately narrow, matching the docstring's actual promise): the StateDbCorruptError/quarantine contract explicitly documents "no further writes, no automatic reopen, no explicit WAL checkpoint at close" — not "no read." I left two other "reuse an already-open connection" paths untouched:

• _checkout_read_conn()'s pool-hit branch (a connection opened before corruption was ever observed, sitting idle in the pool) — reuse, not reopen.
• _read_ctx()'s locked self._conn yield when self._conn is not None — same: reuse, not reopen.

Both are weaker cases (arguably already covered by "no read" not being part of the stated contract), and fixing only the unambiguous "fresh open == reopen" case keeps this PR narrowly scoped and easier to reason about than touching every reuse path at once.

Tests

Added test_get_read_conn_refuses_fresh_open_when_quarantined to tests/hermes_state/test_state_db_corrupt_quarantine.py, reusing this file's own _quarantined_db() fixture and its established "force _wal_active to exercise the WAL-only path directly" technique. Mocks hermes_state._connect_tracked_db and asserts _get_read_conn() returns None without ever calling it.

Mutation-verified: reverting hermes_state.py makes the test fail — _get_read_conn() actually opens a connection (returns the mock) instead of refusing.

Ran tests/state/ + tests/hermes_state/ + the read-conn-pool/read-path-split/conn-lock-audit suites (653 passed, 29 skipped — pre-existing WAL/py3.12+-gated skips in this sandbox) — no regressions.

Competitor check

Searched gh pr list --state all across several keyword combinations. Two open PRs touch nearby code but address different concerns, noted transparently:

• #100882 ("keep WSL recovery alive through transient read IOERR") adds a _RetryingReadConnection wrapper at the very end of _get_read_conn()'s try block (after the CJK-extension load, right before return) — textually close but semantically distinct (retrying transient WSL I/O errors on an otherwise-healthy connection vs. refusing to open at all on a known-corrupt handle). My change is a small, isolated insertion near the top of the function; if #100882 merges first, this PR should still apply cleanly since neither touches the other's lines.
• #85255 ("evict a poisoned pooled read connection instead of requeuing it") touches _read_ctx() but its base predates the entire quarantine feature (confirmed via git merge-base --is-ancestor — its diff shows no _db_corrupt/reopen-refusal logic existing at all yet) and targets a different corruption class (NOTADB/replaced-file, evicting a connection that starts failing mid-use — not the "never open a fresh connection to an already-quarantined handle" gap this PR fixes).

No open or closed PR covers this specific gap.

---

Update (second commit): Since this PR's base commit, hermes_state.py gained two more halted states that _reopen_after_close_locked() already refuses to reopen for on the write side: self._db_replaced (the path now resolves to a different file generation, #89332) and self._db_wal_generation_lost (this handle's WAL/SHM generation was deleted out from under it, added by the merged salvage of #101081/#101221 — after this PR's original base). _get_read_conn()'s fresh-open guard only knew about self._db_corrupt and was unaware of both — same hazard, same fix shape: return None (fall back to the locked path) when either flag is set, checking only the already-set flags (not the proactive _db_file_was_replaced()/_wal_generation_was_lost() detection probes) to keep this hot-path check as cheap as the existing _db_corrupt check.

Added test_get_read_conn_refuses_fresh_open_when_replaced and test_get_read_conn_refuses_fresh_open_when_wal_generation_lost, mirroring the existing test_get_read_conn_refuses_fresh_open_when_quarantined. Mutation-verified (patch-based, not git stash, per this session's worktree-sharing caution): reverting the new hermes_state.py hunk alone makes both new tests fail (mock connection returned instead of None); restoring it makes them pass. Ran tests/hermes_state/ (11 passed, 1 pre-existing skip) and tests/state/ + tests/hermes_state/ together (417 passed, 7 skipped, all pre-existing) — no regressions.

Fresh competitor check: no open or closed PR covers this specific gap. #101042 ("set busy_timeout=5000 on writer and reader connections") also touches _get_read_conn(), but only inside the try block after the connection is already opened (adds a busy_timeout PRAGMA) — no line overlap with this commit's checks near the top of the function.

[Enough1122]

AI code review — automated review for reference; please use your judgment.

PR 101303 — fix(state): refuse a fresh read-pool open on a quarantined DB. Extends the existing quarantine refusal in _reopen_after_close_locked to the WAL read-pool miss path (hermes_state.py:5733): _get_read_conn() now returns None when _db_corrupt, _db_replaced, or _db_wal_generation_lost is set, instead of opening a fresh connection to a known-damaged/new-generation file. Sensible fail-closed direction (falls back to the locked path), with per-flag tests. The comment honestly documents that only already-set flags are checked to keep the hot path cheap.

Non-blocking observations:

• As the comment notes, an out-of-band replace/WAL-loss not yet observed is still caught later; the window between is unchanged from current behavior, so this strictly narrows the hazard. No issue.
• hermes_state.py:5736 — three return None branches share the "fall back to the locked path" outcome; a single combined predicate might read cleaner, but the per-flag comments carry useful context. Style only.