mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-07-27 17:58:07 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 7

fix(state): make the byte-probe guard atomic, path-correct, and fail-closed

Browse source 
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 <cwd>/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.
This commit is contained in:
teknium1 2026-07-25 21:21:36 -07:00 • committed by Teknium
parent 95fb477856
commit fe431651c5
9 changed files with 542 additions and 145 deletions
Download patch file Download diff file Expand all files Collapse all files

19
hermes_cli/kanban_db.py
View file

@ -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:

295
hermes_cli/sqlite_safe_read.py
View file

@ -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 ``<cwd>/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

127
hermes_state.py
View file

@ -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,

9
tests/hermes_cli/test_kanban_db.py
View file

@ -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()

7
tests/hermes_cli/test_session_recovery.py
View file

@ -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

11
tests/test_hermes_state.py
View file

@ -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")

7
tests/test_hermes_state_wal_fallback.py
View file

@ -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):

188
tests/test_sqlite_lock_safe_inspection.py
View file

@ -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
``<cwd>/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()

24
tests/test_sqlite_wal_reset_gate.py
View file

@ -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