From 2438d0a97e472550bc3542619f4a4d651107c9bf Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 15:28:01 -0300 Subject: [PATCH] fix(state): track repair/probe connections so byte-probes can't cancel their locks _connect_repair_durable() -- the single entry point for every repair/probe connection to state.db -- opened the database with a bare sqlite3.connect(), outside the live-connection registry in hermes_cli/sqlite_safe_read.py. While a repair connection was open, has_live_connection() reported false, so any byte-level probe in the process (zeroed-file detector, header verification, kanban's post-commit page check) was free to open()/close() the file -- cancelling every POSIX advisory lock the process holds on it (howtocorrupt 2.2) and letting an external writer commit into a database the repair still believed it owned. These paths hold the strongest locks in the process: _open_exclusive() keeps locking_mode=EXCLUSIVE across the whole snapshot -> strategies -> promotion window. Open through connect_tracked() instead: the fd stays registered for its whole lifetime and is released on close(). The sqlite3.connect(str(db_path), ...) call stays in this module so tests patching it keep control, and installs without hermes_cli keep the durable (untracked) connection as before. Verified on real files, no mocks: with a repair connection holding BEGIN EXCLUSIVE, an external writer is BLOCKED, a byte probe now returns None (refused instead of opening the fd), the same writer stays BLOCKED after it, and the registry is empty again once the repair closes. A real #63386-damaged database (stale B-tree index) still reports exit 1 on --check-only and repairs via reindex_btree to integrity_check 'ok'. Refs #63386 (cherry picked from commit a031f1be26b777bc09e744b3fa6081ce2af1f851) --- hermes_state_repair.py | 24 ++++++- .../test_sqlite_lock_safe_inspection.py | 62 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/hermes_state_repair.py b/hermes_state_repair.py index 7e8cc95879..e90f67b48c 100644 --- a/hermes_state_repair.py +++ b/hermes_state_repair.py @@ -580,8 +580,28 @@ def _connect_repair_durable(db_path: Path, *, timeout: float = 5.0) -> sqlite3.C no ``checkpoint_fullfsync`` — on Darwin an interrupted ``REINDEX``/``VACUUM``/``writable_schema`` rewrite leaves half-written b-tree pages. Autocommit (``isolation_level=None``): DDL and ``VACUUM`` are illegal inside an implicit transaction. Barriers are best-effort: on a malformed schema even ``PRAGMA synchronous=FULL`` raises, - so whole-file rewrites call :func:`_reapply_durability_barriers` once the schema parses again.""" - conn = sqlite3.connect(str(db_path), timeout=timeout, isolation_level=None) + so whole-file rewrites call :func:`_reapply_durability_barriers` once the schema parses again. + + Opened through :func:`hermes_cli.sqlite_safe_read.connect_tracked` so the fd is registered for its whole + lifetime: the repair paths hold the strongest locks in the process (``_open_exclusive`` keeps + ``locking_mode=EXCLUSIVE`` across the snapshot → strategies → promotion window, and the write-health probe + opens a ``BEGIN IMMEDIATE`` reservation). While untracked, every byte-level probe of a *live* state.db — the + zeroed-file detector, header verification, kanban's post-commit page check — was allowed to ``open()``/ + ``close()`` the file, which cancels every POSIX advisory lock this process holds on it + (https://sqlite.org/howtocorrupt.html#_posix_advisory_locks_canceled_by_a_separate_thread_doing_close_) + and lets an external writer commit into a database the repair still believes it owns (#63386). + """ + try: + from hermes_cli.sqlite_safe_read import connect_tracked + except ImportError: + # Scaffold/embed installs without hermes_cli: the durable connection stays, only the guard is off. + logger.debug("hermes_cli.sqlite_safe_read unavailable; opening %s untracked " + "(byte-probe guard inactive in this install)", db_path) + conn = sqlite3.connect(str(db_path), timeout=timeout, isolation_level=None) + else: + # Open through THIS module's sqlite3.connect so tests patching it keep control of the fd. + conn = connect_tracked(db_path, tracking_path=db_path, connect_fn=sqlite3.connect, + timeout=timeout, isolation_level=None) _reapply_durability_barriers(conn) return conn diff --git a/tests/hermes_state/test_sqlite_lock_safe_inspection.py b/tests/hermes_state/test_sqlite_lock_safe_inspection.py index 45a2d05b82..9eaea87b53 100644 --- a/tests/hermes_state/test_sqlite_lock_safe_inspection.py +++ b/tests/hermes_state/test_sqlite_lock_safe_inspection.py @@ -285,6 +285,68 @@ def test_session_db_read_only_is_tracked(tmp_path, clean_registry, monkeypatch): assert read_header_bytes_preopen(db_path, length=16) is not None +def test_repair_connections_are_tracked_for_byte_probe_safety(tmp_path, clean_registry, monkeypatch): + """End-to-end: live repair/probe connections block byte-level reads (#63386). + + The repair paths never go through ``SessionDB`` and they hold the strongest + locks in the process: ``_open_exclusive`` keeps ``locking_mode=EXCLUSIVE`` + across the whole snapshot -> strategies -> promotion window, and the + write-health probe opens a ``BEGIN IMMEDIATE`` reservation. They used to + connect outside the registry, so the byte-probe guard could not see them and + every inspection ``open()``/``close()`` cancelled those POSIX advisory locks + (``howtocorrupt`` §2.2), letting an external writer commit into the database + the repair still believed it owned. + """ + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from hermes_state import SessionDB + from hermes_state_repair import _connect_repair_durable, _repair_conn + + db_path = tmp_path / "state.db" + seed = SessionDB(db_path=db_path) + seed.create_session("s1", source="cli") + seed.close() + assert not has_live_connection(db_path) + + conn = _connect_repair_durable(db_path) + try: + assert has_live_connection(db_path) + assert read_header_bytes_preopen(db_path, length=16) is None + finally: + conn.close() + assert not has_live_connection(db_path) + + with _repair_conn(db_path): + assert has_live_connection(db_path) + assert read_header_bytes_preopen(db_path, length=16) is None + assert not has_live_connection(db_path) + # The guard is released with the connection, not for good. + assert read_header_bytes_preopen(db_path, length=16) is not None + + +def test_byte_probe_never_cancels_the_repair_exclusion(tmp_path, clean_registry): + """A live repair's EXCLUSIVE lock must survive Hermes' own inspection (#63386). + + With the connection tracked the probe is refused, so nothing closes an fd and + the exclusion keeps holding; if the probe were allowed through, its ``close()`` + would cancel the lock and the intruder would commit into the file mid-repair. + """ + import hermes_state_repair as repair + + db_path = tmp_path / "state.db" + _make_db(db_path, "DELETE") + + guard = repair._open_exclusive(db_path, "BEGIN EXCLUSIVE") + try: + assert _external_writer_can_break_in(db_path) is False + assert read_header_bytes_preopen(db_path, length=16) is None + assert _external_writer_can_break_in(db_path) is False, ( + "the repair's EXCLUSIVE lock was cancelled by a byte-level probe " + "on the database it holds" + ) + finally: + guard.close() + +