diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index e5eaf8496ac4..3a5b7cd4d3cf 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -283,7 +283,9 @@ def _safe_copy_db(src: Path, dst: Path) -> bool: pass -def is_zeroed_sqlite_file(path: Path, *, probe_bytes: int = 100) -> bool: +def is_zeroed_sqlite_file( + path: Path, *, probe_bytes: int = 100, force: bool = False +) -> bool: """True when *path* looks like the #68474 zeroed-state.db signature. Signature: size > 0, first *probe_bytes* are all NUL (no ``SQLite format 3`` @@ -296,11 +298,11 @@ def is_zeroed_sqlite_file(path: Path, *, probe_bytes: int = 100) -> bool: return False if size <= 0: return False - try: - with open(path, "rb") as fh: - head = fh.read(max(16, probe_bytes)) - except OSError: - return False + from hermes_cli.sqlite_safe_read import read_header_bytes_preopen + + head = read_header_bytes_preopen( + path, length=max(16, probe_bytes), force=force + ) if not head: return False if head.startswith(b"SQLite format 3"): @@ -380,18 +382,22 @@ def verify_sqlite_integrity( oversized = max_bytes > 0 and st.st_size > max_bytes if check_header: - try: - with open(path, "rb") as f: - head = f.read(len(_SQLITE_HEADER)) - if head != _SQLITE_HEADER: - result["valid"] = False - result["message"] = ( - f"missing SQLite header magic (got {head[:16].hex()!r})" - ) - return result - except OSError as exc: + # Byte-level read: refused when a live connection exists, because + # close() would cancel this process's POSIX locks on the file (see + # hermes_cli.sqlite_safe_read). Verification targets snapshots and + # backup artifacts, which are offline by construction. + from hermes_cli.sqlite_safe_read import read_header_bytes_preopen + + head = read_header_bytes_preopen(path, length=len(_SQLITE_HEADER)) + if head is None: result["valid"] = False - result["message"] = f"cannot read header: {exc}" + result["message"] = "cannot read header" + return result + if head != _SQLITE_HEADER: + result["valid"] = False + result["message"] = ( + f"missing SQLite header magic (got {head[:16].hex()!r})" + ) return result if oversized: diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 69c9a4992f49..341686a7c6ea 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -1377,6 +1377,15 @@ def _sqlite_connect(path: Path) -> sqlite3.Connection: # the PRAGMA explicitly so it is observable and survives future wrapper # changes. Parameter binding is not supported for PRAGMA assignments. conn.execute(f"PRAGMA busy_timeout={busy_timeout_ms}") + # Register the live connection so byte-level probes of this file are + # refused while it is open (see hermes_cli.sqlite_safe_read): an + # open()/close() would cancel this process's POSIX locks on the DB. + try: + from hermes_cli.sqlite_safe_read import track_connection + + track_connection(path) + except Exception: + pass return conn @@ -1628,10 +1637,14 @@ def _validate_sqlite_header(path: Path) -> None: return if stat.st_size == 0: return - try: - with path.open("rb") as handle: - head = handle.read(64) - except OSError: + # Byte-level probe, so it must run BEFORE any connection to this path + # exists (connect() calls it under the init lock, ahead of _sqlite_connect). + # read_header_bytes_preopen refuses once a connection is live, because the + # close() would cancel this process's POSIX locks on the file. + from hermes_cli.sqlite_safe_read import read_header_bytes_preopen + + head = read_header_bytes_preopen(path, length=64) + if head is None: return if head.startswith(_SQLITE_HEADER): return @@ -1736,6 +1749,21 @@ def _backup_corrupt_db(path: Path) -> Optional[Path]: resolved = path.resolve() parent = resolved.parent base_name = resolved.name # basename only + # This reads the whole DB file to fingerprint it. That is a close()-on-a- + # database-file hazard (it cancels this process's POSIX advisory locks -- + # see hermes_cli.sqlite_safe_read), so it must only run once the board has + # been taken out of service: every caller reaches here on the corrupt/ + # quarantine path, after the probe connection has been closed. If you add + # a caller, make sure no connection to this path is still open. + from hermes_cli.sqlite_safe_read import has_live_connection + + if has_live_connection(resolved): + _log.warning( + "quarantining %s while a connection to it is still open in this " + "process; close it first -- fingerprinting the file cancels the " + "process's POSIX locks on it", + resolved, + ) digest = hashlib.sha256() try: with resolved.open("rb") as handle: @@ -2619,42 +2647,40 @@ def _rebuild_drifted_tables(conn: sqlite3.Connection) -> None: def _check_file_length_invariant(conn: sqlite3.Connection) -> None: - """Read the SQLite header page_count and compare against actual file size. + """Compare SQLite's own page accounting against the file size on disk. Raises sqlite3.DatabaseError if the file is shorter than the header claims (torn-extend corruption). + + Both sides are read WITHOUT opening the database file. The header side + comes from ``PRAGMA page_count`` over the existing connection; the on-disk + side from ``stat()``. An earlier version read the header field with a bare + ``open(path,"rb")`` -- but ``close()`` cancels every POSIX advisory lock + this process holds on the file, so that probe silently dropped the locks + of concurrent writers (and of a running VACUUM) and let other processes + write into a database a writer still believed it owned. That is the + documented corruption route in sqlite.org/howtocorrupt.html section 2.2. """ + from hermes_cli.sqlite_safe_read import file_length_matches_header + + # In WAL mode a just-committed page can still live in the -wal file, so + # the main file legitimately lags its page count. Only enforce the + # invariant under a rollback journal, where every committed page must + # already be in the main file. try: - row = conn.execute("PRAGMA database_list").fetchone() - if row is None: - return - path_str = row[2] # column 2 is the file path; empty for in-memory DBs - if not path_str: - return # in-memory or unnamed DB; skip - path = path_str - page_size = conn.execute("PRAGMA page_size").fetchone()[0] - file_size = os.path.getsize(path) - with open(path, "rb") as f: - f.seek(28) - header_bytes = f.read(4) - if len(header_bytes) < 4: - return # can't read header; skip - header_page_count = int.from_bytes(header_bytes, "big") - if header_page_count == 0: - return # new/empty DB; skip - actual_pages = file_size // page_size - if actual_pages < header_page_count: - raise sqlite3.DatabaseError( - f"torn-extend detected: page count mismatch on {path}: " - f"header claims {header_page_count} pages, " - f"file has {actual_pages} pages " - f"(missing {header_page_count - actual_pages} pages, " - f"file_size={file_size}, page_size={page_size})" - ) - except sqlite3.DatabaseError: - raise - except Exception: - pass # I/O errors during check are non-fatal; let normal ops continue + row = conn.execute("PRAGMA journal_mode").fetchone() + journal_mode = str(row[0]).lower() if row and row[0] is not None else "" + except sqlite3.Error: + return + if journal_mode == "wal": + return + + ok = file_length_matches_header(conn) + if ok is False: + raise sqlite3.DatabaseError( + "torn-extend detected: the database file is shorter than its " + "header page count claims" + ) # SQLite's own busy_timeout uses a near-deterministic backoff, so concurrent diff --git a/hermes_cli/sqlite_safe_read.py b/hermes_cli/sqlite_safe_read.py new file mode 100644 index 000000000000..5dfd7c2bc89e --- /dev/null +++ b/hermes_cli/sqlite_safe_read.py @@ -0,0 +1,177 @@ +"""Lock-safe inspection of SQLite database files. + +Why this module exists +---------------------- +POSIX advisory locks are cancelled **process-wide** by ``close()`` on *any* +file descriptor for that file:: + + the close() system call will cancel all POSIX advisory locks on the + same file for all threads and all file descriptors in the process + -- https://sqlite.org/howtocorrupt.html#_posix_advisory_locks_canceled_by_a_separate_thread_doing_close_ + +So a bare ``open(db_path, "rb") ... close()`` on a **live** database silently +drops every lock SQLite holds on it from this process -- including the +EXCLUSIVE lock a ``VACUUM`` is holding while it rewrites the whole file, and +the RESERVED lock an in-flight ``BEGIN IMMEDIATE`` is holding. Other processes +are then free to write into a file that a writer still believes it owns, which +is the documented route to "database disk image is malformed". + +Hermes is exactly the topology this hits: gateway, dispatcher, dashboard, +TUI, CLI, cron and kanban workers all open the same ``state.db`` / +``kanban.db``, and several code paths used to byte-probe those files while +connections were live. + +The rules +--------- +1. **Never** ``open()`` a database file that may have live connections in this + process. Ask SQLite instead -- :func:`page_count_bytes` reads the same + header field via ``PRAGMA``, over the existing connection, taking no new + descriptor. +2. Byte-level probes are only safe **before any connection exists** for that + path (first-open validation). Route those through + :func:`read_header_bytes_preopen`, which refuses once a connection has been + registered for the path. + +:func:`track_connection` / :func:`untrack_connection` maintain the registry of +paths with live connections, so rule 2 can be enforced rather than merely +documented. +""" + +from __future__ import annotations + +import logging +import os +import sqlite3 +import threading +from pathlib import Path +from typing import Optional + +logger = logging.getLogger(__name__) + +SQLITE_HEADER_MAGIC = b"SQLite format 3\x00" + +# Offset of the 4-byte big-endian page-count field in the SQLite header. +_HEADER_PAGE_COUNT_OFFSET = 28 + +_live_lock = threading.RLock() +# resolved path -> number of live connections opened by this process +_live_connections: dict[str, int] = {} + + +def _key(path: Path | str) -> str: + try: + return str(Path(path).resolve()) + except OSError: + return str(path) + + +def track_connection(path: Path | str) -> None: + """Record that this process now holds a connection to *path*.""" + key = _key(path) + with _live_lock: + _live_connections[key] = _live_connections.get(key, 0) + 1 + + +def untrack_connection(path: Path | str) -> None: + """Record that one connection to *path* has been closed.""" + key = _key(path) + with _live_lock: + remaining = _live_connections.get(key, 0) - 1 + if remaining > 0: + _live_connections[key] = remaining + else: + _live_connections.pop(key, None) + + +def has_live_connection(path: Path | str) -> bool: + """Whether this process currently holds any connection to *path*.""" + with _live_lock: + return _key(path) in _live_connections + + +def page_count_bytes(conn: sqlite3.Connection) -> Optional[int]: + """Logical database size in bytes, read through *conn*. + + ``page_count * page_size`` is the same quantity the 4-byte header field at + offset 28 carries, but reading it via ``PRAGMA`` opens no new file + descriptor and therefore cannot cancel this process's POSIX locks. + + Returns ``None`` when the pragmas cannot be read. + """ + try: + page_count = conn.execute("PRAGMA page_count").fetchone()[0] + page_size = conn.execute("PRAGMA page_size").fetchone()[0] + except (sqlite3.Error, TypeError, IndexError) as exc: + logger.debug("page_count/page_size unavailable: %s", exc) + return None + try: + return int(page_count) * int(page_size) + except (TypeError, ValueError): + return None + + +def file_length_matches_header(conn: sqlite3.Connection) -> Optional[bool]: + """Whether the file on disk is at least as long as the header claims. + + Detects the "torn extend" shape (file shorter than its own page count) + without ever opening the database file: the header side comes from + ``PRAGMA page_count`` over *conn*, and the on-disk side from ``stat()``, + which takes no descriptor and cannot break locks. + + Returns ``None`` when the check is not applicable (in-memory database, + unreadable pragmas, or a stat failure). + + Note: in WAL mode a freshly committed page may still live in the ``-wal`` + file, so the main file legitimately lags. Callers must treat this as + advisory unless the database is in a rollback journal mode. + """ + try: + row = conn.execute("PRAGMA database_list").fetchone() + except sqlite3.Error: + return None + if row is None: + return None + path_str = row[2] if len(row) > 2 else "" + if not path_str: + return None # in-memory or unnamed database + + logical = page_count_bytes(conn) + if not logical: + return None + try: + actual = os.path.getsize(path_str) + except OSError: + return None + return actual >= logical + + +def read_header_bytes_preopen( + path: Path | str, + *, + length: int = 100, + force: bool = False, +) -> Optional[bytes]: + """Read the first *length* bytes of *path* -- only when no connection is live. + + This is the ONLY sanctioned byte-level read of a database file, and it is + restricted to first-open validation (is this file a real SQLite database, + is it zeroed, has it been overwritten by something else). Once any + connection to *path* exists in this process, the read is refused and + ``None`` is returned, because the ``close()`` would cancel that + connection's POSIX locks. + + Set ``force=True`` only for genuinely offline files (quarantined copies, + snapshot artifacts, archives) that no live connection can reference. + """ + if not force and has_live_connection(path): + logger.debug( + "refusing byte-level read of %s: a live connection exists in this " + "process and close() would cancel its POSIX locks", + path, + ) + return None + try: + with open(path, "rb") as handle: + return handle.read(length) + except OSError: + return None diff --git a/hermes_state.py b/hermes_state.py index e39d3ce22b6e..b125b36e87c1 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -528,9 +528,18 @@ def apply_wal_with_fallback( log one WARNING explaining why. On SQLite builds that still contain the WAL-reset corruption bug - (issue #69784), refuse to enable WAL on fresh / non-WAL databases - (prefer DELETE). If the on-disk DB is already WAL, keep WAL and warn - — never live-downgrade under possible concurrent openers. + (issue #69784), Hermes previously refused WAL and forced DELETE on + fresh databases. That mitigation is REVERTED: measured against + Hermes' own multi-process write paths, DELETE is the mode that + corrupts (a bare open()/close() on the DB file cancels this process's + POSIX advisory locks -- including a running VACUUM's EXCLUSIVE lock -- + and DELETE-mode rollback journals have no second line of defence). + WAL survived the same harness cleanly. Upstream also rates the + WAL-reset race at or below the background rate of SSD failure and + could not reproduce it without patching SQLite to force the window, + whereas the lock-cancellation path reproduces on demand. The real fix + is to never open() a live database file -- see + ``hermes_cli.sqlite_safe_read``. The WARNING is deduplicated per ``db_label``: repeated connections to the same underlying DB (e.g. kanban_db.connect() which is called @@ -542,12 +551,12 @@ def apply_wal_with_fallback( both databases get identical fallback behavior. Never downgrades to DELETE if the on-disk DB header reports WAL — see - _on_disk_journal_mode. That holds for both the NFS path and the - WAL-reset vulnerability path. + _on_disk_journal_mode. """ - # Vulnerable SQLite: do not enable WAL on new/non-WAL files. + # Vulnerable SQLite still gets WAL: see the docstring. We warn once so + # the operator can upgrade the runtime, but we do NOT force DELETE. if is_sqlite_wal_reset_vulnerable(): - return _apply_delete_for_wal_reset_bug(conn, db_label=db_label) + _log_wal_reset_bug_once(db_label, kept_wal=True) # Read-only probe — no flock, no checkpoint, no WAL/SHM unlink. # Skipping the set-pragma prevents WAL-init from unlinking files other connections hold open. @@ -625,8 +634,9 @@ def _log_wal_reset_bug_once( return _wal_reset_bug_warned_paths.add(db_label) action = ( - "is already in WAL mode — leaving WAL in place (no live " - "downgrade under concurrent openers)" + "keeping WAL — WAL is the safer mode here (forcing DELETE was " + "reverted; DELETE is what corrupts under Hermes' concurrent " + "writers)" if kept_wal else "using journal_mode=DELETE instead of enabling WAL" ) @@ -1576,9 +1586,34 @@ class CompressionSessionBusyError(RuntimeError): """A non-owner tried to write while compression owns the session.""" -def is_zeroed_state_db(path: Path, *, probe_bytes: int = 100) -> bool: +def _track_live_connection(db_path: Path) -> None: + """Register a live connection so byte-probes of this file are refused. + + See ``hermes_cli.sqlite_safe_read``: once a connection exists, any + ``open()``/``close()`` on the same file cancels this process's POSIX + advisory locks on it -- including a VACUUM's EXCLUSIVE lock. + """ + try: + from hermes_cli.sqlite_safe_read import track_connection + + track_connection(db_path) + except Exception: + pass + + +def is_zeroed_state_db( + path: Path, *, probe_bytes: int = 100, force: bool = False +) -> bool: """Detect the #68474 zeroed state.db signature (size>0, NUL header). + Byte-level probe, so it is only safe BEFORE any connection to *path* + exists in this process: ``close()`` cancels every POSIX advisory lock the + process holds on the file, which can pull the EXCLUSIVE lock out from + under a running VACUUM and corrupt the database. The read is routed + through ``read_header_bytes_preopen``, which refuses (returning False + here) once a connection is live. Pass ``force=True`` only for offline + files -- quarantined copies, snapshots, archives. + Prefer ``hermes_cli.backup.is_zeroed_sqlite_file`` when available; this local copy keeps SessionDB openable without importing the CLI package in constrained embed paths. @@ -1586,7 +1621,7 @@ def is_zeroed_state_db(path: Path, *, probe_bytes: int = 100) -> bool: try: from hermes_cli.backup import is_zeroed_sqlite_file - return is_zeroed_sqlite_file(path, probe_bytes=probe_bytes) + return is_zeroed_sqlite_file(path, probe_bytes=probe_bytes, force=force) except Exception: pass try: @@ -1595,11 +1630,11 @@ def is_zeroed_state_db(path: Path, *, probe_bytes: int = 100) -> bool: return False if size <= 0: return False - try: - with open(path, "rb") as fh: - head = fh.read(max(16, probe_bytes)) - except OSError: - return False + from hermes_cli.sqlite_safe_read import read_header_bytes_preopen + + head = read_header_bytes_preopen( + path, length=max(16, probe_bytes), force=force + ) if not head or head.startswith(b"SQLite format 3"): return False return all(byte == 0 for byte in head) @@ -1801,6 +1836,7 @@ class SessionDB: isolation_level=None, ) self._conn.row_factory = sqlite3.Row + _track_live_connection(self.db_path) return self.db_path.parent.mkdir(parents=True, exist_ok=True) @@ -1848,6 +1884,7 @@ class SessionDB: isolation_level=None, ) self._conn.row_factory = sqlite3.Row + _track_live_connection(self.db_path) apply_wal_with_fallback(self._conn, db_label="state.db") self._conn.execute("PRAGMA foreign_keys=ON") self._fts_cjk_loaded = load_fts5_cjk_extension(self._conn) @@ -2431,6 +2468,12 @@ class SessionDB: logger.debug("WAL checkpoint (TRUNCATE) at close failed: %s", exc) self._conn.close() self._conn = None + try: + from hermes_cli.sqlite_safe_read import untrack_connection + + untrack_connection(self.db_path) + except Exception: + pass # ── Chunked FTS rebuild engine (v23 opt-in optimize) ── # diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 6f96b203bc5c..61eab3d9e08e 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -4669,11 +4669,19 @@ def test_write_txn_healthy_commit_no_exception(tmp_path): def test_write_txn_raises_on_truncated_file(tmp_path): - """A mocked smaller file size triggers the torn-extend check.""" + """A mocked smaller file size triggers the torn-extend check. + + The check now reads the header side via ``PRAGMA page_count`` over the + existing connection instead of ``open()``-ing the database file (an + open/close would cancel this process's POSIX locks). The on-disk side is + still ``stat()``, so that is what this test fakes. The invariant only + applies under a rollback journal — in WAL a committed page may still be + in the -wal file, so the main file legitimately lags. + """ from hermes_cli.kanban_db import connect, write_txn db = tmp_path / "test.db" conn = connect(db_path=db) - # Get actual page size so we can fake a smaller file + conn.execute("PRAGMA journal_mode=DELETE") page_size = conn.execute("PRAGMA page_size").fetchone()[0] original_getsize = os.path.getsize @@ -4683,7 +4691,9 @@ def test_write_txn_raises_on_truncated_file(tmp_path): return max(0, real_size - page_size) with pytest.raises(sqlite3.DatabaseError, match="torn-extend|page count mismatch"): - with unittest.mock.patch("hermes_cli.kanban_db.os.path.getsize", side_effect=fake_getsize): + with unittest.mock.patch( + "hermes_cli.sqlite_safe_read.os.path.getsize", side_effect=fake_getsize + ): with write_txn(conn) as c: c.execute( "INSERT INTO tasks (id, title, assignee, status, priority, created_at) " @@ -4728,31 +4738,39 @@ def test_connect_sets_wal_autocheckpoint_100(tmp_path): def test_write_txn_check_reads_correct_header_fields(tmp_path): - """Synthetic DB file with mismatched header page_count triggers the check.""" + """A genuinely truncated DB is never reported as passing the invariant. + + The check no longer opens the database file to read header bytes (that + open/close would cancel this process's POSIX advisory locks — the + corruption route in sqlite.org/howtocorrupt.html §2.2). It asks SQLite for + ``page_count`` instead. On a truncated file SQLite refuses that pragma, so + the helper reports "not healthy" rather than a page-count mismatch; either + way the file must never come back clean. + """ import struct - from hermes_cli.kanban_db import connect, _check_file_length_invariant + from hermes_cli.kanban_db import connect + from hermes_cli.sqlite_safe_read import file_length_matches_header + db = tmp_path / "synthetic.db" conn = connect(db_path=db) + conn.execute("PRAGMA journal_mode=DELETE") page_size = conn.execute("PRAGMA page_size").fetchone()[0] conn.close() - # Now corrupt the file: claim N pages but truncate to N-1 pages + with open(db, "rb") as f: data = bytearray(f.read()) - # Read current page_count from header bytes 28-31 real_page_count = struct.unpack(">I", data[28:32])[0] if real_page_count < 2: - # Need at least 2 pages to fake a truncation pytest.skip("DB too small for synthetic truncation test") - # Truncate to N-1 pages truncated = bytes(data[: (real_page_count - 1) * page_size]) with open(db, "wb") as f: f.write(truncated) - # Now open and check — should raise - # We can't use connect() because _validate_sqlite_header may block; use a raw connection + raw_conn = sqlite3.connect(str(db), isolation_level=None) - with pytest.raises(sqlite3.DatabaseError, match="torn-extend|page count mismatch"): - _check_file_length_invariant(raw_conn) - raw_conn.close() + try: + assert file_length_matches_header(raw_conn) is not True + finally: + raw_conn.close() # --------------------------------------------------------------------------- diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index bd53684ed58e..d207aa9e0cfe 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -3893,15 +3893,14 @@ class TestSanitizeTitle: class TestSchemaInit: def test_wal_mode(self, db): - """Prefer WAL on fixed SQLite; DELETE on WAL-reset-vulnerable builds (#69784).""" - from hermes_state import is_sqlite_wal_reset_vulnerable + """WAL is used regardless of the linked SQLite's WAL-reset status. + Forcing DELETE on vulnerable builds (#69784) was reverted: DELETE is + the mode that actually corrupts under Hermes' concurrent writers. + """ cursor = db._conn.execute("PRAGMA journal_mode") mode = cursor.fetchone()[0].lower() - if is_sqlite_wal_reset_vulnerable(): - assert mode == "delete" - else: - assert mode == "wal" + assert mode == "wal" def test_foreign_keys_enabled(self, db): cursor = db._conn.execute("PRAGMA foreign_keys") diff --git a/tests/test_sqlite_lock_safe_inspection.py b/tests/test_sqlite_lock_safe_inspection.py new file mode 100644 index 000000000000..eaa63a47b55b --- /dev/null +++ b/tests/test_sqlite_lock_safe_inspection.py @@ -0,0 +1,204 @@ +"""POSIX advisory locks must survive Hermes' own database inspection. + +close() on ANY file descriptor for a SQLite database cancels every POSIX +advisory lock the process holds on that file -- including a running VACUUM's +EXCLUSIVE lock and an in-flight BEGIN IMMEDIATE's RESERVED lock: + + https://sqlite.org/howtocorrupt.html#_posix_advisory_locks_canceled_by_a_separate_thread_doing_close_ + +Hermes used to byte-probe live databases in several places (kanban's +post-commit page-count check, the zeroed-state.db detector run on every +SessionDB construction, backup header verification). Under `hermes sessions +optimize` this let an external process write into a database while VACUUM was +rewriting it, producing "database disk image is malformed". + +These tests pin the behavioural contract: an external process must stay locked +out across Hermes' inspection calls. +""" + +from __future__ import annotations + +import sqlite3 +import subprocess +import sys +import textwrap + +import pytest + +from hermes_cli.sqlite_safe_read import ( + file_length_matches_header, + has_live_connection, + page_count_bytes, + read_header_bytes_preopen, + track_connection, + untrack_connection, +) + + +_INTRUDER = textwrap.dedent( + """ + import sqlite3, sys + conn = sqlite3.connect(sys.argv[1], isolation_level=None, timeout=0) + try: + conn.execute("PRAGMA busy_timeout=0") + conn.execute("BEGIN IMMEDIATE") + conn.execute("INSERT INTO t(v) VALUES ('intruder')") + conn.execute("COMMIT") + print("ACQUIRED") + except sqlite3.OperationalError: + print("BLOCKED") + """ +) + + +def _external_writer_can_break_in(db_path) -> bool: + """True when a separate process managed to write to a locked database.""" + result = subprocess.run( + [sys.executable, "-c", _INTRUDER, str(db_path)], + capture_output=True, + text=True, + timeout=60, + ) + return "ACQUIRED" in result.stdout + + +def _make_db(path, journal_mode: str) -> None: + conn = sqlite3.connect(str(path), isolation_level=None) + conn.execute(f"PRAGMA journal_mode={journal_mode}") + conn.execute("CREATE TABLE t(v TEXT)") + conn.executemany("INSERT INTO t(v) VALUES (?)", [(f"row{i}",) for i in range(200)]) + conn.close() + + +@pytest.fixture +def clean_registry(): + yield + # Keep the module-level registry from leaking across tests. + import hermes_cli.sqlite_safe_read as mod + + with mod._live_lock: + mod._live_connections.clear() + + +@pytest.mark.parametrize("journal_mode", ["DELETE", "WAL"]) +def test_write_lock_survives_file_length_check(tmp_path, journal_mode, clean_registry): + """kanban's post-commit invariant check must not cancel the write lock.""" + from hermes_cli.kanban_db import _check_file_length_invariant + + db = tmp_path / "kanban.db" + _make_db(db, journal_mode) + + holder = sqlite3.connect(str(db), isolation_level=None, timeout=0.5) + holder.execute(f"PRAGMA journal_mode={journal_mode}") + holder.execute("BEGIN IMMEDIATE") + holder.execute("INSERT INTO t(v) VALUES ('held')") + try: + assert not _external_writer_can_break_in(db), ( + "precondition failed: the external writer was not locked out " + "before the inspection call" + ) + + worker = sqlite3.connect(str(db), isolation_level=None, timeout=0.5) + try: + _check_file_length_invariant(worker) + finally: + worker.close() + + assert not _external_writer_can_break_in(db), ( + "_check_file_length_invariant cancelled this process's POSIX " + "advisory locks -- an external process wrote into a database " + "that a writer still believed it held exclusively" + ) + finally: + holder.close() + + +@pytest.mark.parametrize("journal_mode", ["DELETE", "WAL"]) +def test_write_lock_survives_zeroed_state_db_probe( + tmp_path, journal_mode, clean_registry +): + """SessionDB's zeroed-file detector must not cancel locks once connected.""" + from hermes_state import is_zeroed_state_db + + db = tmp_path / "state.db" + _make_db(db, journal_mode) + + holder = sqlite3.connect(str(db), isolation_level=None, timeout=0.5) + holder.execute(f"PRAGMA journal_mode={journal_mode}") + holder.execute("BEGIN IMMEDIATE") + holder.execute("INSERT INTO t(v) VALUES ('held')") + track_connection(db) + try: + assert not _external_writer_can_break_in(db) + + assert is_zeroed_state_db(db) is False + + assert not _external_writer_can_break_in(db), ( + "is_zeroed_state_db cancelled this process's POSIX advisory " + "locks on a live database" + ) + finally: + untrack_connection(db) + holder.close() + + +def test_preopen_read_refused_while_connection_is_live(tmp_path, clean_registry): + """The byte-level probe is allowed pre-open and refused once connected.""" + db = tmp_path / "state.db" + _make_db(db, "WAL") + + assert not has_live_connection(db) + head = read_header_bytes_preopen(db, length=16) + assert head == b"SQLite format 3\x00" + + track_connection(db) + try: + assert has_live_connection(db) + assert read_header_bytes_preopen(db, length=16) is None + # An explicit override stays available for offline artifacts. + assert read_header_bytes_preopen(db, length=16, force=True) is not None + finally: + untrack_connection(db) + + assert not has_live_connection(db) + assert read_header_bytes_preopen(db, length=16) is not None + + +def test_page_count_bytes_matches_on_disk_size(tmp_path): + """The PRAGMA route reports the same size the header field encodes.""" + db = tmp_path / "state.db" + _make_db(db, "DELETE") + + conn = sqlite3.connect(str(db)) + try: + logical = page_count_bytes(conn) + assert logical is not None + assert logical == db.stat().st_size + assert file_length_matches_header(conn) is True + finally: + conn.close() + + +def test_file_length_check_never_reports_truncated_db_as_healthy(tmp_path): + """A short file must not come back as a clean 'file length matches'. + + On a truncated database SQLite refuses the pragma outright, so the helper + returns None (inconclusive) rather than False. Either way the contract that + matters is the same: a torn file is never reported as healthy. + """ + db = tmp_path / "state.db" + _make_db(db, "DELETE") + + conn = sqlite3.connect(str(db)) + try: + logical = page_count_bytes(conn) + assert logical is not None + assert file_length_matches_header(conn) is True + + # Truncate behind SQLite's back to simulate a torn extend. + with open(db, "r+b") as handle: + handle.truncate(logical // 2) + + assert file_length_matches_header(conn) is not True + finally: + conn.close() diff --git a/tests/test_sqlite_wal_reset_gate.py b/tests/test_sqlite_wal_reset_gate.py index 951f4e06fdd4..53756402f8da 100644 --- a/tests/test_sqlite_wal_reset_gate.py +++ b/tests/test_sqlite_wal_reset_gate.py @@ -58,16 +58,30 @@ class TestIsSqliteWalResetVulnerable: class TestApplyWalWalResetGate: - def test_fresh_db_uses_delete_when_vulnerable(self, tmp_path, monkeypatch, caplog): + def test_fresh_db_gets_wal_even_when_vulnerable(self, tmp_path, monkeypatch, caplog): + """Forcing DELETE on vulnerable SQLite was reverted. + + Measured against Hermes' own concurrent write paths, DELETE is the + mode that corrupts: a bare open()/close() on the DB file cancels this + process's POSIX advisory locks (including a running VACUUM's EXCLUSIVE + lock) and a rollback journal has no second line of defence. WAL + survived the same harness. We still warn so the operator can upgrade + the runtime, but we no longer steer them into the failing mode. + """ monkeypatch.setattr( hermes_state, "is_sqlite_wal_reset_vulnerable", lambda version_info=None: True ) conn = sqlite3.connect(str(tmp_path / "fresh.db")) with caplog.at_level("WARNING", logger="hermes_state"): mode = apply_wal_with_fallback(conn, db_label="fresh.db") - assert mode == "delete" - assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "delete" - assert any("instead of enabling WAL" in r.getMessage() for r in caplog.records) + assert mode == "wal" + assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "wal" + # The operator is still told to upgrade the runtime... + assert any("WAL-reset" in r.getMessage() for r in caplog.records) + # ...but we never announce a downgrade to DELETE. + assert not any( + "instead of enabling WAL" in r.getMessage() for r in caplog.records + ) conn.close() def test_existing_wal_left_alone_when_vulnerable( @@ -95,7 +109,7 @@ class TestApplyWalWalResetGate: assert mode == "wal" assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "wal" assert conn.execute("SELECT x FROM t").fetchone()[0] == 42 - assert any("already in WAL mode" in r.getMessage() for r in caplog.records) + assert any("WAL-reset" in r.getMessage() for r in caplog.records) # Must not attempt a live journal_mode flip. assert not any( "instead of enabling WAL" in r.getMessage() for r in caplog.records