diff --git a/src-tauri/src/storage/mod.rs b/src-tauri/src/storage/mod.rs index d5e0c9d34..57c030149 100644 --- a/src-tauri/src/storage/mod.rs +++ b/src-tauri/src/storage/mod.rs @@ -75,8 +75,31 @@ impl Database { Arc::clone(&self.conn) } - /// Run all pending migrations + /// Run all pending migrations. pub fn migrate(&self) -> SqliteResult<()> { + self.migrate_with(MIGRATIONS) + } + + /// Apply `migrations` in order, skipping ones `_migrations` already records. + /// + /// **Each migration is one transaction, and the `_migrations` row is written + /// inside it.** SQLite autocommits every statement otherwise, so a migration + /// that failed partway — low disk, an OOM kill, the process dying mid-boot — + /// used to leave its earlier statements applied while recording nothing. + /// `execute_batch` aborts on the first error, so the retry on the next launch + /// then failed at statement 1 ("duplicate column name") and kept failing + /// forever; `Database::open` turns that into a panic, so the app never + /// started again and the only fix was clearing app data. Committing the + /// schema change and the bookkeeping together makes a migration all-or-nothing + /// and a retry always safe. + /// + /// Every migration is pure DDL/DML, which SQLite runs transactionally — a + /// `PRAGMA` or `VACUUM` added to one would not roll back and must not be. + /// + /// Split out from [`Self::migrate`] so tests can inject a failing migration. + /// + /// TRACES: UR-002 | DR-012 | UT-014 + fn migrate_with(&self, migrations: &[(&str, &str)]) -> SqliteResult<()> { info!("Starting database migrations..."); let conn = self.conn.lock_safe(); @@ -107,28 +130,30 @@ impl Database { debug!("Found {} applied migrations", applied.len()); // Apply pending migrations - for (name, sql) in MIGRATIONS { - if !applied.contains(&name.to_string()) { - info!("Applying migration: {}", name); - match conn.execute_batch(sql) { - Ok(_) => { - info!("Successfully applied migration: {}", name); - match conn.execute("INSERT INTO _migrations (name) VALUES (?1)", [name]) { - Ok(_) => debug!("Recorded migration: {}", name), - Err(e) => { - error!("Failed to record migration {}: {}", name, e); - return Err(e); - } - } - } - Err(e) => { - error!("Failed to apply migration {}: {}", name, e); - return Err(e); - } - } - } else { + for (name, sql) in migrations { + if applied.contains(&name.to_string()) { debug!("Skipping already applied migration: {}", name); + continue; } + + info!("Applying migration: {}", name); + + // `unchecked_transaction` because the connection is reached through a + // shared guard rather than `&mut`. Dropping the transaction without + // committing rolls it back, which is exactly what the `?`s below do. + let tx = conn.unchecked_transaction()?; + + if let Err(e) = tx.execute_batch(sql) { + error!("Failed to apply migration {} (rolled back): {}", name, e); + return Err(e); + } + if let Err(e) = tx.execute("INSERT INTO _migrations (name) VALUES (?1)", [name]) { + error!("Failed to record migration {} (rolled back): {}", name, e); + return Err(e); + } + tx.commit()?; + + info!("Successfully applied migration: {}", name); } info!("All migrations completed successfully"); @@ -166,6 +191,99 @@ mod tests { assert_eq!(db.path().to_str(), Some(":memory:")); } + /// A migration that dies partway must leave *nothing* behind. + /// + /// SQLite autocommits each statement, so before migrations were wrapped in a + /// transaction the first `ADD COLUMN` of a failing batch stuck while the + /// `_migrations` row was never written. `execute_batch` aborts on the first + /// error, so the retry on the next launch failed at statement 1 with + /// "duplicate column name" and kept failing forever — and `Database::open` + /// panics on that, so the app never started again. + /// + /// TRACES: UR-002 | DR-012 | UT-014 + #[test] + fn test_failed_migration_rolls_back_and_stays_retryable() { + let db = Database::open_in_memory().unwrap(); + + // Statement 1 succeeds, statement 2 fails, statement 3 never runs. + let poisoned = &[( + "900_partially_failing", + "ALTER TABLE downloads ADD COLUMN audit_a TEXT; + ALTER TABLE downloads ADD COLUMN audit_b TEXT FROM NOWHERE; + ALTER TABLE downloads ADD COLUMN audit_c TEXT;", + )][..]; + + let first = db.migrate_with(poisoned).unwrap_err(); + + // Nothing from the batch may survive, or the retry cannot re-run it. + assert!( + !has_column(&db, "downloads", "audit_a"), + "statement 1 of a failed migration was left applied: the batch did not roll back" + ); + assert!(!has_column(&db, "downloads", "audit_c")); + + // And it must not be recorded as applied. + let recorded: i64 = db + .connection() + .lock_safe() + .query_row( + "SELECT COUNT(*) FROM _migrations WHERE name = ?1", + ["900_partially_failing"], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(recorded, 0, "a failed migration must not be recorded"); + + // The retry must fail the same way it did the first time — reaching the + // real error — rather than tripping over its own leftovers. + let second = db.migrate_with(poisoned).unwrap_err(); + assert!( + !second.to_string().contains("duplicate column"), + "the retry hit leftovers from the failed run instead of the real error: {second}" + ); + assert_eq!(first.to_string(), second.to_string()); + + // A corrected migration under the same name then applies cleanly. + let fixed = &[( + "900_partially_failing", + "ALTER TABLE downloads ADD COLUMN audit_a TEXT; + ALTER TABLE downloads ADD COLUMN audit_c TEXT;", + )][..]; + db.migrate_with(fixed).unwrap(); + assert!(has_column(&db, "downloads", "audit_a")); + assert!(has_column(&db, "downloads", "audit_c")); + } + + /// A committed migration is recorded, so it is never applied twice. + /// + /// TRACES: UR-002 | DR-012 | UT-014 + #[test] + fn test_successful_migration_is_recorded_in_the_same_transaction() { + let db = Database::open_in_memory().unwrap(); + let m = &[( + "901_adds_a_column", + "ALTER TABLE downloads ADD COLUMN audit_d TEXT;", + )][..]; + + db.migrate_with(m).unwrap(); + // Re-running must be a no-op, not a "duplicate column" failure. + db.migrate_with(m).unwrap(); + assert!(has_column(&db, "downloads", "audit_d")); + } + + fn has_column(db: &Database, table: &str, column: &str) -> bool { + let conn = db.connection(); + let conn = conn.lock_safe(); + let mut stmt = conn + .prepare(&format!("PRAGMA table_info({table})")) + .unwrap(); + let mut names = stmt + .query_map([], |row| row.get::<_, String>(1)) + .unwrap() + .filter_map(|r| r.ok()); + names.any(|n| n == column) + } + #[test] fn test_migrations_run() { let db = Database::open_in_memory().unwrap();