dtourolle/jellytau
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 45 Wiki Activity

fix(storage): make each migration atomic so a partial failure can't brick the app

Browse Source 
Migrations ran as bare `execute_batch` calls with the `_migrations` row
written afterwards. SQLite autocommits every statement, so a migration
that died partway — low disk, an OOM kill, the process dying mid-boot —
left its earlier statements applied and recorded nothing.

That is unrecoverable rather than merely untidy. `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` turns a migration error into a `panic!` — the app never
started again and the only fix was clearing app data, losing downloads
and logins. Several migrations have exactly the shape that triggers it:
006 is three `ADD COLUMN`s, 003/005/024 are full table rebuilds.

Each migration now runs in one transaction with its `_migrations` row
committed inside it, so a migration is all-or-nothing and a retry is
always safe. Every migration is pure DDL/DML, which SQLite runs
transactionally; a `PRAGMA` or `VACUUM` added to one would not roll back.

`migrate()` delegates to a new `migrate_with()` so a test can inject a
deliberately-failing migration.
This commit is contained in:
dtourolle
2026-09-07 22:24:45 +02:00
parent 7b738002a0
commit d4f80a4afa
1 changed files with 139 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src-tauri/src/storage/mod.rs
+139 -21
View File
@@ -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();