fix(state): apply macOS write barriers on every state.db repair connection

contributors/emails/dhanesh@users.noreply.github.com

@@ -0,0 +1,2 @@
+dhanesh
+# PR #90747 salvage (state.db repair durability) via #91852

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()

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()