From 12173db5b7d0456a87f50fdaeb9e479bcd2f8007 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 07:20:38 -0700 Subject: [PATCH] fix(state): second-process maintenance on state.db refuses ANY foreign holder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes doctor --fix`'s WAL checkpoint and `repair_state_db_schema`'s preflight documented themselves as fail-OPEN: `live_writer_holds_db` only refused on unknown/deleted/uninspectable holders and then trusted a `BEGIN IMMEDIATE` probe, which is blind to a `journal_mode=DELETE` reader (SHARED only) and cannot run on a malformed file — exactly the states repair and checkpoint get invoked in. A repair in a second process then REINDEXed / VACUUMed a file the gateway still held (#103339 item 2). - `hermes_state_holders.live_writer_holds_db`: any foreign holder of the DB or a sidecar is a live holder; the probe is only an additional positive signal. - doctor `--fix`: the checkpoint runs on `_exclusive_repair_db_guard`'s connection instead of a bare writable `sqlite3.connect`, so an opener arriving after the scan is refused, not joined; `_session_count` is a `mode=ro` reader. - Normal SessionDB writers are untouched: gateway + dashboard in two processes both keep writing (a process-wide flock on the write path — PR #109270's shape — would break that). Tests: the two-process repair race test releases the test process's own header-probe fd (it is a genuine holder now); the mid-repair writer fixture opens its connection after staging starts (a pre-existing holder is refused up front, which is the point). Refs #103339 #100896 --- hermes_cli/doctor_state.py | 34 ++++++----- hermes_state_holders.py | 20 +++---- hermes_state_repair.py | 13 ++--- .../test_doctor_wal_checkpoint_guard.py | 49 ++++++++++++++++ .../test_state_db_malformed_repair.py | 14 +++++ .../test_state_db_repair_non_destructive.py | 15 ++--- ...est_state_db_second_process_maintenance.py | 58 +++++++++++++++++++ 7 files changed, 162 insertions(+), 41 deletions(-) create mode 100644 tests/hermes_cli/test_doctor_wal_checkpoint_guard.py create mode 100644 tests/hermes_state/test_state_db_second_process_maintenance.py diff --git a/hermes_cli/doctor_state.py b/hermes_cli/doctor_state.py index 296371ecb9..abf9408221 100644 --- a/hermes_cli/doctor_state.py +++ b/hermes_cli/doctor_state.py @@ -150,7 +150,8 @@ def _check_directory_structure(should_fix: bool, f: Finding) -> None: def _session_count(state_db_path: Path): import sqlite3 - conn = sqlite3.connect(str(state_db_path)) + # mode=ro: doctor is a reader; a writable open of a gateway-held WAL DB is the second-writer class (#103339). + conn = sqlite3.connect(f"file:{state_db_path}?mode=ro", uri=True) try: return conn.execute("SELECT COUNT(*) FROM sessions").fetchone()[0] finally: @@ -256,24 +257,27 @@ def _state_db_wal(f: Finding, should_fix: bool, state_db_path: Path) -> None: check_warn(f"WAL file is large ({size // (1024*1024)} MB)", "(may indicate missed checkpoints)") if not should_fix: return f.issues.append("Large WAL file — run 'hermes doctor --fix' to checkpoint") - # Checkpoint-lock premise (#40177): a bare connect runs WAL recovery and the checkpoint joins the - # live WAL — under a running gateway that second-writer handling corrupts state.db. Skip instead. + # Checkpoint-lock premise (#40177, #103339): a bare connect runs WAL recovery and the checkpoint + # joins the live WAL — under a running gateway that second-writer handling corrupts state.db. + # Holder scan first (any other process holding the DB, or an unknown, fails closed), then run the + # checkpoint on the exclusive repair guard so an opener arriving in between is refused, not joined. from hermes_state_holders import live_writer_holds_db - from hermes_state_repair import _connect_repair_durable + from hermes_state_repair import _connect_repair_durable, _exclusive_repair_db_guard + _SKIP = ("Large WAL file — cannot prove state.db is quiet (stop the profile's gateway first, then " + "re-run 'hermes doctor --fix' to checkpoint)") if live_writer_holds_db(state_db_path, connect_repair_durable=_connect_repair_durable): - # Honest disjunction (gate C1): a True here means "held OR - # unprovable" — the DatabaseError lane fires when SQLite - # cannot open the file at all, with nobody holding it. Never - # assert a live writer as fact. + # Honest disjunction (gate C1): a True here means "held OR unprovable" — never assert a live + # writer as fact. check_warn("WAL checkpoint skipped: cannot prove state.db is quiet", - "(a live writer holds it, or it is unreadable — stop the profile's gateway " + "(another process holds it, or it is unreadable — stop the profile's gateway " "and re-run 'hermes doctor --fix')") - return f.issues.append("Large WAL file — cannot prove state.db is quiet (stop the profile's " - "gateway first, then re-run 'hermes doctor --fix' to checkpoint)") - import contextlib - import sqlite3 - with contextlib.closing(sqlite3.connect(str(state_db_path))) as conn: - conn.execute("PRAGMA wal_checkpoint(PASSIVE)") + return f.issues.append(_SKIP) + with _exclusive_repair_db_guard(state_db_path) as (guard, guard_error): + if guard is None: + check_warn("WAL checkpoint skipped: could not take exclusive ownership of state.db", + f"({guard_error}; stop the profile's gateway and re-run 'hermes doctor --fix')") + return f.issues.append(_SKIP) + guard.execute("PRAGMA wal_checkpoint(PASSIVE)") check_ok(f"WAL checkpoint performed ({size // 1024}K → {wal_size() // 1024}K)") f.fixed += 1 elif size > 10 * 1024 * 1024: # 10 MB diff --git a/hermes_state_holders.py b/hermes_state_holders.py index 7727cdbe67..1a69807f2b 100644 --- a/hermes_state_holders.py +++ b/hermes_state_holders.py @@ -320,15 +320,14 @@ def live_writer_holds_db( *, connect_repair_durable: Callable[..., sqlite3.Connection], ) -> bool: - """Return whether repair lacks proven exclusive ownership of ``db_path``.""" - foreign_holders = foreign_state_db_holders(db_path) - if any( - pid < 0 - or path.startswith("uninspectable holder:") - or path.startswith("uninspectable descriptor:") - or path.endswith(" (deleted)") - for pid, path in foreign_holders - ): + """Return whether repair lacks proven exclusive ownership of ``db_path``. + + ANY foreign process holding the DB or a sidecar is a live holder (#103339): the lock probe below + cannot see a DELETE-mode reader (SHARED only) and cannot run at all on a malformed file, and those + are exactly the states repair/VACUUM/checkpoint get invoked in. The holder scan is the authority and + fails closed on its own failures (unknown/uninspectable sentinels); the probe only adds a positive + lock signal on top.""" + if foreign_state_db_holders(db_path): return True probe = None @@ -342,8 +341,7 @@ def live_writer_holds_db( lowered = str(exc).lower() return "locked" in lowered or "busy" in lowered except sqlite3.DatabaseError: - return False - except Exception: + # Malformed/unreadable with no holder on the scan: nobody else has it open, so repair may run. return False finally: if probe is not None: diff --git a/hermes_state_repair.py b/hermes_state_repair.py index 94cf209f9d..cf3857f346 100644 --- a/hermes_state_repair.py +++ b/hermes_state_repair.py @@ -824,15 +824,12 @@ def _db_opens_cleanly(db_path: Path) -> Optional[str]: def _live_writer_holds_db(db_path: Path) -> bool: - """True when a connection outside this call still holds ``db_path`` open. + """True when another process (or a connection outside this call) still holds ``db_path``. - Asks SQLite for what a repair needs and a live holder cannot grant: ``locking_mode=EXCLUSIVE`` then - ``BEGIN IMMEDIATE`` — in WAL mode that needs exclusive WAL-index locks, so any other open connection fails - it with SQLITE_BUSY; neither statement parses the schema, so it works on malformed DBs. Fails **open** - (False) on anything but a positive busy/locked signal: refusing to repair a DB nobody holds would strand - the self-heal path. In ``journal_mode=DELETE`` a held reader takes only SHARED and this returns False; - repair is then serialised only by the cross-process repairer lock. Before probing, the foreign-holder scan - (``hermes_state_holders``) fails closed on deleted-WAL-generation, uninspectable, or unknown holders.""" + The foreign-holder scan (``hermes_state_holders``) is the authority: any other process with the DB or a + WAL sidecar open, a deleted WAL generation, or an unknown/uninspectable holder fails CLOSED. The SQLite + probe (``locking_mode=EXCLUSIVE`` + ``BEGIN IMMEDIATE``) is only an additional positive signal — it cannot + see a ``journal_mode=DELETE`` reader and cannot run on a malformed file, which is why the scan comes first.""" import hermes_state_holders as _state_holders return _state_holders.live_writer_holds_db(db_path, connect_repair_durable=_connect_repair_durable) diff --git a/tests/hermes_cli/test_doctor_wal_checkpoint_guard.py b/tests/hermes_cli/test_doctor_wal_checkpoint_guard.py new file mode 100644 index 0000000000..297b92719c --- /dev/null +++ b/tests/hermes_cli/test_doctor_wal_checkpoint_guard.py @@ -0,0 +1,49 @@ +"""#103339 item 2: ``hermes doctor --fix`` never checkpoints state.db through a bare writable ``sqlite3.connect`` +— the checkpoint runs on the exclusive repair guard, so an opener arriving after the holder scan is refused.""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path + +import hermes_state_repair +from hermes_cli.doctor_report import Finding +from hermes_cli.doctor_state import _state_db_wal + + +def test_doctor_checkpoint_runs_only_on_the_exclusive_repair_guard(tmp_path, monkeypatch): + db = tmp_path / "state.db" + setup = sqlite3.connect(str(db)) + setup.execute("CREATE TABLE t(x)") + setup.execute("PRAGMA journal_mode=WAL") + setup.execute("INSERT INTO t VALUES (1)") + setup.commit() + setup.close() + wal = Path(f"{db}-wal") + with open(wal, "ab") as handle: + handle.truncate(51 * 1024 * 1024) + + bare_connects: list[str] = [] + real_connect = sqlite3.connect + + def _spy(database, *args, **kwargs): + if not str(database).startswith("file:") or "mode=ro" not in str(database): + bare_connects.append(str(database)) + return real_connect(database, *args, **kwargs) + + monkeypatch.setattr(sqlite3, "connect", _spy) + guard_connects: list[Path] = [] + real_durable = hermes_state_repair._connect_repair_durable + + def _durable(path, **kwargs): + guard_connects.append(Path(path)) + return real_durable(path, **kwargs) + + monkeypatch.setattr(hermes_state_repair, "_connect_repair_durable", _durable) + + finding = Finding() + _state_db_wal(finding, True, db) + + assert finding.fixed == 1 and not finding.issues + # Every writable open went through the repair connector (probe + exclusive guard); none was a bare connect. + assert bare_connects and len(bare_connects) == len(guard_connects) >= 2 diff --git a/tests/hermes_state/test_state_db_malformed_repair.py b/tests/hermes_state/test_state_db_malformed_repair.py index 02bdff0a30..54a31fd4ad 100644 --- a/tests/hermes_state/test_state_db_malformed_repair.py +++ b/tests/hermes_state/test_state_db_malformed_repair.py @@ -506,6 +506,17 @@ print(json.dumps(repair_state_db_schema({db!r})), flush=True) """ +def _release_header_probe_fds() -> None: + """Close this process's cached header-probe fds (no SQLite connection is live, so no lock is at risk).""" + import os + + import hermes_state_dbfile + with hermes_state_dbfile._HEADER_PROBE_LOCK: + for fd, _dev, _ino in hermes_state_dbfile._HEADER_PROBE_FDS.values(): + os.close(fd) + hermes_state_dbfile._HEADER_PROBE_FDS.clear() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX flock test") def test_two_processes_repairing_at_once_perform_surgery_once(tmp_path): """Concurrent repairers serialise; the loser sees a healed DB and stops. @@ -518,6 +529,9 @@ def test_two_processes_repairing_at_once_perform_surgery_once(tmp_path): db_path = tmp_path / "state.db" _build_healthy_db(db_path) _corrupt_duplicate_fts(db_path) + # The test process itself must not count as a holder: _build_healthy_db's SessionDB left the + # process-lifetime header-probe fd open, and a second process refuses ANY foreign holder (#103339). + _release_header_probe_fds() script = _REPAIR_SCRIPT.format( root=str(Path(hermes_state.__file__).parent), db=str(db_path) diff --git a/tests/hermes_state/test_state_db_repair_non_destructive.py b/tests/hermes_state/test_state_db_repair_non_destructive.py index 2a1e12ecf0..5302a7b6d4 100644 --- a/tests/hermes_state/test_state_db_repair_non_destructive.py +++ b/tests/hermes_state/test_state_db_repair_non_destructive.py @@ -81,16 +81,17 @@ def _writer_after_stage( """Try one real cross-process write after staging has begun. The repair process owns the SQLite exclusion for the complete - stage/strategy/promotion interval. A writer is allowed to fail or to - wait until that interval ends and commit afterwards; what is forbidden is - a successful commit that promotion silently overwrites. + stage/strategy/promotion interval. The writer arrives mid-repair (a + pre-existing foreign holder is refused up front, #103339) and is allowed to + fail or to wait until that interval ends and commit afterwards; what is + forbidden is a successful commit that promotion silently overwrites. """ + ready.set() + if not start.wait(20): + result.put(("not-started", "repair did not reach staging")) + return conn = sqlite3.connect(db_path, timeout=0.75, isolation_level=None) try: - ready.set() - if not start.wait(20): - result.put(("not-started", "repair did not reach staging")) - return try: conn.execute( "INSERT INTO messages (body) VALUES ('committed-after-stage')" diff --git a/tests/hermes_state/test_state_db_second_process_maintenance.py b/tests/hermes_state/test_state_db_second_process_maintenance.py new file mode 100644 index 0000000000..cfc704e091 --- /dev/null +++ b/tests/hermes_state/test_state_db_second_process_maintenance.py @@ -0,0 +1,58 @@ +"""#103339 item 2: structural maintenance from a second PROCESS must refuse while ANY other process holds +state.db, even where SQLite's own lock probe is blind (``journal_mode=DELETE`` reader; malformed file).""" + +from __future__ import annotations + +import select +import subprocess +import sys +import uuid +from pathlib import Path + +import pytest + +from hermes_state import SessionDB +from hermes_state_repair import repair_state_db_schema + +_HOLDER = """ +import sqlite3, sys +conn = sqlite3.connect(sys.argv[1]) +conn.execute("SELECT count(*) FROM sessions").fetchall() +print("ready", flush=True) +sys.stdin.read(1) +conn.close() +""" + + +@pytest.fixture +def delete_mode_db(tmp_path, monkeypatch) -> Path: + import hermes_state_wal + monkeypatch.setattr(hermes_state_wal, "resolve_journal_mode", lambda: "delete") + db = tmp_path / "state.db" + handle = SessionDB(db_path=db) + sid = handle.create_session(session_id=str(uuid.uuid4()), source="cli") + handle.append_message(sid, role="user", content="seed") + handle.close() + assert db.read_bytes()[18] == 1, "fixture must be a rollback-journal DB" + return db + + +@pytest.mark.linux_only +def test_repair_refuses_delete_mode_db_held_open_by_another_process(delete_mode_db): + """A held DELETE-mode reader takes only SHARED, so ``BEGIN IMMEDIATE`` succeeds and the lock probe sees + nothing; the holder scan must still refuse — REINDEX/VACUUM from a second process is the #103339 class.""" + db = delete_mode_db + holder = subprocess.Popen([sys.executable, "-c", _HOLDER, str(db)], stdin=subprocess.PIPE, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + try: + assert select.select([holder.stdout], [], [], 10)[0] and holder.stdout.readline().strip() == "ready" + with open(db, "r+b") as fh: # schema page garbage under the holder: the shape repair is invoked on + fh.seek(100) + fh.write(b"\xff" * (4096 - 100)) + report = repair_state_db_schema(db, backup=False) + finally: + holder.stdin.write("x") + holder.stdin.close() + holder.wait(timeout=10) + assert report["repaired"] is False + assert "stop the gateway" in (report["error"] or "").lower()