diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index e16de6060c9e..dd00f42c871f 100644 --- a/hermes_cli/doctor.py +++ b/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): diff --git a/tests/hermes_cli/test_doctor_journal_modes.py b/tests/hermes_cli/test_doctor_journal_modes.py index acc56ff0ea2a..55828de63134 100644 --- a/tests/hermes_cli/test_doctor_journal_modes.py +++ b/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")