From 88d55f31e4f68d40592f7956ddee5b38694bca2e Mon Sep 17 00:00:00 2001 From: webtecnica <75556242+webtecnica@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:50:19 -0300 Subject: [PATCH] fix(backup): restore state.db through SQLite backup API so live connections see restored data (#65942) --- hermes_cli/backup.py | 71 +++++++++++++++++++++++++++++--- hermes_cli/cli_commands_mixin.py | 17 +++++++- tests/hermes_cli/test_backup.py | 35 ++++++++++++++++ 3 files changed, 117 insertions(+), 6 deletions(-) diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index c9cee26ffc..3d5b24e02a 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -635,6 +635,67 @@ def copy_db_and_verify(src: Path, dst: Path) -> bool: return True +def _safe_restore_db(src: Path, dst: Path) -> bool: + """Restore a SQLite database from snapshot *src* into live *dst*. + + Uses SQLite's backup() API to write snapshot pages into the live + database file, preserving the file's inode and WAL state so that + any other process still holding the DB open (gateway, dashboard, + another CLI session) sees the restored data on the next read — + instead of continuing to serve stale cached pages from a replaced + inode. + + The old approach was ``unlink() + move()``, which replaced the file + under any live connection. SQLite connections cache pages in + per-connection page caches keyed by inode; after an unlink+move the + old inode still existed (the live connection held a reference), so + that connection continued serving the pre-restore data while new + connections saw the restored snapshot — a partial/inconsistent + state (issue #65942). + + By writing pages through the backup API the file inode is preserved, + the WAL journal is updated correctly, and all connections (old and + new) converge on the restored data. + + Falls back to the unlink+move approach on failure so restore never + blocks on a transient error. + """ + try: + dst_conn = sqlite3.connect(str(dst)) + try: + # Force a WAL checkpoint so the backup starts from a clean + # state rather than writing on top of a deep WAL. + dst_conn.execute("PRAGMA wal_checkpoint(TRUNCATE)") + except Exception: + pass + src_conn = sqlite3.connect(f"file:{src}?mode=ro", uri=True) + try: + src_conn.backup(dst_conn) + finally: + src_conn.close() + dst_conn.close() + # Restore original file permissions from the snapshot + try: + mode = src.stat().st_mode + dst.chmod(mode) + except Exception: + pass + return True + except Exception as exc: + logger.warning("SQLite safe restore failed for %s -> %s: %s", src, dst, exc) + # Fallback: unlink+move (the old approach). This still works for + # the common case where no other process holds the DB open. + try: + tmp = dst.parent / f".{dst.name}.snap_restore" + shutil.copy2(src, tmp) + dst.unlink(missing_ok=True) + shutil.move(str(tmp), str(dst)) + return True + except Exception as exc2: + logger.error("Fallback restore also failed for %s -> %s: %s", src, dst, exc2) + return False + + # --------------------------------------------------------------------------- # Backup # --------------------------------------------------------------------------- @@ -1663,11 +1724,11 @@ def restore_quick_snapshot( try: if dst.suffix == ".db": - # Atomic-ish replace for databases - tmp = dst.parent / f".{dst.name}.snap_restore" - shutil.copy2(src, tmp) - dst.unlink(missing_ok=True) - shutil.move(str(tmp), str(dst)) + # Restore through SQLite backup API so live connections + # (gateway, dashboard, another CLI session) see the + # restored data instead of continuing to serve stale + # cached pages from a replaced inode (issue #65942). + _safe_restore_db(src, dst) else: shutil.copy2(src, dst) restored += 1 diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index d16050a495..3ee64cd682 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -385,9 +385,24 @@ class CLICommandsMixin: return except ValueError: pass + + # Close our local SessionDB connection before restore so the + # backup-API restore doesn't contend with a live connection to + # state.db from this same process (issue #65942). + local_session_db = getattr(self, "_session_db", None) + if local_session_db is not None: + try: + local_session_db.close() + self._session_db = None + except Exception: + pass + if restore_quick_snapshot(snap_id): print(f" Restored state from: {snap_id}") - print(" Restart recommended for state.db changes to take effect.") + print( + " Restart recommended for gateway/dashboard processes " + "to pick up state.db changes." + ) else: print(f" Snapshot not found: {snap_id}") diff --git a/tests/hermes_cli/test_backup.py b/tests/hermes_cli/test_backup.py index 6451b5080b..e78cfb57f4 100644 --- a/tests/hermes_cli/test_backup.py +++ b/tests/hermes_cli/test_backup.py @@ -1248,6 +1248,41 @@ class TestQuickSnapshot: assert "state.db" not in data.get("files", {}) assert "state.db" in data.get("failed_dbs", []) + def test_restore_state_db_live_connection(self, hermes_home): + """Restoring state.db must update data visible through a live connection. + + Regression test for #65942: when state.db is open with a live SQLite + connection (as happens with the gateway, dashboard, or another CLI + session), the restore must write pages through the backup API so the + live connection sees the restored data instead of stale cached pages + from a replaced inode. + """ + from hermes_cli.backup import create_quick_snapshot, restore_quick_snapshot + snap_id = create_quick_snapshot(hermes_home=hermes_home) + + # Open a live connection (simulating gateway/dashboard). + live_conn = sqlite3.connect(str(hermes_home / "state.db")) + live_conn.execute("PRAGMA journal_mode=wal") + # Insert data AFTER the snapshot — this is what must be reverted. + live_conn.execute("INSERT INTO sessions VALUES ('s2', 'new-data')") + live_conn.commit() + + rows_before = live_conn.execute("SELECT * FROM sessions").fetchall() + assert len(rows_before) == 2 + + # Restore — the live connection stays open during restore. + result = restore_quick_snapshot(snap_id, hermes_home=hermes_home) + assert result is True + + # The live connection must see the restored (single-row) state. + # A fresh connection would trivially work; the live one is the test. + rows_after = live_conn.execute("SELECT * FROM sessions").fetchall() + live_conn.close() + assert len(rows_after) == 1, ( + f"Live connection still sees {len(rows_after)} rows after restore " + f"(expected 1); the extra row 's2' should have been reverted." + ) +