From fe431651c512a194a5e438142a69858e9044179e Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:21:36 -0700 Subject: [PATCH] fix(state): make the byte-probe guard atomic, path-correct, and fail-closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings on the previous commits. Three of them were real defects I reproduced against my own head before fixing. 1. Check/use race (BLOCKING). read_header_bytes_preopen() checked has_live_connection() under _live_lock, released it, then did the raw open/read/close outside the lock; connect_tracked() opened before registering. A thread could pass the "nothing is live" check, another could open a connection and BEGIN IMMEDIATE, and the first thread's close() then cancelled its POSIX locks -- the exact bug this guard exists to prevent. Reproduced deterministically (BLOCKED -> ACQUIRED). _live_lock now spans all three lifecycle transitions: open+register, unregister+close, and check+open+read+close. 2. Read-only connections keyed by URI spelling (BLOCKING). SessionDB's read-only path opens file:/…/state.db?mode=ro; that string was fed to Path.resolve(), producing /file:/…/state.db?mode=ro. No probe of the real Path could match, so read-only connections were invisible to the guard and their locks cancellable. Reproduced with no forced scheduling. Keys now come from PRAGMA database_list (canonical path), with an explicit tracking_path override. 3. Fail-open wrapper (HIGH). _connect_tracked_db() caught every exception and retried an untracked plain connect, so any error silently disabled the guard. Now only ImportError (scaffold installs without hermes_cli) falls back; real failures propagate. 4. Backup paths that warned and proceeded (MEDIUM). _backup_corrupt_db() and _backup_db_file() raw-read live databases; they now REFUSE when a connection is live rather than warning. Losing a forensic copy beats corrupting the database being rescued. Custom factories are no longer rejected (that broke legitimate callers) nor silently untracked -- the tracking close() is mixed into whatever factory is in play, including when an opener substitutes its own after the fact. WAL POLICY: #70055 is RESTORED, not reverted. My earlier justification was confounded -- the clean WAL result came from 3.53.1, which carries both the WAL-reset fix AND 3.51.0's broken-lock defenses, so it said nothing about the bundled 3.50.4. Re-measured on 3.50.4 with the lock fix in place: WAL 0/3 and DELETE 0/3, i.e. no evidence WAL is safer. Upstream still documents the WAL-reset bug through 3.51.2 as serious. Keeping new databases out of WAL until a fixed runtime ships is the conservative call, and the WAL policy does not belong in this root-cause fix. Six sabotage runs confirm each new test fails when its defect is reinstated (including two that initially did NOT -- the race test was rewritten to pause inside the byte read, and a separate test added for the opener-substituted factory path). 1124 targeted tests green. --- hermes_cli/kanban_db.py | 19 +- hermes_cli/sqlite_safe_read.py | 295 ++++++++++++++++------ hermes_state.py | 127 ++++++++-- tests/hermes_cli/test_kanban_db.py | 9 +- tests/hermes_cli/test_session_recovery.py | 7 +- tests/test_hermes_state.py | 11 +- tests/test_hermes_state_wal_fallback.py | 7 + tests/test_sqlite_lock_safe_inspection.py | 188 +++++++++++++- tests/test_sqlite_wal_reset_gate.py | 24 +- 9 files changed, 542 insertions(+), 145 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 5b8cd7417e88..75f922d0565c 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -1379,6 +1379,7 @@ def _sqlite_connect(path: Path) -> sqlite3.Connection: busy_timeout_ms = _resolve_busy_timeout_ms() conn = connect_tracked( path, + connect_fn=sqlite3.connect, isolation_level=None, timeout=busy_timeout_ms / 1000.0, ) @@ -1752,18 +1753,22 @@ def _backup_corrupt_db(path: Path) -> Optional[Path]: # This reads the whole DB file to fingerprint it. That is a close()-on-a- # database-file hazard (it cancels this process's POSIX advisory locks -- # see hermes_cli.sqlite_safe_read), so it must only run once the board has - # been taken out of service: every caller reaches here on the corrupt/ - # quarantine path, after the probe connection has been closed. If you add - # a caller, make sure no connection to this path is still open. + # been taken out of service. Every caller reaches here on the corrupt/ + # quarantine path after closing its probe connection, but another + # SessionDB/kanban connection elsewhere in the process would still be at + # risk -- so REFUSE rather than warn-and-proceed. Losing a forensic copy + # is strictly better than corrupting the live database we are trying to + # rescue. from hermes_cli.sqlite_safe_read import has_live_connection if has_live_connection(resolved): - _log.warning( - "quarantining %s while a connection to it is still open in this " - "process; close it first -- fingerprinting the file cancels the " - "process's POSIX locks on it", + _log.error( + "refusing to quarantine %s: a connection to it is still open in " + "this process, and fingerprinting the file would cancel that " + "connection's POSIX locks. Close all connections first.", resolved, ) + return None digest = hashlib.sha256() try: with resolved.open("rb") as handle: diff --git a/hermes_cli/sqlite_safe_read.py b/hermes_cli/sqlite_safe_read.py index d394ef93d354..e373a91c850f 100644 --- a/hermes_cli/sqlite_safe_read.py +++ b/hermes_cli/sqlite_safe_read.py @@ -32,9 +32,31 @@ The rules :func:`read_header_bytes_preopen`, which refuses once a connection has been registered for the path. -:func:`track_connection` / :func:`untrack_connection` maintain the registry of -paths with live connections, so rule 2 can be enforced rather than merely -documented. +Concurrency contract +-------------------- +The registry is not advisory bookkeeping -- it is the guard, so the +check and the byte read must be **atomic with respect to connection +lifecycle**. ``_live_lock`` is therefore held across three critical sections, +each of which spans the syscall *and* the registry mutation: + +* open + register (:func:`connect_tracked`) +* unregister + close (:meth:`TrackedConnection.close`) +* check + ``open``/``read``/``close`` (:func:`read_header_bytes_preopen`) + +Without that, a thread could pass the "no live connection" check, a second +thread could open a connection and take a write lock, and the first thread's +``close()`` would then cancel it -- reintroducing the exact bug this module +exists to prevent. The lock is never held while a caller *uses* a connection, +only across these transitions, so it does not serialise database work. + +Path identity +------------- +Connections are keyed by the **canonical database path**, resolved from +``PRAGMA database_list`` on the opened connection. The caller's spelling is +not trustworthy: ``SessionDB``'s read-only path opens +``file:/…/state.db?mode=ro`` with ``uri=True``, and treating that string as a +filesystem path yields a key like ``/file:/…/state.db?mode=ro`` which no +later probe of the real ``Path`` can ever match. """ from __future__ import annotations @@ -53,73 +75,59 @@ SQLITE_HEADER_MAGIC = b"SQLite format 3\x00" # Offset of the 4-byte big-endian page-count field in the SQLite header. _HEADER_PAGE_COUNT_OFFSET = 28 +# Guards BOTH the registry and the lifecycle syscalls it describes. Reentrant +# because connect_tracked -> _canonical_db_path -> ... stays on one thread. _live_lock = threading.RLock() -# resolved path -> number of live connections opened by this process +# canonical path -> number of live connections opened by this process _live_connections: dict[str, int] = {} +class UntrackableConnectionError(RuntimeError): + """A connection to a probe-able database could not be tracked. + + Raised rather than silently returning an untracked connection: on these + paths tracking is part of the correctness contract, not an optimisation. + """ + + def _key(path: Path | str) -> str: + """Canonicalise a *filesystem* path for use as a registry key.""" try: return str(Path(path).resolve()) except OSError: return str(path) +def _canonical_db_path(conn: sqlite3.Connection) -> Optional[str]: + """The on-disk path of ``main``, as SQLite itself reports it. + + Immune to the caller's spelling (``file:`` URIs, relative paths, symlinks). + Returns ``None`` for in-memory or unnamed databases, which cannot be + byte-probed and therefore need no tracking. + """ + try: + row = conn.execute("PRAGMA database_list").fetchone() + except sqlite3.Error: + return None + if not row or len(row) < 3: + return None + path_str = row[2] + if not path_str: + return None + return _key(path_str) + + def track_connection(path: Path | str) -> None: - """Record that this process now holds a connection to *path*.""" + """Record that this process now holds a connection to *path*. + + Prefer :func:`connect_tracked`; this exists for callers that manage their + own connection objects, and for tests. + """ key = _key(path) with _live_lock: _live_connections[key] = _live_connections.get(key, 0) + 1 -class TrackedConnection(sqlite3.Connection): - """A ``sqlite3.Connection`` that untracks its path exactly once on close. - - Counting opens is easy; counting closes reliably is not, because callers - close connections in many places (and some hand them to - ``contextlib.closing``). Subclassing the connection puts the decrement on - the one method every close path must go through, so the live-connection - registry cannot drift upward and permanently disable byte-probes. - - Note ``with conn:`` does NOT close a sqlite3 connection (it only commits or - rolls back), so this hook is not fired spuriously by transaction scopes. - """ - - _hermes_tracked_path: str | None = None - - def close(self) -> None: - path = getattr(self, "_hermes_tracked_path", None) - # Untrack before the actual close: even if close() raises, the - # descriptor is going away and holding the path open would wedge - # every future probe. - if path is not None: - self._hermes_tracked_path = None - untrack_connection(path) - super().close() - - -def connect_tracked(path: Path | str, **kwargs) -> sqlite3.Connection: - """``sqlite3.connect`` that registers the connection for the lifetime of the fd. - - Use for any connection to a database whose file might otherwise be - byte-probed (state.db, kanban.db). The registration is released - automatically on ``close()``. - - A caller (or a test double) that supplies its own ``factory`` wins: we - pass ``factory`` positionally-compatible via kwargs only when it is - absent, and simply skip tracking when the resulting object is not a - :class:`TrackedConnection`. Tracking is an optimisation for the probe - guard, never a correctness requirement for opening the database. - """ - if "factory" not in kwargs: - kwargs["factory"] = TrackedConnection - conn = sqlite3.connect(str(path), **kwargs) - if isinstance(conn, TrackedConnection): - conn._hermes_tracked_path = _key(path) - track_connection(path) - return conn - - def untrack_connection(path: Path | str) -> None: """Record that one connection to *path* has been closed.""" key = _key(path) @@ -137,6 +145,148 @@ def has_live_connection(path: Path | str) -> bool: return _key(path) in _live_connections +class _TrackingMixin: + """Untrack-on-close behaviour, mixable into any Connection subclass.""" + + _hermes_tracked_path: str | None = None + + def close(self) -> None: # type: ignore[misc] + with _live_lock: + path = getattr(self, "_hermes_tracked_path", None) + if path is not None: + self._hermes_tracked_path = None + untrack_connection(path) + super().close() # type: ignore[misc] + + +class TrackedConnection(_TrackingMixin, sqlite3.Connection): + """A ``sqlite3.Connection`` that untracks its path exactly once on close. + + Counting opens is easy; counting closes reliably is not, because callers + close connections in many places (and some hand them to + ``contextlib.closing``). Putting the decrement on ``close()`` — the one + method every close path must go through — keeps the registry from + drifting upward and permanently disabling byte-probes. + + The unregister and the real ``close()`` happen together under + ``_live_lock`` so a concurrent probe can never observe "no live + connection" while this descriptor is still open. + + Note ``with conn:`` does NOT close a sqlite3 connection (it only commits or + rolls back), so this hook is not fired spuriously by transaction scopes. + """ + + +_tracked_factory_cache: dict[type, type] = {} + + +def _tracking_factory(factory: type) -> type: + """Return *factory* augmented with untrack-on-close. + + Callers legitimately supply their own ``Connection`` subclasses (the test + suite uses them to simulate FTS5-less or pragma-failing runtimes). Rather + than refusing those — or silently leaving them untracked, which would + quietly unguard the database — we mix the tracking ``close()`` into the + caller's class so tracking is preserved either way. + """ + if factory is sqlite3.Connection: + return TrackedConnection + if issubclass(factory, _TrackingMixin): + return factory + cached = _tracked_factory_cache.get(factory) + if cached is None: + cached = type(f"Tracked{factory.__name__}", (_TrackingMixin, factory), {}) + _tracked_factory_cache[factory] = cached + return cached + + +def connect_tracked( + path: Path | str, + *, + tracking_path: Path | str | None = None, + connect_fn=None, + **kwargs, +) -> sqlite3.Connection: + """``sqlite3.connect`` that registers the connection for the lifetime of the fd. + + Use for any connection to a database whose file might otherwise be + byte-probed (``state.db``, ``kanban.db``). The registration is released + automatically on ``close()``. + + The open and the registration happen together under ``_live_lock``, so a + concurrent :func:`read_header_bytes_preopen` cannot slip between them and + cancel this connection's locks. + + The registry key is the canonical path reported by ``PRAGMA + database_list`` -- not *path*, which may be a ``file:`` URI. Pass + ``tracking_path`` to override when the caller already knows the real path. + + ``connect_fn`` lets a caller supply its own opener (defaults to + :func:`sqlite3.connect`), so a module that owns the connection — and any + test that patches that module's ``sqlite3.connect`` — keeps control of how + the connection is created while this helper owns tracking. + + A caller-supplied ``factory`` is honoured but is transparently augmented + with untrack-on-close, so tracking is never silently skipped. If a + file-backed connection still cannot be tracked, + :class:`UntrackableConnectionError` is raised rather than handing back a + connection whose database has quietly lost byte-probe protection. + """ + opener = connect_fn if connect_fn is not None else sqlite3.connect + kwargs["factory"] = _tracking_factory(kwargs.get("factory", sqlite3.Connection)) + + with _live_lock: + conn = opener(str(path), **kwargs) + try: + resolved = ( + _key(tracking_path) + if tracking_path is not None + else _canonical_db_path(conn) + ) + if resolved is None: + # In-memory / unnamed: nothing on disk to byte-probe. + return conn + if not isinstance(conn, _TrackingMixin): + # The opener substituted its own factory and discarded ours + # (test doubles simulating FTS5-less runtimes do this). Retag + # the instance's class with the tracking mixin so close() still + # releases the registry entry, rather than handing back a + # connection whose database has silently lost probe safety. + conn = _retrofit_tracking(conn, resolved) + conn._hermes_tracked_path = resolved + _live_connections[resolved] = _live_connections.get(resolved, 0) + 1 + return conn + except Exception: + try: + # Close via sqlite3 directly: the tracking entry was either + # never made or is being unwound here. + sqlite3.Connection.close(conn) + except Exception: + pass + raise + + +def _retrofit_tracking(conn: sqlite3.Connection, resolved: str) -> sqlite3.Connection: + """Give an already-open connection untrack-on-close semantics. + + ``sqlite3.Connection`` subclasses are ordinary Python classes, so the + instance's ``__class__`` can be swapped for one that mixes in the tracking + ``close()``. Used when an opener ignored the factory we asked for. + """ + cls = type(conn) + if issubclass(cls, _TrackingMixin): + return conn + try: + conn.__class__ = _tracking_factory(cls) # type: ignore[assignment] + return conn + except TypeError as exc: + raise UntrackableConnectionError( + f"connection to {resolved} uses factory {cls.__name__}, which " + "cannot release its tracking entry on close; byte-probe safety " + "for this database would be silently lost" + ) from exc + + def page_count_bytes(conn: sqlite3.Connection) -> Optional[int]: """Logical database size in bytes, read through *conn*. @@ -173,15 +323,9 @@ def file_length_matches_header(conn: sqlite3.Connection) -> Optional[bool]: file, so the main file legitimately lags. Callers must treat this as advisory unless the database is in a rollback journal mode. """ - try: - row = conn.execute("PRAGMA database_list").fetchone() - except sqlite3.Error: + path_str = _canonical_db_path(conn) + if path_str is None: return None - if row is None: - return None - path_str = row[2] if len(row) > 2 else "" - if not path_str: - return None # in-memory or unnamed database logical = page_count_bytes(conn) if not logical: @@ -208,18 +352,23 @@ def read_header_bytes_preopen( ``None`` is returned, because the ``close()`` would cancel that connection's POSIX locks. + The registry check and the ``open``/``read``/``close`` are performed + together under ``_live_lock``, so a connection cannot be opened in the + window between deciding "nothing is live" and closing this descriptor. + Set ``force=True`` only for genuinely offline files (quarantined copies, snapshot artifacts, archives) that no live connection can reference. """ - if not force and has_live_connection(path): - logger.debug( - "refusing byte-level read of %s: a live connection exists in this " - "process and close() would cancel its POSIX locks", - path, - ) - return None - try: - with open(path, "rb") as handle: - return handle.read(length) - except OSError: - return None + with _live_lock: + if not force and _key(path) in _live_connections: + logger.debug( + "refusing byte-level read of %s: a live connection exists in " + "this process and close() would cancel its POSIX locks", + path, + ) + return None + try: + with open(path, "rb") as handle: + return handle.read(length) + except OSError: + return None diff --git a/hermes_state.py b/hermes_state.py index 343c2b6858dd..2a4af57978b4 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -528,18 +528,20 @@ def apply_wal_with_fallback( log one WARNING explaining why. On SQLite builds that still contain the WAL-reset corruption bug - (issue #69784), Hermes previously refused WAL and forced DELETE on - fresh databases. That mitigation is REVERTED: measured against - Hermes' own multi-process write paths, DELETE is the mode that - corrupts (a bare open()/close() on the DB file cancels this process's - POSIX advisory locks -- including a running VACUUM's EXCLUSIVE lock -- - and DELETE-mode rollback journals have no second line of defence). - WAL survived the same harness cleanly. Upstream also rates the - WAL-reset race at or below the background rate of SSD failure and - could not reproduce it without patching SQLite to force the window, - whereas the lock-cancellation path reproduces on demand. The real fix - is to never open() a live database file -- see - ``hermes_cli.sqlite_safe_read``. + (issue #69784), refuse to enable WAL on fresh / non-WAL databases + (prefer DELETE). If the on-disk DB is already WAL, keep WAL and warn + — never live-downgrade under possible concurrent openers. + + This gate (#70055) is deliberately RETAINED. An earlier revision of the + lock-cancellation fix (#71724) reverted it on the theory that DELETE was + "the mode that corrupts", but that comparison was confounded: the clean + WAL result came from SQLite 3.53.1, which carries BOTH the WAL-reset fix + AND 3.51.0's defenses against close()-broken POSIX locks, so it says + nothing about 3.50.4. Re-measured on the actually-bundled 3.50.4 with + the lock fix in place, WAL and DELETE are both clean (0/3 each) — i.e. + there is no evidence that WAL is safer here, and upstream still documents + the WAL-reset bug as real through 3.51.2 with serious consequences. Until + a fixed runtime is delivered, keep new databases out of WAL. The WARNING is deduplicated per ``db_label``: repeated connections to the same underlying DB (e.g. kanban_db.connect() which is called @@ -551,12 +553,12 @@ def apply_wal_with_fallback( both databases get identical fallback behavior. Never downgrades to DELETE if the on-disk DB header reports WAL — see - _on_disk_journal_mode. + _on_disk_journal_mode. That holds for both the NFS path and the + WAL-reset vulnerability path. """ - # Vulnerable SQLite still gets WAL: see the docstring. We warn once so - # the operator can upgrade the runtime, but we do NOT force DELETE. + # Vulnerable SQLite: do not enable WAL on new/non-WAL files. if is_sqlite_wal_reset_vulnerable(): - _log_wal_reset_bug_once(db_label, kept_wal=True) + return _apply_delete_for_wal_reset_bug(conn, db_label=db_label) # Read-only probe — no flock, no checkpoint, no WAL/SHM unlink. # Skipping the set-pragma prevents WAL-init from unlinking files other connections hold open. @@ -588,6 +590,41 @@ def apply_wal_with_fallback( return "delete" +def _apply_delete_for_wal_reset_bug( + conn: sqlite3.Connection, + *, + db_label: str, +) -> str: + """Avoid enabling WAL when the linked SQLite has the WAL-reset bug. + + - Already-WAL on disk: leave WAL alone (no live downgrade) and warn. + - Otherwise: set DELETE and warn. + """ + current = "" + try: + row = conn.execute("PRAGMA journal_mode").fetchone() + if row and row[0] is not None: + current = str(row[0]).strip().lower() + except sqlite3.OperationalError: + current = "" + + if current == "wal": + # Do not TRUNCATE / journal_mode=DELETE while other processes may + # still hold this WAL DB open — same safety rule as the NFS path. + _log_wal_reset_bug_once(db_label, kept_wal=True) + _apply_macos_checkpoint_barrier(conn) + _enforce_macos_synchronous_full(conn) + return "wal" + + try: + conn.execute("PRAGMA journal_mode=DELETE") + except sqlite3.OperationalError: + # Best-effort: DELETE is usually already the default for new files. + pass + _log_wal_reset_bug_once(db_label, kept_wal=False) + return "delete" + + def _log_wal_reset_bug_once( db_label: str, *, @@ -599,9 +636,8 @@ def _log_wal_reset_bug_once( return _wal_reset_bug_warned_paths.add(db_label) action = ( - "keeping WAL — WAL is the safer mode here (forcing DELETE was " - "reverted; DELETE is what corrupts under Hermes' concurrent " - "writers)" + "is already in WAL mode — leaving WAL in place (no live " + "downgrade under concurrent openers)" if kept_wal else "using journal_mode=DELETE instead of enabling WAL" ) @@ -706,10 +742,30 @@ def _backup_db_file(db_path: Path) -> Optional[Path]: Raw file copy on purpose: the DB won't open cleanly, so we preserve the bytes exactly for forensics / manual restore. WAL and SHM sidecars are copied too when present. Returns the backup path, or None on failure. + + Refuses when a connection to this database is still live in the process: + reading the file would ``close()`` a descriptor for it and cancel that + connection's POSIX advisory locks (see ``hermes_cli.sqlite_safe_read``). + The repair path can be entered by one SessionDB while the gateway holds + others, so this is a real possibility rather than a theoretical one. """ import datetime import shutil + try: + from hermes_cli.sqlite_safe_read import has_live_connection + except ImportError: + has_live_connection = None # type: ignore[assignment] + + if has_live_connection is not None and has_live_connection(db_path): + logger.error( + "Refusing to raw-copy %s for backup: a connection to it is still " + "open in this process and the copy would cancel that connection's " + "POSIX locks. Close all SessionDB handles first.", + db_path, + ) + return None + stamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S") backup_path = db_path.with_name(f"{db_path.name}.malformed-backup-{stamp}") try: @@ -1551,22 +1607,40 @@ class CompressionSessionBusyError(RuntimeError): """A non-owner tried to write while compression owns the session.""" -def _connect_tracked_db(path, **kwargs): +def _connect_tracked_db(path, tracking_path=None, **kwargs): """``sqlite3.connect`` that registers the open fd for lock-safety. While a connection is live, byte-level probes of the same file are refused: an ``open()``/``close()`` cancels every POSIX advisory lock this - process holds on it -- including a running VACUUM's EXCLUSIVE lock. The - registration is released automatically on ``close()``. Falls back to a - plain connect if the helper is unavailable (scaffold/embed installs). + process holds on it -- including a running VACUUM's EXCLUSIVE lock. + Released automatically on ``close()``. + + The ONLY tolerated fallback is the helper being absent entirely + (scaffold/embed installs that ship hermes_state without hermes_cli). A + real connection failure must propagate: silently retrying an *untracked* + connect would disable the guard for the lifetime of that connection, + which is precisely the failure mode this module exists to prevent. """ try: from hermes_cli.sqlite_safe_read import connect_tracked - - return connect_tracked(path, **kwargs) - except Exception: + except ImportError: + logger.debug( + "hermes_cli.sqlite_safe_read unavailable; opening %s untracked " + "(byte-probe guard inactive in this install)", + path, + ) return sqlite3.connect(str(path), **kwargs) + # Open through THIS module's sqlite3.connect so callers (and tests) that + # patch hermes_state.sqlite3.connect keep control of connection creation; + # the helper still owns tracking. + return connect_tracked( + path, + tracking_path=tracking_path, + connect_fn=sqlite3.connect, + **kwargs, + ) + def is_zeroed_state_db( path: Path, *, probe_bytes: int = 100, force: bool = False @@ -1797,6 +1871,7 @@ class SessionDB: # the caller degrades per-profile. self._conn = _connect_tracked_db( f"file:{self.db_path}?mode=ro", + tracking_path=self.db_path, uri=True, check_same_thread=False, timeout=1.0, diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 21c6d857d08f..f4e552352554 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -3276,17 +3276,14 @@ def test_connect_falls_back_to_delete_on_locking_protocol(tmp_path, monkeypatch, return super().execute(sql, *args, **kwargs) def wal_blocking_connect(*args, **kwargs): - # connect_tracked supplies its own factory; the caller's wins, and it - # skips tracking for non-TrackedConnection results. + # connect_tracked passes a tracking-augmented factory; drop it and + # substitute the double, which connect_tracked will re-augment. kwargs.pop("factory", None) return real_connect( *args, factory=_WalBlockingConnection, **kwargs ) - with _patch( - "hermes_cli.sqlite_safe_read.sqlite3.connect", - side_effect=wal_blocking_connect, - ): + with _patch("hermes_cli.kanban_db.sqlite3.connect", side_effect=wal_blocking_connect): with caplog.at_level("WARNING", logger="hermes_state"): conn = kb.connect() diff --git a/tests/hermes_cli/test_session_recovery.py b/tests/hermes_cli/test_session_recovery.py index 6b82635ab169..ad19530846dc 100644 --- a/tests/hermes_cli/test_session_recovery.py +++ b/tests/hermes_cli/test_session_recovery.py @@ -126,9 +126,8 @@ def test_recovery_rebuilds_canonical_data_without_opening_source( source_hash = _sha256(source) source_stat = source.stat() - # A recovered DB is born in WAL even on a WAL-reset-vulnerable runtime: - # forcing DELETE there was reverted (DELETE is the mode that corrupts - # under Hermes' concurrent writers -- see hermes_cli.sqlite_safe_read). + # Exercise the vulnerable-runtime fallback: a fresh recovered DB must be + # born in DELETE mode instead of enabling WAL (#70055, retained). monkeypatch.setattr( hermes_state, "is_sqlite_wal_reset_vulnerable", @@ -145,7 +144,7 @@ def test_recovery_rebuilds_canonical_data_without_opening_source( assert report["complete"] is True assert report["installed"] is False assert report["source_unchanged"] is True - assert report["verification"]["journal_mode"] == "wal" + assert report["verification"]["journal_mode"] == "delete" assert report["verification"]["integrity_check"] == ["ok"] assert report["verification"]["foreign_key_check"] == [] assert report["verification"]["schema_version"] == SCHEMA_VERSION diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index d207aa9e0cfe..bd53684ed58e 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -3893,14 +3893,15 @@ class TestSanitizeTitle: class TestSchemaInit: def test_wal_mode(self, db): - """WAL is used regardless of the linked SQLite's WAL-reset status. + """Prefer WAL on fixed SQLite; DELETE on WAL-reset-vulnerable builds (#69784).""" + from hermes_state import is_sqlite_wal_reset_vulnerable - Forcing DELETE on vulnerable builds (#69784) was reverted: DELETE is - the mode that actually corrupts under Hermes' concurrent writers. - """ cursor = db._conn.execute("PRAGMA journal_mode") mode = cursor.fetchone()[0].lower() - assert mode == "wal" + if is_sqlite_wal_reset_vulnerable(): + assert mode == "delete" + else: + assert mode == "wal" def test_foreign_keys_enabled(self, db): cursor = db._conn.execute("PRAGMA foreign_keys") diff --git a/tests/test_hermes_state_wal_fallback.py b/tests/test_hermes_state_wal_fallback.py index 73141ce43885..3bccd9b32ec7 100644 --- a/tests/test_hermes_state_wal_fallback.py +++ b/tests/test_hermes_state_wal_fallback.py @@ -308,6 +308,9 @@ class TestGetLastInitError: return super().execute(sql, *args, **kwargs) def gated_connect(*args, **kwargs): + # connect_tracked passes a tracking-augmented factory; drop it and + # substitute the double, which connect_tracked will re-augment. + kwargs.pop("factory", None) return real_connect(str(target), factory=_BothPragmasFailConnection, **kwargs) with patch("hermes_state.sqlite3.connect", side_effect=gated_connect): @@ -359,6 +362,10 @@ class TestSessionDbUsesWalFallback: factory = _make_blocking_factory("locking protocol", attempts) def gated_connect(*args, **kwargs): + # connect_tracked passes a tracking-augmented factory; drop it and + # substitute the double, which connect_tracked re-applies to the + # returned instance. + kwargs.pop("factory", None) return real_connect(str(target), factory=factory, **kwargs) with patch("hermes_state.sqlite3.connect", side_effect=gated_connect): diff --git a/tests/test_sqlite_lock_safe_inspection.py b/tests/test_sqlite_lock_safe_inspection.py index 792a320a8f15..8a196417d582 100644 --- a/tests/test_sqlite_lock_safe_inspection.py +++ b/tests/test_sqlite_lock_safe_inspection.py @@ -22,6 +22,7 @@ import sqlite3 import subprocess import sys import textwrap +import threading import pytest @@ -219,11 +220,125 @@ def test_tracking_registry_does_not_leak_across_close_paths(tmp_path, clean_regi assert not has_live_connection(db) -def test_caller_supplied_connection_factory_still_works(tmp_path, clean_registry): - """A caller's own factory wins; tracking is skipped rather than crashing. +def test_probe_and_connect_do_not_race(tmp_path, clean_registry, monkeypatch): + """The check and the raw read must be atomic w.r.t. connection lifecycle. - Tracking is an optimisation for the probe guard, never a precondition for - opening the database — passing a custom factory must not raise. + Deterministic interleaving: pause the probe *inside* the byte read — after + it has decided "nothing is live" and opened its descriptor — then let + another thread open a tracked connection and take a write lock, then let + the probe close. + + With the guard holding ``_live_lock`` across check+open+read+close, the + other thread blocks on that lock until the probe is done, so no + interleaving is possible. If the lock is only held across the check, that + thread slips in and the probe's ``close()`` cancels its POSIX locks. + """ + import hermes_cli.sqlite_safe_read as ssr + + db = tmp_path / "state.db" + _make_db(db, "DELETE") + + inside_read = threading.Event() + may_close = threading.Event() + failures: list[str] = [] + real_open = open + + def slow_open(*args, **kwargs): + handle = real_open(*args, **kwargs) + inside_read.set() + # Hold the descriptor open while the writer tries to get in. + may_close.wait(10) + return handle + + def writer(): + inside_read.wait(10) + # If the guard is correct this blocks until the probe releases the + # lock; if not, it opens and locks inside the probe's window. + conn = ssr.connect_tracked(db, isolation_level=None, timeout=0.5) + try: + conn.execute("BEGIN IMMEDIATE") + conn.execute("INSERT INTO t(v) VALUES ('holder')") + may_close.set() # let the probe's close() land + if _external_writer_can_break_in(db): + failures.append( + "external writer broke in: the probe's close() cancelled " + "this connection's POSIX locks" + ) + conn.execute("COMMIT") + except sqlite3.Error as exc: # a cancelled lock surfaces here too + failures.append(f"holder transaction failed: {exc}") + finally: + conn.close() + + monkeypatch.setattr(ssr, "open", slow_open, raising=False) + t = threading.Thread(target=writer, daemon=True) + t.start() + try: + read_header_bytes_preopen(db, length=16) + finally: + may_close.set() # never wedge the probe if the writer died + t.join(20) + + assert not failures, failures[0] + + +def test_read_only_uri_connection_is_tracked_by_real_path(tmp_path, clean_registry): + """A ``file:...?mode=ro`` connection must register under the real path. + + SessionDB's read-only path opens a URI, not a filesystem path. Keying the + registry on the caller's spelling produces something like + ``/file:/…/state.db?mode=ro``, which no probe of the actual Path can + match -- leaving the read-only connection invisible to the guard and its + locks cancellable. + """ + from hermes_cli.sqlite_safe_read import connect_tracked + + db = tmp_path / "state.db" + _make_db(db, "DELETE") + + conn = connect_tracked( + f"file:{db}?mode=ro", uri=True, isolation_level=None, timeout=0.5 + ) + try: + assert has_live_connection(db), ( + "read-only URI connection was registered under a URI-shaped key; " + "a probe of the real path cannot see it" + ) + assert read_header_bytes_preopen(db, length=16) is None + finally: + conn.close() + + assert not has_live_connection(db) + + +def test_session_db_read_only_is_tracked(tmp_path, clean_registry, monkeypatch): + """End-to-end: a real read-only SessionDB blocks byte-probes.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from hermes_state import SessionDB + + db_path = tmp_path / "state.db" + seed = SessionDB(db_path=db_path) + seed.create_session("s1", source="cli") + seed.close() + + ro = SessionDB(db_path=db_path, read_only=True) + try: + assert has_live_connection(db_path) + assert read_header_bytes_preopen(db_path, length=16) is None + finally: + ro.close() + + assert not has_live_connection(db_path) + assert read_header_bytes_preopen(db_path, length=16) is not None + + +def test_custom_factory_is_honoured_and_still_tracked(tmp_path, clean_registry): + """A caller's factory must work AND keep byte-probe protection. + + Callers legitimately pass their own Connection subclasses (the suite uses + them to simulate FTS5-less runtimes). Neither rejecting them nor silently + leaving them untracked is acceptable — the former breaks real callers, the + latter quietly unguards the database. The factory is augmented instead. """ from hermes_cli.sqlite_safe_read import connect_tracked @@ -235,8 +350,71 @@ def test_caller_supplied_connection_factory_still_works(tmp_path, clean_registry conn = connect_tracked(db, factory=CustomConnection) try: - assert isinstance(conn, CustomConnection) + assert isinstance(conn, CustomConnection), "caller's factory must be honoured" assert conn.execute("SELECT COUNT(*) FROM t").fetchone()[0] == 200 + assert has_live_connection(db), "custom factory must still be tracked" + assert read_header_bytes_preopen(db, length=16) is None + finally: + conn.close() + + assert not has_live_connection(db), "custom factory must untrack on close" + + +def test_opener_that_discards_our_factory_is_still_tracked(tmp_path, clean_registry): + """Tracking must survive an opener that substitutes its own factory. + + Two distinct paths reach a custom Connection subclass: the caller passing + ``factory=`` (augmented before the open) and an opener that ignores the + factory we asked for and supplies its own (retrofitted after the open). + The suite's FTS5-less doubles take the second path, so it needs its own + coverage -- otherwise a regression there is invisible. + """ + from hermes_cli.sqlite_safe_read import connect_tracked + + class CustomConnection(sqlite3.Connection): + pass + + db = tmp_path / "state.db" + _make_db(db, "DELETE") + + def opener_that_ignores_factory(path, **kwargs): + kwargs.pop("factory", None) + return sqlite3.connect(path, factory=CustomConnection, **kwargs) + + conn = connect_tracked(db, connect_fn=opener_that_ignores_factory) + try: + assert isinstance(conn, CustomConnection), "opener's factory must survive" + assert has_live_connection(db), ( + "connection opened with a substituted factory was left untracked; " + "its database silently lost byte-probe protection" + ) + assert read_header_bytes_preopen(db, length=16) is None + finally: + conn.close() + + assert not has_live_connection(db), "retrofitted connection must untrack on close" + + +def test_session_db_read_only_tracks_under_canonical_path(tmp_path, clean_registry): + """The read-only URI must resolve to the real path even without a hint. + + ``SessionDB`` passes ``tracking_path`` explicitly, but the helper must not + depend on that: ``PRAGMA database_list`` is the authority. This exercises + the no-hint path directly so removing the fallback is caught. + """ + from hermes_cli.sqlite_safe_read import connect_tracked + + db = tmp_path / "state.db" + _make_db(db, "DELETE") + + conn = connect_tracked( + f"file:{db}?mode=ro", uri=True, isolation_level=None, timeout=0.5 + ) + try: + assert has_live_connection(db), ( + "canonical-path resolution failed: the URI spelling was used as " + "the registry key" + ) finally: conn.close() diff --git a/tests/test_sqlite_wal_reset_gate.py b/tests/test_sqlite_wal_reset_gate.py index 53756402f8da..951f4e06fdd4 100644 --- a/tests/test_sqlite_wal_reset_gate.py +++ b/tests/test_sqlite_wal_reset_gate.py @@ -58,30 +58,16 @@ class TestIsSqliteWalResetVulnerable: class TestApplyWalWalResetGate: - def test_fresh_db_gets_wal_even_when_vulnerable(self, tmp_path, monkeypatch, caplog): - """Forcing DELETE on vulnerable SQLite was reverted. - - Measured against Hermes' own concurrent write paths, DELETE is the - mode that corrupts: a bare open()/close() on the DB file cancels this - process's POSIX advisory locks (including a running VACUUM's EXCLUSIVE - lock) and a rollback journal has no second line of defence. WAL - survived the same harness. We still warn so the operator can upgrade - the runtime, but we no longer steer them into the failing mode. - """ + def test_fresh_db_uses_delete_when_vulnerable(self, tmp_path, monkeypatch, caplog): monkeypatch.setattr( hermes_state, "is_sqlite_wal_reset_vulnerable", lambda version_info=None: True ) conn = sqlite3.connect(str(tmp_path / "fresh.db")) with caplog.at_level("WARNING", logger="hermes_state"): mode = apply_wal_with_fallback(conn, db_label="fresh.db") - assert mode == "wal" - assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "wal" - # The operator is still told to upgrade the runtime... - assert any("WAL-reset" in r.getMessage() for r in caplog.records) - # ...but we never announce a downgrade to DELETE. - assert not any( - "instead of enabling WAL" in r.getMessage() for r in caplog.records - ) + assert mode == "delete" + assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "delete" + assert any("instead of enabling WAL" in r.getMessage() for r in caplog.records) conn.close() def test_existing_wal_left_alone_when_vulnerable( @@ -109,7 +95,7 @@ class TestApplyWalWalResetGate: assert mode == "wal" assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "wal" assert conn.execute("SELECT x FROM t").fetchone()[0] == 42 - assert any("WAL-reset" in r.getMessage() for r in caplog.records) + assert any("already in WAL mode" in r.getMessage() for r in caplog.records) # Must not attempt a live journal_mode flip. assert not any( "instead of enabling WAL" in r.getMessage() for r in caplog.records