From 332fbadd7b8294fe015d4b1c8150d5501e586c76 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:56:15 -0700 Subject: [PATCH] fix(backup): fail closed on sqlite snapshot errors --- hermes_cli/backup.py | 49 ++++++++++---- tests/hermes_cli/test_backup.py | 112 +++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 14 deletions(-) diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index 787916436b1..33d07169d5b 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -257,23 +257,30 @@ def _safe_copy_db(src: Path, dst: Path) -> bool: """Copy a SQLite database safely using the backup() API. Handles WAL mode — produces a consistent snapshot even while - the DB is being written to. Falls back to raw copy on failure. + the DB is being written to. Fail closed if a consistent snapshot cannot + be created: copying only the live main file can omit committed WAL data. """ + conn = None + backup_conn = None try: conn = sqlite3.connect(f"file:{src}?mode=ro", uri=True) backup_conn = sqlite3.connect(str(dst)) conn.backup(backup_conn) - backup_conn.close() - conn.close() return True except Exception as exc: logger.warning("SQLite safe copy failed for %s: %s", src, exc) try: - shutil.copy2(src, dst) - return True - except Exception as exc2: - logger.error("Raw copy also failed for %s: %s", src, exc2) - return False + dst.unlink(missing_ok=True) + except OSError: + pass + return False + finally: + for connection in (backup_conn, conn): + if connection is not None: + try: + connection.close() + except Exception: + pass # --------------------------------------------------------------------------- @@ -429,7 +436,10 @@ def run_backup(args) -> None: # Summary print() - print(f"Backup complete: {out_path}") + if errors: + print(f"Backup incomplete: {out_path}") + else: + print(f"Backup complete: {out_path}") print(f" Files: {file_count}") print(f" Original: {_format_size(total_bytes)}") print(f" Compressed: {_format_size(zip_size)}") @@ -461,7 +471,8 @@ def run_backup(args) -> None: if len(errors) > 10: print(f" ... and {len(errors) - 10} more") - print(f"\nRestore with: hermes import {out_path.name}") + if not errors: + print(f"\nRestore with: hermes import {out_path.name}") # --------------------------------------------------------------------------- @@ -1215,6 +1226,7 @@ def _write_full_zip_backup(out_path: Path, hermes_root: Path) -> Optional[Path]: if not files_to_add: return None + sqlite_snapshot_failed = False try: with zipfile.ZipFile(out_path, "w", zipfile.ZIP_DEFLATED, compresslevel=6) as zf: for abs_path, rel_path in files_to_add: @@ -1229,8 +1241,14 @@ def _write_full_zip_backup(out_path: Path, hermes_root: Path) -> Optional[Path]: ) as tmp: tmp_db = Path(tmp.name) try: - if _safe_copy_db(abs_path, tmp_db): - zf.write(tmp_db, arcname=str(rel_path)) + if not _safe_copy_db(abs_path, tmp_db): + logger.warning( + "Full-zip backup aborted: SQLite snapshot failed for %s", + rel_path, + ) + sqlite_snapshot_failed = True + break + zf.write(tmp_db, arcname=str(rel_path)) finally: tmp_db.unlink(missing_ok=True) else: @@ -1247,6 +1265,13 @@ def _write_full_zip_backup(out_path: Path, hermes_root: Path) -> Optional[Path]: pass return None + if sqlite_snapshot_failed: + try: + out_path.unlink(missing_ok=True) + except OSError: + pass + return None + return out_path diff --git a/tests/hermes_cli/test_backup.py b/tests/hermes_cli/test_backup.py index 4f30d50c750..4a0f8ce5bdd 100644 --- a/tests/hermes_cli/test_backup.py +++ b/tests/hermes_cli/test_backup.py @@ -19,8 +19,10 @@ def _make_hermes_tree(root: Path) -> None: """Create a realistic ~/.hermes directory structure for testing.""" (root / "config.yaml").write_text("model:\n provider: openrouter\n") (root / ".env").write_text("OPENROUTER_API_KEY=sk-test-123\n") - (root / "memory_store.db").write_bytes(b"fake-sqlite") - (root / "hermes_state.db").write_bytes(b"fake-state") + for db_name in ("memory_store.db", "hermes_state.db"): + with sqlite3.connect(root / db_name) as conn: + conn.execute("CREATE TABLE sample (value TEXT)") + conn.execute("INSERT INTO sample VALUES ('test')") # Sessions (root / "sessions").mkdir(exist_ok=True) @@ -225,6 +227,65 @@ class TestBackup: # Skins assert "skins/cyber.yaml" in names + def test_failed_sqlite_backup_never_raw_copies_live_wal_db(self, tmp_path, monkeypatch, capsys): + """A failed backup() must not silently archive the stale main DB file. + + Keep a real, uncheckpointed WAL transaction live so a raw copy of only + ``state.db`` would be a valid-looking but torn snapshot. + """ + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text("model: test\n") + db_path = hermes_home / "state.db" + + writer = sqlite3.connect(db_path) + writer.execute("PRAGMA journal_mode=WAL") + writer.execute("PRAGMA wal_autocheckpoint=0") + writer.execute("CREATE TABLE events (value TEXT)") + writer.commit() + writer.execute("PRAGMA wal_checkpoint(TRUNCATE)") + writer.execute("INSERT INTO events VALUES ('only-in-wal')") + writer.commit() + assert Path(f"{db_path}-wal").stat().st_size > 0 + + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + + import hermes_cli.backup as backup_mod + real_connect = backup_mod.sqlite3.connect + + class FailingBackupConnection: + def __init__(self, connection): + self._connection = connection + + def backup(self, _destination): + raise sqlite3.OperationalError("forced backup failure") + + def close(self): + self._connection.close() + + def connect_with_failed_backup(database, *args, **kwargs): + connection = real_connect(database, *args, **kwargs) + if str(database).startswith(f"file:{db_path}"): + return FailingBackupConnection(connection) + return connection + + monkeypatch.setattr(backup_mod.sqlite3, "connect", connect_with_failed_backup) + out_zip = tmp_path / "backup.zip" + try: + backup_mod.run_backup(Namespace(output=str(out_zip))) + finally: + writer.close() + + with zipfile.ZipFile(out_zip) as zf: + assert "config.yaml" in zf.namelist() + assert "state.db" not in zf.namelist() + + output = capsys.readouterr().out + assert "Backup incomplete" in output + assert "state.db: SQLite safe copy failed" in output + assert "Restore with:" not in output + def test_db_snapshots_staged_beside_output_zip(self, tmp_path, monkeypatch): """SQLite staging temp files must be created on the output zip's filesystem (dir=out_path.parent), NOT the system /tmp default — a @@ -1902,6 +1963,53 @@ class TestPreUpdateBackup: """Tests for create_pre_update_backup — the auto-backup ``hermes update`` runs before touching anything.""" + def test_failed_sqlite_snapshot_removes_incomplete_archive(self, tmp_path, monkeypatch): + """The non-interactive full-zip helper must fail the entire archive + rather than return success after omitting a live WAL database.""" + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text("model: test\n") + db_path = hermes_home / "state.db" + + writer = sqlite3.connect(db_path) + writer.execute("PRAGMA journal_mode=WAL") + writer.execute("PRAGMA wal_autocheckpoint=0") + writer.execute("CREATE TABLE events (value TEXT)") + writer.commit() + writer.execute("PRAGMA wal_checkpoint(TRUNCATE)") + writer.execute("INSERT INTO events VALUES ('only-in-wal')") + writer.commit() + assert Path(f"{db_path}-wal").stat().st_size > 0 + + import hermes_cli.backup as backup_mod + real_connect = backup_mod.sqlite3.connect + + class FailingBackupConnection: + def __init__(self, connection): + self._connection = connection + + def backup(self, _destination): + raise sqlite3.OperationalError("forced backup failure") + + def close(self): + self._connection.close() + + def connect_with_failed_backup(database, *args, **kwargs): + connection = real_connect(database, *args, **kwargs) + if str(database).startswith(f"file:{db_path}"): + return FailingBackupConnection(connection) + return connection + + monkeypatch.setattr(backup_mod.sqlite3, "connect", connect_with_failed_backup) + out_zip = tmp_path / "pre-update.zip" + try: + result = backup_mod._write_full_zip_backup(out_zip, hermes_home) + finally: + writer.close() + + assert result is None + assert not out_zip.exists() + @pytest.fixture def hermes_home(self, tmp_path): root = tmp_path / ".hermes"