From 75e155ab09b9e3fefd2856d199083b6dcbc270a9 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 13 Sep 2026 19:58:58 -0700 Subject: [PATCH] fix(state): a live writer's WAL generation survives lock cancellation and sibling closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQLite protects a WAL generation with per-PROCESS POSIX locks (SHARED on state.db, DMS byte on -shm). Any in-process open()/close() of either file cancels both (sqlite.org/howtocorrupt.html §2.2); the next last-connection close in ANY process then checkpoints and unlinks -wal/-shm, and the holder sticky-halts with DeletedWalGenerationError. #109841 removed one such close (mode tightening) but the class is open-ended: raw header probes, plugins, tool reads of ~/.hermes, any library that touches the files. hermes_state_lockguard re-holds the same two ranges as OFD locks (F_OFD_SETLK) on private descriptors for as long as a writer handle is open. OFD locks belong to the open file description, so a stray close() cannot cancel them, and they conflict with the EXCLUSIVE a sibling needs for the close-time reset exactly like SQLite's own. Released before the handle's own close so a true last close still ends the generation; the descriptors are closed only once no connection to the path remains, so a holder scan from another process never counts them. Works on Python 3.11 (where sqlite3 cannot arm SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE) and on macOS (F_OFD_SETLK=90 per XNU bsd/sys/fcntl.h); no-op on Windows. Live repro (Linux, Python 3.11.15, SQLite 3.53.1): holder = SessionDB writer; in-process os.open/os.close of state.db and -shm; then a foreign sqlite3.connect()+close(). Before: -wal unlinked, holder write raises DeletedWalGenerationError. After: -wal keeps its inode, holder writes. --- hermes_state.py | 23 +++ hermes_state_dbfile.py | 4 + hermes_state_lockguard.py | 220 ++++++++++++++++++++++ tests/hermes_state/test_wal_lock_guard.py | 45 +++++ 4 files changed, 292 insertions(+) create mode 100644 hermes_state_lockguard.py create mode 100644 tests/hermes_state/test_wal_lock_guard.py diff --git a/hermes_state.py b/hermes_state.py index 5d5cdcc6c6..17c43e7018 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -45,6 +45,7 @@ from hermes_state_portability import SessionPortabilityMixin from hermes_state_telegram import SessionTelegramTopicsMixin from hermes_state_schema import SessionSchemaMixin import hermes_state_holders as _state_holders +import hermes_state_lockguard as _lockguard from hermes_state_dbfile import ( _canonical_sqlite_path, _connect_tracked_db, _fd_is_truly_unlinked, _prepare_connection_retirement, _read_sqlite_application_id, _stat_sqlite_sidecar_identity, @@ -521,6 +522,7 @@ class SessionDB( self._retired_capture_lock = threading.Lock() self._retire_connection: Optional[Callable[[Any], None]] = None self._connection_pinned = False # one unmatched C reference taken at most once per handle + self._wal_lock_guard_held = False # hermes_state_lockguard.hold() taken by _open_writer self._db_corrupt, self._db_corrupt_reason = False, "" # sticky quarantine (StateDbCorruptError) self._fts_usermerge_floor_applied = False # one-shot usermerge-floor write guard self._fts_enabled = self._fts_stale = self._trigram_available = False @@ -599,6 +601,12 @@ class SessionDB( self._connect_and_init_with_lock_patience() # FTS optimization is OPT-IN (`hermes db optimize`); no background worker races session lifecycle. self._ensure_db_file_generation() + if self._wal_active: + # Independent copies of the two POSIX locks that keep a sibling's close from unlinking + # this WAL generation: any in-process open()/close() of state.db or -shm cancels SQLite's + # own (howtocorrupt §2.2), these survive it. Released in close(). + _lockguard.hold(self.db_path) + self._wal_lock_guard_held = True def _open_read_only(self) -> None: """Read-only attach for cross-profile aggregation: no schema init, NO write @@ -1103,8 +1111,11 @@ class SessionDB( return False if sys.platform.startswith("linux"): watched = _watched_sqlite_sidecar_paths(self.db_path) + guard_fds = _lockguard.owned_fds() try: for target, fd_path in _proc_fd_targets(os.getpid()): + if int(fd_path.rsplit("/", 1)[1]) in guard_fds: + continue # the lock guard's own descriptor (see hermes_state_lockguard) canonical = _canonical_sqlite_path(target) if (" (deleted)" in target and canonical in watched and _fd_is_truly_unlinked(fd_path, watched[canonical])): @@ -1323,6 +1334,8 @@ class SessionDB( """ if self._quarantine_reason() is not None: return + if self._wal_lock_guard_held: + _lockguard.refresh(self.db_path) # -shm minted after open, or path re-pointed try: with self._lock: result = self._conn.execute("PRAGMA wal_checkpoint(PASSIVE)").fetchone() @@ -1401,6 +1414,10 @@ class SessionDB( self._conn.execute("PRAGMA wal_checkpoint(PASSIVE)") except Exception as exc: logger.debug("WAL checkpoint (PASSIVE) at close failed: %s", exc) + # Release the guard first: SQLite's close-time reset then sees only real holders + # (a sibling process's own intact locks still refuse the unlink; a true last close + # ends the generation, so a later state.db replace never pairs with a stale WAL). + self._release_wal_lock_guard() if retire_without_close: self._pin_connection(self._conn) self._conn = None @@ -1410,6 +1427,12 @@ class SessionDB( # Only a clean close ends the generation; retain the recorded # identity when retiring an unsafe handle. self._db_sidecar_identity = {} + _lockguard.retire_idle(self.db_path) + + def _release_wal_lock_guard(self) -> None: + if self._wal_lock_guard_held: + self._wal_lock_guard_held = False + _lockguard.release(self.db_path) def __del__(self) -> None: """Safety net: close() if the caller forgot. Attribute access stays diff --git a/hermes_state_dbfile.py b/hermes_state_dbfile.py index a65c8f900e..2c2302e559 100644 --- a/hermes_state_dbfile.py +++ b/hermes_state_dbfile.py @@ -194,7 +194,11 @@ def iter_deleted_sqlite_sidecar_holders(db_path) -> List[Tuple[int, str]]: holders: List[Tuple[int, str]] = [] watched = _watched_sqlite_sidecar_paths(db_path) try: + from hermes_state_lockguard import owned_fds + own_pid, guard_fds = os.getpid(), owned_fds() for pid, target, fd_path in _iter_proc_fd_targets(): + if pid == own_pid and int(fd_path.rsplit("/", 1)[1]) in guard_fds: + continue # our lock guard's descriptor, not a connection on a dead generation canonical = _canonical_sqlite_path(target) if (" (deleted)" in target and canonical in watched and _fd_is_truly_unlinked(fd_path, watched[canonical])): diff --git a/hermes_state_lockguard.py b/hermes_state_lockguard.py new file mode 100644 index 0000000000..e41256bf57 --- /dev/null +++ b/hermes_state_lockguard.py @@ -0,0 +1,220 @@ +"""Hold a state.db writer's WAL-mode file locks on descriptors SQLite does not own. + +SQLite protects a live WAL generation with two POSIX advisory locks: a SHARED lock on the main +file's lock range and a shared lock on the DMS byte of ``state.db-shm``. A sibling process may +checkpoint and unlink ``-wal``/``-shm`` at its close only after taking both EXCLUSIVE. POSIX locks +are per process, so any ``open()``/``close()`` of those two files inside the holder — a raw probe, +a plugin, a stray ``head -c`` in-process — cancels both (sqlite.org/howtocorrupt.html §2.2) and +the next foreign close strands the holder on a deleted generation (``DeletedWalGenerationError``). + +This module re-holds the same two ranges as *open file description* locks (``F_OFD_SETLK``) on +private descriptors that are never closed. OFD locks belong to the description, not the process: +a stray ``close()`` elsewhere cannot cancel them, and releasing them with ``F_UNLCK`` never +disturbs SQLite's own locks. Both lock types conflict with a foreign EXCLUSIVE, so the sibling's +close-time unlink is refused for as long as a writer handle is open here. The same refusal applies +to THIS process's close: the last writer no longer deletes the sidecars, which is the +``SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE`` behaviour on runtimes whose ``sqlite3`` cannot arm it. + +One guard per database path per process, refcounted across writer handles; descriptors are +retired (never closed) when the path is re-pointed at a new inode. No-op on Windows and on +runtimes without OFD locks. +""" + +from __future__ import annotations + +import logging +import os +import struct +import sys +import threading +from pathlib import Path +from typing import Dict, List, Optional + +logger = logging.getLogger("hermes_state") + +# SQLite's unix VFS lock geometry (os_unix.c): the SHARED range on the main file and the +# deadman-switch byte of the -shm file. +_PENDING_BYTE = 0x40000000 +_SHARED_FIRST = _PENDING_BYTE + 2 +_SHARED_SIZE = 510 +_SHM_DMS_BYTE = 128 + +try: + import fcntl + # CPython exports F_OFD_SETLK only from 3.12. The kernel ABI values are stable: 37 on every + # Linux arch (asm-generic/fcntl.h), 90 on XNU (bsd/sys/fcntl.h, documented in fcntl(2)). + _F_OFD_SETLK: Optional[int] = getattr( + fcntl, "F_OFD_SETLK", {"linux": 37, "darwin": 90}.get(sys.platform.rstrip("0123456789"))) + _F_RDLCK, _F_UNLCK, _SEEK_SET = fcntl.F_RDLCK, fcntl.F_UNLCK, os.SEEK_SET +except ImportError: # Windows + fcntl = None # type: ignore[assignment] + _F_OFD_SETLK = None + _F_RDLCK = _F_UNLCK = _SEEK_SET = 0 + +# struct flock differs per libc: glibc/musl put type+whence first, Darwin/BSD last. +_FLOCK_FORMAT = "@qqihh" if sys.platform == "darwin" or "bsd" in sys.platform else "@hhqqi" + + +def _flock(lock_type: int, start: int, length: int) -> bytes: + if _FLOCK_FORMAT == "@qqihh": + return struct.pack(_FLOCK_FORMAT, start, length, 0, lock_type, _SEEK_SET) + return struct.pack(_FLOCK_FORMAT, lock_type, _SEEK_SET, start, length, 0) + + +def _ofd_lock(fd: int, lock_type: int, start: int, length: int) -> bool: + """Apply a non-blocking OFD lock; False when the range is held EXCLUSIVE elsewhere.""" + assert fcntl is not None and _F_OFD_SETLK is not None + try: + fcntl.fcntl(fd, _F_OFD_SETLK, _flock(lock_type, start, length)) + except BlockingIOError: + return False + return True + + +class _PathGuard: + __slots__ = ("main_fd", "main_ident", "shm_fd", "shm_ident", "refs", "main_locked", "shm_locked") + + def __init__(self) -> None: + self.main_fd = self.shm_fd = -1 + self.main_ident = self.shm_ident = None + self.refs = 0 + self.main_locked = self.shm_locked = False + + +_LOCK = threading.Lock() +_GUARDS: Dict[str, _PathGuard] = {} +_RETIRED_FDS: List[int] = [] # descriptors for re-pointed paths; closing one would cancel SQLite's locks + + +def supported() -> bool: + return _F_OFD_SETLK is not None + + +def _bind_fd(path: str, fd: int, ident) -> tuple: + """Return ``(fd, ident)`` for *path*, reusing *fd* while it still names the path's inode.""" + try: + st = os.stat(path) + except OSError: + return fd, ident + current = (st.st_dev, st.st_ino) + if fd >= 0 and ident == current: + return fd, ident + if fd >= 0: + _RETIRED_FDS.append(fd) + try: + fd = os.open(path, os.O_RDONLY | getattr(os, "O_CLOEXEC", 0)) + except OSError: + return -1, None + return fd, current + + +def _apply_locked(guard: _PathGuard, db_path: str) -> None: + guard.main_fd, guard.main_ident = _bind_fd(db_path, guard.main_fd, guard.main_ident) + if guard.main_fd >= 0: + guard.main_locked = _ofd_lock(guard.main_fd, _F_RDLCK, _SHARED_FIRST, _SHARED_SIZE) + guard.shm_fd, guard.shm_ident = _bind_fd(db_path + "-shm", guard.shm_fd, guard.shm_ident) + if guard.shm_fd >= 0: + guard.shm_locked = _ofd_lock(guard.shm_fd, _F_RDLCK, _SHM_DMS_BYTE, 1) + + +def hold(db_path: Path) -> None: + """Take (or add a reference to) the guard for *db_path*. Call once per writer handle after + its connection is open in WAL mode; pair with :func:`release`.""" + if not supported(): + return + key = os.fspath(db_path) + with _LOCK: + guard = _GUARDS.setdefault(key, _PathGuard()) + guard.refs += 1 + try: + _apply_locked(guard, key) + except OSError: + logger.debug("WAL lock guard unavailable for %s", key, exc_info=True) + + +def refresh(db_path: Path) -> None: + """Re-arm a held guard: a ``-shm`` that did not exist at :func:`hold` time, or a path re-pointed + at a new inode since. Cheap when everything is in place (one ``stat`` per file).""" + if not supported(): + return + key = os.fspath(db_path) + with _LOCK: + guard = _GUARDS.get(key) + if guard is None or guard.refs <= 0: + return + try: + _apply_locked(guard, key) + except OSError: + logger.debug("WAL lock guard refresh failed for %s", key, exc_info=True) + + +def release(db_path: Path) -> None: + """Drop one reference; the last one unlocks both ranges. Call BEFORE closing the handle's own + connection so SQLite's close-time reset sees only real holders (a sibling process's intact + locks still refuse the unlink; a true last close ends the generation normally, so a later + ``state.db`` replace never pairs with a stale WAL). Descriptors are closed by + :func:`retire_idle` once no connection to the path remains.""" + if not supported(): + return + key = os.fspath(db_path) + with _LOCK: + guard = _GUARDS.get(key) + if guard is None or guard.refs <= 0: + return + guard.refs -= 1 + if guard.refs: + return + for fd, start, length in ((guard.main_fd, _SHARED_FIRST, _SHARED_SIZE), (guard.shm_fd, _SHM_DMS_BYTE, 1)): + if fd >= 0: + try: + _ofd_lock(fd, _F_UNLCK, start, length) + except OSError: + logger.debug("WAL lock guard unlock failed for %s", key, exc_info=True) + guard.main_locked = guard.shm_locked = False + + +def retire_idle(db_path: Path) -> None: + """Close the guard descriptors once no tracked SQLite connection to *db_path* remains in this + process. Closing then cancels nothing, and a lingering fd on the path would make another + process's holder scan (``hermes doctor`` repair, snapshot restore) count this one as live. + While any connection is still open the descriptors stay put: closing would cancel its locks.""" + if not supported(): + return + key = os.fspath(db_path) + with _LOCK: + guard = _GUARDS.get(key) + if guard is None or guard.refs or _path_has_live_connection(key): + return + del _GUARDS[key] + for fd in (guard.main_fd, guard.shm_fd): + if fd >= 0: + try: + os.close(fd) + except OSError: + pass + + +def _path_has_live_connection(key: str) -> bool: + try: + from hermes_cli.sqlite_safe_read import has_live_connection + except ImportError: + return True # cannot prove quiescence: keep the descriptors + return has_live_connection(key) + + +def held(db_path: Path) -> bool: + """Both ranges currently guarded for *db_path* (diagnostics and tests).""" + with _LOCK: + guard = _GUARDS.get(os.fspath(db_path)) + return bool(guard and guard.refs and guard.main_locked and guard.shm_locked) + + +def owned_fds() -> frozenset: + """Every descriptor this module keeps open (active and retired). Deleted-sidecar holder scans + must skip these: a retired guard fd on an unlinked ``-shm`` is not a SQLite connection reading + a dead generation, and reporting it would refuse every later open in this process.""" + with _LOCK: + fds = set(_RETIRED_FDS) + for guard in _GUARDS.values(): + fds.update(fd for fd in (guard.main_fd, guard.shm_fd) if fd >= 0) + return frozenset(fds) diff --git a/tests/hermes_state/test_wal_lock_guard.py b/tests/hermes_state/test_wal_lock_guard.py new file mode 100644 index 0000000000..f77b176cfd --- /dev/null +++ b/tests/hermes_state/test_wal_lock_guard.py @@ -0,0 +1,45 @@ +"""A live writer's WAL generation survives lock cancellation and sibling closes. + +SQLite guards a WAL generation with per-PROCESS POSIX locks, so any raw ``open()``/``close()`` of +``state.db`` or ``-shm`` inside the holder (howtocorrupt.html §2.2) silently drops them; the next +last-connection close anywhere then checkpoints and unlinks ``-wal``/``-shm`` and the holder +sticky-halts with ``DeletedWalGenerationError`` (#109727, #110042, #110276, Desktop "chat fails +after update"). ``hermes_state_lockguard`` re-holds the same ranges as OFD locks that a stray +close cannot cancel. Linux-only: the assertions read kernel truth from ``/proc``. +""" + +import os +import sqlite3 +import subprocess +import sys +from pathlib import Path + +import pytest + +from hermes_state import SessionDB +from tests.hermes_state._wal_generation_harness import make_db, pin_wal, require_wal + +pytestmark = pytest.mark.linux_only + + +def _foreign_open_close(db_path: Path) -> None: + """Another process opens state.db, reads, closes: SQLite's last-connection WAL reset runs there.""" + subprocess.run([sys.executable, "-c", + f"import sqlite3; c = sqlite3.connect({str(db_path)!r}); " + "c.execute('select count(*) from messages').fetchone(); c.close()"], check=True) + + +def test_stray_in_process_close_does_not_let_a_sibling_unlink_the_wal(tmp_path, monkeypatch): + pin_wal(monkeypatch) + db = make_db(tmp_path / "state.db", "s", "seed") + wal = require_wal(db) + wal_inode = wal.stat().st_ino + try: + for name in ("state.db", "state.db-shm"): # the §2.2 bug, e.g. a raw header probe + os.close(os.open(tmp_path / name, os.O_RDONLY)) + _foreign_open_close(db.db_path) + assert wal.exists() and wal.stat().st_ino == wal_inode, "sibling close unlinked the live WAL" + db.append_message("s", role="user", content="after") # would raise DeletedWalGenerationError + finally: + db.close() + assert sqlite3.connect(db.db_path).execute("SELECT COUNT(*) FROM messages").fetchone()[0] == 2