fix(state): use /proc readlinks + cmdline fallback for holder detection
Address review feedback from @jackulau on PR #90871: 1. psutil.open_files() silently drops '(deleted)' WAL sidecar entries on Linux because isfile_strict() stats the literal path including the suffix and fails. Switch to direct /proc/<pid>/fd readlinks which preserve the '(deleted)' suffix so _canonical can match. 2. psutil.process_iter() converts AccessDenied to None, which or-() skips silently — the fail-closed branch never runs. For the root-gateway vs user-desktop topology in the issue, the fd table is unreadable but /proc/<pid>/cmdline is world-readable. Add a cmdline fallback that flags uninspectable processes. Also keep the psutil path for macOS/BSD (no '(deleted)' convention).
This commit is contained in:
kshitijk4poor
+70
-5
@@ -3049,6 +3049,22 @@ def count_db_holders(db_path: Path) -> Optional[int]:
|
||||
return None
|
||||
|
||||
|
||||
def _read_proc_cmdline(pid: int) -> Optional[str]:
|
||||
"""Read /proc/<pid>/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
|
||||
|
||||
|
||||
# Lifecycle statuses surfaced by session pickers. Classification looks ONLY at
|
||||
# a session's final message row — role, whether it carries tool_calls, and its
|
||||
# finish_reason — so it stays O(1) per session (see
|
||||
@@ -4217,9 +4233,7 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin)
|
||||
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. ``psutil`` reads the kernel's open-file
|
||||
table, including Linux ``(deleted)`` descriptors, so this also catches
|
||||
a split brain already in progress.
|
||||
through different WAL inodes.
|
||||
|
||||
A scan failure is represented as an unknown holder. Skipping optional
|
||||
automatic maintenance is safer than assuming quiescence; canonical
|
||||
@@ -4245,20 +4259,71 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin)
|
||||
_canonical(db_path + "-shm"),
|
||||
}
|
||||
holders: List[Tuple[int, str]] = []
|
||||
|
||||
# On Linux, read /proc/<pid>/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/<pid>/cmdline is world-readable by default,
|
||||
# so check whether this is a Hermes process.
|
||||
cmdline = _read_proc_cmdline(pid)
|
||||
if cmdline is not None:
|
||||
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(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(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",
|
||||
"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}")]
|
||||
|
||||
@@ -13,6 +13,7 @@ sync triggers, and retries the canonical write. Search degrades to ``LIKE``
|
||||
until a later open atomically rebuilds the index and restores the triggers.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sqlite3
|
||||
from types import SimpleNamespace
|
||||
|
||||
@@ -122,11 +123,105 @@ class TestRuntimeFtsRebuild:
|
||||
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)")
|
||||
]
|
||||
|
||||
def test_foreign_holder_detection_proc_readlink_deleted_wal(
|
||||
self, db, tmp_path, monkeypatch
|
||||
):
|
||||
"""Linux /proc/<pid>/fd readlinks preserve '(deleted)' suffix.
|
||||
|
||||
psutil.open_files() drops these entries (isfile_strict stats the
|
||||
literal path and fails). The /proc path catches the split-brain
|
||||
holder that psutil silently misses.
|
||||
"""
|
||||
db_path = tmp_path / "state.db"
|
||||
db_path_wal = str(db_path) + "-wal"
|
||||
|
||||
# Build a fake /proc with two PIDs: self (111) and foreign (222).
|
||||
proc_root = tmp_path / "proc"
|
||||
for pid in (111, 222, 333):
|
||||
fd_dir = proc_root / str(pid) / "fd"
|
||||
fd_dir.mkdir(parents=True)
|
||||
# PID 222 holds the deleted WAL sidecar
|
||||
os.symlink(db_path_wal + " (deleted)", str(proc_root / "222" / "fd" / "3"))
|
||||
# PID 111 (self) holds the db — should be excluded
|
||||
os.symlink(str(db_path), str(proc_root / "111" / "fd" / "3"))
|
||||
# PID 333 holds an unrelated file
|
||||
other = tmp_path / "other.db"
|
||||
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")
|
||||
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)
|
||||
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)
|
||||
|
||||
holders = db._foreign_state_db_holders()
|
||||
assert holders == [(222, db_path_wal + " (deleted)")]
|
||||
|
||||
def test_foreign_holder_uninspectable_process_cmdline_fallback(
|
||||
self, db, tmp_path, monkeypatch
|
||||
):
|
||||
"""A process whose fd table is unreadable (different user) is still
|
||||
flagged when /proc/<pid>/cmdline identifies it as a Hermes process."""
|
||||
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's fd dir is unreadable (PermissionError)
|
||||
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")
|
||||
|
||||
monkeypatch.setattr(hermes_state, "_IS_WINDOWS", False)
|
||||
monkeypatch.setattr(hermes_state.os, "getpid", lambda: 111)
|
||||
monkeypatch.setattr(hermes_state.sys, "platform", "linux")
|
||||
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)
|
||||
# _read_proc_cmdline opens /proc/<pid>/cmdline directly; redirect
|
||||
# it to our fake proc tree.
|
||||
def _fake_cmdline(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()
|
||||
except OSError:
|
||||
return None
|
||||
monkeypatch.setattr(hermes_state, "_read_proc_cmdline", _fake_cmdline)
|
||||
|
||||
holders = db._foreign_state_db_holders()
|
||||
# Should include PID 222 with the cmdline info
|
||||
assert len(holders) == 1
|
||||
assert holders[0][0] == 222
|
||||
assert "hermes_cli.main" in holders[0][1]
|
||||
|
||||
# Cleanup
|
||||
os.chmod(proc_root / "222" / "fd", 0o755)
|
||||
|
||||
def test_corruption_error_classification_covers_both_sqlite_messages(self):
|
||||
"""SQLite's message for a corrupt FTS index varies by version: older
|
||||
builds raise the generic malformed-image error, newer builds raise an
|
||||
|
||||
Reference in New Issue
Block a user