From a7f2a593d11edbcce465f6088678b3af310f32f5 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:33:52 +0530 Subject: [PATCH] fix(db): honour require_wal on the probe-unknown path; touch nothing at all Follow-up to the probe-unknown guard: - `require_wal=True` now raises WalUnsupportedError when the on-disk mode cannot be read instead of reporting an unverified "wal" (the function's contract is "the mode actually set"). - Drop `_apply_wal_companions` from the branch: journal_size_limit / synchronous pragmas were being applied to a file whose mode is unknown, contradicting the "touch nothing" rule the branch exists for. - Cut the retracted #104596 mechanism narrative from comments, log text and docstrings; the guard is hardening (same rule the DELETE branch already applied), not a root-cause fix. - Tests: one binding test per file (no pragma of any kind reaches the connection; require_wal raises); drop the log-dedupe test (`_log_once` is already covered). test_captures_cause_on_failed_init's double now fails only set-pragmas, matching the read-only-mount it simulates. --- hermes_state_wal.py | 42 +++++------- tests/test_hermes_state_wal_fallback.py | 5 +- tests/test_journal_mode_config.py | 86 +++++-------------------- tests/test_sqlite_wal_reset_gate.py | 17 +---- 4 files changed, 40 insertions(+), 110 deletions(-) diff --git a/hermes_state_wal.py b/hermes_state_wal.py index 5e3e5ffeae..c01a0bbc41 100644 --- a/hermes_state_wal.py +++ b/hermes_state_wal.py @@ -33,8 +33,7 @@ _WAL_SIZE_LIMIT_BYTES = 64 * 1024 * 1024 # 64 MiB _wal_fallback_warned_paths: set[str] = set() _wal_fallback_warned_lock = threading.Lock() -# Dedup WARNING for the #104596 probe-unknown guard (WAL path touching nothing -# while ownership is not provably exclusive). +# Dedup for the probe-unknown WARNING (on-disk journal mode unreadable, nothing touched). _wal_probe_unknown_paths: set[str] = set() _wal_probe_unknown_lock = threading.Lock() _wal_reset_bug_warned_paths: set[str] = set() @@ -165,7 +164,8 @@ def resolve_journal_mode() -> str: class WalUnsupportedError(sqlite3.OperationalError): """Raised by :func:`apply_wal_with_fallback` under ``require_wal=True`` when the filesystem cannot provide WAL (SQLITE_PROTOCOL raised, or macOS-NFS silent - refusal). Subclasses ``OperationalError`` so DB-init handlers still catch it.""" + refusal) or the on-disk mode cannot be verified (probe blocked by a concurrent + opener). Subclasses ``OperationalError`` so DB-init handlers still catch it.""" def _verify_configured_delete(actual: str) -> str: @@ -178,8 +178,9 @@ def _verify_configured_delete(actual: str) -> str: def apply_wal_with_fallback(conn: sqlite3.Connection, *, db_label: str = "state.db", require_wal: bool = False) -> str: """Set ``journal_mode=WAL`` on ``conn``, falling back to DELETE on failure. - Returns the mode actually set. Shared by :class:`SessionDB` and ``hermes_cli.kanban_db_connect.connect``. - WAL-incompatible filesystems either raise ``OperationalError`` ("locking protocol" / "disk I/O error") or — + Returns the mode actually set — or, when the read-only mode probe is blocked by a concurrent opener, ``"wal"`` + as the assumed mode with nothing touched (``require_wal=True`` raises instead). Shared by :class:`SessionDB` + and ``hermes_cli.kanban_db_connect.connect``. WAL-incompatible filesystems either raise ``OperationalError`` ("locking protocol" / "disk I/O error") or — macOS NFS / SMB / AgentFS — silently refuse and stay in DELETE; either way log ERROR once per process per ``db_label`` and fall back. ``require_wal=True`` raises :class:`WalUnsupportedError` instead. WAL-reset-bug builds (https://sqlite.org/wal.html#walresetbug) never enable WAL on non-WAL files; an already-WAL DB keeps WAL @@ -220,17 +221,13 @@ def apply_wal_with_fallback(conn: sqlite3.Connection, *, db_label: str = "state. raise sqlite3.OperationalError(_CANNOT_VERIFY_DELETE_MSG) return _verify_configured_delete(_set_journal_mode_no_wait(conn, "DELETE")) if current_mode is None: - # #104596: the probe failed (locked/busy under load) and the on-disk mode is unknown. A fresh 0-page - # DB probes "delete" cleanly, so None here means the probe could not read a real file — most often - # because a sibling connection (same process or another) holds it, in WAL, with live -wal/-shm sidecars. - # Emitting the set-pragma inside _enable_wal would re-run WAL-init and UNLINK those sidecars under the - # holder: two -shm generations that cannot see each other's locks -> split-brain corruption - # (btreeInitPage error 11). Ownership is not provably exclusive, so touch nothing: an already-WAL DB - # keeps working (this connection inherits the mode from the on-disk header, exactly like the early - # return above), the rare true-DELETE file degrades to DELETE with the warning below, and a new DB - # reaches _enable_wal on its next open. Mirrors the vulnerable-gate path's indeterminate handling. + # Probe failed (locked/busy): ownership not provably exclusive, same as the DELETE branch above. A fresh + # 0-page DB probes "delete" cleanly, so this is a real file some other connection holds — running WAL-init + # would unlink its -wal/-shm sidecars. Touch nothing: the connection inherits whatever mode the header has. + if require_wal: + raise WalUnsupportedError("could not verify the on-disk journal mode (database is locked — possible " + "concurrent openers); cannot guarantee WAL") _log_once("wal_probe_unknown", db_label) - _apply_wal_companions(conn) return "wal" return _enable_wal(conn, db_label, require_wal, current_mode) @@ -408,16 +405,11 @@ _ONCE_LOGS = { "this DB and run a one-time offline 'PRAGMA journal_mode=DELETE' on the file. This message fires once per " "process per database."), "wal_probe_unknown": (_wal_probe_unknown_lock, "_wal_probe_unknown_paths", logging.WARNING, - # #104596: the read-only probe failed (locked/busy under load). A fresh 0-page DB probes "delete" cleanly, - # so None means the probe could not read a real file — most often because a sibling connection holds it, in - # WAL, with live -wal/-shm sidecars. Emitting the set-pragma here re-runs WAL-init and unlinks those - # sidecars under the holder: two -shm generations that cannot see each other's locks -> split-brain - # corruption. Keep the WARNING (not ERROR): the connection still inherits WAL from the on-disk header, so - # the common case is harmless — only the rare true-DELETE file degrades (silently, to DELETE). - "%s: could not verify the on-disk journal mode (database is locked / busy under load); refusing to issue a " - "journal-mode set-pragma while ownership is not provably exclusive (it could unlink the -wal/-shm sidecars " - "a sibling connection still holds open — split-brain corruption, #104596). Assuming the configured " - "journal_mode=wal and leaving the file untouched. This message fires once per process per database."), + # WARNING, not ERROR: the connection inherits the header's mode, so an already-WAL file (the common case) + # keeps working; only a true-DELETE file stays DELETE for this connection. + "%s: could not verify the on-disk journal mode (database is locked / busy); not issuing a journal-mode " + "set-pragma while another connection may hold the file (it could unlink the -wal/-shm sidecars that " + "connection still uses). Leaving the file untouched; this connection inherits the on-disk mode. This message fires once per process per database."), } diff --git a/tests/test_hermes_state_wal_fallback.py b/tests/test_hermes_state_wal_fallback.py index d43098aef0..96e4d0bd33 100644 --- a/tests/test_hermes_state_wal_fallback.py +++ b/tests/test_hermes_state_wal_fallback.py @@ -428,7 +428,8 @@ class TestGetLastInitError: """When SessionDB() raises, the cause is preserved for slash commands. Simulates a filesystem where BOTH WAL and DELETE journal modes fail — - e.g. a read-only mount where no ``PRAGMA journal_mode=X`` works. The + e.g. a read-only mount where no ``PRAGMA journal_mode=X`` works (the + read-only mode probe still succeeds, as on a real mount). The fallback tries DELETE and also gets rejected; the exception bubbles out of ``SessionDB.__init__`` and the cause is captured. """ @@ -437,7 +438,7 @@ class TestGetLastInitError: class _BothPragmasFailConnection(sqlite3.Connection): def execute(self, sql, *args, **kwargs): # type: ignore[override] - if "journal_mode" in sql.lower(): + if "journal_mode=" in sql.lower().replace(" ", ""): raise sqlite3.OperationalError( "locking protocol: read-only filesystem" ) diff --git a/tests/test_journal_mode_config.py b/tests/test_journal_mode_config.py index a78f9427e1..b0b865c7e3 100644 --- a/tests/test_journal_mode_config.py +++ b/tests/test_journal_mode_config.py @@ -43,19 +43,13 @@ def _reset_configured_delete_override_warned_paths(): def test_wal_probe_unknown_never_emits_set_pragma(monkeypatch, tmp_path, caplog): - """#104596: probe failure (None) must not reach the WAL set-pragma. - - The DELETE branch refuses to flip modes when the on-disk mode cannot be - verified (ownership not provably exclusive). The configured-WAL branch - used to fall through to ``_enable_wal`` and emit ``PRAGMA - journal_mode=WAL`` anyway; when a sibling connection holds the DB in WAL - with live -wal/-shm sidecars, WAL-init unlinks those sidecars under the - holder -> two -shm generations -> split-brain corruption (btreeInitPage - error 11). The guard must touch nothing and assume the configured mode. - """ + """Probe failure (None) on the configured-WAL path must not reach any journal-mode + pragma: the file may be held by a sibling whose -wal/-shm sidecars WAL-init would + unlink. The DELETE branch already refused; this binds the WAL branch to the same rule, + and ``require_wal=True`` must raise instead of reporting an unverified "wal".""" import logging - from hermes_state_wal import apply_wal_with_fallback + from hermes_state_wal import WalUnsupportedError, apply_wal_with_fallback _configure_mode(monkeypatch, tmp_path, "wal") _disable_vulnerable_gate(monkeypatch) @@ -64,80 +58,34 @@ def test_wal_probe_unknown_never_emits_set_pragma(monkeypatch, tmp_path, caplog) class _SpyConnection(sqlite3.Connection): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.emitted: list[str] = [] + self.pragmas: list[str] = [] def execute(self, sql, *args, **kwargs): # type: ignore[override] - if "journal_mode" in sql.lower() and "=" in sql.lower(): - self.emitted.append(str(sql)) + if str(sql).lstrip().lower().startswith("pragma"): + self.pragmas.append(str(sql)) return super().execute(sql, *args, **kwargs) db_path = tmp_path / "probe-unknown.db" sibling = sqlite3.connect(str(db_path)) try: - # A DB that is genuinely WAL on disk; the sibling holds the sidecars. - assert ( - sibling.execute("PRAGMA journal_mode=WAL").fetchone()[0].lower() - == "wal" - ) - # The probe fails under load (locked/busy) -> on-disk mode unknown. - monkeypatch.setattr( - "hermes_state_wal._on_disk_journal_mode", lambda _conn: None - ) + assert sibling.execute("PRAGMA journal_mode=WAL").fetchone()[0].lower() == "wal" + monkeypatch.setattr("hermes_state_wal._on_disk_journal_mode", lambda _conn: None) conn = sqlite3.connect(str(db_path), factory=_SpyConnection) try: with caplog.at_level(logging.WARNING, logger="hermes_state_wal"): - result = apply_wal_with_fallback( - conn, db_label="probe-unknown.db" - ) - assert result == "wal" # assumed configured mode - assert conn.emitted == [] # no set-pragma while ownership unproven - # The sibling's on-disk WAL state is untouched. - assert ( - sibling.execute("PRAGMA journal_mode").fetchone()[0].lower() - == "wal" - ) - assert any( - "could not verify the on-disk journal mode" in r.getMessage() - for r in caplog.records - ) + assert apply_wal_with_fallback(conn, db_label="probe-unknown.db") == "wal" + assert conn.pragmas == [] # nothing touched while ownership is unproven + assert sibling.execute("PRAGMA journal_mode").fetchone()[0].lower() == "wal" + assert any("could not verify the on-disk journal mode" in r.getMessage() for r in caplog.records) + with pytest.raises(WalUnsupportedError, match="could not verify the on-disk journal mode"): + apply_wal_with_fallback(conn, db_label="probe-unknown.db", require_wal=True) + assert conn.pragmas == [] finally: conn.close() finally: sibling.close() -def test_wal_probe_unknown_warns_once_per_database(monkeypatch, tmp_path, caplog): - """Dedupe contract: one WARNING per (process, db_label).""" - import logging - - from hermes_state_wal import apply_wal_with_fallback - - _configure_mode(monkeypatch, tmp_path, "wal") - _disable_vulnerable_gate(monkeypatch) - hermes_state_wal._wal_probe_unknown_paths.clear() - - monkeypatch.setattr( - "hermes_state_wal._on_disk_journal_mode", lambda _conn: None - ) - db_path = tmp_path / "warn.db" - conn = sqlite3.connect(str(db_path)) - try: - with caplog.at_level(logging.WARNING, logger="hermes_state_wal"): - assert apply_wal_with_fallback(conn, db_label="warn.db") == "wal" - assert apply_wal_with_fallback(conn, db_label="warn.db") == "wal" - assert ( - apply_wal_with_fallback(conn, db_label="other.db") == "wal" - ) - hits = [ - r - for r in caplog.records - if "could not verify the on-disk journal mode" in r.getMessage() - ] - assert len(hits) == 2 # warn.db once, other.db once - finally: - conn.close() - - def test_database_journal_mode_has_a_canonical_default(): from hermes_cli.config import DEFAULT_CONFIG diff --git a/tests/test_sqlite_wal_reset_gate.py b/tests/test_sqlite_wal_reset_gate.py index 09354c59cb..e6baa623e8 100644 --- a/tests/test_sqlite_wal_reset_gate.py +++ b/tests/test_sqlite_wal_reset_gate.py @@ -351,20 +351,9 @@ class TestNoDowngradeUnderConcurrentOpeners: check.close() def test_probe_unreadable_touches_nothing(self, tmp_path, monkeypatch, caplog): - """#104596: when the on-disk mode cannot be verified (possible - concurrent openers — same-process siblings holding live -wal/-shm - sidecars), the WAL path must not emit the set-pragma at all. - - Previously it fell through to ``PRAGMA journal_mode=WAL``; on an - incompatible filesystem that raised, but on a WAL DB whose sidecars - a sibling connection still holds, WAL-init silently unlinked them -> - two -shm generations -> split-brain corruption (btreeInitPage error - 11). The guard now refuses to touch anything while ownership is - unproven: it assumes the configured WAL mode, logs one warning per - database, and returns without issuing the set-pragma — strictly more - conservative than the old raise (which still required emitting the - dangerous pragma first). - """ + """Non-vulnerable runtime, probe blocked ("database is locked"): the WAL path + must not emit the set-pragma (here it would raise "locking protocol"); it + warns once and reports the inherited mode instead.""" import logging monkeypatch.setattr(