DB backups: verify integrity + never evict the last good backup
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).
This commit is contained in:
BoulderBadgeDad
parent
eea0f5ead0
commit
ca2f4da9f4
4 changed files with 325 additions and 23 deletions
|
|
@ -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(
|
||||
|
|
|
|||
153
core/db_integrity.py
Normal file
153
core/db_integrity.py
Normal file
|
|
@ -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
|
||||
133
tests/test_db_integrity.py
Normal file
133
tests/test_db_integrity.py
Normal file
|
|
@ -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) == []
|
||||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Reference in a new issue