fix(cli): stop doctor's journal-mode probe from cancelling live SQLite locks

hermes_cli/doctor.py

@@ -118,18 +118,48 @@ def _hermes_database_paths(hermes_home: Path) -> list[tuple[str, Path]]:
 _SQLITE_HEADER_MAGIC = b"SQLite format 3\x00"
 
 
+def _unreadable_reason(db_path: Path) -> str:
+    """Explain why a database file could not be read, without opening it.
+
+    ``read_header_bytes_preopen`` collapses every ``OSError`` into ``None``,
+    but doctor's job is to say *which* problem it hit. ``stat()`` and
+    ``access()`` answer that from directory metadata alone — neither takes a
+    file descriptor, so neither can cancel the file's POSIX advisory locks.
+    """
+    try:
+        db_path.stat()
+    except OSError as exc:
+        return str(exc)
+    if not os.access(db_path, os.R_OK):
+        return f"permission denied: {db_path}"
+    return "file could not be read"
+
+
 def _read_journal_mode(db_path: Path) -> tuple[str | None, str | None]:
     """Return (journal mode, error) from the file header without opening the database.
 
     Header byte 18 is 2 for WAL and 1 for a rollback journal. Opening the
     database through the SQLite engine — even read-only — creates -wal/-shm
     sidecar files, which a diagnostic must not do.
+
+    The byte read is routed through ``read_header_bytes_preopen`` rather than
+    a bare ``open()``: closing *any* descriptor for a database file cancels
+    this process's POSIX advisory locks on it, so a raw read would drop the
+    locks a live connection is holding (see ``hermes_cli.sqlite_safe_read``).
+    ``run_doctor`` is also called in-process by the dashboard console, which
+    holds live ``SessionDB`` connections. The helper refuses in that case and
+    the mode is reported as unreadable instead.
     """
-    try:
-        with open(db_path, "rb") as fh:
-            header = fh.read(20)
-    except OSError as exc:
-        return None, str(exc)
+    from hermes_cli.sqlite_safe_read import (
+        has_live_connection,
+        read_header_bytes_preopen,
+    )
+
+    header = read_header_bytes_preopen(db_path, length=20)
+    if header is None:
+        if has_live_connection(db_path):
+            return None, "database is open in this process"
+        return None, _unreadable_reason(db_path)
     if len(header) == 0:
         return None, "file is empty"
     if len(header) < 20 or not header.startswith(_SQLITE_HEADER_MAGIC):

tests/hermes_cli/test_doctor_journal_modes.py

@@ -14,6 +14,12 @@
 import pytest
 
 import hermes_cli.doctor as doctor
+from hermes_cli.sqlite_safe_read import (
+    connect_tracked,
+    has_live_connection,
+    track_connection,
+    untrack_connection,
+)
 
 VULNERABLE = (3, 50, 4)
 FIXED_VERSIONS = [(3, 51, 3), (3, 52, 0), (3, 50, 7), (3, 44, 6)]
@@ -38,6 +44,28 @@ def _sidecars(directory):
     )
 
 
+@pytest.fixture
+def clean_registry():
+    """Isolate a test from the module-level connection registry.
+
+    Clears on both sides, not just teardown: a test that leaks a tracked
+    connection (an earlier failure, or a test that does not take this
+    fixture) would otherwise leave the registry dirty and make the *next*
+    test's refusal assertion pass for the wrong reason.
+    """
+    import hermes_cli.sqlite_safe_read as mod
+
+    def _clear():
+        with mod._live_lock:
+            mod._live_connections.clear()
+
+    _clear()
+    try:
+        yield
+    finally:
+        _clear()
+
+
 class TestReadJournalMode:
     def test_reads_wal(self, tmp_path):
         db = tmp_path / "state.db"
@@ -141,6 +169,156 @@ def test_does_not_mutate_database_files(self, tmp_path):
         assert _sidecars(tmp_path) == []
 
 
+class TestLiveConnectionSafety:
+    """The probe must not raw-open a database this process has connections to.
+
+    close() on any descriptor cancels every POSIX advisory lock the process
+    holds on that file, so a byte-probe run while a connection is live drops
+    that connection's locks — including the EXCLUSIVE lock a VACUUM holds
+    mid-rewrite. run_doctor is reachable in-process (the dashboard console
+    imports and calls it directly while holding live SessionDB connections),
+    so the probe must defer to the registry rather than open the file.
+    """
+
+    def test_probe_is_refused_while_a_tracked_connection_is_live(
+        self, tmp_path, clean_registry
+    ):
+        db = tmp_path / "state.db"
+        _make_db(db, journal_mode="WAL")
+
+        track_connection(db)
+        try:
+            assert has_live_connection(db)
+
+            mode, error = doctor._read_journal_mode(db)
+
+            assert mode is None
+            assert error == "database is open in this process"
+        finally:
+            untrack_connection(db)
+
+    def test_probe_is_refused_for_a_real_tracked_connection(
+        self, tmp_path, clean_registry
+    ):
+        """The same, through connect_tracked — the path SessionDB actually takes."""
+        db = tmp_path / "state.db"
+        _make_db(db, journal_mode="WAL")
+
+        conn = connect_tracked(db)
+        try:
+            assert has_live_connection(db)
+
+            mode, error = doctor._read_journal_mode(db)
+
+            assert mode is None
+            assert error == "database is open in this process"
+        finally:
+            conn.close()
+
+    def test_probe_resumes_once_the_connection_closes(self, tmp_path, clean_registry):
+        db = tmp_path / "state.db"
+        _make_db(db, journal_mode="WAL")
+
+        conn = connect_tracked(db)
+        assert doctor._read_journal_mode(db)[0] is None
+        conn.close()
+
+        assert not has_live_connection(db)
+        assert doctor._read_journal_mode(db) == ("wal", None)
+
+    def test_refusal_creates_no_new_sidecars(self, tmp_path, clean_registry):
+        db = tmp_path / "state.db"
+        _make_db(db, journal_mode="WAL")
+
+        conn = connect_tracked(db)
+        try:
+            before = _sidecars(tmp_path)
+
+            doctor._read_journal_mode(db)
+
+            assert _sidecars(tmp_path) == before
+        finally:
+            conn.close()
+
+    def test_report_degrades_instead_of_probing_a_live_database(
+        self, tmp_path, capsys, clean_registry
+    ):
+        db = tmp_path / "state.db"
+        _make_db(db, journal_mode="WAL")
+
+        conn = connect_tracked(db)
+        try:
+            doctor._report_database_journal_modes(tmp_path, VULNERABLE)
+        finally:
+            conn.close()
+
+        out = capsys.readouterr().out
+        assert "state.db: journal mode could not be read" in out
+        assert "database is open in this process" in out
+        assert "cannot rule out WAL exposure" in out
+
+    def test_an_untracked_lock_holder_does_not_block_the_probe(self, tmp_path):
+        """Only this process's *registered* connections gate the read.
+
+        A plain sqlite3.connect elsewhere is not in the registry, and a lock
+        held by another process is irrelevant — neither can be cancelled by a
+        close() we never perform. Guards against over-correcting into refusing
+        every read.
+        """
+        db = tmp_path / "state.db"
+        _make_db(db)
+        holder = sqlite3.connect(db, isolation_level=None)
+        try:
+            holder.execute("BEGIN EXCLUSIVE")
+
+            assert doctor._read_journal_mode(db) == ("rollback", None)
+        finally:
+            holder.close()
+
+
+class TestUnreadableReason:
+    def test_missing_file_keeps_the_os_error_text(self, tmp_path):
+        reason = doctor._unreadable_reason(tmp_path / "gone.db")
+
+        assert "No such file or directory" in reason
+
+    @pytest.mark.skipif(os.name == "nt", reason="chmod is a no-op on Windows")
+    @pytest.mark.skipif(
+        # os.geteuid is POSIX-only, and a skipif condition is evaluated at
+        # collection time — calling it unguarded would raise AttributeError
+        # and take the whole module down on Windows.
+        hasattr(os, "geteuid") and os.geteuid() == 0,
+        reason="root ignores file permissions",
+    )
+    def test_unreadable_file_is_reported_as_permission_denied(self, tmp_path):
+        db = tmp_path / "state.db"
+        _make_db(db)
+        os.chmod(db, 0o000)
+        try:
+            mode, error = doctor._read_journal_mode(db)
+        finally:
+            os.chmod(db, 0o644)
+
+        assert mode is None
+        assert "permission denied" in error.lower()
+
+    def test_reason_does_not_open_the_file(self, tmp_path, monkeypatch):
+        """_unreadable_reason must answer from metadata only.
+
+        It runs on database paths, so taking a descriptor would reintroduce
+        the very close() this module's guard exists to prevent.
+        """
+        db = tmp_path / "state.db"
+        _make_db(db)
+
+        def _fail(*args, **kwargs):
+            raise AssertionError("_unreadable_reason must not open the file")
+
+        monkeypatch.setattr("builtins.open", _fail)
+
+        assert doctor._unreadable_reason(db) == "file could not be read"
+
+
 class TestReportDatabaseJournalModes:
     def test_vulnerable_runtime_wal_db_is_exposed(self, tmp_path, capsys):
         _make_db(tmp_path / "state.db", journal_mode="WAL")