fix(state): tighten existing db files by chmod(2), not an open/fchmod/close cycle
POSIX fcntl locks are owned per (process, inode): closing any descriptor for state.db releases every lock the process holds on that inode, including the locks of an already-open SQLite connection. The owner-only hardening cycle opened the live database and its -wal/-shm read-only, fchmod'ed, and closed, so any process that already held a connection (gateway, desktop hermes serve, dashboard share one) dropped its live locks on every SessionDB init. A sibling process then took the shared-memory DMS exclusively at its own close, checkpointed, and unlinked the sidecars while long-lived holders kept the deleted inodes open, tripping the deleted-WAL generation guard. chmod(2) on the path never opens the file, so it cannot disturb locks. The descriptor path remains only for first-time main-db creation, where no locks can exist yet.
This commit is contained in:
committed by
Teknium
parent
b6b53c69a6
commit
ccd360e94f
@@ -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 = (
|
||||
|
||||
@@ -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
|
||||
):
|
||||
|
||||
Reference in New Issue
Block a user