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.
This commit is contained in:
@@ -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/<id>.jsonl` and the gateway
|
||||
|
||||
+40
-3
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user