diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index a7dd5fe4b0..699b8017f8 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -12089,7 +12089,7 @@ def _open_session_db_at_path(db_path: Path, *, read_only: bool): """ import sqlite3 - from hermes_state import SessionDB, is_malformed_db_error + from hermes_state import SessionDB, is_malformed_schema_error if not read_only: return SessionDB(db_path=db_path, read_only=False) @@ -12126,7 +12126,7 @@ def _open_session_db_at_path(db_path: Path, *, read_only: bool): except sqlite3.DatabaseError as exc: message = str(exc).lower() stale_schema = "no such table" in message or "no such column" in message - if not stale_schema and not is_malformed_db_error(exc): + if not stale_schema and not is_malformed_schema_error(exc): raise SessionDB(db_path=db_path, read_only=False).close() try: diff --git a/hermes_state.py b/hermes_state.py index 71ba271af0..24834f5901 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -1529,8 +1529,9 @@ def apply_database_pragmas( # The canonical ``sessions`` / ``messages`` data is intact in these cases — # only the derived schema is broken — so recovery preserves all transcripts # and merely rebuilds the FTS layer. -_MALFORMED_SCHEMA_MARKERS = ( - "malformed database schema", +_MALFORMED_SCHEMA_MARKERS = ("malformed database schema",) +_MALFORMED_DB_MARKERS = ( + *_MALFORMED_SCHEMA_MARKERS, "database disk image is malformed", ) @@ -1542,32 +1543,29 @@ _repair_attempt_lock = threading.Lock() def is_malformed_db_error(exc: BaseException) -> bool: - """True if *exc* is a SQLite 'malformed schema / disk image' error. + """True for explicit malformed-schema or generic corrupt-image errors. - These are the corruption classes where the schema fails to parse, so - targeted ``sqlite_master`` surgery (not an ordinary FTS rebuild) is the - only recovery path. + This broad classifier is for diagnostics and explicit offline recovery + dispatch. Runtime repair must use :func:`is_malformed_schema_error`, since + a generic corrupt-image error does not identify the damaged object. + """ + if not isinstance(exc, sqlite3.DatabaseError): + return False + return any(marker in str(exc).lower() for marker in _MALFORMED_DB_MARKERS) + + +def is_malformed_schema_error(exc: BaseException) -> bool: + """True only when SQLite explicitly reports malformed schema text. + + A generic ``database disk image is malformed`` error is SQLITE_CORRUPT + and may come from any B-tree or freelist page. It does not prove that + canonical rows are intact, so runtime schema/FTS repair must fail closed. """ if not isinstance(exc, sqlite3.DatabaseError): return False return any(marker in str(exc).lower() for marker in _MALFORMED_SCHEMA_MARKERS) -def _is_not_a_database_error(exc: BaseException) -> bool: - """True if *exc* is SQLite's 'file is not a database' error. - - Raised when a connection's backing file is not a SQLite database — the - runtime connection-corruption class: a sibling process (forked curator - agent, external repair pass) replaced/truncated the file out from under - the live connection. The file on disk may be perfectly healthy; the - CONNECTION is broken. Distinct from the malformed-schema class: the fix - is a reconnect, not schema surgery. - """ - if not isinstance(exc, sqlite3.DatabaseError): - return False - return "file is not a database" in str(exc).lower() - - # Markers that mean the host filesystem cannot accept another write. Kept as # plain substrings so OSError, sqlite3.OperationalError, and wrapped RPC # error strings all match the same helper. @@ -3781,13 +3779,6 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) # in place at most once per SessionDB instance so a genuinely # unrecoverable database can't put writers into a rebuild loop. self._fts_runtime_rebuild_attempted = False - # One-shot guard for the runtime connection-reopen recovery on the - # write path. A connection whose backing file was replaced/truncated - # by a sibling process surfaces as "file is not a database" on every - # write; we close and reopen the connection at most once per - # SessionDB instance so a genuinely unrecoverable database can't put - # writers into a reconnect loop. - self._notadb_reconnect_attempted = False # One-shot guard for the usermerge-floor config write on the # incremental FTS merge cadence (see _merge_fts_incrementally). self._fts_usermerge_floor_applied = False @@ -3973,7 +3964,7 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) # place (backup first; canonical sessions/messages preserved), # then reopen once. This is what lets Desktop/Dashboard # self-heal instead of silently showing "no sessions". - if not is_malformed_db_error(exc) or not _claim_repair_attempt(self.db_path): + if not is_malformed_schema_error(exc) or not _claim_repair_attempt(self.db_path): raise logger.error( "state.db schema is malformed (%s) — attempting automatic " @@ -4566,20 +4557,6 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) except sqlite3.DatabaseError as exc: if _is_no_more_rows(exc) and self._sleep_before_write_retry(deadline, patience_s): continue - # Runtime connection-corruption self-heal: a connection whose - # backing file was replaced/truncated by a sibling process - # (e.g. a forked curator agent inheriting and closing the - # write fd, or an external repair pass) surfaces as "file is - # not a database" on EVERY subsequent write. Without a - # reconnect branch the gateway wedges permanently: every - # transcript/routing write raises, messages stay in memory, - # and swap grows without bound until the process is killed. - # Close the broken connection, reopen the DB file, and retry - # the write once. - if _is_not_a_database_error(exc): - if not self._reconnect_after_notadb(): - raise - continue # Corrupt FTS shadow tables make every write raise the # malformed/corrupt error class through the FTS sync triggers # while the canonical messages table is intact. Recover here, @@ -4630,77 +4607,16 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) time.sleep(min(jitter, max(deadline - now, 0.001))) return True - def _reconnect_after_notadb(self) -> bool: - """Close the corrupted write connection and reopen state.db. - - Returns True when the connection was successfully replaced and the - failed write should be retried. Mirrors the constructor's - ``_connect_and_init`` so WAL/schema reconciliation runs on the fresh - connection. Never raises — logs and returns False on failure so the - original error propagates. - - One-shot per instance: a genuinely unrecoverable database must not - put writers into a reconnect loop that pins CPU on every write. - """ - if self._notadb_reconnect_attempted: - return False - self._notadb_reconnect_attempted = True - logger.warning( - "state.db connection reported 'file is not a database' — closing " - "and reopening the connection to self-heal (one-shot)." - ) - try: - with self._lock: - if self._conn is not None: - try: - self._conn.close() - except Exception: - pass - self._conn = None - new_conn = _connect_tracked_db( - str(self.db_path), - tracking_path=self.db_path, - check_same_thread=False, - timeout=1.0, - isolation_level=None, - ) - new_conn.row_factory = sqlite3.Row - # Publish BEFORE schema init: _init_schema/_reconcile_columns - # operate on self._conn, not on the local variable. - self._conn = new_conn - self._wal_active = ( - apply_wal_with_fallback(new_conn, db_label="state.db") - == "wal" - ) - apply_database_pragmas(new_conn, db_label="state.db") - new_conn.execute("PRAGMA foreign_keys=ON") - self._fts_cjk_loaded = load_fts5_cjk_extension(new_conn) - self._init_schema() - except Exception as exc: - logger.error( - "state.db reconnect after 'file is not a database' failed (%s); " - "the database may need the full offline repair path.", - exc, - ) - return False - logger.warning( - "state.db connection reopened successfully; retrying the failed write." - ) - return True - @staticmethod def _is_fts_write_corruption_error(exc: sqlite3.DatabaseError) -> bool: """True for the error class a corrupt FTS index raises on writes. - The message varies by SQLite version: older builds raise the generic - ``database disk image is malformed`` (covered by - ``is_malformed_db_error``); newer builds (e.g. ubuntu-latest CI) - raise the FTS5-specific ``fts5: corrupt structure record for table - "messages_fts"``. Both mean the same thing for the write path: the - canonical rows are fine, the FTS shadow tables are not. + Only an FTS5-specific error is safe to recover online. SQLite's + generic ``database disk image is malformed`` does not identify the + damaged object and may represent canonical B-tree or freelist damage; + treating it as FTS-only corruption would continue writing to a + structurally damaged database. """ - if is_malformed_db_error(exc): - return True msg = str(exc).lower() return "fts5" in msg and "corrupt" in msg diff --git a/tests/hermes_cli/test_web_server.py b/tests/hermes_cli/test_web_server.py index 5c1fb1e985..b28bc6a8fa 100644 --- a/tests/hermes_cli/test_web_server.py +++ b/tests/hermes_cli/test_web_server.py @@ -604,6 +604,30 @@ class TestWebServerEndpoints: db.close() assert len(writable_opens) == 1 + def test_generic_corruption_does_not_trigger_writable_heal( + self, tmp_path, monkeypatch + ): + """Unscoped SQLITE_CORRUPT must not escalate a dashboard read to writes.""" + import sqlite3 + + import hermes_state + from hermes_cli import web_server + + db_path = tmp_path / "state.db" + db_path.write_bytes(b"not-empty") + opens = [] + + def corrupt_open(*_args, **kwargs): + opens.append(kwargs.get("read_only", False)) + raise sqlite3.DatabaseError("database disk image is malformed") + + monkeypatch.setattr(hermes_state, "SessionDB", corrupt_open) + + with pytest.raises(sqlite3.DatabaseError, match="disk image is malformed"): + web_server._open_session_db_at_path(db_path, read_only=True) + + assert opens == [True] + def test_get_sessions_zero_byte_store_returns_empty_list(self): from hermes_constants import get_hermes_home diff --git a/tests/state/test_fts_runtime_rebuild.py b/tests/state/test_fts_runtime_rebuild.py index fee735ebb4..68aa917d98 100644 --- a/tests/state/test_fts_runtime_rebuild.py +++ b/tests/state/test_fts_runtime_rebuild.py @@ -222,11 +222,9 @@ class TestRuntimeFtsRebuild: # Cleanup os.chmod(proc_root / "222" / "fd", 0o755) - def test_corruption_error_classification_covers_both_sqlite_messages(self): - """SQLite's message for a corrupt FTS index varies by version: older - builds raise the generic malformed-image error, newer builds raise an - FTS5-specific one. Both must trigger the self-heal.""" - assert SessionDB._is_fts_write_corruption_error( + def test_corruption_error_classification_requires_fts_provenance(self): + """Generic SQLITE_CORRUPT cannot prove only derived data is damaged.""" + assert not SessionDB._is_fts_write_corruption_error( sqlite3.DatabaseError("database disk image is malformed") ) assert SessionDB._is_fts_write_corruption_error( @@ -238,6 +236,21 @@ class TestRuntimeFtsRebuild: sqlite3.DatabaseError("no such table: nothing_fts_related") ) + def test_generic_malformed_write_fails_closed(self, db, monkeypatch): + db.create_session("s1", source="test") + monkeypatch.setattr( + db, "rebuild_fts", lambda: pytest.fail("must not rebuild FTS") + ) + + def _structural_corruption(_conn): + raise sqlite3.DatabaseError("database disk image is malformed") + + with pytest.raises(sqlite3.DatabaseError, match="disk image is malformed"): + db._execute_write(_structural_corruption) + + assert db._fts_runtime_rebuild_attempted is False + assert db._fts_enabled is True + def test_append_self_heals_after_fts_corruption(self, db, tmp_path): if not db._fts_enabled: pytest.skip("FTS5 unavailable in this build") @@ -557,4 +570,3 @@ class TestRuntimeFtsRebuild: assert recovered.search_messages("canonical survives") finally: recovered.close() - diff --git a/tests/test_state_db_malformed_repair.py b/tests/test_state_db_malformed_repair.py index 177abd7109..2d634e3f16 100644 --- a/tests/test_state_db_malformed_repair.py +++ b/tests/test_state_db_malformed_repair.py @@ -70,6 +70,27 @@ def test_duplicate_fts_makes_every_statement_fail(tmp_path): assert is_malformed_db_error(exc_info.value) +def test_generic_malformed_open_does_not_attempt_schema_surgery( + tmp_path, monkeypatch +): + """A generic SQLITE_CORRUPT error has no schema/FTS provenance.""" + db_path = tmp_path / "state.db" + repair_calls = [] + + def _generic_corruption(*_args, **_kwargs): + raise sqlite3.DatabaseError("database disk image is malformed") + + monkeypatch.setattr(hermes_state, "apply_wal_with_fallback", _generic_corruption) + monkeypatch.setattr( + hermes_state, + "repair_state_db_schema", + lambda *args, **kwargs: repair_calls.append((args, kwargs)), + ) + + with pytest.raises(sqlite3.DatabaseError, match="disk image is malformed"): + SessionDB(db_path=db_path) + + assert repair_calls == [] def test_repaired_db_search_works(tmp_path): diff --git a/tests/test_state_db_notadb_fail_closed.py b/tests/test_state_db_notadb_fail_closed.py new file mode 100644 index 0000000000..fbdf376fef --- /dev/null +++ b/tests/test_state_db_notadb_fail_closed.py @@ -0,0 +1,77 @@ +"""Tests for fail-closed state.db NOTADB handling and journal-mode EIO retries. + +Covers the two independently-valuable pieces salvaged from the state.db +hardening rollup: + +* fail closed when a live write connection reports ``file is not a database``; +* transient ``disk i/o error`` retry in ``_on_disk_journal_mode`` so a + one-shot EIO doesn't push callers onto the fail-closed unknown-mode branch. +""" + +import sqlite3 +from unittest.mock import MagicMock + +import pytest + +from hermes_state import SessionDB, _on_disk_journal_mode + + +class _NotADbOnce: + """Connection proxy that raises 'file is not a database' on execute.""" + + def __init__(self, real_conn): + self._real = real_conn + + def execute(self, *args, **kwargs): + raise sqlite3.DatabaseError("file is not a database") + + def __getattr__(self, name): + return getattr(self._real, name) + + +class TestFailClosedAfterNotADb: + def test_write_does_not_reopen_after_connection_identity_breaks( + self, tmp_path, monkeypatch + ): + """One connection cannot safely heal a shared DB identity change.""" + db = SessionDB(db_path=tmp_path / "state.db") + real_conn = db._conn + try: + db.create_session(session_id="s1", source="cli", model="test") + reopen = MagicMock() + monkeypatch.setattr("hermes_state._connect_tracked_db", reopen) + db._conn = _NotADbOnce(real_conn) + with pytest.raises(sqlite3.DatabaseError, match="not a database"): + db.create_session(session_id="s2", source="cli", model="test") + reopen.assert_not_called() + finally: + db._conn = real_conn + db.close() + + +class TestOnDiskJournalModeEioRetry: + def _conn_raising_then(self, failures, result_rows): + conn = MagicMock() + cursor = MagicMock() + cursor.fetchone.return_value = result_rows + conn.execute.side_effect = list(failures) + [cursor] + return conn + + def test_transient_eio_clears_on_retry(self): + conn = self._conn_raising_then( + [sqlite3.OperationalError("disk i/o error")] * 2, ("wal",) + ) + assert _on_disk_journal_mode(conn) == "wal" + + def test_persistent_eio_returns_none(self): + conn = MagicMock() + conn.execute.side_effect = sqlite3.OperationalError("disk i/o error") + assert _on_disk_journal_mode(conn) is None + # Bounded: retried a handful of times, not forever. + assert conn.execute.call_count == 4 + + def test_non_eio_operational_error_fails_fast(self): + conn = MagicMock() + conn.execute.side_effect = sqlite3.OperationalError("database is locked") + assert _on_disk_journal_mode(conn) is None + assert conn.execute.call_count == 1 diff --git a/tests/test_state_db_notadb_selfheal.py b/tests/test_state_db_notadb_selfheal.py deleted file mode 100644 index b30027044a..0000000000 --- a/tests/test_state_db_notadb_selfheal.py +++ /dev/null @@ -1,125 +0,0 @@ -"""Tests for the state.db runtime connection self-heal (PR #82280 remainder). - -Covers the two independently-valuable pieces salvaged from the state.db -hardening rollup: - -* one-shot reconnect when a live write connection reports - ``file is not a database`` (backing file replaced/truncated by a sibling - process — the connection is broken, the on-disk file may be healthy); -* transient ``disk i/o error`` retry in ``_on_disk_journal_mode`` so a - one-shot EIO doesn't push callers onto the fail-closed unknown-mode branch. -""" - -import sqlite3 -from unittest.mock import MagicMock - -import pytest - -from hermes_state import SessionDB, _is_not_a_database_error, _on_disk_journal_mode - - -class _NotADbOnce: - """Connection proxy that raises 'file is not a database' on execute.""" - - def __init__(self, real_conn): - self._real = real_conn - - def execute(self, *args, **kwargs): - raise sqlite3.DatabaseError("file is not a database") - - def __getattr__(self, name): - return getattr(self._real, name) - - -class TestIsNotADatabaseError: - def test_matches_sqlite_message(self): - assert _is_not_a_database_error( - sqlite3.DatabaseError("file is not a database") - ) - - def test_rejects_other_database_errors(self): - assert not _is_not_a_database_error( - sqlite3.DatabaseError("database disk image is malformed") - ) - - def test_rejects_non_sqlite_exceptions(self): - assert not _is_not_a_database_error(ValueError("file is not a database")) - - -class TestReconnectAfterNotADb: - def test_write_self_heals_when_connection_breaks(self, tmp_path): - """A broken connection over a healthy file reconnects and retries.""" - db = SessionDB(db_path=tmp_path / "state.db") - try: - db.create_session(session_id="s1", source="cli", model="test") - # Simulate the runtime corruption class: the connection starts - # raising 'file is not a database' while the on-disk file is - # perfectly healthy (sibling replaced/truncated the old inode). - db._conn = _NotADbOnce(db._conn) - - db.create_session(session_id="s2", source="cli", model="test") - - assert db._notadb_reconnect_attempted is True - assert db.get_session("s2") is not None - # The pre-existing row survived (same on-disk file). - assert db.get_session("s1") is not None - finally: - db.close() - - def test_reconnect_is_one_shot(self, tmp_path): - """A second 'file is not a database' propagates instead of looping.""" - db = SessionDB(db_path=tmp_path / "state.db") - try: - db._notadb_reconnect_attempted = True - db._conn = _NotADbOnce(db._conn) - with pytest.raises(sqlite3.DatabaseError, match="not a database"): - db.create_session(session_id="s3", source="cli", model="test") - finally: - db._conn = None - db.close() - - def test_failed_reconnect_returns_false_and_original_error_propagates( - self, tmp_path, monkeypatch - ): - """If the reopen itself fails, the original write error surfaces.""" - db = SessionDB(db_path=tmp_path / "state.db") - try: - monkeypatch.setattr( - "hermes_state._connect_tracked_db", - MagicMock(side_effect=sqlite3.DatabaseError("file is not a database")), - ) - db._conn = _NotADbOnce(db._conn) - with pytest.raises(sqlite3.DatabaseError, match="not a database"): - db.create_session(session_id="s4", source="cli", model="test") - assert db._notadb_reconnect_attempted is True - finally: - db._conn = None - db.close() - - -class TestOnDiskJournalModeEioRetry: - def _conn_raising_then(self, failures, result_rows): - conn = MagicMock() - cursor = MagicMock() - cursor.fetchone.return_value = result_rows - conn.execute.side_effect = list(failures) + [cursor] - return conn - - def test_transient_eio_clears_on_retry(self): - conn = self._conn_raising_then( - [sqlite3.OperationalError("disk i/o error")] * 2, ("wal",) - ) - assert _on_disk_journal_mode(conn) == "wal" - - def test_persistent_eio_returns_none(self): - conn = MagicMock() - conn.execute.side_effect = sqlite3.OperationalError("disk i/o error") - assert _on_disk_journal_mode(conn) is None - # Bounded: retried a handful of times, not forever. - assert conn.execute.call_count == 4 - - def test_non_eio_operational_error_fails_fast(self): - conn = MagicMock() - conn.execute.side_effect = sqlite3.OperationalError("database is locked") - assert _on_disk_journal_mode(conn) is None - assert conn.execute.call_count == 1