From f91c71b8e0554f85007696b3f4ef7a75b561060f Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Wed, 22 Jul 2026 08:58:33 -0400 Subject: [PATCH 1/2] Snapshot fabro.sqlite3 before applying new migrations A binary downgrade after new SQLite migrations have been applied fails sqlx's startup validation ("migration was previously applied but is missing in the resolved migrations") and previously left the operator with no rollback artifact: the shared database had no backup, so recovering meant hand-editing _sqlx_migrations and dropping tables. Database::migrate now writes a consistent single-file snapshot to .pre-migration.bak (via VACUUM INTO, mode 0600) before applying any migration the database has not seen. Rollback is: stop the server, replace the database file with the snapshot, delete -wal/-shm siblings, start the previous binary. Fresh databases and no-op migrates skip the snapshot, so the file always preserves the state from immediately before the most recent schema change. A snapshot failure fails the migration: no rollback artifact, no schema change. Co-Authored-By: Claude Fable 5 --- Cargo.lock | 1 + lib/crates/fabro-db/Cargo.toml | 1 + lib/crates/fabro-db/src/lib.rs | 121 +++++++++++++++++++++++++++- lib/crates/fabro-db/tests/sqlite.rs | 97 ++++++++++++++++++++++ 4 files changed, 218 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 242b12da7f..44c3c565be 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2563,6 +2563,7 @@ dependencies = [ "sqlx", "tempfile", "tokio", + "tracing", ] [[package]] diff --git a/lib/crates/fabro-db/Cargo.toml b/lib/crates/fabro-db/Cargo.toml index c29362b0fc..34963bb7a6 100644 --- a/lib/crates/fabro-db/Cargo.toml +++ b/lib/crates/fabro-db/Cargo.toml @@ -16,6 +16,7 @@ workspace = true anyhow.workspace = true sqlx.workspace = true tokio.workspace = true +tracing.workspace = true [dev-dependencies] tempfile = "3" diff --git a/lib/crates/fabro-db/src/lib.rs b/lib/crates/fabro-db/src/lib.rs index fcf5fa8753..bce58d9913 100644 --- a/lib/crates/fabro-db/src/lib.rs +++ b/lib/crates/fabro-db/src/lib.rs @@ -1,10 +1,13 @@ -use std::path::Path; +use std::collections::HashSet; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::Context as _; use sqlx::migrate::Migrator; use sqlx::sqlite::{SqliteConnectOptions, SqliteJournalMode, SqlitePoolOptions, SqliteSynchronous}; use tokio::fs; +use tracing::info; pub type DbPool = sqlx::SqlitePool; @@ -13,6 +16,7 @@ static MIGRATOR: Migrator = sqlx::migrate!("./migrations"); #[derive(Clone)] pub struct Database { pool: DbPool, + path: PathBuf, } impl Database { @@ -37,16 +41,87 @@ impl Database { .await .with_context(|| format!("opening SQLite database {}", path.display()))?; - Ok(Self { pool }) + Ok(Self { + pool, + path: path.to_path_buf(), + }) } pub async fn migrate(&self) -> anyhow::Result<()> { + self.snapshot_before_new_migrations() + .await + .context("snapshotting SQLite database before migrations")?; MIGRATOR .run(&self.pool) .await .context("running SQLite migrations") } + /// Copy the database aside before applying migrations it has not seen. + /// + /// A binary downgrade after new migrations have been applied fails sqlx's + /// startup validation (`migration N was previously applied but is missing + /// in the resolved migrations`), so the snapshot written to + /// [`pre_migration_snapshot_path`] is the operator's rollback artifact: + /// stop the server, replace the database file with the snapshot (and + /// delete any `-wal`/`-shm` siblings), and the previous binary boots + /// again. Writes made after the upgrade are lost on rollback, as with any + /// point-in-time restore. + /// + /// The snapshot is only taken when the database has applied migrations + /// before (a fresh database has nothing worth preserving) and at least + /// one bundled migration is pending, so the file always holds the state + /// from immediately before the most recent schema change. Failing to + /// write the snapshot fails the migration: no rollback artifact, no + /// schema change. + async fn snapshot_before_new_migrations(&self) -> anyhow::Result<()> { + let applied = applied_migration_versions(&self.pool).await?; + if applied.is_empty() { + return Ok(()); + } + if !MIGRATOR + .iter() + .any(|migration| !applied.contains(&migration.version)) + { + return Ok(()); + } + + let snapshot_path = pre_migration_snapshot_path(&self.path); + match fs::remove_file(&snapshot_path).await { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => { + return Err(err).with_context(|| { + format!( + "removing stale pre-migration snapshot {}", + snapshot_path.display() + ) + }); + } + } + + // VACUUM INTO produces a consistent single-file copy from the live + // pool, so the snapshot needs no -wal/-shm siblings to restore. + let snapshot_target = snapshot_path + .to_str() + .context("database path is not valid UTF-8")?; + sqlx::query("VACUUM INTO ?") + .bind(snapshot_target) + .execute(&self.pool) + .await + .with_context(|| { + format!("writing pre-migration snapshot {}", snapshot_path.display()) + })?; + set_private_permissions(&snapshot_path).await?; + + info!( + database = %self.path.display(), + snapshot = %snapshot_path.display(), + "Snapshotted SQLite database before applying new migrations" + ); + Ok(()) + } + pub async fn health_check(&self) -> anyhow::Result<()> { sqlx::query("SELECT 1") .execute(&self.pool) @@ -63,3 +138,45 @@ impl Database { self.pool.clone() } } + +/// Rollback artifact written by [`Database::migrate`] before applying new +/// migrations: the database file name with `.pre-migration.bak` appended, +/// next to the database. +pub fn pre_migration_snapshot_path(database_path: &Path) -> PathBuf { + let mut file_name = database_path + .file_name() + .map_or_else(|| OsString::from("fabro.sqlite3"), OsString::from); + file_name.push(".pre-migration.bak"); + database_path.with_file_name(file_name) +} + +async fn applied_migration_versions(pool: &DbPool) -> anyhow::Result> { + let table_count: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = '_sqlx_migrations'", + ) + .fetch_one(pool) + .await + .context("checking for the sqlx migrations table")?; + if table_count == 0 { + return Ok(HashSet::new()); + } + let versions: Vec = sqlx::query_scalar("SELECT version FROM _sqlx_migrations") + .fetch_all(pool) + .await + .context("listing applied migration versions")?; + Ok(versions.into_iter().collect()) +} + +#[cfg(unix)] +async fn set_private_permissions(path: &Path) -> anyhow::Result<()> { + use std::os::unix::fs::PermissionsExt; + + fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + .await + .with_context(|| format!("setting permissions on {}", path.display())) +} + +#[cfg(not(unix))] +async fn set_private_permissions(_path: &Path) -> anyhow::Result<()> { + Ok(()) +} diff --git a/lib/crates/fabro-db/tests/sqlite.rs b/lib/crates/fabro-db/tests/sqlite.rs index a030dafa77..0c471bdc16 100644 --- a/lib/crates/fabro-db/tests/sqlite.rs +++ b/lib/crates/fabro-db/tests/sqlite.rs @@ -123,3 +123,100 @@ async fn variables_schema_enforces_env_style_names() -> anyhow::Result<()> { Ok(()) } + +#[tokio::test] +async fn fresh_database_migrate_takes_no_snapshot() -> anyhow::Result<()> { + let dir = tempfile::tempdir()?; + let db_path = dir.path().join("fabro.sqlite3"); + + let database = fabro_db::Database::connect(&db_path).await?; + database.migrate().await?; + + assert!( + !fabro_db::pre_migration_snapshot_path(&db_path).exists(), + "a fresh database has no pre-migration state worth snapshotting" + ); + Ok(()) +} + +// Simulates a binary upgrade: a database whose `_sqlx_migrations` table is +// missing an entry for a bundled migration is exactly what an older binary +// leaves behind for a newer one. The environments migration is pure CREATE +// TABLE, so dropping the table and deleting its version row makes it pending +// again without violating checksums. +#[tokio::test] +async fn migrate_snapshots_database_before_applying_new_migrations() -> anyhow::Result<()> { + let dir = tempfile::tempdir()?; + let db_path = dir.path().join("fabro.sqlite3"); + let snapshot_path = fabro_db::pre_migration_snapshot_path(&db_path); + + let database = fabro_db::Database::connect(&db_path).await?; + database.migrate().await?; + sqlx::query( + "INSERT INTO variables (name, value, created_at, updated_at) \ + VALUES ('SNAPSHOT_MARKER', 'kept', '2026-07-22T00:00:00Z', '2026-07-22T00:00:00Z')", + ) + .execute(database.pool()) + .await?; + sqlx::query("DROP TABLE environments") + .execute(database.pool()) + .await?; + sqlx::query("DELETE FROM _sqlx_migrations WHERE version = 2026063002") + .execute(database.pool()) + .await?; + + database.migrate().await?; + + assert!( + snapshot_path.exists(), + "pending migration must snapshot first" + ); + let snapshot = + sqlx::SqlitePool::connect(&format!("sqlite://{}?mode=ro", snapshot_path.display())).await?; + let snapshot_environments: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'environments'", + ) + .fetch_one(&snapshot) + .await?; + assert_eq!( + snapshot_environments, 0, + "snapshot must hold the pre-migration schema" + ); + let snapshot_marker: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM variables WHERE name = 'SNAPSHOT_MARKER'") + .fetch_one(&snapshot) + .await?; + assert_eq!(snapshot_marker, 1, "snapshot must preserve row data"); + snapshot.close().await; + + let migrated_environments: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'environments'", + ) + .fetch_one(database.pool()) + .await?; + assert_eq!(migrated_environments, 1, "migration must still apply"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&snapshot_path)?.permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "snapshot must be private"); + } + + // With nothing pending, migrate must not rewrite the snapshot: it still + // holds the state from before the most recent schema change. + database.migrate().await?; + let snapshot = + sqlx::SqlitePool::connect(&format!("sqlite://{}?mode=ro", snapshot_path.display())).await?; + let snapshot_environments: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'environments'", + ) + .fetch_one(&snapshot) + .await?; + assert_eq!( + snapshot_environments, 0, + "no-pending migrate must leave the snapshot untouched" + ); + snapshot.close().await; + Ok(()) +} From f97ac8df3c6821a5d6af99d7c1ddd9f96b07a73d Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Wed, 22 Jul 2026 12:08:25 -0400 Subject: [PATCH 2/2] Simplify pre-migration snapshot internals - Detect applied migrations via sqlx's Migrate trait (ensure_migrations_table + list_applied_migrations) instead of hand-querying the _sqlx_migrations bookkeeping table, so the check cannot drift from what Migrator::run actually applies. - Write the snapshot to a staging file and rename it into place, so a failure mid-copy never leaves a partial file at the snapshot path. - Derive the database path from the pool's connect options instead of storing a duplicate copy on Database. - Drop the invented "fabro.sqlite3" fallback filename from pre_migration_snapshot_path; append the suffix to the path directly. - Deduplicate the snapshot-inspection blocks in the test behind small connect_read_only/table_exists helpers. Co-Authored-By: Claude Fable 5 --- lib/crates/fabro-db/src/lib.rs | 127 ++++++++++++++++------------ lib/crates/fabro-db/tests/sqlite.rs | 47 +++++----- 2 files changed, 98 insertions(+), 76 deletions(-) diff --git a/lib/crates/fabro-db/src/lib.rs b/lib/crates/fabro-db/src/lib.rs index bce58d9913..f895ee07a0 100644 --- a/lib/crates/fabro-db/src/lib.rs +++ b/lib/crates/fabro-db/src/lib.rs @@ -1,10 +1,9 @@ use std::collections::HashSet; -use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::Context as _; -use sqlx::migrate::Migrator; +use sqlx::migrate::{Migrate as _, Migrator}; use sqlx::sqlite::{SqliteConnectOptions, SqliteJournalMode, SqlitePoolOptions, SqliteSynchronous}; use tokio::fs; use tracing::info; @@ -16,7 +15,6 @@ static MIGRATOR: Migrator = sqlx::migrate!("./migrations"); #[derive(Clone)] pub struct Database { pool: DbPool, - path: PathBuf, } impl Database { @@ -41,10 +39,7 @@ impl Database { .await .with_context(|| format!("opening SQLite database {}", path.display()))?; - Ok(Self { - pool, - path: path.to_path_buf(), - }) + Ok(Self { pool }) } pub async fn migrate(&self) -> anyhow::Result<()> { @@ -76,46 +71,61 @@ impl Database { /// schema change. async fn snapshot_before_new_migrations(&self) -> anyhow::Result<()> { let applied = applied_migration_versions(&self.pool).await?; - if applied.is_empty() { - return Ok(()); - } - if !MIGRATOR + let has_pending = MIGRATOR .iter() - .any(|migration| !applied.contains(&migration.version)) - { + .any(|migration| !applied.contains(&migration.version)); + if applied.is_empty() || !has_pending { return Ok(()); } - let snapshot_path = pre_migration_snapshot_path(&self.path); - match fs::remove_file(&snapshot_path).await { - Ok(()) => {} - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => { - return Err(err).with_context(|| { - format!( - "removing stale pre-migration snapshot {}", - snapshot_path.display() - ) - }); - } - } + let connect_options = self.pool.connect_options(); + let database_path = connect_options.get_filename(); + let snapshot_path = pre_migration_snapshot_path(database_path); // VACUUM INTO produces a consistent single-file copy from the live - // pool, so the snapshot needs no -wal/-shm siblings to restore. - let snapshot_target = snapshot_path + // pool, so the snapshot needs no -wal/-shm siblings to restore. It + // writes to a staging file that is renamed into place afterwards, so + // a failure mid-copy never leaves a partial file at the snapshot + // path. + let staging_path = append_to_path(&snapshot_path, ".tmp"); + remove_file_if_exists(&staging_path) + .await + .with_context(|| { + format!( + "removing stale snapshot staging file {}", + staging_path.display() + ) + })?; + let staging_target = staging_path .to_str() - .context("database path is not valid UTF-8")?; + .context("snapshot staging path is not valid UTF-8")?; sqlx::query("VACUUM INTO ?") - .bind(snapshot_target) + .bind(staging_target) .execute(&self.pool) .await .with_context(|| { - format!("writing pre-migration snapshot {}", snapshot_path.display()) + format!("writing pre-migration snapshot {}", staging_path.display()) + })?; + set_private_permissions(&staging_path).await?; + remove_file_if_exists(&snapshot_path) + .await + .with_context(|| { + format!( + "removing stale pre-migration snapshot {}", + snapshot_path.display() + ) + })?; + fs::rename(&staging_path, &snapshot_path) + .await + .with_context(|| { + format!( + "publishing pre-migration snapshot {}", + snapshot_path.display() + ) })?; - set_private_permissions(&snapshot_path).await?; info!( - database = %self.path.display(), + database = %database_path.display(), snapshot = %snapshot_path.display(), "Snapshotted SQLite database before applying new migrations" ); @@ -140,31 +150,44 @@ impl Database { } /// Rollback artifact written by [`Database::migrate`] before applying new -/// migrations: the database file name with `.pre-migration.bak` appended, -/// next to the database. +/// migrations: the database path with `.pre-migration.bak` appended. pub fn pre_migration_snapshot_path(database_path: &Path) -> PathBuf { - let mut file_name = database_path - .file_name() - .map_or_else(|| OsString::from("fabro.sqlite3"), OsString::from); - file_name.push(".pre-migration.bak"); - database_path.with_file_name(file_name) + append_to_path(database_path, ".pre-migration.bak") +} + +fn append_to_path(path: &Path, suffix: &str) -> PathBuf { + let mut path = path.as_os_str().to_os_string(); + path.push(suffix); + PathBuf::from(path) } async fn applied_migration_versions(pool: &DbPool) -> anyhow::Result> { - let table_count: i64 = sqlx::query_scalar( - "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = '_sqlx_migrations'", - ) - .fetch_one(pool) - .await - .context("checking for the sqlx migrations table")?; - if table_count == 0 { - return Ok(HashSet::new()); - } - let versions: Vec = sqlx::query_scalar("SELECT version FROM _sqlx_migrations") - .fetch_all(pool) + let mut conn = pool + .acquire() + .await + .context("acquiring a SQLite connection")?; + // Migrator::run performs this same ensure + list as its first step, so + // asking the Migrate trait (rather than querying sqlx's bookkeeping + // table by hand) cannot drift from what it will actually apply. + conn.ensure_migrations_table(&MIGRATOR.table_name) + .await + .context("ensuring the sqlx migrations table exists")?; + let applied = conn + .list_applied_migrations(&MIGRATOR.table_name) .await .context("listing applied migration versions")?; - Ok(versions.into_iter().collect()) + Ok(applied + .into_iter() + .map(|migration| migration.version) + .collect()) +} + +async fn remove_file_if_exists(path: &Path) -> std::io::Result<()> { + match fs::remove_file(path).await { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err), + } } #[cfg(unix)] diff --git a/lib/crates/fabro-db/tests/sqlite.rs b/lib/crates/fabro-db/tests/sqlite.rs index 0c471bdc16..28af2aadd3 100644 --- a/lib/crates/fabro-db/tests/sqlite.rs +++ b/lib/crates/fabro-db/tests/sqlite.rs @@ -171,15 +171,9 @@ async fn migrate_snapshots_database_before_applying_new_migrations() -> anyhow:: snapshot_path.exists(), "pending migration must snapshot first" ); - let snapshot = - sqlx::SqlitePool::connect(&format!("sqlite://{}?mode=ro", snapshot_path.display())).await?; - let snapshot_environments: i64 = sqlx::query_scalar( - "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'environments'", - ) - .fetch_one(&snapshot) - .await?; - assert_eq!( - snapshot_environments, 0, + let snapshot = connect_read_only(&snapshot_path).await?; + assert!( + !table_exists(&snapshot, "environments").await?, "snapshot must hold the pre-migration schema" ); let snapshot_marker: i64 = @@ -189,12 +183,10 @@ async fn migrate_snapshots_database_before_applying_new_migrations() -> anyhow:: assert_eq!(snapshot_marker, 1, "snapshot must preserve row data"); snapshot.close().await; - let migrated_environments: i64 = sqlx::query_scalar( - "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'environments'", - ) - .fetch_one(database.pool()) - .await?; - assert_eq!(migrated_environments, 1, "migration must still apply"); + assert!( + table_exists(database.pool(), "environments").await?, + "migration must still apply" + ); #[cfg(unix)] { @@ -206,17 +198,24 @@ async fn migrate_snapshots_database_before_applying_new_migrations() -> anyhow:: // With nothing pending, migrate must not rewrite the snapshot: it still // holds the state from before the most recent schema change. database.migrate().await?; - let snapshot = - sqlx::SqlitePool::connect(&format!("sqlite://{}?mode=ro", snapshot_path.display())).await?; - let snapshot_environments: i64 = sqlx::query_scalar( - "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'environments'", - ) - .fetch_one(&snapshot) - .await?; - assert_eq!( - snapshot_environments, 0, + let snapshot = connect_read_only(&snapshot_path).await?; + assert!( + !table_exists(&snapshot, "environments").await?, "no-pending migrate must leave the snapshot untouched" ); snapshot.close().await; Ok(()) } + +async fn connect_read_only(path: &std::path::Path) -> anyhow::Result { + Ok(sqlx::SqlitePool::connect(&format!("sqlite://{}?mode=ro", path.display())).await?) +} + +async fn table_exists(pool: &sqlx::SqlitePool, table: &str) -> anyhow::Result { + let count: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = ?") + .bind(table) + .fetch_one(pool) + .await?; + Ok(count == 1) +}