mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-26 17:38:36 +00:00
fix(kanban): auto-repair index-only kanban.db corruption via REINDEX
_guard_existing_db_is_healthy previously failed closed on ANY
integrity_check failure, including the index-scoped class ('wrong # of
entries in index <name>' / 'row N missing from index <name>') where the
table b-trees are intact and REINDEX rebuilds the damaged indexes
losslessly. Boards hit by that class were bricked until manual surgery
even though SQLite can fix them in-place.
Now, when integrity_check output consists ONLY of index-scoped errors
(index name parsed generically from the message — no hardcoded list):
1. quarantine the corrupt bytes FIRST via the existing content-
addressed _backup_corrupt_db,
2. under the caller-held cross-process init flock, REINDEX each named
index (falling back to bare REINDEX if a parsed name doesn't
resolve),
3. re-run integrity_check and proceed only if it comes back clean.
Any non-index error class (page corruption, malformed image, freelist
damage) — or a REINDEX whose re-check is still dirty — fails closed
exactly as before: backup + KanbanDbCorruptError, no silent recreation.
Transient OperationalError (locked/busy) still propagates raw with no
quarantine.
Tests build a real board DB and corrupt a live index via the
writable_schema/partial-index REINDEX trick to produce the genuine
'wrong # of entries in index' shape, then assert auto-repair recovers
with data intact, page corruption still raises, and a dirty re-check
fails closed.
This commit is contained in:
parent
2d6b95cf2f
commit
8995131458
2 changed files with 326 additions and 7 deletions
|
|
@ -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<index>.+)$"),
|
||||
re.compile(r"^row \d+ missing from index (?P<index>.+)$"),
|
||||
)
|
||||
|
||||
|
||||
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 "<name>"`` 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 <name>`` / ``row N
|
||||
missing from index <name>``), 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 '<no row>'!r}"
|
||||
if not _integrity_messages_ok(messages):
|
||||
reason = (
|
||||
f"integrity_check returned "
|
||||
f"{messages[0] if messages else '<no row>'!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 "<backup failed>",
|
||||
)
|
||||
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 "<backup failed>",
|
||||
)
|
||||
return
|
||||
reason = (
|
||||
f"{reason}; REINDEX auto-repair attempted but integrity_check "
|
||||
f"still returned {post[0] if post else '<no row>'!r}"
|
||||
)
|
||||
raise KanbanDbCorruptError(resolved, backup, reason)
|
||||
|
||||
|
||||
|
|
|
|||
196
tests/hermes_cli/test_kanban_db_repair.py
Normal file
196
tests/hermes_cli/test_kanban_db_repair.py
Normal file
|
|
@ -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 <name>' +
|
||||
'row N missing from index <name>') 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
|
||||
Loading…
Add table
Add a link
Reference in a new issue