From ca2f4da9f4cdfea629fde719bc07f3c25a3f579c Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 30 May 2026 21:13:04 -0700 Subject: [PATCH] DB backups: verify integrity + never evict the last good backup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-incident hardening. A WAL-mode DB corrupted (most likely an interrupted write during a hard restart), and the backup routine made it unrecoverable: it (a) never checked integrity, so src.backup() faithfully copied the corrupt pages into every rolling backup, and (b) pruned oldest-by-mtime, so each new corrupt backup evicted the last good one. Result: all snapshots poisoned. New core/db_integrity.py (pure, unit-tested): - quick_check()/is_healthy(): fast read-only PRAGMA quick_check probe. - safe_backup(): verifies the SOURCE is healthy BEFORE the Online-Backup copy and the RESULT after; refuses + discards rather than save a corrupt copy. - prune_backups(): rotation that NEVER deletes the most-recent verified-healthy backup, even to honor max_keep — so a run of bad backups can't drop your last good snapshot. Wired into BOTH backup paths (the /api/database/backup endpoint and the auto_backup_database automation handler) — they now refuse on integrity failure (409 / error status, existing backups untouched) and prune safely. Tests: tests/test_db_integrity.py (8) using REAL temp DBs incl. a physically corrupted one — proves refuse-corrupt-source, discard-corrupt-result, and the exact incident scenario (newest backups corrupt -> the older healthy one is protected from pruning). Existing maintenance-handler backup test still green (29 passed). compile + ruff clean. NOTE: this prevents silent backup poisoning; it does NOT stop the underlying corruption. Follow-ups still worth doing: WAL-checkpoint on clean shutdown + a periodic live-DB integrity alert (so corruption is caught on day 1). --- core/automation/handlers/maintenance.py | 28 +++-- core/db_integrity.py | 153 ++++++++++++++++++++++++ tests/test_db_integrity.py | 133 ++++++++++++++++++++ web_server.py | 34 ++++-- 4 files changed, 325 insertions(+), 23 deletions(-) create mode 100644 core/db_integrity.py create mode 100644 tests/test_db_integrity.py diff --git a/core/automation/handlers/maintenance.py b/core/automation/handlers/maintenance.py index dacd13c8..e8180f12 100644 --- a/core/automation/handlers/maintenance.py +++ b/core/automation/handlers/maintenance.py @@ -115,19 +115,27 @@ def auto_backup_database(config: Dict[str, Any], deps: AutomationDeps) -> Dict[s timestamp = datetime.now().strftime('%Y%m%d_%H%M%S') backup_path = f"{db_path}.backup_{timestamp}" - # Use SQLite backup API for a safe hot-copy of an active database. - src = sqlite3.connect(db_path) - dst = sqlite3.connect(backup_path) - src.backup(dst) - dst.close() - src.close() + # safe_backup verifies source + result integrity, so an automated backup + # can never silently snapshot a corrupt DB (the incident where every + # rolling backup faithfully copied the corruption). + from core.db_integrity import DBIntegrityError, safe_backup, prune_backups + try: + safe_backup(db_path, backup_path) + except DBIntegrityError as integ: + deps.logger.error("Auto-backup refused — DB integrity check failed: %s", integ) + deps.update_progress( + automation_id, + log_line=f'Backup SKIPPED — database failed integrity check: {integ}', + log_type='error', + ) + return {'status': 'error', 'reason': f'Database integrity check failed: {integ}'} size_mb = round(os.path.getsize(backup_path) / (1024 * 1024), 1) - # Rolling cleanup — keep only the newest N backups. - existing = sorted(_glob.glob(f"{db_path}.backup_*"), key=os.path.getmtime) - while len(existing) > _MAX_BACKUPS: + # Rolling cleanup — never evict the most-recent verified-healthy backup. + existing = list(_glob.glob(f"{db_path}.backup_*")) + for removed in prune_backups(existing, _MAX_BACKUPS): try: - os.remove(existing.pop(0)) + os.remove(removed) except Exception as e: # noqa: BLE001 — best-effort cleanup deps.logger.debug("rolling backup cleanup failed: %s", e) deps.update_progress( diff --git a/core/db_integrity.py b/core/db_integrity.py new file mode 100644 index 00000000..8e84d1e1 --- /dev/null +++ b/core/db_integrity.py @@ -0,0 +1,153 @@ +"""SQLite integrity + safe-backup helpers. + +Born out of a real incident: a WAL-mode DB got corrupted (most likely an +interrupted write during a hard restart), and because the backup routine +(a) never checked integrity and (b) rotated the oldest backup out by mtime, +every rolling backup ended up being a faithful copy of the already-corrupt +file — so when recovery was needed, all snapshots were poisoned. + +This module makes that impossible: + +* ``quick_check(path)`` / ``is_healthy(path)`` — fast read-only integrity probe. +* ``safe_backup(...)`` — verifies the SOURCE is healthy before copying, uses the + SQLite Online Backup API, then verifies the RESULT. A corrupt source never + produces (or keeps) a backup. +* ``prune_backups(...)`` — rotation that NEVER deletes the most recent + *verified-healthy* backup, even to honor the max-count, so a run of bad + backups can't evict your last good one. + +Pure-ish: only touches sqlite3 + the filesystem paths it's given; no Flask, no +app globals. Unit-testable with real (and deliberately-corrupted) temp DBs. +""" + +from __future__ import annotations + +import logging +import os +import sqlite3 +from typing import Optional + +logger = logging.getLogger("db_integrity") + + +def _close_quietly(conn) -> None: + """Best-effort close; a failure to close during cleanup must not mask the + real error we're handling, but we log it rather than swallow silently.""" + if conn is None: + return + try: + conn.close() + except Exception as e: # noqa: BLE001 — cleanup path, real error already in flight + logger.debug("db_integrity: connection close failed: %s", e) + + +class DBIntegrityError(Exception): + """Raised when a database fails its integrity check.""" + + +def quick_check(db_path: str, *, timeout: float = 30.0) -> str: + """Run ``PRAGMA quick_check`` read-only and return its first result row. + + Returns ``'ok'`` for a healthy DB, otherwise the first error line. Raises + ``DBIntegrityError`` if the file can't even be opened/read (malformed + header, I/O error) — i.e. unambiguously bad. + """ + if not os.path.exists(db_path): + raise DBIntegrityError(f"Database file not found: {db_path}") + conn = None + try: + conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True, timeout=timeout) + row = conn.execute("PRAGMA quick_check(1)").fetchone() + return (row[0] if row else "no result") + except sqlite3.DatabaseError as e: + # malformed header / disk image malformed / disk I/O error + raise DBIntegrityError(f"{db_path}: {e}") from e + finally: + _close_quietly(conn) + + +def is_healthy(db_path: str, *, timeout: float = 30.0) -> bool: + """True iff the DB opens and ``quick_check`` reports 'ok'. Never raises.""" + try: + return quick_check(db_path, timeout=timeout) == "ok" + except DBIntegrityError: + return False + + +def safe_backup(src_path: str, dst_path: str, *, verify_source: bool = True, + verify_result: bool = True) -> None: + """Back up ``src_path`` to ``dst_path`` via the SQLite Online Backup API, + refusing to produce a backup from (or keep a backup of) a corrupt DB. + + Raises ``DBIntegrityError`` and removes any partial ``dst_path`` when the + source is unhealthy (``verify_source``) or the produced backup fails its + own check (``verify_result``). On success ``dst_path`` is a verified-good + copy. + """ + if verify_source and not is_healthy(src_path): + # Don't immortalize corruption — surface it so the caller can alert + # and, crucially, NOT rotate out the existing good backups. + raise DBIntegrityError( + f"Refusing to back up: source database failed integrity check ({src_path})" + ) + + src = dst = None + try: + src = sqlite3.connect(src_path) + dst = sqlite3.connect(dst_path) + src.backup(dst) + finally: + _close_quietly(dst) + _close_quietly(src) + + if verify_result and not is_healthy(dst_path): + # The copy itself came out bad — discard it rather than keep a dud. + try: + os.remove(dst_path) + except OSError: + pass + raise DBIntegrityError( + f"Backup produced a corrupt file and was discarded ({dst_path})" + ) + + +def prune_backups(backup_paths, max_keep: int, + health_check=is_healthy) -> list: + """Decide which backups to delete to honor ``max_keep`` WITHOUT ever + deleting the most-recent verified-healthy backup. + + ``backup_paths`` is an iterable of paths; order does not matter (we sort by + mtime). Returns the list of paths that SHOULD be deleted (does not delete + them — the caller does the IO, so this stays pure/testable). + + Rule: oldest-first deletion until <= max_keep, but the single newest + *healthy* backup is protected and never selected for deletion. So even if + the newest few backups are corrupt, the last good snapshot survives. + """ + paths = [p for p in backup_paths] + # Newest first. + paths.sort(key=lambda p: _safe_mtime(p), reverse=True) + + # Find the newest healthy backup — the one we must never drop. + protected: Optional[str] = None + for p in paths: + if health_check(p): + protected = p + break + + if len(paths) <= max_keep: + return [] + + # Delete oldest-first beyond max_keep, but skip the protected one. + deletable = [p for p in paths if p != protected] + # oldest first among deletable + deletable.sort(key=lambda p: _safe_mtime(p)) + num_to_delete = len(paths) - max_keep + return deletable[:num_to_delete] + + +def _safe_mtime(path: str) -> float: + try: + return os.path.getmtime(path) + except OSError: + return 0.0 diff --git a/tests/test_db_integrity.py b/tests/test_db_integrity.py new file mode 100644 index 00000000..497350a5 --- /dev/null +++ b/tests/test_db_integrity.py @@ -0,0 +1,133 @@ +"""Tests for core.db_integrity — the post-incident backup-integrity hardening. + +Incident: a WAL-mode DB corrupted on an interrupted write; the backup routine +never checked integrity and rotated oldest-by-mtime, so every rolling backup +copied the corruption and evicted the last good one. These tests pin the +guarantees that make that impossible. +""" + +from __future__ import annotations + +import os +import sqlite3 + +import pytest + +from core.db_integrity import ( + DBIntegrityError, + is_healthy, + prune_backups, + quick_check, + safe_backup, +) + + +def _make_db(path, rows=50): + c = sqlite3.connect(path) + c.execute("CREATE TABLE t (id INTEGER PRIMARY KEY, v TEXT)") + c.executemany("INSERT INTO t (v) VALUES (?)", [(f"row-{i}",) for i in range(rows)]) + c.commit() + c.close() + + +def _corrupt_file(path): + """Physically scribble over the middle of a DB file so SQLite sees a + malformed image (mirrors real page damage).""" + size = os.path.getsize(path) + with open(path, "r+b") as f: + f.seek(size // 2) + f.write(b"\x00\xff\x00\xff" * 512) + + +# ── quick_check / is_healthy ─────────────────────────────────────────────── + +def test_quick_check_ok_on_healthy_db(tmp_path): + db = str(tmp_path / "good.db") + _make_db(db) + assert quick_check(db) == "ok" + assert is_healthy(db) is True + + +def test_missing_file_raises(tmp_path): + with pytest.raises(DBIntegrityError): + quick_check(str(tmp_path / "nope.db")) + assert is_healthy(str(tmp_path / "nope.db")) is False + + +def test_corrupt_db_is_unhealthy(tmp_path): + db = str(tmp_path / "bad.db") + _make_db(db, rows=2000) # big enough that midpoint hits real pages + _corrupt_file(db) + # Either quick_check returns a non-'ok' string OR it raises — both mean bad. + assert is_healthy(db) is False + + +# ── safe_backup ──────────────────────────────────────────────────────────── + +def test_safe_backup_of_healthy_db_succeeds(tmp_path): + src = str(tmp_path / "src.db"); dst = str(tmp_path / "dst.db") + _make_db(src) + safe_backup(src, dst) + assert os.path.exists(dst) + assert is_healthy(dst) + # data really copied + c = sqlite3.connect(dst) + assert c.execute("SELECT count(*) FROM t").fetchone()[0] == 50 + c.close() + + +def test_safe_backup_refuses_corrupt_source(tmp_path): + """The core fix: never produce a backup from a corrupt DB.""" + src = str(tmp_path / "src.db"); dst = str(tmp_path / "dst.db") + _make_db(src, rows=2000) + _corrupt_file(src) + with pytest.raises(DBIntegrityError): + safe_backup(src, dst) + # No poisoned backup left behind. + assert not os.path.exists(dst) + + +# ── prune_backups (never evict the last good one) ────────────────────────── + +def test_prune_keeps_newest_and_deletes_oldest(tmp_path): + paths = [] + for i in range(7): + p = str(tmp_path / f"b{i}.db") + _make_db(p) # all healthy + os.utime(p, (1000 + i, 1000 + i)) # b0 oldest ... b6 newest + paths.append(p) + to_delete = prune_backups(paths, max_keep=5) + # 7 - 5 = 2 oldest deleted + assert set(to_delete) == {str(tmp_path / "b0.db"), str(tmp_path / "b1.db")} + + +def test_prune_never_deletes_last_healthy_even_when_newer_are_corrupt(tmp_path): + """The incident scenario: the newest backups are all corrupt. Pruning to + max_keep must NOT delete the one older healthy backup.""" + healthy = str(tmp_path / "old_good.db") + _make_db(healthy) + os.utime(healthy, (1000, 1000)) # oldest + + corrupt = [] + for i in range(6): + p = str(tmp_path / f"new_bad{i}.db") + _make_db(p, rows=2000) + _corrupt_file(p) + os.utime(p, (2000 + i, 2000 + i)) # all newer than healthy + corrupt.append(p) + + all_paths = [healthy] + corrupt # 7 total, max_keep 5 -> delete 2 + to_delete = prune_backups(all_paths, max_keep=5) + + assert len(to_delete) == 2 + # The single healthy (oldest) backup must be protected despite being oldest. + assert healthy not in to_delete + # Only corrupt ones get deleted. + assert all(p in corrupt for p in to_delete) + + +def test_prune_noop_under_limit(tmp_path): + paths = [] + for i in range(3): + p = str(tmp_path / f"b{i}.db"); _make_db(p); paths.append(p) + assert prune_backups(paths, max_keep=5) == [] diff --git a/web_server.py b/web_server.py index f767e7e6..d01687a4 100644 --- a/web_server.py +++ b/web_server.py @@ -15354,18 +15354,27 @@ _BACKUP_FILENAME_RE = re.compile(r'^music_library\.db\.backup_\d{8}_\d{6}$') def backup_database_endpoint(): """Create a rolling backup of the database (max 5).""" try: - import sqlite3, glob as _glob + import glob as _glob + from core.db_integrity import DBIntegrityError, safe_backup, prune_backups db_path = os.environ.get('DATABASE_PATH', 'database/music_library.db') if not os.path.exists(db_path): return jsonify({"success": False, "error": "Database file not found"}), 404 max_backups = 5 timestamp = datetime.now().strftime('%Y%m%d_%H%M%S') backup_path = f"{db_path}.backup_{timestamp}" - src = sqlite3.connect(db_path) - dst = sqlite3.connect(backup_path) - src.backup(dst) - dst.close() - src.close() + # safe_backup verifies the SOURCE is healthy before copying and the + # RESULT after — so a corrupt DB can never silently produce a backup + # (the incident where every rolling backup copied the corruption). + try: + safe_backup(db_path, backup_path) + except DBIntegrityError as integ: + logger.error("Backup refused — database integrity check failed: %s", integ) + return jsonify({ + "success": False, + "error": "Database failed its integrity check — backup refused to avoid " + "saving a corrupt copy. Your existing backups are untouched. " + str(integ), + "integrity_failed": True, + }), 409 size_mb = round(os.path.getsize(backup_path) / (1024 * 1024), 1) # Write version metadata sidecar meta_path = backup_path + '.meta.json' @@ -15374,15 +15383,14 @@ def backup_database_endpoint(): json.dump({"version": SOULSYNC_VERSION, "created": timestamp}, mf) except Exception as e: logger.debug("backup meta sidecar write: %s", e) - # Rolling cleanup - existing = sorted(_glob.glob(f"{db_path}.backup_*"), key=os.path.getmtime) - # Filter out .meta.json files from the backup list - existing = [f for f in existing if not f.endswith('.meta.json')] - while len(existing) > max_backups: + # Rolling cleanup — prune_backups never deletes the most-recent + # VERIFIED-HEALTHY backup, even to honor max_backups, so a run of bad + # backups can't evict your last good snapshot (the incident). + existing = [f for f in _glob.glob(f"{db_path}.backup_*") + if not f.endswith('.meta.json')] + for removed in prune_backups(existing, max_backups): try: - removed = existing.pop(0) os.remove(removed) - # Also remove sidecar if present if os.path.exists(removed + '.meta.json'): os.remove(removed + '.meta.json') except Exception as e: