From e9160625dcac7aac94e893f2a22113e05305a8bb Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:41:52 +0530 Subject: [PATCH] fix(state): also disable SQLite's internal close-time checkpoint on quarantine (py3.12+) Skipping the explicit PRAGMA wal_checkpoint(PASSIVE) in close() left sqlite3.Connection.close() running SQLite's own last-connection PASSIVE checkpoint, which still checkpoints the WAL and unlinks -wal/-shm on a structurally corrupt file (E2E: the -wal vanished on close despite the quarantine). Python 3.12+ exposes SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE via Connection.setconfig(); arm it in _halt_db_corrupt so the WAL image survives close() for forensics/recovery. On 3.11 the switch does not exist; the docstring and docs now say so instead of claiming sqlite3 cannot reach it at all. Follow-up to #101095; flagged by JoaoMarcos44 on #101093. --- docs/state-db-recovery.md | 9 ++-- hermes_state.py | 43 +++++++++++++++++-- .../test_state_db_corrupt_quarantine.py | 24 +++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/docs/state-db-recovery.md b/docs/state-db-recovery.md index c776aaea9f..5cb69fb928 100644 --- a/docs/state-db-recovery.md +++ b/docs/state-db-recovery.md @@ -40,9 +40,12 @@ writing for ~50 minutes after the first structural error checkpointed 15 pages under the wrong page numbers on shutdown (page 1 received a `messages_fts_trigram_data` leaf) and turned a damaged-but-readable file into one that no longer opened at all. Skipping the explicit checkpoint is the -second line of defence; SQLite may still run its own last-connection -checkpoint when the connection closes, so copy `state.db`, `state.db-wal` and -`state.db-shm` together before restarting anything. +second line of defence; on Python 3.12+ the quarantine also disables +SQLite's own last-connection checkpoint (`SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE`), +so the `-wal` sidecar survives `close()` for forensics. On Python 3.11 that +switch is unavailable and SQLite may still checkpoint once on close, so copy +`state.db`, `state.db-wal` and `state.db-shm` together before restarting +anything. The gateway and the agent flush path treat the quarantine like a replaced file: pending transcripts go to `sessions/.jsonl` and the gateway diff --git a/hermes_state.py b/hermes_state.py index 3d2066c9b1..f6a928f431 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -4380,9 +4380,14 @@ class StateDbCorruptError(sqlite3.DatabaseError): minutes after the first structural error checkpointed 15 pages under the wrong page numbers on shutdown, turning a still-readable file into ``file is not a database``. Stopping the writes is what prevents that; - skipping the explicit checkpoint is the second line of defence (SQLite - may still run its own last-connection checkpoint on close — Python's - ``sqlite3`` does not expose ``SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE``). The + skipping the explicit checkpoint is the second line of defence. SQLite + still runs its own last-connection checkpoint inside ``close()`` (and + deletes the ``-wal`` sidecar) unless ``SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE`` + is set — Python exposes it via ``Connection.setconfig()`` on 3.12+, so + quarantine disables the close-time checkpoint there and the WAL survives + on disk for forensics; on 3.11 the internal checkpoint is unavoidable + (post-quarantine it can only carry pre-corruption committed frames, since + no further writes are accepted). The recovery boundary is a process restart on a repaired or restored file. """ @@ -6408,6 +6413,7 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) """Quarantine this handle and raise; never run in-file repair here.""" self._db_corrupt = True self._db_corrupt_reason = str(exc) + self._disable_close_time_checkpoint() logger.error( "state.db %s reported structural corruption outside the FTS " "indexes (%s); quarantining this handle: no further writes, no " @@ -6425,6 +6431,37 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) setattr(err, attr, value) raise err from exc + def _disable_close_time_checkpoint(self) -> None: + """Best-effort: stop SQLite's own last-connection checkpoint on close. + + Skipping our explicit ``PRAGMA wal_checkpoint(PASSIVE)`` in + ``close()`` is not enough on its own: ``sqlite3.Connection.close()`` + still runs SQLite's internal last-connection PASSIVE checkpoint and + unlinks the ``-wal``/``-shm`` sidecars. On the field incident's file + that close-time checkpoint is exactly what wrote 15 pages under the + wrong page numbers. Python 3.12+ exposes the switch as + ``Connection.setconfig(SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE)``; on 3.11 + neither the constant nor ``setconfig`` exists, so the internal + checkpoint remains (it can only carry pre-quarantine committed + frames — no further writes are accepted on this handle). + """ + flag = getattr(sqlite3, "SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE", None) + if flag is None: + return + conn = self._conn + setconfig = getattr(conn, "setconfig", None) + if conn is None or setconfig is None: + return + try: + setconfig(flag, True) + except Exception: + logger.debug( + "Could not disable SQLite's close-time checkpoint on the " + "quarantined handle for %s", + self.db_path, + exc_info=True, + ) + def _raise_if_db_corrupt(self) -> None: if self._db_corrupt: raise self._corrupt_error() diff --git a/tests/hermes_state/test_state_db_corrupt_quarantine.py b/tests/hermes_state/test_state_db_corrupt_quarantine.py index 258672bb77..2acac22939 100644 --- a/tests/hermes_state/test_state_db_corrupt_quarantine.py +++ b/tests/hermes_state/test_state_db_corrupt_quarantine.py @@ -102,6 +102,30 @@ class TestQuarantinedHandleStopsTouchingTheFile: for rec in caplog.records ) + def test_close_disables_sqlite_internal_checkpoint_on_py312(self, tmp_path): + """Quarantine must also stop SQLite's own last-connection checkpoint. + + Skipping the explicit PRAGMA is not enough: sqlite3.Connection.close() + runs an internal PASSIVE checkpoint and unlinks -wal/-shm unless + SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE is set (Connection.setconfig, + Python 3.12+). On 3.11 the switch is unavailable — skip there. + """ + flag = getattr(sqlite3, "SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE", None) + db = SessionDB(db_path=tmp_path / "state.db") + if flag is None or not hasattr(db._conn, "setconfig"): + db.close() + pytest.skip("SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE needs Python 3.12+") + real_conn = db._conn + db.create_session(session_id="s1", source="cli", model="test") + assert real_conn.getconfig(flag) is False + db._conn = _MalformedConn(real_conn) + with pytest.raises(StateDbCorruptError): + db.create_session(session_id="s2", source="cli", model="test") + db._conn = real_conn + # _halt_db_corrupt armed the no-checkpoint-on-close switch. + assert real_conn.getconfig(flag) is True + db.close() + def test_reopen_after_close_refused_when_quarantined(self, tmp_path, monkeypatch): from unittest.mock import MagicMock