fix(state): a live writer's WAL generation survives lock cancellation and sibling closes
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.
This commit is contained in:
teknium1
@@ -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
|
||||
|
||||
@@ -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])):
|
||||
|
||||
220
hermes_state_lockguard.py
Normal file
220
hermes_state_lockguard.py
Normal file
@@ -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)
|
||||
45
tests/hermes_state/test_wal_lock_guard.py
Normal file
45
tests/hermes_state/test_wal_lock_guard.py
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user