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)
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user