diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index de332f36ee44..189ec4b32234 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -1621,15 +1621,111 @@ def _backup_corrupt_db(path: Path) -> Optional[Path]: return candidate +# Repairable integrity_check error classes. Both shapes are *index-scoped*: +# the table b-tree is intact and only a secondary index disagrees with it, +# which REINDEX rebuilds losslessly from the table data. The index name is +# parsed generically from the message — no hardcoded index list. Any other +# integrity_check message (page corruption, "database disk image is +# malformed", freelist damage, …) is NOT repairable this way and keeps the +# fail-closed behavior. +_REPAIRABLE_INDEX_ERROR_PATTERNS = ( + re.compile(r"^wrong # of entries in index (?P.+)$"), + re.compile(r"^row \d+ missing from index (?P.+)$"), +) + + +def _integrity_messages_ok(messages: list[str]) -> bool: + """True iff ``PRAGMA integrity_check`` output is the single ``ok`` row.""" + return len(messages) == 1 and messages[0].strip().lower() == "ok" + + +def _run_integrity_check(conn: sqlite3.Connection) -> list[str]: + """Return all ``PRAGMA integrity_check`` message rows as strings.""" + rows = conn.execute("PRAGMA integrity_check").fetchall() + return [str(row[0]) for row in rows if row is not None and row[0] is not None] + + +def _repairable_index_names(messages: list[str]) -> Optional[list[str]]: + """Return the distinct index names iff EVERY message is index-repairable. + + ``None`` when any line falls outside the repairable index-class errors + (or when there are no messages at all) — the caller must then fail + closed exactly as before. Order of first appearance is preserved so the + REINDEX pass is deterministic. + """ + names: list[str] = [] + saw_any = False + for raw in messages: + message = (raw or "").strip() + if not message: + continue + for pattern in _REPAIRABLE_INDEX_ERROR_PATTERNS: + match = pattern.match(message) + if match: + break + else: + return None + saw_any = True + name = match.group("index").strip() + if name and name not in names: + names.append(name) + if not saw_any or not names: + return None + return names + + +def _attempt_index_reindex_repair( + path: Path, index_names: list[str], +) -> tuple[bool, list[str]]: + """REINDEX the named indexes, then re-run ``PRAGMA integrity_check``. + + Tries a per-index ``REINDEX ""`` first (cheapest, most targeted); + if any per-index statement fails — e.g. the parsed name does not resolve + because integrity_check reported an internal/auto index — falls back to + a bare ``REINDEX`` of the whole database. Returns + ``(clean, post_repair_messages)``; never raises. Callers must hold the + board's cross-process init flock so no other process connects mid-repair. + """ + try: + conn = _sqlite_connect(path) + except sqlite3.Error as exc: + return False, [f"could not reopen for REINDEX: {exc}"] + try: + try: + for name in index_names: + escaped = name.replace('"', '""') + conn.execute(f'REINDEX "{escaped}"') + except sqlite3.Error: + # Per-index rebuild failed (unresolvable parsed name, auto + # index, …) — bare REINDEX rebuilds every index in the DB. + conn.execute("REINDEX") + messages = _run_integrity_check(conn) + except sqlite3.Error as exc: + return False, [f"REINDEX failed: {exc}"] + finally: + conn.close() + return _integrity_messages_ok(messages), messages + + def _guard_existing_db_is_healthy(path: Path) -> None: """Run ``PRAGMA integrity_check`` on an existing non-empty DB file. Opens the probe in read/write mode so SQLite can recover or checkpoint a healthy WAL/hot-journal DB before we declare it - corrupt. If the file is malformed, copy it (and any WAL/SHM - sidecars) to a timestamped backup and raise - :class:`KanbanDbCorruptError` so callers cannot silently recreate - the schema on top of a damaged DB. + corrupt. + + **Narrow auto-repair:** when the integrity failure consists *only* of + index-scoped errors (``wrong # of entries in index `` / ``row N + missing from index ``), the table b-trees are intact and REINDEX + rebuilds the damaged indexes losslessly. In that case we take the + corrupt backup FIRST (same content-addressed quarantine as the + fail-closed path), run REINDEX under the caller-held init flock, + re-run ``integrity_check``, and proceed only if it comes back clean. + Anything else — page corruption, ``malformed`` images, a REINDEX that + does not produce a clean re-check — fails closed exactly as before: + copy the file (and any WAL/SHM sidecars) to a backup and raise + :class:`KanbanDbCorruptError` so callers cannot silently recreate the + schema on top of a damaged DB. Transient lock/busy errors (``sqlite3.OperationalError``) are NOT treated as corruption; they propagate raw so the caller sees a @@ -1660,14 +1756,18 @@ def _guard_existing_db_is_healthy(path: Path) -> None: if str(resolved) in _INITIALIZED_PATHS: return reason: Optional[str] = None + messages: list[str] = [] try: probe = _sqlite_connect(resolved) try: - row = probe.execute("PRAGMA integrity_check").fetchone() + messages = _run_integrity_check(probe) finally: probe.close() - if not row or (row[0] or "").lower() != "ok": - reason = f"integrity_check returned {row[0] if row else ''!r}" + if not _integrity_messages_ok(messages): + reason = ( + f"integrity_check returned " + f"{messages[0] if messages else ''!r}" + ) except sqlite3.OperationalError: # Lock contention, busy, transient IO — not corruption. Let it propagate. raise @@ -1675,7 +1775,30 @@ def _guard_existing_db_is_healthy(path: Path) -> None: reason = f"sqlite refused to open file: {exc}" if reason is None: return + # Quarantine FIRST — both the repair path and the fail-closed path + # preserve the pre-touch bytes before anything mutates the file. backup = _backup_corrupt_db(resolved) + index_names = _repairable_index_names(messages) + if index_names: + _log.warning( + "kanban DB %s failed integrity_check with index-only errors " + "(%s); pre-repair backup at %s — attempting REINDEX auto-repair.", + resolved, ", ".join(index_names), + backup if backup is not None else "", + ) + repaired, post = _attempt_index_reindex_repair(resolved, index_names) + if repaired: + _log.warning( + "kanban DB %s auto-repaired via REINDEX (%s); " + "integrity_check now clean. Pre-repair copy kept at %s.", + resolved, ", ".join(index_names), + backup if backup is not None else "", + ) + return + reason = ( + f"{reason}; REINDEX auto-repair attempted but integrity_check " + f"still returned {post[0] if post else ''!r}" + ) raise KanbanDbCorruptError(resolved, backup, reason) diff --git a/tests/hermes_cli/test_kanban_db_repair.py b/tests/hermes_cli/test_kanban_db_repair.py new file mode 100644 index 000000000000..bb06c46d7d9a --- /dev/null +++ b/tests/hermes_cli/test_kanban_db_repair.py @@ -0,0 +1,196 @@ +"""Tests for kanban DB corruption repair, backup retention, WAL checkpointing, +and the ``hermes kanban repair`` CLI verb.""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb + + +# --------------------------------------------------------------------------- +# Fixture helpers +# --------------------------------------------------------------------------- + +def _build_board_db(db_path: Path, tasks: int = 12) -> None: + """Create a real board DB with data so indexes have entries.""" + kb._INITIALIZED_PATHS.discard(str(db_path.resolve())) + kb.init_db(db_path=db_path) + with kb.connect(db_path=db_path) as conn: + for i in range(tasks): + kb.create_task(conn, title=f"task-{i}") + conn.close() + # Force the next connect() to re-run the health guard. + kb._INITIALIZED_PATHS.discard(str(db_path.resolve())) + + +def _corrupt_index(db_path: Path, index_name: str) -> None: + """Make ``index_name`` disagree with its table → 'wrong # of entries'. + + writable_schema approach: temporarily rewrite the index's schema SQL to + a partial index matching no rows, REINDEX under that lie (emptying the + index b-tree), then restore the original SQL. integrity_check now sees a + non-partial index whose b-tree is missing every row — exactly the + index-scoped corruption class ('wrong # of entries in index ' + + 'row N missing from index ') with intact table b-trees. + """ + conn = sqlite3.connect(db_path, isolation_level=None) + original_sql = conn.execute( + "SELECT sql FROM sqlite_master WHERE name = ?", (index_name,) + ).fetchone()[0] + lie = original_sql + " WHERE 0" + conn.execute("PRAGMA writable_schema=ON") + conn.execute( + "UPDATE sqlite_master SET sql = ? WHERE name = ?", (lie, index_name) + ) + conn.execute("PRAGMA writable_schema=OFF") + conn.close() + # New connection so the rewritten schema is what REINDEX parses. + conn = sqlite3.connect(db_path, isolation_level=None) + conn.execute(f'REINDEX "{index_name}"') + conn.execute("PRAGMA writable_schema=ON") + conn.execute( + "UPDATE sqlite_master SET sql = ? WHERE name = ?", + (original_sql, index_name), + ) + conn.execute("PRAGMA writable_schema=OFF") + conn.close() + kb._INITIALIZED_PATHS.discard(str(db_path.resolve())) + + +def _write_page_corrupt_db(path: Path) -> bytes: + """Valid SQLite header, garbage pages — NON-index corruption class.""" + header = b"SQLite format 3\x00" + b"\x10\x00\x02\x02\x00\x40\x20\x20" + header += b"\x00\x00\x00\x0c\x00\x00\x23\x46\x00\x00\x00\x00" + header = header.ljust(100, b"\x00") + blob = header + b"definitely not a valid sqlite page \x00\x01\x02\x03" * 64 + path.write_bytes(blob) + kb._INITIALIZED_PATHS.discard(str(path.resolve())) + return blob + + +def _integrity_messages(db_path: Path) -> list[str]: + conn = sqlite3.connect(db_path) + try: + return [r[0] for r in conn.execute("PRAGMA integrity_check").fetchall()] + finally: + conn.close() + + +# --------------------------------------------------------------------------- +# Index-error parsing (generic, no hardcoded index names) +# --------------------------------------------------------------------------- + +def test_repairable_index_names_parses_generically(): + messages = [ + "wrong # of entries in index idx_anything_at_all", + "row 3 missing from index idx_anything_at_all", + "wrong # of entries in index some_other_index", + ] + assert kb._repairable_index_names(messages) == [ + "idx_anything_at_all", "some_other_index", + ] + + +@pytest.mark.parametrize("messages", [ + [], + ["ok"], + ["database disk image is malformed"], + ["*** in database main ***", "Page 5: btreeInitPage() returns error code 11"], + # Mixed: one repairable line + one non-index line → NOT repairable. + ["wrong # of entries in index idx_tasks_status", + "database disk image is malformed"], +]) +def test_repairable_index_names_rejects_non_index_classes(messages): + assert kb._repairable_index_names(messages) is None + + +# --------------------------------------------------------------------------- +# Narrow auto-repair in the connect-time guard +# --------------------------------------------------------------------------- + +def test_connect_auto_repairs_index_only_corruption(tmp_path, caplog): + """Index-only integrity errors are REINDEXed and connect proceeds.""" + import logging + + db_path = tmp_path / "kanban.db" + _build_board_db(db_path) + _corrupt_index(db_path, "idx_tasks_status") + + # Precondition: the fixture really produced the index-scoped class. + messages = _integrity_messages(db_path) + assert any(m.startswith("wrong # of entries in index") for m in messages) + assert kb._repairable_index_names(messages) == ["idx_tasks_status"] + + with caplog.at_level(logging.WARNING, logger="hermes_cli.kanban_db"): + conn = kb.connect(db_path=db_path) + try: + # DB is clean again and data survived. + row = conn.execute("PRAGMA integrity_check").fetchone() + assert row[0] == "ok" + titles = {t.title for t in kb.list_tasks(conn)} + assert "task-0" in titles and "task-11" in titles + finally: + conn.close() + assert "auto-repaired via REINDEX" in caplog.text + + # The corrupt bytes were quarantined BEFORE the repair mutated the file. + backups = list(tmp_path.glob("kanban.db.corrupt.*.bak")) + assert len(backups) == 1 + backup_messages_db = backups[0] + # The backup still exhibits the pre-repair corruption. + pre = _integrity_messages(backup_messages_db) + assert any(m.startswith("wrong # of entries in index") for m in pre) + + +def test_connect_still_fails_closed_on_page_corruption(tmp_path): + """Non-index corruption keeps the exact fail-closed contract.""" + db_path = tmp_path / "kanban.db" + original = _write_page_corrupt_db(db_path) + + with pytest.raises(kb.KanbanDbCorruptError) as excinfo: + kb.connect(db_path=db_path) + + err = excinfo.value + assert err.backup_path is not None and err.backup_path.exists() + # No repair was attempted: original bytes untouched on the live path. + assert db_path.read_bytes() == original + + +def test_guard_fails_closed_when_reindex_does_not_clean(tmp_path, monkeypatch): + """If the post-REINDEX re-check is not clean, raise exactly as today.""" + db_path = tmp_path / "kanban.db" + _build_board_db(db_path) + _corrupt_index(db_path, "idx_tasks_status") + + monkeypatch.setattr( + kb, "_attempt_index_reindex_repair", + lambda path, names: (False, ["wrong # of entries in index idx_tasks_status"]), + ) + with pytest.raises(kb.KanbanDbCorruptError) as excinfo: + kb.connect(db_path=db_path) + assert "REINDEX auto-repair attempted" in str(excinfo.value) + assert excinfo.value.backup_path is not None + assert excinfo.value.backup_path.exists() + + +def test_repaired_db_connects_normally_afterwards(tmp_path): + """After one auto-repair, subsequent connects are ordinary fast-path.""" + db_path = tmp_path / "kanban.db" + _build_board_db(db_path) + _corrupt_index(db_path, "idx_tasks_status") + + conn = kb.connect(db_path=db_path) + conn.close() + # Second connect: healthy cache path, no new backups minted. + before = set(tmp_path.glob("kanban.db.corrupt.*.bak")) + conn = kb.connect(db_path=db_path) + try: + kb.create_task(conn, title="post-repair") + assert "post-repair" in {t.title for t in kb.list_tasks(conn)} + finally: + conn.close() + assert set(tmp_path.glob("kanban.db.corrupt.*.bak")) == before