From d6b036b726817f2818cdfa4f42dd51a982bd1289 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Thu, 10 Sep 2026 15:32:46 +0000 Subject: [PATCH] fix(state): compare fd identity against the watched path, not st_nlink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit st_nlink == 0 alone cannot distinguish a genuine orphan from one that still has a surviving hard link (e.g. a backup) after the watched sidecar path itself was removed or replaced — that left st_nlink >= 1 on a truly orphaned generation, letting a new opener through while a live writer still owned the old one. Compare (st_dev, st_ino) between the fd and the current watched sidecar path instead: only an exact match means they're the same live file, so any mismatch or unstattable watched path still fails closed. --- hermes_state.py | 5 ++- hermes_state_dbfile.py | 40 +++++++++++++------ .../test_deleted_wal_generation_guard.py | 27 +++++++++++++ 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/hermes_state.py b/hermes_state.py index f233ce858e..270d2ca4bc 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -1008,8 +1008,9 @@ class SessionDB( watched = _watched_sqlite_sidecar_paths(self.db_path) try: 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)): + canonical = _canonical_sqlite_path(target) + if (" (deleted)" in target and canonical in watched + and _fd_is_truly_unlinked(fd_path, watched[canonical])): return True except OSError: return False diff --git a/hermes_state_dbfile.py b/hermes_state_dbfile.py index 327c4b6a6d..a65c8f900e 100644 --- a/hermes_state_dbfile.py +++ b/hermes_state_dbfile.py @@ -21,7 +21,7 @@ import sys import threading import time from pathlib import Path -from typing import Any, Callable, Dict, List, Optional, Set, Tuple +from typing import Any, Callable, Dict, List, Optional, Tuple from hermes_state_common import ( FTS_REBUILD_DEFERRAL_KEY, stat_db_file_identity as _stat_db_file_identity @@ -136,23 +136,36 @@ def _canonical_sqlite_path(path: str) -> str: return os.path.normcase(os.path.abspath(path.removesuffix(" (deleted)"))) -def _watched_sqlite_sidecar_paths(db_path) -> Set[str]: +def _watched_sqlite_sidecar_paths(db_path) -> Dict[str, str]: + """Map each sidecar's canonical (/proc-comparable) form to its literal, still-named path, + so a canonical match can be re-``stat``'d for identity rather than trusted as text.""" base = os.path.abspath(os.fspath(db_path)) - return {_canonical_sqlite_path(base + "-wal"), _canonical_sqlite_path(base + "-shm")} + literal = (base + "-wal", base + "-shm") + return {_canonical_sqlite_path(path): path for path in literal} -def _fd_is_truly_unlinked(fd_path: str) -> bool: - """Confirm a `` (deleted)`` /proc fd target really lost its last name. +def _fd_is_truly_unlinked(fd_path: str, watched_path: str) -> bool: + """Confirm a `` (deleted)`` /proc fd target really names an orphaned generation, not the + CURRENT watched sidecar. - 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.""" + The suffix alone is not proof: on OpenZFS a live, still-linked file whose dentry was + unhashed is reported as deleted while it is still the very same inode the watched path + names. Conversely ``st_nlink == 0`` is not proof of the opposite — a stale generation can + keep a surviving hard link (a backup, an operator copy) after the watched path itself is + removed or replaced, leaving ``st_nlink >= 1`` on a truly orphaned inode. So compare + identity, not link count: only an exact ``(st_dev, st_ino)`` match between the fd and the + CURRENT watched path proves they are the same live file. A mismatch, or a watched path + that cannot be stat'd at all, means the fd holds a generation the watched path no longer + names — the guard keeps failing closed.""" try: - return os.stat(fd_path).st_nlink == 0 + fd_stat = os.stat(fd_path) except OSError: return True + try: + watched_stat = os.stat(watched_path) + except OSError: + return True + return (fd_stat.st_dev, fd_stat.st_ino) != (watched_stat.st_dev, watched_stat.st_ino) def _iter_proc_fd_targets(): @@ -182,8 +195,9 @@ def iter_deleted_sqlite_sidecar_holders(db_path) -> List[Tuple[int, str]]: watched = _watched_sqlite_sidecar_paths(db_path) try: 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)): + canonical = _canonical_sqlite_path(target) + if (" (deleted)" in target and canonical in watched + and _fd_is_truly_unlinked(fd_path, watched[canonical])): holders.append((pid, target)) except Exception as exc: logger.debug("deleted-WAL holder scan failed for %s: %s", db_path, exc) diff --git a/tests/hermes_state/test_deleted_wal_generation_guard.py b/tests/hermes_state/test_deleted_wal_generation_guard.py index 34ba9703ae..8a936b55c6 100644 --- a/tests/hermes_state/test_deleted_wal_generation_guard.py +++ b/tests/hermes_state/test_deleted_wal_generation_guard.py @@ -186,6 +186,33 @@ def test_write_path_ignores_live_unhashed_dentry(tmp_path, force_wal, monkeypatc db.close() +@pytest.mark.skipif( + not sys.platform.startswith("linux"), + reason="deleted-WAL /proc scan is Linux-only", +) +def test_iter_holders_flags_orphan_kept_alive_by_hardlink(tmp_path, force_wal): + """`st_nlink == 0` is not proof of an orphan either: a stale generation can keep a + surviving hard link (a backup, an operator copy) after the watched path itself is + unlinked or replaced, so `st_nlink` stays >= 1 on a truly orphaned inode. The guard + must still flag it by comparing the fd's identity against the CURRENT watched path, + not by trusting the link count.""" + path = tmp_path / "state.db" + db = _make_db(path, "s", "held") + wal = _require_wal(db) + backup = tmp_path / "backup-wal" + os.link(wal, backup) # keeps the old inode's nlink >= 1 after the unlink below + try: + _unlink_sidecars(path) + wal.write_bytes(b"new-generation") # watched path recreated on a different inode + assert backup.stat().st_nlink >= 1 + holders = iter_deleted_sqlite_sidecar_holders(path) + assert any( + target.removesuffix(" (deleted)").endswith("-wal") for _pid, target in holders + ) + finally: + db.close() + + @pytest.mark.skipif( not sys.platform.startswith("linux"), reason="deleted-WAL write halt uses Linux unlink semantics",