fix(state): require nlink==0 before treating a /proc fd as an unlinked WAL sidecar
iter_deleted_sqlite_sidecar_holders() and SessionDB._wal_generation_was_lost() both treated a `` (deleted)`` suffix on a /proc/<pid>/fd/* target as proof that state.db-wal or state.db-shm was unlinked. On OpenZFS that suffix is not proof: a live, still-linked file whose dentry was unhashed is reported the same way, with st_nlink still 1 and the same (dev, ino) as the path. The guard then fires permanently and the gateway falls back to JSONL forever, because the WAL was never actually deleted. Add _fd_is_truly_unlinked(), which confirms via os.stat(fd_path).st_nlink == 0 before a target counts as an orphaned generation. An unstattable descriptor still counts as deleted, so the guard keeps failing closed. _iter_proc_fd_targets() and _proc_fd_targets() now also yield the /proc fd path itself so both call sites (open-path and the sticky write-path probe) can run the check.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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/<pid>/fd`` entry."""
|
||||
"""Yield ``(pid, readlink target, fd path)`` for every readable ``/proc/<pid>/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
|
||||
|
||||
|
||||
@@ -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/<pid>/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/<pid>/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
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user