diff --git a/hermes_state_lockguard.py b/hermes_state_lockguard.py index 74d456e6bb..06ccd1edc0 100644 --- a/hermes_state_lockguard.py +++ b/hermes_state_lockguard.py @@ -12,8 +12,9 @@ descriptors SQLite itself holds. OFD locks belong to the description, not the pr ``close()`` elsewhere cannot cancel them, they die with the connection's own descriptor (nothing extra to track or retire), and they conflict with a foreign EXCLUSIVE exactly like SQLite's own, so the sibling's close-time unlink is refused while a guarded handle is open. The guard is -lifted before the handle's own close so a true last close still ends the generation normally. -No-op on Windows and on runtimes without OFD locks. +lifted before the handle's own close so a true last close still ends the generation normally; +lifting it first re-takes SQLite's own POSIX locks on the same ranges, so the handle is never +unlocked while its connection is still open. No-op on Windows and on runtimes without OFD locks. Ownership model: the guard is a property of the *descriptor*, and a descriptor number is reusable. Each ``hold()`` therefore locks every matching descriptor unconditionally (an OFD @@ -49,7 +50,7 @@ _SHM_DMS_BYTE = 128 if os.name == "nt": fcntl = None # type: ignore[assignment] _F_OFD_SETLK: Optional[int] = None - _F_RDLCK = _F_UNLCK = _SEEK_SET = 0 + _F_RDLCK = _F_UNLCK = _SEEK_SET = _F_SETLK = 0 else: try: import fcntl @@ -58,6 +59,7 @@ else: _F_OFD_SETLK = getattr( fcntl, "F_OFD_SETLK", {"linux": 37, "darwin": 90}.get(sys.platform.rstrip("0123456789"))) _F_RDLCK, _F_UNLCK, _SEEK_SET = fcntl.F_RDLCK, fcntl.F_UNLCK, os.SEEK_SET + _F_SETLK = fcntl.F_SETLK # The constants alone do not make the guard usable: _ofd_lock() calls fcntl.fcntl(), so a # module that has the constants but no callable would pass supported() and then raise # from the first hold(). Probe the whole capability here, not just the symbols. @@ -66,7 +68,7 @@ else: except (ImportError, AttributeError): fcntl = None # type: ignore[assignment] _F_OFD_SETLK = None - _F_RDLCK = _F_UNLCK = _SEEK_SET = 0 + _F_RDLCK = _F_UNLCK = _SEEK_SET = _F_SETLK = 0 # struct flock differs per libc: glibc/musl put type+whence first, Darwin/BSD last. _FLOCK_FORMAT = "@qqihh" if sys.platform == "darwin" or "bsd" in sys.platform else "@hhqqi" @@ -90,11 +92,12 @@ def _flock(lock_type: int, start: int, length: int) -> bytes: return struct.pack(_FLOCK_FORMAT, lock_type, _SEEK_SET, start, length, 0) -def _ofd_lock(fd: int, lock_type: int, start: int, length: int) -> bool: - """Apply a non-blocking OFD lock; False when the range is held EXCLUSIVE elsewhere.""" +def _ofd_lock(fd: int, lock_type: int, start: int, length: int, *, cmd: Optional[int] = None) -> bool: + """Apply a non-blocking OFD lock (a process-owned POSIX one with ``cmd=_F_SETLK``); False when + the range is held EXCLUSIVE elsewhere.""" assert fcntl is not None and _F_OFD_SETLK is not None try: - fcntl.fcntl(fd, _F_OFD_SETLK, _flock(lock_type, start, length)) + fcntl.fcntl(fd, _F_OFD_SETLK if cmd is None else cmd, _flock(lock_type, start, length)) except BlockingIOError: return False return True @@ -165,7 +168,12 @@ def release(held: Held) -> None: """Drop this handle's claim. The last handle on an inode unlocks the range on every descriptor still referencing it. Call BEFORE the handle's own close so SQLite's close-time reset sees only real holders: a sibling process's intact locks still refuse the unlink, and a true last close - ends the generation, so a later ``state.db`` replace never pairs with a stale WAL.""" + ends the generation, so a later ``state.db`` replace never pairs with a stale WAL. + + The OFD copy is swapped back for the POSIX lock SQLite believes it still holds (a stray close + cancelled the real one): unlocked outright, the still-open connection would let a sibling's + close take EXCLUSIVE and unlink ``-wal``/``-shm`` under it before its own close runs. SQLite's + close upgrades or drops that process-owned lock itself, so a true last close is unaffected.""" if not supported() or not held: return with _LOCK: @@ -183,6 +191,7 @@ def release(held: Held) -> None: try: for fd, ident in _own_fds_for(set(to_unlock)): start, length = to_unlock[ident] + _ofd_lock(fd, _F_RDLCK, start, length, cmd=_F_SETLK) # never refused: our OFD lock excludes writers _ofd_lock(fd, _F_UNLCK, start, length) except OSError: pass diff --git a/tests/hermes_state/test_wal_lock_guard_lifecycle.py b/tests/hermes_state/test_wal_lock_guard_lifecycle.py index 559d5bbf54..9557f30cde 100644 --- a/tests/hermes_state/test_wal_lock_guard_lifecycle.py +++ b/tests/hermes_state/test_wal_lock_guard_lifecycle.py @@ -71,3 +71,32 @@ def test_guard_never_outlives_the_handle_under_fd_reuse(tmp_path, monkeypatch): assert _foreign_exclusive_ok(str(path)), "a lock survived the last handle's close" assert not lg._HANDLES assert sqlite3.connect(path).execute("SELECT COUNT(*) FROM messages").fetchone()[0] == 2 + + +def test_closing_writer_stays_guarded_until_sqlite_closes(tmp_path, monkeypatch): + """close() lifts the OFD guard before SQLite's own close so a true last close still ends the + generation. A stray close had already cancelled this process's POSIX locks, so in that gap a + sibling's close could take EXCLUSIVE and unlink -wal/-shm under the still-open connection + (every concurrent opener then refuses with DeletedWalGenerationError).""" + pin_wal(monkeypatch) + path = tmp_path / "state.db" + db = make_db(path, "s", "seed") + require_wal(db) + for name in ("state.db", "state.db-shm"): # the stray close the guard exists for + os.close(os.open(tmp_path / name, os.O_RDONLY)) + shm = os.stat(f"{path}-shm").st_ino + in_gap: dict = {} + real_close = db._close_connection_quietly + + def _sibling_closes_in_the_gap(conn): + subprocess.run([sys.executable, "-c", + f"import sqlite3; c = sqlite3.connect({str(path)!r}); " + "c.execute('select count(*) from messages').fetchone(); c.close()"], check=True) + in_gap["shm"] = os.stat(f"{path}-shm").st_ino if os.path.exists(f"{path}-shm") else None + real_close(conn) + + monkeypatch.setattr(db, "_close_connection_quietly", _sibling_closes_in_the_gap) + db.close() + assert in_gap["shm"] == shm, "a sibling's close unlinked -shm under the closing writer" + assert not os.path.exists(f"{path}-wal"), "the true last close must still end the generation" + assert _foreign_exclusive_ok(str(path)) and not lg._HANDLES