diff --git a/contributors/emails/dhanesh@users.noreply.github.com b/contributors/emails/dhanesh@users.noreply.github.com new file mode 100644 index 0000000000000..7601ad7763088 --- /dev/null +++ b/contributors/emails/dhanesh@users.noreply.github.com @@ -0,0 +1,2 @@ +dhanesh +# PR #90747 salvage (state.db repair durability) via #91852 diff --git a/hermes_state.py b/hermes_state.py index 4f4916761e591..b5fa73b8f84cd 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -2137,6 +2137,61 @@ def _ensure_writable(p: Path, *, is_dir: bool = False) -> None: _ensure_writable(p) +def _connect_repair_durable(db_path: Path) -> sqlite3.Connection: + """``sqlite3.connect`` for the repair/probe paths, with macOS write barriers. + + These paths open ``state.db`` directly rather than through ``SessionDB`` + (which routes via :func:`apply_wal_with_fallback`), so they inherited + SQLite's ``synchronous=NORMAL`` default and no ``checkpoint_fullfsync``. + On Darwin that is exactly the combination :func:`_enforce_macos_synchronous_full` + exists to prevent: ``fsync()`` there guarantees neither data-on-platter nor + write ordering, so a rewrite interrupted by process or OS termination can + leave half-written b-tree pages behind. + + That matters more here than anywhere else in the module, because what runs + through these connections is ``REINDEX``, ``VACUUM`` and ``writable_schema`` + surgery — the operations that rewrite nearly every page of the file. The + 2026-08-19 recurrence tore ``messages`` (root page 5) and + ``idx_messages_session``, reporting the unmistakable signature: repeated + "2nd reference to page", a rowid out of order, and long runs of leaked + "never used" pages. + + Autocommit (``isolation_level=None``) is preserved: callers run DDL and + ``VACUUM``, which are illegal inside an implicit transaction. + + Applying the barriers is best-effort *by necessity*: SQLite loads the + schema before it runs any statement, so on a malformed schema even + ``PRAGMA synchronous=FULL`` raises ``DatabaseError`` ("malformed database + schema (messages_fts) - table messages_fts already exists"). A malformed + database is precisely this helper's input, so raising there would leave + repair unable to open the file it exists to fix. Strategies that go on to + rewrite the whole file call :func:`_reapply_durability_barriers` once the + schema parses again, which is the point at which the pragmas can stick. + """ + conn = sqlite3.connect(str(db_path), isolation_level=None) + _reapply_durability_barriers(conn) + return conn + + +def _reapply_durability_barriers(conn: sqlite3.Connection) -> bool: + """Best-effort (re)application of the macOS write barriers. Never raises. + + Returns True when the pragmas were accepted. Callers about to rewrite the + file wholesale (``VACUUM``, ``REINDEX``) should call this after the schema + becomes parseable, because a connection opened against a malformed schema + could not take them at open time. + """ + try: + _apply_macos_checkpoint_barrier(conn) + _enforce_macos_synchronous_full(conn) + return True + except sqlite3.DatabaseError: + # Schema still unparseable — the pragmas cannot be set yet. + return False + except Exception: + return False + + def _db_opens_cleanly(db_path: Path) -> Optional[str]: """Probe a DB on a fresh connection. Returns None if healthy, else a reason. @@ -2148,7 +2203,7 @@ def _db_opens_cleanly(db_path: Path) -> Optional[str]: through the FTS triggers — is reported as unhealthy rather than slipping past as a false "ok" (#50502). """ - conn = sqlite3.connect(str(db_path), isolation_level=None) + conn = _connect_repair_durable(db_path) try: # Best-effort tokenizer load: a DB carrying the messages_fts_cjk # index needs the cjk_unicode61 extension before any statement can @@ -2263,6 +2318,60 @@ def _db_opens_cleanly(db_path: Path) -> Optional[str]: conn.close() +def _live_writer_holds_db(db_path: Path) -> bool: + """True when a connection outside this call still holds ``db_path`` open. + + Detection works by asking SQLite for the thing a repair actually needs and + a live writer cannot grant: ``PRAGMA locking_mode=EXCLUSIVE`` followed by + ``BEGIN IMMEDIATE``. In WAL mode, entering exclusive locking mode + requires exclusive locks on the WAL index, so any other open connection — + reader or writer — makes it fail with SQLITE_BUSY. Neither statement + parses the schema, so this works on the malformed databases repair exists + to handle. + + Fails **open** (returns False) on anything other than a positive + busy/locked signal: refusing to repair a database that nobody is actually + holding would strand the very self-heal path this guard protects. + + Scope: the WAL-index exclusive lock is what makes this detect a holder, so + the guard is effective in WAL mode. On SQLite builds carrying the WAL-reset + bug and on NFS/SMB, Hermes deliberately runs ``state.db`` in + ``journal_mode=DELETE`` (see :func:`apply_wal_with_fallback`); there a held + reader takes only a SHARED lock, ``BEGIN IMMEDIATE`` still acquires + RESERVED, and this probe returns False. In that mode repair is serialised + only by the cross-process repairer lock rather than by this holder probe. + The 2026-08 incident that motivated the guard was in WAL mode, which this + covers; broadening detection to DELETE mode is left to a follow-up. + """ + probe = None + try: + probe = sqlite3.connect(str(db_path), timeout=0.0, isolation_level=None) + probe.execute("PRAGMA locking_mode=EXCLUSIVE") + probe.execute("BEGIN IMMEDIATE") + probe.execute("ROLLBACK") + return False + except sqlite3.OperationalError as exc: + lowered = str(exc).lower() + return "locked" in lowered or "busy" in lowered + except sqlite3.DatabaseError: + # Malformed/unreadable: no evidence of a live holder either way. + return False + except Exception: + return False + finally: + if probe is not None: + try: + # Drop exclusive locking mode before closing so the probe + # itself never leaves the file pinned. + probe.execute("PRAGMA locking_mode=NORMAL") + except Exception: + pass + try: + probe.close() + except Exception: + pass + + def repair_state_db_schema(db_path: Path, *, backup: bool = True) -> Dict[str, Any]: """Repair a state.db whose ``sqlite_master`` schema is malformed or whose FTS indexes reject writes. @@ -2341,6 +2450,23 @@ def repair_state_db_schema(db_path: Path, *, backup: bool = True) -> Dict[str, A "schema surgery to avoid racing it" ) return report + + # The cross-process lock serialises repairers against each other; it + # says nothing about the gateway, Desktop or a CLI still holding the + # database open. Rewriting b-tree pages under a concurrent writer is + # what spread the 2026-08-18/19 damage out of the FTS shadow tables + # and into the canonical ones. The caller closes only its own + # connection — the incident process held seven descriptors on + # state.db — so probe for the rest before touching anything. + if _live_writer_holds_db(db_path): + report["error"] = ( + "a live writer still holds state.db; skipped schema surgery " + "to avoid tearing b-tree pages under a concurrent writer. " + "Stop the gateway (hermes gateway stop) and retry." + ) + logger.error("state.db repair skipped: %s", report["error"]) + return report + result = _repair_state_db_schema_locked(db_path, backup=backup, report=report) # Persist the outcome AFTER surgery, keyed on the post-attempt # fingerprint — that is the file state the NEXT attempt's exhaustion @@ -2391,7 +2517,7 @@ def _repair_state_db_schema_locked( # content table. This is the recommended, least-destructive recovery for a # corrupt FTS index that rejects message writes while reads still succeed. try: - conn = sqlite3.connect(str(db_path), isolation_level=None) + conn = _connect_repair_durable(db_path) try: # The cjk index can only be rebuilt with its tokenizer loaded; # best-effort (a tokenizer-less host skips it at the probe below). @@ -2427,8 +2553,11 @@ def _repair_state_db_schema_locked( # rows using the existing index definition, fixing the mismatch without # touching data or FTS schema. try: - conn = sqlite3.connect(str(db_path), isolation_level=None) + conn = _connect_repair_durable(db_path) try: + # REINDEX rewrites every index b-tree; take the barriers now that + # the schema parses, in case the open-time attempt was refused. + _reapply_durability_barriers(conn) conn.execute("REINDEX") conn.commit() finally: @@ -2445,7 +2574,7 @@ def _repair_state_db_schema_locked( # ── Strategy 1: de-duplicate sqlite_master (keeps FTS index) ── try: - conn = sqlite3.connect(str(db_path), isolation_level=None) + conn = _connect_repair_durable(db_path) try: conn.execute("PRAGMA writable_schema=ON") dupes = conn.execute( @@ -2477,13 +2606,17 @@ def _repair_state_db_schema_locked( # ── Strategy 2: drop all FTS schema, VACUUM, rebuild on next open ── try: - conn = sqlite3.connect(str(db_path), isolation_level=None) + conn = _connect_repair_durable(db_path) try: conn.execute("PRAGMA writable_schema=ON") conn.execute("DELETE FROM sqlite_master WHERE name LIKE 'messages_fts%'") _bump_schema_cookie(conn) conn.execute("PRAGMA writable_schema=OFF") conn.commit() + # The schema is repaired and parseable now, so the barriers can + # finally stick — and VACUUM, which rewrites the entire file, is + # the single most damaging operation to lose halfway. + _reapply_durability_barriers(conn) conn.execute("VACUUM") finally: conn.close() diff --git a/tests/test_state_db_repair_live_writer_guard.py b/tests/test_state_db_repair_live_writer_guard.py new file mode 100644 index 0000000000000..8288cb322bd7d --- /dev/null +++ b/tests/test_state_db_repair_live_writer_guard.py @@ -0,0 +1,93 @@ +"""Regression: the state.db repair path must never run surgery against a +database another connection is still writing. + +Incident (2026-08-18/19): FTS5 shadow-table corruption escalated into b-tree +page damage across `system_prompts`, `session_model_usage` and the `sessions` +index. `repair_state_db_schema` ran its REINDEX/FTS-rebuild strategies while +other connections still held the database open. The caller closes only its own +`self._conn`; the incident process held seven descriptors on state.db. +Rewriting b-tree pages under concurrent writers is what spread the damage out +of the FTS shadow tables and into the canonical tables. + +(The companion repair-attempt-ledger fingerprint fix — keying the budget on +something stable across ongoing writes so the cap can actually be reached — is +tracked separately in the fingerprint/repair-loop salvage PR #88425, which +preserves @jirathip-k's #88224 diagnosis and credit. This file covers only the +live-writer guard.) +""" + +from __future__ import annotations + +import sqlite3 +import uuid +from pathlib import Path + +import pytest + +from hermes_state import ( + SessionDB, + repair_state_db_schema, +) + + +def _make_wal_db(tmp_path: Path) -> Path: + """A state.db the repair path will actually work on. + + Built through the real ``SessionDB`` rather than a hand-rolled two-table + schema. The repair path probes the canonical schema as it goes — + ``_db_opens_cleanly`` runs ``SELECT COUNT(*) FROM sessions`` and a + rolled-back ``messages`` write — so a toy schema aborted every repair + ("no such table: sessions", then "table sessions has no column named id") + long before reaching the guards these tests exist to cover. The + assertions below were passing over a code path that never ran. + """ + 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() + return db + + +# --------------------------------------------------------------------------- +# Repair must refuse to operate under a live writer +# --------------------------------------------------------------------------- + + +@pytest.mark.requires_wal +def test_repair_refuses_while_another_connection_holds_the_db(tmp_path): + """Surgery under concurrent writers is what spread the corruption. + + Gated on ``requires_wal``: ``_live_writer_holds_db`` detects an + out-of-process holder via ``PRAGMA locking_mode=EXCLUSIVE`` + a + ``BEGIN IMMEDIATE`` that a concurrent connection makes fail with + SQLITE_BUSY through the WAL index. On SQLite builds carrying the + WAL-reset bug (and on NFS/SMB) Hermes deliberately runs ``state.db`` in + ``journal_mode=DELETE``, where a held reader takes only a SHARED lock and + ``BEGIN IMMEDIATE`` can still acquire RESERVED — so the probe cannot see + the holder and the guard fails open. In DELETE mode repair is instead + serialised only by the cross-process repairer lock (see + ``_live_writer_holds_db``'s docstring). The conftest auto-skips this test + where WAL is unusable rather than assert a guarantee the runtime doesn't + make there. + """ + db = _make_wal_db(tmp_path) + + holder = sqlite3.connect(str(db)) + holder.execute("SELECT count(*) FROM messages").fetchone() + try: + report = repair_state_db_schema(db, backup=False) + finally: + holder.close() + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + + +def test_repair_proceeds_once_the_database_is_quiescent(tmp_path): + """The guard must not deadlock repair on an exclusively-held file.""" + db = _make_wal_db(tmp_path) + + report = repair_state_db_schema(db, backup=False) + + assert "live writer" not in (report["error"] or "").lower() diff --git a/tests/test_state_db_write_durability.py b/tests/test_state_db_write_durability.py new file mode 100644 index 0000000000000..eeac5e3e56b74 --- /dev/null +++ b/tests/test_state_db_write_durability.py @@ -0,0 +1,147 @@ +"""Regression: state.db repair-path writes must be durable on macOS. + +Incident (2026-08-19, recurrence of 2026-08-18/19): `state.db` was recovered +clean at 01:02, tore again in the pages holding rows written 02:18-02:22, and +the damage went undetected until 13:36 when a write finally landed on a +damaged page (`append_message failed: constraint failed`). `PRAGMA +integrity_check` on the file reported the torn-b-tree signature: + + Tree 5 page 47256 cell 423..429: 2nd reference to page ... + Tree 5 page 60788 cell 4: Rowid 34637 out of order + Page 50549..52587: never used + +The defect: hermes_state already knows macOS `fsync()` does not guarantee +write ordering, and mitigates it with `synchronous=FULL` + +`checkpoint_fullfsync=1` (see `_enforce_macos_synchronous_full`, whose +docstring names this exact failure: "a WAL checkpoint race with process +termination ... can leave the main DB with half-written btree pages"). +Those pragmas are per-connection and were applied only via +`apply_wal_with_fallback()`. The repair path opened `state.db` with a bare +`sqlite3.connect()` five times and then ran REINDEX, VACUUM and +`writable_schema` surgery through it — the operations that rewrite nearly +every page of the file — with no barrier at all. + +(The proactive `verify_state_db_integrity()` gate the original PR #90747 also +carried is deferred to the follow-up that wires it into gateway startup — +PR #91754 — since it ships as dead code without that caller. This file covers +only the repair-connection durability half.) +""" + +from __future__ import annotations + +import re +import sqlite3 +import sys +from pathlib import Path + +import pytest + +import hermes_state +from hermes_state import ( + _connect_repair_durable, + repair_state_db_schema, +) + + +def _make_db(tmp_path: Path) -> Path: + db = tmp_path / "state.db" + conn = sqlite3.connect(str(db)) + conn.execute("PRAGMA journal_mode=WAL") + conn.execute("CREATE TABLE sessions (session_id TEXT PRIMARY KEY)") + conn.execute("CREATE TABLE messages (id INTEGER PRIMARY KEY, body TEXT)") + conn.execute("INSERT INTO messages (body) VALUES ('seed')") + conn.commit() + conn.close() + return db + + +# ── Defect 1: repair-path write durability ────────────────────────────── + + +def test_connect_repair_durable_sets_macos_barriers(tmp_path: Path) -> None: + """The repair connection must carry both macOS durability barriers.""" + db = _make_db(tmp_path) + conn = _connect_repair_durable(db) + try: + synchronous = conn.execute("PRAGMA synchronous").fetchone()[0] + checkpoint_fullfsync = conn.execute( + "PRAGMA checkpoint_fullfsync" + ).fetchone()[0] + finally: + conn.close() + + if sys.platform == "darwin": + # SQLite: 0=OFF, 1=NORMAL, 2=FULL, 3=EXTRA. NORMAL is what tore the + # b-tree pages; FULL is what _enforce_macos_synchronous_full sets. + assert synchronous == 2, ( + f"repair connection opened with synchronous={synchronous}; on " + "Darwin this lets REINDEX/VACUUM leave half-written b-tree pages" + ) + assert checkpoint_fullfsync == 1, ( + "repair connection has no F_FULLFSYNC barrier at checkpoint " + "boundaries; macOS fsync() does not flush the drive cache" + ) + else: + # Elsewhere the helper is a plain connect — no behaviour change. + assert synchronous in (0, 1, 2, 3) + + +def test_connect_repair_durable_is_autocommit(tmp_path: Path) -> None: + """Must preserve isolation_level=None — repair runs DDL and VACUUM.""" + db = _make_db(tmp_path) + conn = _connect_repair_durable(db) + try: + assert conn.isolation_level is None + # VACUUM is only legal outside an implicit transaction. + conn.execute("VACUUM") + finally: + conn.close() + + +def test_repair_path_has_no_bare_connects() -> None: + """No repair/probe site may bypass the durability helper. + + Source-level guard: the bare form is exactly what regressed, and a unit + test on the helper alone would not notice a sixth site being added. + """ + source = Path(hermes_state.__file__).read_text() + pattern = r"^\s*conn = sqlite3\.connect\(str\(db_path\), isolation_level=None\)" + + # The one legitimate bare connect is inside the helper itself; everything + # after that definition must go through it. + helper = source.index("def _connect_repair_durable(") + body_end = source.index("\ndef ", helper + 1) + inside_helper = re.findall(pattern, source[helper:body_end], flags=re.MULTILINE) + assert len(inside_helper) == 1, ( + "_connect_repair_durable no longer opens the connection itself" + ) + + elsewhere = re.findall( + pattern, source[:helper] + source[body_end:], flags=re.MULTILINE + ) + assert elsewhere == [], ( + f"{len(elsewhere)} repair-path connection(s) still bypass " + "_connect_repair_durable() and write state.db without the macOS " + "fsync barriers" + ) + + +def test_repair_still_works_through_durable_connection(tmp_path: Path) -> None: + """Routing every strategy through the helper must not break the path. + + The helper is entered once per strategy, so a plumbing fault (recursion, + a leaked connection, a refused pragma) surfaces as an exception rather + than a report. Whether this fixture's minimal schema is *repairable* is + beside the point — the assertion is that the path runs to completion. + """ + db = _make_db(tmp_path) + report = repair_state_db_schema(db, backup=False) + assert isinstance(report, dict) + assert set(report) >= {"repaired", "strategy", "backup_path"} + # The file must still open afterwards — repair may fail, but it must not + # leave the database less usable than it found it. + conn = sqlite3.connect(str(db)) + try: + assert conn.execute("SELECT COUNT(*) FROM messages").fetchone()[0] == 1 + finally: + conn.close()