diff --git a/hermes_state.py b/hermes_state.py index 8bec8e91f1..154c269dae 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -15,6 +15,7 @@ import queue import random import re import sqlite3 +import stat import sys import threading import time @@ -222,41 +223,58 @@ def _secure_state_db_files(db_path: Path, *, create_main: bool = False) -> None: """Create/tighten a writable state database and its sidecars to 0600. SQLite otherwise creates ``state.db``, ``-wal``, and ``-shm`` according to - the process umask (commonly 0644 under 0022). Use file descriptors so a - missing main database is private from its first byte and O_NOFOLLOW can - refuse a planted symlink. Read-only SessionDB attachments never call this - helper and remain observational. + the process umask (commonly 0644 under 0022). Read-only SessionDB + attachments never call this helper and remain observational. + + Existing files are tightened with ``chmod(2)`` on the path: opening the + file and closing that descriptor would drop every POSIX ``fcntl`` lock the + process holds on its inode — including the locks of an already-open SQLite + connection to the same database. A lock-losing close in one process lets a + sibling's connection take the shared-memory DMS exclusively at its own + close, checkpoint, and unlink the sidecars while long-lived holders + (gateway, desktop ``hermes serve``) keep using the deleted inodes. """ if os.name == "nt": return - for index, path in enumerate( - ( - db_path, - db_path.with_name(db_path.name + "-wal"), - db_path.with_name(db_path.name + "-shm"), - ) - ): - flags = os.O_RDONLY - if index == 0 and create_main: - flags = os.O_WRONLY | os.O_CREAT + main_path = db_path + if create_main: + flags = os.O_WRONLY | os.O_CREAT if hasattr(os, "O_NOFOLLOW"): flags |= os.O_NOFOLLOW if hasattr(os, "O_CLOEXEC"): flags |= os.O_CLOEXEC try: - fd = os.open(path, flags, 0o600) - except FileNotFoundError: - continue + fd = os.open(main_path, flags, 0o600) except IsADirectoryError: # Not a database file at all; sqlite3.connect() raises the # canonical error for this, and a directory leaks no row data. - continue + return try: os.fchmod(fd, 0o600) finally: os.close(fd) + for path in ( + main_path, + db_path.with_name(db_path.name + "-wal"), + db_path.with_name(db_path.name + "-shm"), + ): + # fchmod on an fd of a pre-existing file cannot be used here: close(fd) + # would release this process's POSIX locks on that inode, stripping the + # locks of any live SQLite connection to the same database. chmod(2) + # never opens the file, so it leaves the lock state untouched. + try: + st = os.lstat(path) + except FileNotFoundError: + continue + if stat.S_ISLNK(st.st_mode): + # Refuse a planted symlink exactly like O_NOFOLLOW would. + continue + if not stat.S_ISREG(st.st_mode): + continue + os.chmod(path, 0o600) + # Openings of the background-review harness prompts (agent/background_review.py). _REVIEW_HARNESS_PREFIXES = ( diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index 53849a3904..8b78aa7aac 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -164,6 +164,42 @@ class TestConnectionLifecycle: finally: session_db.close() + @pytest.mark.skipif(os.name == "nt", reason="POSIX fcntl locks") + def test_writable_state_db_keeps_locks_across_second_open(self, tmp_path): + """Opening a second SessionDB in this process must not unlink live sidecars. + + POSIX locks are owned per (process, inode): closing any descriptor for + state.db drops every lock this process holds on it, including the locks + of the first SessionDB's connection. A sibling process reading the + database after that close takes the shared-memory DMS exclusively on its + own close, checkpoints, and unlinks -wal/-shm while the first handle + keeps using the deleted inodes. + """ + import subprocess + import sys + + from hermes_state_dbfile import iter_deleted_sqlite_sidecar_holders + + db_path = tmp_path / "state.db" + first = SessionDB(db_path=db_path) + second = SessionDB(db_path=db_path) + try: + assert not iter_deleted_sqlite_sidecar_holders(db_path) + subprocess.run( + [sys.executable, "-c", + "import sqlite3,sys; c=sqlite3.connect(sys.argv[1]); " + "c.execute('SELECT count(*) FROM sessions').fetchone(); c.close()", + str(db_path)], + check=True, timeout=30, + ) + assert not iter_deleted_sqlite_sidecar_holders(db_path), ( + "a second SessionDB open or a sibling reader unlinked the live " + "WAL/SHM inodes out from under this process" + ) + finally: + second.close() + first.close() + def test_failed_writable_open_does_not_leak_tracked_connection( self, tmp_path, monkeypatch ):