Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions hermes_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -2791,9 +2791,9 @@ def _try_wal_checkpoint(self) -> None:
cannot corrupt B-tree pages under I/O pressure.

PASSIVE does not truncate the WAL file — it stays at its
high-water mark. WAL truncation happens in :meth:`close`
(TRUNCATE) and pre-VACUUM checkpoints, which run infrequently
under controlled conditions.
high-water mark. All checkpoint paths (periodic, :meth:`close`,
pre-VACUUM) use PASSIVE for the same corruption-avoidance
reason (issue #80255).

Previous TRUNCATE strategy caused B-tree corruption on large
databases (65K+ pages) due to the exclusive-lock I/O pressure
Expand All @@ -2816,8 +2816,9 @@ def close(self):
"""Close the database connection.

Drains queued token deltas first (the background writer needs the
connection). Writable connections then attempt a TRUNCATE WAL
checkpoint so exiting writer processes help shrink the WAL file.
connection). Writable connections then attempt a PASSIVE WAL
checkpoint — TRUNCATE's exclusive-lock I/O can corrupt B-tree
pages if shutdown is interrupted mid-checkpoint (issue #80255).
Read-only connections never request a checkpoint.
"""
self._stop_token_writer()
Expand Down Expand Up @@ -2848,10 +2849,10 @@ def close(self):
if self._conn:
if not self.read_only:
try:
self._conn.execute("PRAGMA wal_checkpoint(TRUNCATE)")
self._conn.execute("PRAGMA wal_checkpoint(PASSIVE)")
except Exception as exc:
logger.debug(
"WAL checkpoint (TRUNCATE) at close failed: %s",
"WAL checkpoint (PASSIVE) at close failed: %s",
exc,
)
self._conn.close()
Expand Down Expand Up @@ -9421,9 +9422,9 @@ def vacuum(self) -> int:
with self._lock:
# Best-effort WAL checkpoint first, then VACUUM.
try:
self._conn.execute("PRAGMA wal_checkpoint(TRUNCATE)")
self._conn.execute("PRAGMA wal_checkpoint(PASSIVE)")
except Exception as exc:
logger.debug("WAL checkpoint (TRUNCATE) before VACUUM failed: %s", exc)
logger.debug("WAL checkpoint (PASSIVE) before VACUUM failed: %s", exc)
self._conn.execute("VACUUM")
return optimized

Expand Down
4 changes: 2 additions & 2 deletions tests/test_hermes_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def test_read_only_close_never_requests_wal_checkpoint(self, tmp_path):

assert not any("wal_checkpoint" in sql.lower() for sql in executed)

def test_writable_close_retains_truncate_checkpoint(self, tmp_path):
def test_writable_close_uses_passive_checkpoint(self, tmp_path):
db_path = tmp_path / "state.db"
writable = SessionDB(db_path=db_path)
executed = []
Expand All @@ -124,7 +124,7 @@ def test_writable_close_retains_truncate_checkpoint(self, tmp_path):
writable.close()

assert any(
"pragma wal_checkpoint(truncate)" == " ".join(sql.lower().split())
"pragma wal_checkpoint(passive)" == " ".join(sql.lower().split())
for sql in executed
)

Expand Down
49 changes: 37 additions & 12 deletions tests/test_wal_checkpoint_strategy.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
"""Tests for SessionDB WAL checkpoint strategy (issue #45383).
"""Tests for SessionDB WAL checkpoint strategy (issues #45383, #80255).

Verifies that periodic checkpoints use PASSIVE mode (safe for large DBs)
while close() and pre-VACUUM paths still use TRUNCATE.
Verifies that ALL checkpoint paths — periodic, close(), and pre-VACUUM —
use PASSIVE mode, which is safe for large DBs and cannot corrupt B-tree
pages under I/O pressure or interrupted shutdown.
"""

import sqlite3
Expand Down Expand Up @@ -73,11 +74,11 @@ def test_checkpoint_returns_result_on_success(self, db):
db._try_wal_checkpoint()


class TestCloseUsesTruncate:
"""close() should still use TRUNCATE to shrink WAL on shutdown."""
class TestCloseUsesPassive:
"""close() should use PASSIVE to avoid corruption on interrupted shutdown."""

def test_close_uses_truncate_mode(self, db):
"""TRUNCATE at close is safe — no concurrent writers during shutdown."""
def test_close_uses_passive_mode(self, db):
"""PASSIVE at close avoids TRUNCATE's exclusive-lock corruption risk."""
real_conn = db._conn
execute_calls = []

Expand All @@ -91,22 +92,46 @@ def tracking_execute(sql, *args, **kwargs):

db.close()

passive_calls = [c for c in execute_calls if "wal_checkpoint(PASSIVE)" in c]
truncate_calls = [c for c in execute_calls if "wal_checkpoint(TRUNCATE)" in c]
assert len(truncate_calls) == 1, (
f"Expected 1 TRUNCATE checkpoint at close, got {len(truncate_calls)}"
assert len(passive_calls) == 1, (
f"Expected 1 PASSIVE checkpoint at close, got {len(passive_calls)}"
)
assert len(truncate_calls) == 0, (
"close() must not use TRUNCATE (issue #80255)"
)

def test_close_logs_debug_on_failure(self, db, caplog):
"""Failed TRUNCATE at close logs debug (not warning — close is best-effort)."""
"""Failed PASSIVE at close logs debug (not warning — close is best-effort)."""
mock_conn = MagicMock()
mock_conn.execute.side_effect = sqlite3.OperationalError("database is locked")
db._conn = mock_conn

with caplog.at_level(logging.DEBUG):
db.close()

assert any("WAL checkpoint (TRUNCATE) at close failed" in r.message for r in caplog.records), (
f"Expected debug log about TRUNCATE failure at close, got: {caplog.text}"
assert any("WAL checkpoint (PASSIVE) at close failed" in r.message for r in caplog.records), (
f"Expected debug log about PASSIVE failure at close, got: {caplog.text}"
)


class TestVacuumUsesPassive:
"""vacuum() should use PASSIVE before VACUUM (issue #80255)."""

def test_vacuum_uses_passive_mode(self, db):
"""Pre-VACUUM checkpoint must not use TRUNCATE."""
executed = []
db._conn.set_trace_callback(executed.append)

db.vacuum()

passive_calls = [c for c in executed if "WAL_CHECKPOINT(PASSIVE)" in c.upper()]
truncate_calls = [c for c in executed if "WAL_CHECKPOINT(TRUNCATE)" in c.upper()]
assert len(passive_calls) == 1, (
f"Expected 1 PASSIVE checkpoint before VACUUM, got {len(passive_calls)}"
)
assert len(truncate_calls) == 0, (
"Pre-VACUUM checkpoint must not use TRUNCATE (issue #80255)"
)


Expand Down
Loading