diff --git a/contributors/emails/296402666+ciabata-git@users.noreply.github.com b/contributors/emails/296402666+ciabata-git@users.noreply.github.com new file mode 100644 index 0000000000..317632f3c5 --- /dev/null +++ b/contributors/emails/296402666+ciabata-git@users.noreply.github.com @@ -0,0 +1,2 @@ +ciabata-git +# PR #96011 author email preserved by PR #97330 diff --git a/contributors/emails/benjaminperry6@yahoo.fr b/contributors/emails/benjaminperry6@yahoo.fr new file mode 100644 index 0000000000..16b94399dd --- /dev/null +++ b/contributors/emails/benjaminperry6@yahoo.fr @@ -0,0 +1,2 @@ +benperry6 +# PR #97330 author email diff --git a/hermes_state.py b/hermes_state.py index e374fea4e0..38a969083e 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -60,6 +60,7 @@ from hermes_cli.sqlite_runtime import ( ) from typing import Any, Callable, Dict, Iterator, List, Optional, Set, Tuple, TypeVar, cast +import hermes_state_holders as _state_holders from hermes_state_common import ( # noqa: F401 (re-exported for back-compat) _BRANCH_CHILD_SQL, _COMPRESSION_CHILD_SQL, @@ -3595,58 +3596,17 @@ def _db_opens_cleanly(db_path: Path) -> Optional[str]: conn.close() +def _foreign_state_db_holders(db_path: Path) -> List[Tuple[int, str]]: + """Compatibility delegate to the state-holder authority.""" + return _state_holders.foreign_state_db_holders(db_path) + + 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 = _connect_repair_durable(db_path, timeout=0.0) - 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 + """Compatibility delegate to the repair-admission authority.""" + return _state_holders.live_writer_holds_db( + db_path, + connect_repair_durable=_connect_repair_durable, + ) def repair_state_db_schema(db_path: Path, *, backup: bool = True) -> Dict[str, Any]: @@ -5114,35 +5074,8 @@ def _concrete_state_db_holder_pids( return pids -def _read_proc_cmdline(pid: int) -> Optional[str]: - """Read /proc//cmdline, world-readable even when fd table is not. - - Returns the cmdline as a space-joined string, or None when unreadable - (process exited, or hidepid mount). - """ - try: - with open(f"/proc/{pid}/cmdline", "rb") as f: - raw = f.read() - if not raw: - return None - return raw.replace(b"\x00", b" ").decode("utf-8", "replace").strip() - except OSError: - return None - - -_HERMES_CMDLINE_MARKERS = ("hermes_cli.main", "hermes_cli/main", "hermes serve", - "hermes-agent", "hermes gateway", "hermes chat") - - -def _looks_like_hermes(cmdline: str) -> bool: - """Heuristic: does this cmdline look like a Hermes process? - - Used to decide whether an uninspectable process (fd table unreadable - due to different user) should be treated as a potential state.db holder. - We only flag processes that look like Hermes, not every system daemon. - """ - lower = cmdline.lower() - return any(marker in lower for marker in _HERMES_CMDLINE_MARKERS) +_read_proc_argv = _state_holders._read_proc_argv +_looks_like_hermes = _state_holders._looks_like_hermes # Lifecycle statuses surfaced by session pickers. Classification looks ONLY at @@ -6720,104 +6653,8 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) return msg.startswith("fts5:") and "corrupt structure" in msg def _foreign_state_db_holders(self) -> List[Tuple[int, str]]: - """Return foreign processes holding this DB or its WAL sidecars. - - Automatic FTS repair is structural maintenance, not an ordinary WAL - write. It must not run while another process remains attached: a - sidecar reset under that holder can leave the two processes writing - through different WAL inodes. - - A scan failure is represented as an unknown holder. Skipping optional - automatic maintenance is safer than assuming quiescence; canonical - writes continue through the stale-FTS fail-open path. - """ - # The split-brain mechanism requires POSIX unlink semantics: Windows - # refuses to replace SQLite sidecars while another process has them - # open. Avoid psutil.open_files() there; querying arbitrary Windows - # processes can block for minutes on device-backed handles. - if _IS_WINDOWS: - return [] - if psutil is None: - return [(-1, "open-file scan unavailable")] - - db_path = os.path.abspath(os.fspath(self.db_path)) - watched = { - _canonical_sqlite_path(db_path), - _canonical_sqlite_path(db_path + "-wal"), - _canonical_sqlite_path(db_path + "-shm"), - } - holders: List[Tuple[int, str]] = [] - - # On Linux, read /proc//fd symlinks directly. psutil's - # open_files() filters through isfile_strict(), which stats the - # literal path — for an unlinked WAL sidecar the kernel returns - # "/path/state.db-wal (deleted)" and stat fails, so the entry is - # silently dropped and the split-brain holder is never seen. - # /proc readlinks preserve the "(deleted)" suffix so _canonical can - # strip it and match. - if sys.platform.startswith("linux"): - try: - own_pid = os.getpid() - for pid_str in os.listdir("/proc"): - if not pid_str.isdigit(): - continue - pid = int(pid_str) - if pid == own_pid: - continue - fd_dir = f"/proc/{pid}/fd" - try: - fds = os.listdir(fd_dir) - except OSError: - # Cannot read this process's fd table (different - # user, e.g. root gateway vs user desktop). - # /proc//cmdline is world-readable by default, - # so check whether this is a Hermes process — - # only flag uninspectable holders that look like - # another Hermes instance, not every system daemon. - cmdline = _read_proc_cmdline(pid) - if cmdline is not None and _looks_like_hermes(cmdline): - holders.append((pid, f"uninspectable holder: {cmdline[:80]}")) - continue - for fd in fds: - try: - target = os.readlink(f"{fd_dir}/{fd}") - except OSError: - continue - if _canonical_sqlite_path(target) in watched: - holders.append((pid, target)) - except Exception as exc: - logger.warning( - "Could not prove state.db has no foreign holders; " - "deferring automatic FTS maintenance: %s", - exc, - ) - return holders or [(-1, f"open-file scan failed: {exc}")] - return holders - - # macOS / BSD: use psutil.open_files(). macOS does not use the - # "(deleted)" suffix convention, so psutil's filtering is safe here. - try: - for process in psutil.process_iter(["pid", "open_files"]): - info = process.info - pid = int(info["pid"]) - if pid == os.getpid(): - continue - # psutil's as_dict() converts AccessDenied to None, which - # or-() turns into an empty iteration. On macOS this is - # acceptable: the gateway/desktop topology from the issue is - # Linux-specific (systemd units running as root). - for opened in info.get("open_files") or (): - path = getattr(opened, "path", "") - if path and _canonical_sqlite_path(path) in watched: - holders.append((pid, path)) - except Exception as exc: - logger.warning( - "Could not prove state.db has no foreign holders; " - "deferring automatic FTS maintenance: %s", - exc, - ) - return holders or [(-1, f"open-file scan failed: {exc}")] - return holders + """Return foreign processes holding this DB or its WAL sidecars.""" + return _foreign_state_db_holders(self.db_path) def _reap_inactive_orphan_desktop_holders( self, holders: List[Tuple[int, str]], *, min_age_seconds: float diff --git a/hermes_state_holders.py b/hermes_state_holders.py new file mode 100644 index 0000000000..5b542c4e4c --- /dev/null +++ b/hermes_state_holders.py @@ -0,0 +1,291 @@ +"""Process and descriptor authority for state.db structural maintenance. + +This module owns the proof that no foreign process still holds the active or +an unlinked SQLite DB/WAL/SHM generation. ``hermes_state`` supplies only the +SQLite connection factory needed by the final lock probe. +""" + +from __future__ import annotations + +import errno +import logging +import os +import sqlite3 +import sys +from pathlib import Path +from typing import Callable, List, Optional, Sequence, Set, Tuple + +try: # Hard dependency, but tolerate scaffold-phase imports before pip install. + import psutil +except ImportError: # pragma: no cover - stripped/scaffold installs only + psutil = None # type: ignore[assignment] + + +logger = logging.getLogger(__name__) + +_IS_WINDOWS = sys.platform == "win32" +_HERMES_EXECUTABLES = frozenset({"hermes", "hermes-agent", "hermes-acp"}) +_HERMES_PYTHON_MODULES = frozenset({"acp_adapter", "hermes_cli.main"}) +_HERMES_PYTHON_SCRIPTS = frozenset({"hermes_cli/main.py", "run_agent.py"}) +_PYTHON_SHORT_OPTIONS_WITH_OPERANDS = frozenset({"Q", "W", "X"}) +_PYTHON_LONG_OPTIONS_WITH_OPERANDS = frozenset( + {"--check-hash-based-pycs", "--jit"} +) + + +def _read_proc_argv(pid: int) -> Optional[List[str]]: + """Read /proc//cmdline without losing argv boundaries.""" + try: + with open(f"/proc/{pid}/cmdline", "rb") as handle: + raw = handle.read() + if not raw: + return None + argv = raw.decode("utf-8", "replace").split("\x00") + if argv[-1] == "": + argv.pop() + return argv or None + except OSError: + return None + + +def _looks_like_python_executable(program: str) -> bool: + name = os.path.basename(program).lower().removesuffix(".exe") + for prefix in ("python", "pypy"): + if name.startswith(prefix): + suffix = name[len(prefix) :] + return not suffix or all(char.isdigit() or char == "." for char in suffix) + return False + + +def _python_execution_target(argv: Sequence[str]) -> Optional[Tuple[str, str]]: + """Return the Python module or script selected by interpreter options.""" + index = 1 + while index < len(argv): + arg = argv[index] + if arg == "--": + index += 1 + return ("script", argv[index]) if index < len(argv) else None + if arg in _PYTHON_LONG_OPTIONS_WITH_OPERANDS: + index += 2 + continue + if arg.startswith("--check-hash-based-pycs=") or arg.startswith("--jit="): + index += 1 + continue + if arg.startswith("--"): + index += 1 + continue + if arg.startswith("-") and arg != "-": + options = arg[1:] + option_index = 0 + consumed_next = False + while option_index < len(options): + option = options[option_index] + attached = options[option_index + 1 :] + if option == "c": + return None + if option == "m": + if attached: + return "module", attached + index += 1 + return ("module", argv[index]) if index < len(argv) else None + if option in _PYTHON_SHORT_OPTIONS_WITH_OPERANDS: + consumed_next = not attached + break + option_index += 1 + index += 2 if consumed_next else 1 + continue + return "script", arg + return None + + +def _looks_like_hermes(argv: Sequence[str]) -> bool: + """Return whether argv identifies a supported Hermes execution target.""" + if not argv: + return False + program = os.path.basename(argv[0]).lower().removesuffix(".exe") + if program in _HERMES_EXECUTABLES: + return True + if not _looks_like_python_executable(program): + return False + target = _python_execution_target(argv) + if target is None: + return False + kind, value = target + normalized = value.lower().replace("\\", "/") + if kind == "module": + return normalized in _HERMES_PYTHON_MODULES + return any( + normalized == script or normalized.endswith(f"/{script}") + for script in _HERMES_PYTHON_SCRIPTS + ) + + +def foreign_state_db_holders(db_path: Path) -> List[Tuple[int, str]]: + """Return foreign holders of the DB or one of its WAL sidecars. + + A scan failure is represented as an unknown holder. Structural maintenance + must not assume quiescence when an old, unlinked SQLite generation may + still be open by another process. + """ + if _IS_WINDOWS: + return [] + + def _canonical(path: str) -> str: + clean = path.removesuffix(" (deleted)") + return os.path.normcase(os.path.abspath(clean)) + + db_path_str = os.path.abspath(os.fspath(db_path)) + watched = { + _canonical(db_path_str), + _canonical(db_path_str + "-wal"), + _canonical(db_path_str + "-shm"), + } + holders: List[Tuple[int, str]] = [] + watched_ids: Set[Tuple[int, int]] = set() + db_dev: Optional[int] = None + for candidate in (db_path_str, db_path_str + "-wal", db_path_str + "-shm"): + try: + stat_result = os.stat(candidate) + except OSError as exc: + if exc.errno not in (errno.ENOENT, errno.ESRCH): + holders.append( + (-1, f"watched-file stat failed: {candidate}: {exc}") + ) + continue + watched_ids.add((stat_result.st_dev, stat_result.st_ino)) + if candidate == db_path_str: + db_dev = stat_result.st_dev + + if sys.platform.startswith("linux"): + try: + own_pid = os.getpid() + for pid_str in os.listdir("/proc"): + if not pid_str.isdigit(): + continue + pid = int(pid_str) + if pid == own_pid: + continue + fd_dir = f"/proc/{pid}/fd" + try: + fds = os.listdir(fd_dir) + except OSError: + argv = _read_proc_argv(pid) + if argv is not None and _looks_like_hermes(argv): + cmdline = " ".join(argv) + holders.append((pid, f"uninspectable holder: {cmdline[:80]}")) + continue + for fd in fds: + fd_path = f"{fd_dir}/{fd}" + try: + target = os.readlink(fd_path) + except OSError as exc: + if exc.errno in (errno.ENOENT, errno.ESRCH): + continue + argv = _read_proc_argv(pid) + if argv is not None and _looks_like_hermes(argv): + holders.append( + ( + pid, + f"uninspectable descriptor: {fd_path}: {exc}", + ) + ) + continue + target_is_watched = _canonical(target) in watched + try: + fd_stat = os.stat(fd_path) + except OSError as exc: + if exc.errno in (errno.ENOENT, errno.ESRCH): + continue + if target_is_watched: + holders.append( + (pid, f"uninspectable descriptor: {target}: {exc}") + ) + else: + argv = _read_proc_argv(pid) + if argv is not None and _looks_like_hermes(argv): + holders.append( + ( + pid, + "uninspectable descriptor: " + f"{target}: {exc}", + ) + ) + continue + if (fd_stat.st_dev, fd_stat.st_ino) in watched_ids or ( + target_is_watched + and target.endswith(" (deleted)") + and db_dev is not None + and fd_stat.st_dev == db_dev + ): + holders.append((pid, target)) + except Exception as exc: + logger.warning( + "Could not prove state.db has no foreign holders; " + "deferring structural maintenance: %s", + exc, + ) + holders.append((-1, f"open-file scan failed: {exc}")) + return holders + + if psutil is None: + return [(-1, "open-file scan unavailable")] + try: + for process in psutil.process_iter(["pid", "open_files"]): + info = process.info + pid = int(info["pid"]) + if pid == os.getpid(): + continue + for opened in info.get("open_files") or (): + path = getattr(opened, "path", "") + if path and _canonical(path) in watched: + holders.append((pid, path)) + except Exception as exc: + logger.warning( + "Could not prove state.db has no foreign holders; " + "deferring structural maintenance: %s", + exc, + ) + holders.append((-1, f"open-file scan failed: {exc}")) + return holders + + +def live_writer_holds_db( + db_path: Path, + *, + 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 True + + probe = None + try: + probe = connect_repair_durable(db_path, timeout=0.0) + 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: + return False + except Exception: + return False + finally: + if probe is not None: + try: + probe.execute("PRAGMA locking_mode=NORMAL") + except Exception: + pass + try: + probe.close() + except Exception: + pass diff --git a/tests/state/test_fts_runtime_rebuild.py b/tests/state/test_fts_runtime_rebuild.py index ac030f4cb6..d6f0a6eeea 100644 --- a/tests/state/test_fts_runtime_rebuild.py +++ b/tests/state/test_fts_runtime_rebuild.py @@ -16,11 +16,11 @@ rebuild later, outside the failed live write/search operation. import json import os import sqlite3 -from types import SimpleNamespace import pytest import hermes_state +import hermes_state_holders import hermes_state_schema from hermes_state import ( FTS_REBUILD_DEFERRAL_KEY, @@ -140,49 +140,55 @@ class TestRuntimeFtsRebuild: } ) - def test_foreign_holder_detection_includes_deleted_wal( - self, db, tmp_path, monkeypatch - ): - db_path = tmp_path / "state.db" + @pytest.mark.parametrize( + "argv", + ( + ("journalctl", "-u", "hermes-agent.service"), + ("grep", "hermes-agent", "/var/log/syslog"), + ( + "/usr/sbin/tailscaled", + "be-child", + "ssh", + "--cmd=python -m hermes_cli.main gateway", + ), + ("tmux", "new-session", "/opt/hermes-agent/.venv/bin/hermes gateway"), + ("python3", "/opt/hermes-agent/tools/check_state.py"), + ("hermes-monitor", "gateway"), + ("hermesctl", "serve"), + ("python3", "worker.py", "hermes_cli.main"), + ("python3", "-m", "other.module", "hermes_cli.main"), + ("python3", "-c", "hermes_cli.main"), + ("python3", "-Icprint('hermes_cli.main')", "hermes_cli/main.py"), + ), + ) + def test_uninspectable_non_hermes_process_is_not_a_holder(self, argv): + assert not hermes_state_holders._looks_like_hermes(argv) - class FakePsutil: - @staticmethod - def process_iter(_attrs): - return iter( - ( - SimpleNamespace( - info={ - "pid": 111, - "open_files": [SimpleNamespace(path=str(db_path))], - } - ), - SimpleNamespace( - info={ - "pid": 222, - "open_files": [ - SimpleNamespace(path=f"{db_path}-wal (deleted)") - ], - } - ), - SimpleNamespace( - info={ - "pid": 333, - "open_files": [SimpleNamespace(path=str(tmp_path / "other.db"))], - } - ), - ) - ) - - monkeypatch.setattr(hermes_state, "psutil", FakePsutil) - monkeypatch.setattr(hermes_state, "_IS_WINDOWS", False) - monkeypatch.setattr(hermes_state.os, "getpid", lambda: 111) - # Force the macOS/psutil path even on Linux test runners - monkeypatch.setattr(hermes_state.sys, "platform", "darwin") - - assert db._foreign_state_db_holders() == [ - (222, f"{db_path}-wal (deleted)") - ] + @pytest.mark.parametrize( + "argv", + ( + ("/usr/local/bin/hermes", "gateway"), + ("/usr/local/bin/hermes-agent", "serve"), + ("/usr/local/bin/hermes-acp", "--stdio"), + ("/usr/bin/python3", "-m", "hermes_cli.main", "gateway"), + ("/usr/bin/python3", "-m", "acp_adapter"), + ("/usr/bin/python3", "-Im", "hermes_cli.main", "gateway"), + ("/usr/bin/python3", "-mhermes_cli.main", "gateway"), + ("/usr/bin/python3", "-W", "ignore", "-m", "hermes_cli.main"), + ("/usr/bin/python3", "-Xdev", "-m", "hermes_cli.main"), + ( + "/opt/hermes-agent/.venv/bin/python", + "/opt/hermes-agent/hermes_cli/main.py", + "gateway", + ), + ("python.exe", "--", "hermes_cli/main.py", "gateway"), + ("python3", "/opt/hermes-agent/run_agent.py", "--query", "hello"), + ), + ) + def test_uninspectable_hermes_process_remains_a_holder(self, argv): + assert hermes_state_holders._looks_like_hermes(argv) + @pytest.mark.linux_only def test_foreign_holder_detection_proc_readlink_deleted_wal( self, db, tmp_path, monkeypatch ): @@ -209,24 +215,93 @@ class TestRuntimeFtsRebuild: other.touch() os.symlink(str(other), str(proc_root / "333" / "fd" / "3")) - monkeypatch.setattr(hermes_state, "_IS_WINDOWS", False) - monkeypatch.setattr(hermes_state.os, "getpid", lambda: 111) - monkeypatch.setattr(hermes_state.sys, "platform", "linux") + monkeypatch.setattr(hermes_state_holders.os, "getpid", lambda: 111) real_listdir = os.listdir def _listdir(path): if isinstance(path, str): path = path.replace("/proc", str(proc_root)) return real_listdir(path) - monkeypatch.setattr(hermes_state.os, "listdir", _listdir) + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) real_readlink = os.readlink def _readlink(path): path = path.replace("/proc", str(proc_root)) return real_readlink(path) - monkeypatch.setattr(hermes_state.os, "readlink", _readlink) + monkeypatch.setattr(hermes_state_holders.os, "readlink", _readlink) + real_stat = os.stat + def _stat(path, *args, **kwargs): + path_s = str(path).replace("/proc", str(proc_root)) + if path_s.endswith("/222/fd/3"): + # A real /proc fd remains statable after unlink and retains + # the deleted sidecar's filesystem identity: same device as + # state.db, but an inode no live watched path can reach. + fields = list(real_stat(db_path)) + fields[1] += 1000 + return os.stat_result(fields) + return real_stat(path_s, *args, **kwargs) + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) - holders = db._foreign_state_db_holders() + holders = hermes_state_holders.foreign_state_db_holders(db_path) assert holders == [(222, db_path_wal + " (deleted)")] + @pytest.mark.linux_only + @pytest.mark.parametrize("different_device", (False, True)) + def test_foreign_holder_ignores_same_path_with_different_file_identity( + self, db, tmp_path, monkeypatch, different_device + ): + """A namespace peer's different state.db is not a holder of the host's. + + A peer process can appear in /proc with a string-identical path for a + different inode, either on the same filesystem or a different one. + Matching on path alone -- or on device alone -- would defer automatic + FTS maintenance forever while corruption compounds. + + Identity must come from (st_dev, st_ino), not the path text. + """ + db_path = tmp_path / "state.db" + + proc_root = tmp_path / "proc" + for pid in (111, 222): + (proc_root / str(pid) / "fd").mkdir(parents=True) + # PID 222 = container process holding ITS OWN state.db, which happens + # to have the identical absolute path inside its mount namespace. + guest_db = tmp_path / "guest_state.db" + guest_db.touch() + os.symlink(str(guest_db), str(proc_root / "222" / "fd" / "3")) + + monkeypatch.setattr(hermes_state_holders.os, "getpid", lambda: 111) + + real_listdir = os.listdir + def _listdir(path): + if isinstance(path, str): + path = path.replace("/proc", str(proc_root)) + return real_listdir(path) + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + + # The guest fd reports the host's path (identical string), which is + # exactly what the kernel shows across mount namespaces. + def _readlink(path): + path = path.replace("/proc", str(proc_root)) + if path.endswith(f"{proc_root}/222/fd/3") or "222" in path: + return str(db_path) + return os.readlink(path) + monkeypatch.setattr(hermes_state_holders.os, "readlink", _readlink) + + # ...but stat()ing the descriptor resolves to the peer's own inode. + real_stat = os.stat + def _stat(path, *a, **kw): + path_s = str(path).replace("/proc", str(proc_root)) + st = real_stat(path_s, *a, **kw) + if different_device and path_s.endswith("/222/fd/3"): + fields = list(st) + # os.stat_result positional layout: st_dev is index 2. + fields[2] = st.st_dev + 1000 + return os.stat_result(fields) + return st + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) + + assert hermes_state_holders.foreign_state_db_holders(db_path) == [] + + @pytest.mark.linux_only def test_foreign_holder_uninspectable_process_cmdline_fallback( self, db, tmp_path, monkeypatch ): @@ -241,32 +316,34 @@ class TestRuntimeFtsRebuild: os.chmod(proc_root / "222" / "fd", 0o000) # PID 222's cmdline is world-readable and looks like Hermes cmdline_path = proc_root / "222" / "cmdline" - cmdline_path.write_bytes(b"python3\x00hermes_cli.main\x00chat\x00") + cmdline_path.write_bytes( + b"python3\x00-m\x00hermes_cli.main\x00chat\x00" + ) - monkeypatch.setattr(hermes_state, "_IS_WINDOWS", False) - monkeypatch.setattr(hermes_state.os, "getpid", lambda: 111) - monkeypatch.setattr(hermes_state.sys, "platform", "linux") + monkeypatch.setattr(hermes_state_holders.os, "getpid", lambda: 111) real_listdir = os.listdir def _listdir(path): if isinstance(path, str): + if path == "/proc/222/fd": + raise PermissionError(path) path = path.replace("/proc", str(proc_root)) return real_listdir(path) - monkeypatch.setattr(hermes_state.os, "listdir", _listdir) - # _read_proc_cmdline opens /proc//cmdline directly; redirect + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + # _read_proc_argv opens /proc//cmdline directly; redirect # it to our fake proc tree. - def _fake_cmdline(pid): + def _fake_argv(pid): fake_path = str(proc_root / str(pid) / "cmdline") try: with open(fake_path, "rb") as f: raw = f.read() if not raw: return None - return raw.replace(b"\x00", b" ").decode("utf-8", "replace").strip() + return raw.decode("utf-8", "replace").rstrip("\x00").split("\x00") except OSError: return None - monkeypatch.setattr(hermes_state, "_read_proc_cmdline", _fake_cmdline) + monkeypatch.setattr(hermes_state_holders, "_read_proc_argv", _fake_argv) - holders = db._foreign_state_db_holders() + holders = hermes_state_holders.foreign_state_db_holders(db_path) # Should include PID 222 with the cmdline info assert len(holders) == 1 assert holders[0][0] == 222 diff --git a/tests/state/test_state_db_holders.py b/tests/state/test_state_db_holders.py new file mode 100644 index 0000000000..4fb5c24f38 --- /dev/null +++ b/tests/state/test_state_db_holders.py @@ -0,0 +1,50 @@ +"""Behavioral tests for the state-holder and repair-admission authority.""" + +import os + +import pytest + +import hermes_state_holders + + +@pytest.mark.linux_only +def test_foreign_holder_accepts_same_inode_reached_through_an_alias( + tmp_path, monkeypatch +): + """Descriptor identity is authoritative even when /proc spells another path.""" + db_path = tmp_path / "state.db" + db_path.touch() + alias_path = tmp_path / "namespace-alias" / "state.db" + + proc_root = tmp_path / "proc" + for pid in (111, 222): + (proc_root / str(pid) / "fd").mkdir(parents=True) + os.symlink(db_path, proc_root / "222" / "fd" / "3") + + monkeypatch.setattr(hermes_state_holders.os, "getpid", lambda: 111) + real_listdir = os.listdir + + def _listdir(path): + if isinstance(path, str): + path = path.replace("/proc", str(proc_root)) + return real_listdir(path) + + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + + def _readlink(path): + if path == "/proc/222/fd/3": + return str(alias_path) + return os.readlink(path.replace("/proc", str(proc_root))) + + monkeypatch.setattr(hermes_state_holders.os, "readlink", _readlink) + real_stat = os.stat + + def _stat(path, *args, **kwargs): + path = str(path).replace("/proc", str(proc_root)) + return real_stat(path, *args, **kwargs) + + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) + + assert hermes_state_holders.foreign_state_db_holders(db_path) == [ + (222, str(alias_path)) + ] diff --git a/tests/test_state_db_repair_live_writer_guard.py b/tests/test_state_db_repair_live_writer_guard.py index 8288cb322b..dbf3c6667e 100644 --- a/tests/test_state_db_repair_live_writer_guard.py +++ b/tests/test_state_db_repair_live_writer_guard.py @@ -18,12 +18,18 @@ live-writer guard.) from __future__ import annotations +import errno +import select import sqlite3 +import subprocess +import sys import uuid from pathlib import Path import pytest +import hermes_state +import hermes_state_holders from hermes_state import ( SessionDB, repair_state_db_schema, @@ -84,6 +90,312 @@ def test_repair_refuses_while_another_connection_holds_the_db(tmp_path): assert "live writer" in (report["error"] or "").lower() +def test_repair_checks_foreign_holders_before_opening_sqlite(tmp_path, monkeypatch): + """A replacement pathname cannot expose locks on the deleted old inode.""" + db = _make_wal_db(tmp_path) + monkeypatch.setattr( + hermes_state_holders, + "foreign_state_db_holders", + lambda _path: [(4242, f"{db}-wal (deleted)")], + ) + + def _unexpected_probe(*_args, **_kwargs): + pytest.fail("repair opened SQLite before excluding foreign holders") + + monkeypatch.setattr(hermes_state, "_connect_repair_durable", _unexpected_probe) + + report = repair_state_db_schema(db, backup=False) + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + + +@pytest.mark.linux_only +def test_linux_holder_scan_does_not_require_psutil(tmp_path, monkeypatch): + """The Linux safety scan must not make psutil a repair dependency.""" + monkeypatch.setattr(hermes_state_holders, "psutil", None) + + holders = hermes_state_holders.foreign_state_db_holders( + tmp_path / "absent-state.db" + ) + + assert holders == [] + + +@pytest.mark.linux_only +def test_incomplete_holder_scan_keeps_unknown_sentinel(tmp_path, monkeypatch): + """A partial scan must not hide uncertainty behind an ordinary holder.""" + db = tmp_path / "state.db" + db.touch() + + def _listdir(path): + if path == "/proc": + return ["4242", "4343"] + if path == "/proc/4242/fd": + return ["7"] + if path == "/proc/4343/fd": + raise RuntimeError("scan interrupted") + raise AssertionError(f"unexpected scan path: {path}") + + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + monkeypatch.setattr(hermes_state_holders.os, "readlink", lambda _path: str(db)) + real_stat = hermes_state_holders.os.stat + + def _stat(path, *args, **kwargs): + if path == "/proc/4242/fd/7": + return real_stat(db) + return real_stat(path, *args, **kwargs) + + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) + + holders = hermes_state_holders.foreign_state_db_holders(db) + + assert (4242, str(db)) in holders + assert any(pid < 0 and "scan interrupted" in path for pid, path in holders) + + +@pytest.mark.linux_only +def test_uninspectable_watched_descriptor_blocks_repair_before_sqlite( + tmp_path, monkeypatch +): + """A watched fd whose identity cannot be read is not proven safe.""" + db = _make_wal_db(tmp_path) + + def _listdir(path): + if path == "/proc": + return ["4242"] + if path == "/proc/4242/fd": + return ["7"] + raise AssertionError(f"unexpected scan path: {path}") + + real_stat = hermes_state_holders.os.stat + + def _stat(path, *args, **kwargs): + if path == "/proc/4242/fd/7": + raise PermissionError(errno.EACCES, "descriptor denied", path) + return real_stat(path, *args, **kwargs) + + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + monkeypatch.setattr(hermes_state_holders.os, "readlink", lambda _path: str(db)) + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) + + def _unexpected_probe(*_args, **_kwargs): + pytest.fail("repair opened SQLite with unproven descriptor identity") + + monkeypatch.setattr(hermes_state, "_connect_repair_durable", _unexpected_probe) + + report = repair_state_db_schema(db, backup=False) + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + + +@pytest.mark.linux_only +@pytest.mark.parametrize( + ("argv", "should_block"), + ( + (["python3", "backup.py"], False), + (["python3", "-m", "hermes_cli.main", "gateway"], True), + ), +) +def test_uninspectable_unknown_descriptor_uses_hermes_identity_at_repair_boundary( + tmp_path, monkeypatch, argv, should_block +): + """An unknown fd target blocks only when argv identifies Hermes.""" + db = _make_wal_db(tmp_path) + + def _listdir(path): + if path == "/proc": + return ["4242"] + if path == "/proc/4242/fd": + return ["7"] + raise AssertionError(f"unexpected scan path: {path}") + + def _readlink(path): + raise PermissionError(errno.EACCES, "descriptor denied", path) + + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + monkeypatch.setattr(hermes_state_holders.os, "readlink", _readlink) + monkeypatch.setattr( + hermes_state_holders, + "_read_proc_argv", + lambda _pid: argv, + ) + + if should_block: + + def _unexpected_probe(*_args, **_kwargs): + pytest.fail("repair opened SQLite with an unproven Hermes descriptor") + + monkeypatch.setattr( + hermes_state, + "_connect_repair_durable", + _unexpected_probe, + ) + report = repair_state_db_schema(db, backup=False) + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + else: + real_connect = hermes_state._connect_repair_durable + probe_reached = False + + def _record_probe(*args, **kwargs): + nonlocal probe_reached + probe_reached = True + return real_connect(*args, **kwargs) + + monkeypatch.setattr( + hermes_state, + "_connect_repair_durable", + _record_probe, + ) + report = repair_state_db_schema(db, backup=False) + + assert probe_reached is True + assert "live writer" not in (report["error"] or "").lower() + + +@pytest.mark.linux_only +def test_uninspectable_watched_identity_blocks_alias_before_sqlite( + tmp_path, monkeypatch +): + """A non-disappearance stat error cannot prove an aliased holder safe.""" + db = _make_wal_db(tmp_path) + alias = tmp_path / "namespace-alias" / "state.db" + + def _listdir(path): + if path == "/proc": + return ["4242"] + if path == "/proc/4242/fd": + return ["7"] + raise AssertionError(f"unexpected scan path: {path}") + + real_stat = hermes_state_holders.os.stat + + def _stat(path, *args, **kwargs): + if str(path) == str(db) and not args and not kwargs: + raise PermissionError(errno.EACCES, "watched identity denied", path) + if path == "/proc/4242/fd/7": + return real_stat(db) + return real_stat(path, *args, **kwargs) + + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + monkeypatch.setattr( + hermes_state_holders.os, "readlink", lambda _path: str(alias) + ) + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) + + def _unexpected_probe(*_args, **_kwargs): + pytest.fail("repair opened SQLite with an unproven watched identity") + + monkeypatch.setattr(hermes_state, "_connect_repair_durable", _unexpected_probe) + + report = repair_state_db_schema(db, backup=False) + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + + +@pytest.mark.linux_only +def test_uninspectable_alias_descriptor_for_hermes_blocks_before_sqlite( + tmp_path, monkeypatch +): + """Hermes cannot make an aliased fd safe when its identity is unreadable.""" + db = _make_wal_db(tmp_path) + alias = tmp_path / "namespace-alias" / "state.db" + + def _listdir(path): + if path == "/proc": + return ["4242"] + if path == "/proc/4242/fd": + return ["7"] + raise AssertionError(f"unexpected scan path: {path}") + + real_stat = hermes_state_holders.os.stat + + def _stat(path, *args, **kwargs): + if path == "/proc/4242/fd/7": + raise PermissionError(errno.EACCES, "descriptor denied", path) + return real_stat(path, *args, **kwargs) + + monkeypatch.setattr(hermes_state_holders.os, "listdir", _listdir) + monkeypatch.setattr( + hermes_state_holders.os, "readlink", lambda _path: str(alias) + ) + monkeypatch.setattr(hermes_state_holders.os, "stat", _stat) + monkeypatch.setattr( + hermes_state_holders, + "_read_proc_argv", + lambda _pid: ["python3", "-m", "hermes_cli.main", "gateway"], + ) + + def _unexpected_probe(*_args, **_kwargs): + pytest.fail("repair opened SQLite with an unproven Hermes alias fd") + + monkeypatch.setattr(hermes_state, "_connect_repair_durable", _unexpected_probe) + + report = repair_state_db_schema(db, backup=False) + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + + +@pytest.mark.requires_wal +@pytest.mark.linux_only +def test_repair_refuses_while_foreign_process_holds_deleted_wal(tmp_path): + """Reproduce the inode split that a pathname lock probe cannot observe.""" + db = _make_wal_db(tmp_path) + holder_code = """ +import sqlite3 +import sys + +conn = sqlite3.connect(sys.argv[1]) +conn.execute("PRAGMA journal_mode=WAL") +conn.execute("BEGIN IMMEDIATE") +print("ready", flush=True) +sys.stdin.read(1) +conn.rollback() +conn.close() +""" + holder = subprocess.Popen( + [sys.executable, "-c", holder_code, str(db)], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + try: + assert holder.stdout is not None + readable, _, _ = select.select([holder.stdout], [], [], 10) + assert readable, "holder subprocess did not signal readiness" + assert holder.stdout.readline().strip() == "ready" + deleted = [] + for suffix in ("-wal", "-shm"): + sidecar = Path(f"{db}{suffix}") + if sidecar.exists(): + sidecar.unlink() + deleted.append(sidecar) + assert deleted + + report = repair_state_db_schema(db, backup=False) + + assert report["repaired"] is False + assert "live writer" in (report["error"] or "").lower() + finally: + if holder.poll() is None and holder.stdin is not None: + try: + holder.stdin.write("x") + holder.stdin.close() + except (BrokenPipeError, ValueError): + pass + try: + holder.wait(timeout=10) + except subprocess.TimeoutExpired: + holder.kill() + holder.wait(timeout=10) + + 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)