db migrate: retry the legacy reminders copy while the legacy file is still there

hive-agent/src/db_migrate.rs

@@ -4,7 +4,8 @@
 //! another sqlite ... add it to one of the existing ones"). Idempotent + best
 //! effort: `run` is called once in `main.rs`, before anything opens the
 //! consolidated db — a fresh harness boot with no legacy files (or one that
-//! already migrated) is a silent no-op.
+//! already migrated) is a silent no-op, and a failed step leaves its legacy
+//! file in place to be retried on the next boot.
 //!
 //! Two legacy shapes fold in:
 //! - `hyperhive-todos.sqlite` already has exactly the `todos` table the
@@ -23,26 +24,24 @@ use anyhow::{Context, Result};
 use rusqlite::{Connection, params};
 
 /// Fold the legacy `legacy_todos`/`legacy_reminders` files (if present) into
-/// the single consolidated `state_db` path. No-op if `state_db` already
+/// the single consolidated `state_db` path. The legacy todos file is only
-/// exists — legacy files are only ever consulted on the first boot after
+/// renamed in while `state_db` does not exist yet; the legacy reminders file
-/// the db-consolidation upgrade.
+/// is copied in whenever it is still on disk, so a copy that failed on one
+/// boot is retried on the next.
 ///
 /// # Errors
 ///
 /// Propagates filesystem rename / sqlite attach-copy failures. Callers
 /// treat this as best-effort (log and continue) — `Todos`/`Reminders::open`
 /// against `state_db` still work standalone either way (todos would simply
-/// start empty rather than inheriting the legacy rows). Retry-next-boot
+/// start empty rather than inheriting the legacy rows). A failed todos
-/// only holds for failures before `state_db` exists (the todos rename, or
+/// rename or reminders copy leaves its legacy file in place and is retried
-/// the reminders copy itself): once the copy has landed, `state_db` exists,
+/// on the next boot, and so is a failed `remove_file(legacy_reminders)`.
-/// so a `remove_file(legacy_reminders)` failure afterwards leaves an
+/// A retry skips rows already present with the same content, but a legacy
-/// orphaned (already-copied, now-inert) legacy file behind rather than
+/// row that was copied and then cancelled or pruned from the consolidated
-/// retrying — the rows are safe, but the leftover file won't self-clean.
+/// db comes back, which is the cost of retrying at all.
 pub fn run(state_db: &Path, legacy_todos: &Path, legacy_reminders: &Path) -> Result<()> {
-    if state_db.exists() {
+    if !state_db.exists() && legacy_todos.exists() {
-        return Ok(());
-    }
-    if legacy_todos.exists() {
         std::fs::rename(legacy_todos, state_db).with_context(|| {
             format!(
                 "rename legacy todos db {} -> {}",
@@ -80,11 +79,31 @@ fn migrate_reminders_rows(state_db: &Path, legacy_reminders: &Path) -> Result<()
     )
     .context("attach legacy reminders db")?;
     let copied = conn
-        .execute_batch(
+        .query_row("SELECT COUNT(*) FROM main.reminders", [], |row| {
-            "INSERT INTO reminders (id, message, file_path, due_at, created_at, sent_at) \
+            row.get::<_, i64>(0)
-             SELECT id, message, file_path, due_at, created_at, sent_at FROM legacy.reminders;",
+        })
-        )
+        .context("count consolidated reminders")
-        .context("copy legacy reminders rows");
+        .and_then(|existing| {
+            // Ids are kept while the table is empty (the first-boot case an
+            // id seen just before the upgrade relies on). A retry after the
+            // harness already stored reminders of its own cannot reuse them:
+            // a colliding id would silently drop the legacy row. Those rows
+            // are appended under fresh ids instead, skipping any that are
+            // already there with the same content.
+            let sql = if existing == 0 {
+                "INSERT INTO reminders (id, message, file_path, due_at, created_at, sent_at) \
+                 SELECT id, message, file_path, due_at, created_at, sent_at FROM legacy.reminders;"
+            } else {
+                "INSERT INTO reminders (message, file_path, due_at, created_at, sent_at) \
+                 SELECT l.message, l.file_path, l.due_at, l.created_at, l.sent_at \
+                 FROM legacy.reminders l WHERE NOT EXISTS ( \
+                     SELECT 1 FROM main.reminders r WHERE r.message = l.message \
+                     AND r.due_at = l.due_at AND r.created_at = l.created_at \
+                     AND r.file_path IS l.file_path);"
+            };
+            conn.execute_batch(sql)
+                .context("copy legacy reminders rows")
+        });
     // DETACH regardless of the copy's outcome so the connection doesn't
     // leak the attachment on an early return via `?` inside this fn.
     let detach = conn.execute("DETACH DATABASE legacy", []);
@@ -109,7 +128,7 @@ mod tests {
     }
 
     #[test]
-    fn state_db_already_present_short_circuits() {
+    fn an_existing_state_db_leaves_a_legacy_todos_file_alone() {
         let dir = tempfile::tempdir().unwrap();
         let state = dir.path().join("hyperhive-state.sqlite");
         let legacy_todos = dir.path().join("hyperhive-todos.sqlite");
@@ -175,6 +194,74 @@ mod tests {
         assert_eq!(pending[0].due_at.timestamp(), 1000);
     }
 
+    /// A copy that fails after `state_db` was created must be retried on the
+    /// next boot, not abandoned because the consolidated db now exists.
+    #[test]
+    fn a_failed_reminders_copy_is_retried_once_the_state_db_exists() {
+        let dir = tempfile::tempdir().unwrap();
+        let legacy_reminders = dir.path().join("hyperhive-reminders.sqlite");
+        let state = dir.path().join("hyperhive-state.sqlite");
+        let legacy_todos = dir.path().join("hyperhive-todos.sqlite");
+        std::fs::write(&legacy_reminders, b"not a sqlite file").unwrap();
+
+        run(&state, &legacy_todos, &legacy_reminders).unwrap_err();
+        assert!(state.exists(), "the failed copy still created the db");
+        assert!(legacy_reminders.exists(), "legacy file kept for a retry");
+
+        std::fs::remove_file(&legacy_reminders).unwrap();
+        Reminders::open(&legacy_reminders)
+            .unwrap()
+            .store("ping", None, 500)
+            .unwrap();
+        run(&state, &legacy_todos, &legacy_reminders).unwrap();
+
+        assert!(!legacy_reminders.exists(), "removed after the retry");
+        assert_eq!(
+            Reminders::open(&state)
+                .unwrap()
+                .list_pending()
+                .unwrap()
+                .len(),
+            1
+        );
+    }
+
+    /// The harness boots after a failed copy and stores reminders of its own,
+    /// so their ids overlap the legacy ones. The retry must keep both sets.
+    #[test]
+    fn a_retry_keeps_legacy_reminders_whose_ids_collide_with_new_ones() {
+        let dir = tempfile::tempdir().unwrap();
+        let legacy_reminders = dir.path().join("hyperhive-reminders.sqlite");
+        let state = dir.path().join("hyperhive-state.sqlite");
+        let legacy_todos = dir.path().join("hyperhive-todos.sqlite");
+        std::fs::write(&legacy_reminders, b"not a sqlite file").unwrap();
+        run(&state, &legacy_todos, &legacy_reminders).unwrap_err();
+        Reminders::open(&state)
+            .unwrap()
+            .store("new while degraded", None, 900)
+            .unwrap();
+
+        std::fs::remove_file(&legacy_reminders).unwrap();
+        {
+            let legacy = Reminders::open(&legacy_reminders).unwrap();
+            legacy.store("legacy one", None, 100).unwrap();
+            legacy.store("legacy two", None, 200).unwrap();
+        }
+        run(&state, &legacy_todos, &legacy_reminders).unwrap();
+
+        let messages: Vec<String> = Reminders::open(&state)
+            .unwrap()
+            .list_pending()
+            .unwrap()
+            .into_iter()
+            .map(|r| r.message)
+            .collect();
+        assert_eq!(messages.len(), 3, "got {messages:?}");
+        for expected in ["new while degraded", "legacy one", "legacy two"] {
+            assert!(messages.iter().any(|m| m == expected), "missing {expected}");
+        }
+    }
+
     #[test]
     fn folds_both_legacy_files_into_one_consolidated_db() {
         let dir = tempfile::tempdir().unwrap();