fix(state): compare fd identity against the watched path, not st_nlink
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.
This commit is contained in:
+3
-2
@@ -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
|
||||
|
||||
+27
-13
@@ -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)
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user