diff --git a/hermes_state.py b/hermes_state.py index 36961edabb..f233ce858e 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -46,7 +46,7 @@ from hermes_state_telegram import SessionTelegramTopicsMixin from hermes_state_schema import SessionSchemaMixin import hermes_state_holders as _state_holders from hermes_state_dbfile import ( - _canonical_sqlite_path, _connect_tracked_db, _prepare_connection_retirement, + _canonical_sqlite_path, _connect_tracked_db, _fd_is_truly_unlinked, _prepare_connection_retirement, _read_sqlite_application_id, _stat_sqlite_sidecar_identity, _watched_sqlite_sidecar_paths, has_invalid_sqlite_header_preopen, is_zeroed_state_db, quarantine_cross_process_lock, quarantine_invalid_state_db, @@ -1007,8 +1007,9 @@ class SessionDB( if sys.platform.startswith("linux"): watched = _watched_sqlite_sidecar_paths(self.db_path) try: - for target in _proc_fd_targets(os.getpid()): - if " (deleted)" in target and _canonical_sqlite_path(target) in watched: + for target, fd_path in _proc_fd_targets(os.getpid()): + if (" (deleted)" in target and _canonical_sqlite_path(target) in watched + and _fd_is_truly_unlinked(fd_path)): return True except OSError: return False diff --git a/hermes_state_dbfile.py b/hermes_state_dbfile.py index e55a33b062..327c4b6a6d 100644 --- a/hermes_state_dbfile.py +++ b/hermes_state_dbfile.py @@ -141,8 +141,22 @@ def _watched_sqlite_sidecar_paths(db_path) -> Set[str]: return {_canonical_sqlite_path(base + "-wal"), _canonical_sqlite_path(base + "-shm")} +def _fd_is_truly_unlinked(fd_path: str) -> bool: + """Confirm a `` (deleted)`` /proc fd target really lost its last name. + + The suffix alone is not proof: on OpenZFS a live, still-linked file whose + dentry was unhashed is reported as deleted while ``st_nlink`` is still 1 and + the path resolves to the very same inode. Only ``st_nlink == 0`` means the + inode is an orphan generation. An unstattable descriptor counts as deleted + so the guard keeps failing closed.""" + try: + return os.stat(fd_path).st_nlink == 0 + except OSError: + return True + + def _iter_proc_fd_targets(): - """Yield ``(pid, readlink target)`` for every readable ``/proc//fd`` entry.""" + """Yield ``(pid, readlink target, fd path)`` for every readable ``/proc//fd`` entry.""" for pid_str in os.listdir("/proc"): if not pid_str.isdigit(): continue @@ -153,7 +167,8 @@ def _iter_proc_fd_targets(): continue # process gone or not ours for fd in fds: with contextlib.suppress(OSError): - yield int(pid_str), os.readlink(f"{fd_dir}/{fd}") + fd_path = f"{fd_dir}/{fd}" + yield int(pid_str), os.readlink(fd_path), fd_path def iter_deleted_sqlite_sidecar_holders(db_path) -> List[Tuple[int, str]]: @@ -166,8 +181,9 @@ 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: - for pid, target in _iter_proc_fd_targets(): - if " (deleted)" in target and _canonical_sqlite_path(target) in watched: + for pid, target, fd_path in _iter_proc_fd_targets(): + if (" (deleted)" in target and _canonical_sqlite_path(target) in watched + and _fd_is_truly_unlinked(fd_path)): holders.append((pid, target)) except Exception as exc: logger.debug("deleted-WAL holder scan failed for %s: %s", db_path, exc) @@ -614,7 +630,7 @@ def count_db_holders(db_path: Path) -> Optional[int]: if not sys.platform.startswith("linux"): return None target = os.path.realpath(str(db_path)) - return len({pid for pid, link in _iter_proc_fd_targets() if link == target}) + return len({pid for pid, link, _fd_path in _iter_proc_fd_targets() if link == target}) except Exception: return None diff --git a/hermes_state_readpool.py b/hermes_state_readpool.py index 23f6f26c25..5e3b27c26a 100644 --- a/hermes_state_readpool.py +++ b/hermes_state_readpool.py @@ -69,13 +69,14 @@ _fd_usage_lock = threading.Lock() _fd_usage_cache: "tuple[float, Optional[int]]" = (0.0, None) -def _proc_fd_targets(pid: int) -> Iterator[str]: - """readlink() of every entry in /proc//fd (unreadable links skipped). - Raises OSError when the fd directory itself cannot be listed.""" +def _proc_fd_targets(pid: int) -> "Iterator[tuple[str, str]]": + """Yield ``(readlink target, fd path)`` for every entry in /proc//fd (unreadable + links skipped). Raises OSError when the fd directory itself cannot be listed.""" fd_dir = f"/proc/{pid}/fd" for fd in os.listdir(fd_dir): + fd_path = f"{fd_dir}/{fd}" try: - yield os.readlink(f"{fd_dir}/{fd}") + yield os.readlink(fd_path), fd_path except OSError: continue diff --git a/tests/hermes_state/test_deleted_wal_generation_guard.py b/tests/hermes_state/test_deleted_wal_generation_guard.py index bdab0ba1c5..34ba9703ae 100644 --- a/tests/hermes_state/test_deleted_wal_generation_guard.py +++ b/tests/hermes_state/test_deleted_wal_generation_guard.py @@ -16,6 +16,8 @@ from pathlib import Path import pytest import hermes_state +import hermes_state_dbfile +import hermes_state_readpool import hermes_state_wal from hermes_state import ( DeletedWalGenerationError, SessionDB, _close_time_checkpoint_configurable, classify_persistence_error, @@ -127,6 +129,63 @@ def test_second_sessiondb_open_refuses_and_does_not_mint_wal(tmp_path, force_wal writer.close() +@pytest.mark.skipif( + not sys.platform.startswith("linux"), + reason="deleted-WAL /proc scan is Linux-only", +) +def test_iter_holders_ignores_live_unhashed_dentry(tmp_path, force_wal, monkeypatch): + """OpenZFS can report a live, still-linked file's /proc fd target with the + `` (deleted)`` suffix (dentry unhashed, nlink still 1) even though nothing + was actually unlinked. The scan must not treat that as an orphaned WAL.""" + path = tmp_path / "state.db" + db = _make_db(path, "s", "held") + wal = _require_wal(db) + real_readlink = os.readlink + + def fake_readlink(fd_path, *args, **kwargs): + target = real_readlink(fd_path, *args, **kwargs) + if target.endswith(("-wal", "-shm")): + return target + " (deleted)" + return target + + monkeypatch.setattr(hermes_state_dbfile.os, "readlink", fake_readlink) + try: + assert iter_deleted_sqlite_sidecar_holders(path) == [] + assert wal.exists() + finally: + db.close() + + +@pytest.mark.skipif( + not sys.platform.startswith("linux"), + reason="deleted-WAL write halt uses Linux unlink semantics", +) +def test_write_path_ignores_live_unhashed_dentry(tmp_path, force_wal, monkeypatch): + """Same OpenZFS artifact as above, but on the sticky in-process write-path + probe (_wal_generation_was_lost), which the open-path fix alone does not cover.""" + path = tmp_path / "state.db" + db = _make_db(path, "s", "held") + _require_wal(db) + # Mimic the post-close-race state that forces the /proc probe path. + db._db_sidecar_identity = {} + real_readlink = os.readlink + + def fake_readlink(fd_path, *args, **kwargs): + target = real_readlink(fd_path, *args, **kwargs) + if target.endswith(("-wal", "-shm")): + return target + " (deleted)" + return target + + monkeypatch.setattr(hermes_state_readpool.os, "readlink", fake_readlink) + try: + assert db._wal_generation_was_lost() is False + db.append_message("s", role="user", content="after-artifact") + rows = db.get_messages("s") + assert any(m["content"] == "after-artifact" for m in rows) + finally: + db.close() + + @pytest.mark.skipif( not sys.platform.startswith("linux"), reason="deleted-WAL write halt uses Linux unlink semantics",