From 8c5b813ae84068912fa77ef3ec8e0759d458843c Mon Sep 17 00:00:00 2001 From: Juan Munoz Date: Mon, 3 Aug 2026 18:42:29 -0300 Subject: [PATCH 1/3] chore: improve sql migrations --- .github/workflows/lint.yml | 16 ++ CHANGELOG.md | 1 + crates/sqlite-store/README.md | 31 +++ .../sqlite-store/src/db_management/errors.rs | 41 +++- .../sqlite-store/src/db_management/utils.rs | 202 ++++++++++++++++-- crates/sqlite-store/src/lib.rs | 3 +- .../{store.sql => migrations/0001_init.sql} | 0 scripts/check-migrations.sh | 31 +++ 8 files changed, 296 insertions(+), 29 deletions(-) rename crates/sqlite-store/src/{store.sql => migrations/0001_init.sql} (100%) create mode 100755 scripts/check-migrations.sh diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5a3a6fab32..0b9e49c194 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -17,6 +17,22 @@ env: RUST_CACHE_KEY: rust-cache-2026.02.18 jobs: + migrations: + name: Migrations are append-only + runs-on: ubuntu-latest + timeout-minutes: 5 + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + with: + fetch-depth: 0 + persist-credentials: false + - name: Check that no released migration was modified + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: ./scripts/check-migrations.sh + shell: bash + unused_deps: name: Check for unused dependencies runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index 6407e1ef95..0f547a6da8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ * [FEATURE][rust,store] Added `NoteFilter::ScriptRoots` to query input notes by their note script root directly at the store level, without loading and screening unrelated notes. The filter doesn't apply to output notes: querying output notes with it returns an empty list ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)). * [rust] Added `PartialBlockchainUpdates::block_headers_to_store`, which narrows the staged headers to the ones a sync must persist: those marked as relevant, genesis, and the block at the sync height. `block_headers` still yields all staged headers ([#2297](https://github.com/0xMiden/rust-sdk/pull/2297)). * [rust] State sync now authenticates every relevant note block but only persists block headers and MMR authentication nodes for blocks containing notes that remain unspent or that a `NoteObserver` explicitly marks as relevant ([#2297](https://github.com/0xMiden/rust-sdk/pull/2297)). +* [store] The SQLite store's schema is now built from append-only migrations under `crates/sqlite-store/src/migrations/`, starting with the frozen `0001_init.sql`. An existing store is checked against the fingerprints pinned in `PINNED_SCHEMA_HASHES` rather than against a replay of the current migration files, so editing a released migration fails a test instead of silently redefining the schema those stores were built with. A CI job rejects any pull request that modifies an existing migration file ([#2346](https://github.com/0xMiden/rust-sdk/issues/2346)). ## 0.16.0-alpha.1 (2026-07-17) diff --git a/crates/sqlite-store/README.md b/crates/sqlite-store/README.md index 0de71a65f7..1cc56393aa 100644 --- a/crates/sqlite-store/README.md +++ b/crates/sqlite-store/README.md @@ -17,5 +17,36 @@ miden-client = { version = "0.13" } miden-client-sqlite-store = { version = "0.13" } ``` +## Migrations + +The schema is built by replaying the migrations listed in `MIGRATION_SCRIPTS` +(`src/db_management/utils.rs`), which include the files under `src/migrations/` in order. A file's +four-digit prefix is its schema version, which is the value SQLite records in `PRAGMA user_version`. + +Migrations are **append-only**. Every store on a user's disk was built by replaying these exact +files, and the client verifies on open that the schema it finds matches `PINNED_SCHEMA_HASHES` for +the version the database claims. That constant, not a replay of the current migration files, is the +definition of what each version's schema is, so editing a released migration is caught rather than +silently redefining the schema those databases were supposed to have. Unlike chain state, a store +holds private notes and account seeds that cannot be recovered from the network. + +Upgrades are forward-only. There are no down migrations. + +### Adding a migration + +1. Add `src/migrations/000N_short_name.sql` with the next unused prefix. Never edit an existing + file, including its comments. +2. Append `include_str!("../migrations/000N_short_name.sql")` to `MIGRATION_SCRIPTS` in + `src/db_management/utils.rs`. Nothing scans the directory, so a file that is not listed here is + never applied. +3. Append one entry to `PINNED_SCHEMA_HASHES` in the same file. Run + `cargo test -p miden-client-sqlite-store --lib migration_schema_hashes_are_stable` and take the + new hash from the failure output. Leave the existing entries alone. If they changed, the + migration edited the schema an older version built. +4. Add a `CHANGELOG.md` entry under `[store]`. + +`scripts/check-migrations.sh` runs in CI and fails a pull request that modifies, renames or deletes +a file that already exists on the base branch. + ## License This project is licensed under the MIT License. See the [LICENSE](../../LICENSE) file for details. diff --git a/crates/sqlite-store/src/db_management/errors.rs b/crates/sqlite-store/src/db_management/errors.rs index 5132aac830..3282694c97 100644 --- a/crates/sqlite-store/src/db_management/errors.rs +++ b/crates/sqlite-store/src/db_management/errors.rs @@ -11,21 +11,50 @@ use thiserror::Error; #[derive(Debug, Error)] pub enum SqliteStoreError { #[error("Database error: {0}")] - DatabaseError(String), + Database(String), #[error("Migration error: {0}")] - MigrationError(String), - #[error("Database schema does not match the schema expected for its migration version")] - SchemaHashMismatch, + Migration(String), + #[error( + "stored schema at version {version} does not match the schema this client builds for that version (expected {expected}, found {actual})" + )] + SchemaDrift { + version: u32, + expected: String, + actual: String, + }, + #[error( + "store is at schema version {found}, which is newer than the highest version this client supports ({supported})" + )] + SchemaTooNew { found: u32, supported: u32 }, } impl From for SqliteStoreError { fn from(err: RusqliteError) -> Self { - SqliteStoreError::DatabaseError(err.to_string()) + SqliteStoreError::Database(err.to_string()) } } impl From for SqliteStoreError { fn from(err: MigrationError) -> Self { - SqliteStoreError::MigrationError(err.to_string()) + SqliteStoreError::Migration(describe_migration_error(&err)) + } +} + +/// Renders a migration failure without reproducing the migration script. +pub fn describe_migration_error(err: &MigrationError) -> String { + match err { + MigrationError::RusqliteError { err, .. } => describe_sqlite_error(err), + MigrationError::ForeignKeyCheck(violations) => { + format!("{} foreign key violation(s) after applying the migration", violations.len()) + }, + other => other.to_string(), + } +} + +/// Renders a `SQLite` failure without reproducing the statement that caused it. +fn describe_sqlite_error(err: &RusqliteError) -> String { + match err { + RusqliteError::SqlInputError { msg, .. } => msg.clone(), + other => other.to_string(), } } diff --git a/crates/sqlite-store/src/db_management/utils.rs b/crates/sqlite-store/src/db_management/utils.rs index f6642db3dc..2d81069a4c 100644 --- a/crates/sqlite-store/src/db_management/utils.rs +++ b/crates/sqlite-store/src/db_management/utils.rs @@ -61,23 +61,40 @@ type Hash = Blake3Digest<32>; const SCHEMA_HASH_DOMAIN: &[u8] = b"miden-client-sqlite-schema-v1"; -const MIGRATION_SCRIPTS: [&str; 1] = [include_str!("../store.sql")]; +/// The migrations that build the store schema, in the order they are applied. +const MIGRATION_SCRIPTS: [&str; 1] = [include_str!("../migrations/0001_init.sql")]; + +/// The schema fingerprint each migration in [`MIGRATION_SCRIPTS`] produced when it was released. +const PINNED_SCHEMA_HASHES: [&str; MIGRATION_SCRIPTS.len()] = + ["0x6300110a9f3efa3476fac4e736f94c33e07935ab7eedf357b38a50f55cabf140"]; + static MIGRATIONS: LazyLock = LazyLock::new(prepare_migrations); -static EXPECTED_SCHEMA_HASHES: LazyLock> = LazyLock::new(compute_expected_schema_hashes); fn up(s: &'static str) -> M<'static> { M::up(s).foreign_key_check() } -/// Applies the migrations to the database. +/// Brings the database up to the latest schema version, creating it if it is empty. pub fn apply_migrations(conn: &mut Connection) -> Result<(), SqliteStoreError> { - let version_before = MIGRATIONS.current_version(conn)?; - - if let SchemaVersion::Inside(ver) = version_before { - let actual_hash = schema_hash(conn)?; - if actual_hash != EXPECTED_SCHEMA_HASHES[ver.get() - 1] { - return Err(SqliteStoreError::SchemaHashMismatch); - } + match MIGRATIONS.current_version(conn)? { + SchemaVersion::NoneSet => {}, + SchemaVersion::Inside(ver) => { + let expected = PINNED_SCHEMA_HASHES[ver.get() - 1]; + let actual = String::from(schema_hash(conn)?); + if actual != expected { + return Err(SqliteStoreError::SchemaDrift { + version: schema_version(ver.get()), + expected: expected.to_string(), + actual, + }); + } + }, + SchemaVersion::Outside(ver) => { + return Err(SqliteStoreError::SchemaTooNew { + found: schema_version(ver.get()), + supported: schema_version(MIGRATION_SCRIPTS.len()), + }); + }, } MIGRATIONS.to_latest(conn)?; @@ -85,12 +102,21 @@ pub fn apply_migrations(conn: &mut Connection) -> Result<(), SqliteStoreError> { Ok(()) } +/// Narrows a migration index to the width schema versions are reported in. +/// +/// `SQLite` stores the version in `PRAGMA user_version`, which is an `i32`, so a version that does +/// not fit is unreachable. +fn schema_version(version: usize) -> u32 { + u32::try_from(version).expect("schema version should fit in a u32") +} + fn prepare_migrations() -> Migrations<'static> { Migrations::new(MIGRATION_SCRIPTS.map(up).to_vec()) } -/// Computes the schema fingerprint expected after each migration by replaying the migrations on an +/// Computes the schema fingerprint each migration produces by replaying the migrations on an /// in-memory database. +#[cfg(test)] fn compute_expected_schema_hashes() -> Vec { let mut conn = Connection::open_in_memory().expect("in-memory database creation should not fail"); @@ -144,14 +170,83 @@ fn push_field(buf: &mut Vec, field: &[u8]) { buf.extend_from_slice(field); } -/// Collapses runs of whitespace to single spaces and trims a trailing semicolon so cosmetic -/// differences in stored SQL text do not change the fingerprint. +/// Rewrites the SQL text stored for a schema object into a form that ignores differences `SQLite` +/// itself ignores, so cosmetic edits do not change the fingerprint. fn normalize_sql(sql: &str) -> String { - sql.trim_end() - .trim_end_matches(';') - .split_whitespace() - .collect::>() - .join(" ") + let mut out = String::with_capacity(sql.len()); + let mut chars = sql.chars().peekable(); + + while let Some(ch) = chars.next() { + match ch { + // A doubled quote inside a quoted region escapes itself and does not close it. + '\'' | '"' | '`' => { + out.push(ch); + while let Some(inner) = chars.next() { + out.push(inner); + if inner == ch { + if chars.peek() == Some(&ch) { + out.push(ch); + chars.next(); + } else { + break; + } + } + } + }, + // Bracketed identifiers do not nest and have no escape sequence. + '[' => { + out.push(ch); + for inner in chars.by_ref() { + out.push(inner); + if inner == ']' { + break; + } + } + }, + // A comment collapses to a separator rather than to nothing, because `SQLite` does not + // require whitespace before `--` and fusing the tokens on either side of it would + // change what the text means. + '-' if chars.peek() == Some(&'-') => { + chars.next(); + while chars.peek().is_some_and(|&inner| inner != '\n') { + chars.next(); + } + push_separator(&mut out); + }, + '/' if chars.peek() == Some(&'*') => { + chars.next(); + let mut prev = '\0'; + for inner in chars.by_ref() { + if prev == '*' && inner == '/' { + break; + } + prev = inner; + } + push_separator(&mut out); + }, + _ if is_sql_whitespace(ch) => push_separator(&mut out), + _ => out.push(ch), + } + } + + let normalized = out.trim_end().trim_end_matches(';').trim(); + normalized.to_string() +} + +/// Appends a single space unless one is already there, so adjacent separators do not stack up. +fn push_separator(out: &mut String) { + if !out.is_empty() && !out.ends_with(' ') { + out.push(' '); + } +} + +/// Returns whether `ch` separates tokens for `SQLite`. +/// +/// This is deliberately narrower than [`char::is_whitespace`]. `SQLite` treats every byte above +/// the ASCII range as part of an identifier, so a Unicode space between two tokens makes them one +/// token and must not be normalized away. +fn is_sql_whitespace(ch: char) -> bool { + matches!(ch, ' ' | '\t' | '\n' | '\r' | '\u{0c}') } pub fn get_setting(conn: &mut Connection, name: &str) -> Result, StoreError> { @@ -196,9 +291,21 @@ pub fn list_setting_keys(conn: &Connection) -> Result, StoreError> { mod tests { use rusqlite::Connection; - use super::{EXPECTED_SCHEMA_HASHES, MIGRATION_SCRIPTS, apply_migrations, schema_hash}; + use super::{ + MIGRATION_SCRIPTS, + PINNED_SCHEMA_HASHES, + apply_migrations, + compute_expected_schema_hashes, + schema_hash, + }; use crate::db_management::errors::SqliteStoreError; + fn hash_of(schema: &str) -> super::Hash { + let conn = Connection::open_in_memory().unwrap(); + conn.execute_batch(schema).unwrap(); + schema_hash(&conn).unwrap() + } + #[test] fn honest_database_reopens_without_error() { let mut conn = Connection::open_in_memory().unwrap(); @@ -217,7 +324,28 @@ mod tests { conn.execute("ALTER TABLE input_notes ADD COLUMN injected TEXT", []).unwrap(); let err = apply_migrations(&mut conn).unwrap_err(); - assert!(matches!(err, SqliteStoreError::SchemaHashMismatch)); + let SqliteStoreError::SchemaDrift { version, expected, actual } = err else { + panic!("drifted schema should be reported as drift, got {err:?}"); + }; + assert_eq!(version, 1); + assert_ne!(expected, actual); + } + + #[test] + fn database_from_a_newer_client_is_rejected() { + let mut conn = Connection::open_in_memory().unwrap(); + apply_migrations(&mut conn).unwrap(); + + // A version this client has no migration for, as written by a later release. + let ahead = MIGRATION_SCRIPTS.len() + 3; + conn.pragma_update(None, "user_version", ahead).unwrap(); + + let err = apply_migrations(&mut conn).unwrap_err(); + let SqliteStoreError::SchemaTooNew { found, supported } = err else { + panic!("a database from a newer client should be reported as too new, got {err:?}"); + }; + assert_eq!(found as usize, ahead); + assert_eq!(supported as usize, MIGRATION_SCRIPTS.len()); } #[test] @@ -241,7 +369,37 @@ mod tests { } #[test] - fn expected_schema_hash_per_migration() { - assert_eq!(EXPECTED_SCHEMA_HASHES.len(), MIGRATION_SCRIPTS.len()); + fn migration_schema_hashes_are_stable() { + let replayed = compute_expected_schema_hashes() + .into_iter() + .map(String::from) + .collect::>(); + let pinned = PINNED_SCHEMA_HASHES.map(str::to_string).to_vec(); + + assert_eq!( + replayed, pinned, + "a released migration builds a different schema than it did when it was pinned. \ + Append a new migration instead of editing an existing one. If this is a new \ + migration, append its hash rather than rewriting the entries before it." + ); + } + + #[test] + fn schema_hash_ignores_comment_edits() { + let documented = hash_of( + "CREATE TABLE items ( + id INTEGER PRIMARY KEY, -- the identifier + /* the payload */ + value TEXT + );", + ); + let reworded = hash_of( + "CREATE TABLE items ( + id INTEGER PRIMARY KEY, -- a completely different explanation + value TEXT + );", + ); + + assert_eq!(documented, reworded); } } diff --git a/crates/sqlite-store/src/lib.rs b/crates/sqlite-store/src/lib.rs index 2d6d22ad24..0fa3344534 100644 --- a/crates/sqlite-store/src/lib.rs +++ b/crates/sqlite-store/src/lib.rs @@ -76,7 +76,8 @@ pub use builder::ClientBuilderSqliteExt; /// Represents a pool of connections with an `SQLite` database. The pool is used to interact /// concurrently with the underlying database in a safe and efficient manner. /// -/// Current table definitions can be found at `store.sql` migration file. +/// Current table definitions are the result of applying every migration under `migrations/` in +/// order. pub struct SqliteStore { pub(crate) pool: Pool, database_filepath: String, diff --git a/crates/sqlite-store/src/store.sql b/crates/sqlite-store/src/migrations/0001_init.sql similarity index 100% rename from crates/sqlite-store/src/store.sql rename to crates/sqlite-store/src/migrations/0001_init.sql diff --git a/scripts/check-migrations.sh b/scripts/check-migrations.sh new file mode 100755 index 0000000000..866a8df2ec --- /dev/null +++ b/scripts/check-migrations.sh @@ -0,0 +1,31 @@ +#!/bin/bash +# Fails closed: any error resolving the base branch or running git aborts the script rather than +# reporting success. This is the only automated guard against an edited migration, so a silent +# pass would be worse than a false alarm. +set -euo pipefail + +MIGRATIONS_DIR="${1:-crates/sqlite-store/src/migrations}" +BASE="origin/${BASE_REF:?must be set to the base branch of the pull request}" + +if ! git rev-parse --verify --quiet "${BASE}^{commit}" > /dev/null; then + >&2 echo "Cannot resolve \"${BASE}\". Fetch the base branch before running this check." + exit 1 +fi + +# Compared against the merge base rather than the tip of the base branch, so a migration added on +# the base branch after this one forked is not attributed to this pull request. +CHANGED=$(git diff --name-only --diff-filter=MDR --merge-base "${BASE}" -- "${MIGRATIONS_DIR}") + +if [ -z "${CHANGED}" ]; then + echo "No released migration was modified." + exit 0 +fi + +>&2 echo "The following released migrations were modified, renamed or deleted:" +>&2 echo "${CHANGED}" +>&2 echo "" +>&2 echo "Migrations are append-only. Add a new file under \"${MIGRATIONS_DIR}\" with the next +version prefix instead, register it in MIGRATION_SCRIPTS and append its schema hash to +PINNED_SCHEMA_HASHES, both in crates/sqlite-store/src/db_management/utils.rs, rather than editing +the existing entries." +exit 1 From 0e10af00ba7c1d4a10487924361a4d853746a190 Mon Sep 17 00:00:00 2001 From: Juan Munoz Date: Thu, 6 Aug 2026 17:36:12 -0300 Subject: [PATCH 2/3] chore: address comments --- CHANGELOG.md | 2 +- Cargo.lock | 1 + crates/sqlite-store/Cargo.toml | 1 + crates/sqlite-store/README.md | 23 +- .../sqlite-store/src/db_management/backup.rs | 177 +++++++++++++ .../sqlite-store/src/db_management/errors.rs | 53 ++-- crates/sqlite-store/src/db_management/mod.rs | 1 + .../sqlite-store/src/db_management/utils.rs | 233 +++++++++--------- crates/sqlite-store/src/lib.rs | 94 ++++++- 9 files changed, 434 insertions(+), 151 deletions(-) create mode 100644 crates/sqlite-store/src/db_management/backup.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f547a6da8..bd47816b35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ * [FEATURE][rust,store] Added `NoteFilter::ScriptRoots` to query input notes by their note script root directly at the store level, without loading and screening unrelated notes. The filter doesn't apply to output notes: querying output notes with it returns an empty list ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)). * [rust] Added `PartialBlockchainUpdates::block_headers_to_store`, which narrows the staged headers to the ones a sync must persist: those marked as relevant, genesis, and the block at the sync height. `block_headers` still yields all staged headers ([#2297](https://github.com/0xMiden/rust-sdk/pull/2297)). * [rust] State sync now authenticates every relevant note block but only persists block headers and MMR authentication nodes for blocks containing notes that remain unspent or that a `NoteObserver` explicitly marks as relevant ([#2297](https://github.com/0xMiden/rust-sdk/pull/2297)). -* [store] The SQLite store's schema is now built from append-only migrations under `crates/sqlite-store/src/migrations/`, starting with the frozen `0001_init.sql`. An existing store is checked against the fingerprints pinned in `PINNED_SCHEMA_HASHES` rather than against a replay of the current migration files, so editing a released migration fails a test instead of silently redefining the schema those stores were built with. A CI job rejects any pull request that modifies an existing migration file ([#2346](https://github.com/0xMiden/rust-sdk/issues/2346)). +* [store] The SQLite store's schema is now built from append-only migrations under `crates/sqlite-store/src/migrations/`, starting with the frozen `0001_init.sql`. Opening a store verifies its schema against a fingerprint derived by replaying the migrations, both before migrating and after. A store that is behind is copied before it is migrated and put back from that copy if the migration fails, so a failed upgrade leaves it as it was. A pinned snapshot of the fingerprints and a CI job together reject any pull request that modifies an existing migration file ([#2346](https://github.com/0xMiden/rust-sdk/issues/2346)). ## 0.16.0-alpha.1 (2026-07-17) diff --git a/Cargo.lock b/Cargo.lock index 2100c450a0..274f5be37e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2121,6 +2121,7 @@ dependencies = [ "miden-standards", "rusqlite", "rusqlite_migration", + "tempfile", "thiserror", "tokio", ] diff --git a/crates/sqlite-store/Cargo.toml b/crates/sqlite-store/Cargo.toml index 38e4834df1..402946d11b 100644 --- a/crates/sqlite-store/Cargo.toml +++ b/crates/sqlite-store/Cargo.toml @@ -37,3 +37,4 @@ workspace = true # Enable client testing utilities only for tests miden-client = { features = ["testing"], workspace = true } miden-standards = { features = ["testing"], workspace = true } +tempfile = { workspace = true } diff --git a/crates/sqlite-store/README.md b/crates/sqlite-store/README.md index 1cc56393aa..9063eb0608 100644 --- a/crates/sqlite-store/README.md +++ b/crates/sqlite-store/README.md @@ -24,11 +24,11 @@ The schema is built by replaying the migrations listed in `MIGRATION_SCRIPTS` four-digit prefix is its schema version, which is the value SQLite records in `PRAGMA user_version`. Migrations are **append-only**. Every store on a user's disk was built by replaying these exact -files, and the client verifies on open that the schema it finds matches `PINNED_SCHEMA_HASHES` for -the version the database claims. That constant, not a replay of the current migration files, is the -definition of what each version's schema is, so editing a released migration is caught rather than -silently redefining the schema those databases were supposed to have. Unlike chain state, a store -holds private notes and account seeds that cannot be recovered from the network. +files. On open the client replays the migrations against an in-memory database to derive the +fingerprint each version should have, and verifies that the schema it finds on disk matches the one +for the version the database claims. A store that was altered outside the migrations is rejected +rather than migrated further. Unlike chain state, a store holds private notes and account seeds +that cannot be recovered from the network. Upgrades are forward-only. There are no down migrations. @@ -39,7 +39,7 @@ Upgrades are forward-only. There are no down migrations. 2. Append `include_str!("../migrations/000N_short_name.sql")` to `MIGRATION_SCRIPTS` in `src/db_management/utils.rs`. Nothing scans the directory, so a file that is not listed here is never applied. -3. Append one entry to `PINNED_SCHEMA_HASHES` in the same file. Run +3. Append one entry to `PINNED_SCHEMA_HASHES` in that file's test module. Run `cargo test -p miden-client-sqlite-store --lib migration_schema_hashes_are_stable` and take the new hash from the failure output. Leave the existing entries alone. If they changed, the migration edited the schema an older version built. @@ -48,5 +48,16 @@ Upgrades are forward-only. There are no down migrations. `scripts/check-migrations.sh` runs in CI and fails a pull request that modifies, renames or deletes a file that already exists on the base branch. +### Migrations that transform data + +Some upgrades cannot be expressed in SQL. The store holds serialized protocol objects as blobs, so +a change to how an account, note or transaction is encoded has to be applied by decoding each row +with the old type and re-encoding it with the new one. SQLite has no way to do that. + +`rusqlite_migration` covers this with `up_with_hook`, where the hook is a Rust +closure taking the migration's `&Transaction`. Per migration the library runs the SQL, then the +foreign key check, then the hook, and all of it is inside the transaction the whole upgrade commits +at the end, so a hook that returns an error rolls back the migration exactly like failing SQL does. + ## License This project is licensed under the MIT License. See the [LICENSE](../../LICENSE) file for details. diff --git a/crates/sqlite-store/src/db_management/backup.rs b/crates/sqlite-store/src/db_management/backup.rs new file mode 100644 index 0000000000..fd0f67dd24 --- /dev/null +++ b/crates/sqlite-store/src/db_management/backup.rs @@ -0,0 +1,177 @@ +use std::ffi::OsString; +use std::path::{Path, PathBuf}; + +use rusqlite::{Connection, params}; + +use super::errors::SqliteStoreError; + +// PRE-MIGRATION BACKUP +// ================================================================================================ + +/// Suffix appended to the store's filename to name its pre-migration backup. +const BACKUP_SUFFIX: &str = ".pre-migration-backup"; + +/// Files `SQLite` keeps next to the database, which describe the database they were written for and +/// must not outlive it. +const SIDECAR_SUFFIXES: [&str; 3] = ["-journal", "-wal", "-shm"]; + +/// Returns the path of the backup taken before migrating the store at `database_filepath`. +pub fn backup_path(database_filepath: &Path) -> PathBuf { + let mut path = OsString::from(database_filepath); + path.push(BACKUP_SUFFIX); + PathBuf::from(path) +} + +/// Copies the database into `backup_filepath`, replacing a backup left behind by an earlier run. +/// +/// `VACUUM INTO` writes a consistent snapshot even while the connection is open, so this does not +/// depend on the caller quiescing the store. +pub fn create_backup(conn: &Connection, backup_filepath: &Path) -> Result<(), SqliteStoreError> { + // A backup that is still here was left by a run that died mid-migration. Its database was + // already restored or abandoned, and `VACUUM INTO` refuses to write to a file that exists. + discard_backup(backup_filepath)?; + + conn.execute("VACUUM INTO ?1", params![path_argument(backup_filepath)?])?; + + Ok(()) +} + +/// Puts the backup back in place of the database, consuming the backup. +/// +/// The caller must have closed every connection to the database first. Restoring under an open +/// connection would leave that connection reading a file that no longer exists, and on Windows the +/// replacement cannot happen at all. +pub fn restore_backup( + database_filepath: &Path, + backup_filepath: &Path, +) -> Result<(), SqliteStoreError> { + // These describe the database being replaced, not the backup, so leaving one behind would let + // `SQLite` apply it to the restored file. + for suffix in SIDECAR_SUFFIXES { + discard_backup(&sidecar_path(database_filepath, suffix))?; + } + + std::fs::rename(backup_filepath, database_filepath).map_err(|err| { + SqliteStoreError::BackupRestoreFailed { + backup: backup_filepath.display().to_string(), + reason: err.to_string(), + } + }) +} + +/// Removes `backup_filepath` if it exists. +pub fn discard_backup(backup_filepath: &Path) -> Result<(), SqliteStoreError> { + match std::fs::remove_file(backup_filepath) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(SqliteStoreError::BackupFailed { + backup: backup_filepath.display().to_string(), + reason: err.to_string(), + }), + } +} + +/// Renders a path for `SQLite`, which takes filenames as text. +fn path_argument(path: &Path) -> Result<&str, SqliteStoreError> { + path.to_str().ok_or_else(|| SqliteStoreError::BackupFailed { + backup: path.display().to_string(), + reason: String::from("backup path is not valid UTF-8"), + }) +} + +/// Returns the path of the `SQLite` sidecar file for `database_filepath` with the given suffix. +fn sidecar_path(database_filepath: &Path, suffix: &str) -> PathBuf { + let mut path = OsString::from(database_filepath); + path.push(suffix); + PathBuf::from(path) +} + +// TESTS +// ================================================================================================ + +#[cfg(test)] +mod tests { + use rusqlite::Connection; + + use super::{backup_path, create_backup, restore_backup, sidecar_path}; + + fn table_names(conn: &Connection) -> Vec { + let mut stmt = conn + .prepare("SELECT name FROM sqlite_schema WHERE type = 'table' ORDER BY name") + .unwrap(); + stmt.query_map([], |row| row.get::<_, String>(0)) + .unwrap() + .collect::, _>>() + .unwrap() + } + + #[test] + fn backup_restores_the_database_as_it_was() { + let dir = tempfile::tempdir().unwrap(); + let database = dir.path().join("store.sqlite3"); + let backup = backup_path(&database); + + let conn = Connection::open(&database).unwrap(); + conn.execute_batch( + "CREATE TABLE items (id INTEGER PRIMARY KEY, value TEXT); + INSERT INTO items (id, value) VALUES (1, 'before');", + ) + .unwrap(); + + create_backup(&conn, &backup).unwrap(); + assert!(backup.exists()); + + // The change the restore is meant to undo. + conn.execute_batch( + "DROP TABLE items; + CREATE TABLE migrated (id INTEGER PRIMARY KEY);", + ) + .unwrap(); + drop(conn); + + restore_backup(&database, &backup).unwrap(); + assert!(!backup.exists(), "a consumed backup should not be left behind"); + + let conn = Connection::open(&database).unwrap(); + assert_eq!(table_names(&conn), vec![String::from("items")]); + let value: String = conn + .query_row("SELECT value FROM items WHERE id = 1", [], |row| row.get(0)) + .unwrap(); + assert_eq!(value, "before"); + } + + #[test] + fn backup_replaces_one_left_by_an_earlier_run() { + let dir = tempfile::tempdir().unwrap(); + let database = dir.path().join("store.sqlite3"); + let backup = backup_path(&database); + std::fs::write(&backup, b"not a database").unwrap(); + + let conn = Connection::open(&database).unwrap(); + conn.execute_batch("CREATE TABLE items (id INTEGER PRIMARY KEY);").unwrap(); + + create_backup(&conn, &backup).unwrap(); + + let restored = Connection::open(&backup).unwrap(); + assert_eq!(table_names(&restored), vec![String::from("items")]); + } + + #[test] + fn restore_removes_sidecars_of_the_replaced_database() { + let dir = tempfile::tempdir().unwrap(); + let database = dir.path().join("store.sqlite3"); + let backup = backup_path(&database); + + let conn = Connection::open(&database).unwrap(); + conn.execute_batch("CREATE TABLE items (id INTEGER PRIMARY KEY);").unwrap(); + create_backup(&conn, &backup).unwrap(); + drop(conn); + + let journal = sidecar_path(&database, "-journal"); + std::fs::write(&journal, b"stale").unwrap(); + + restore_backup(&database, &backup).unwrap(); + + assert!(!journal.exists(), "a sidecar of the replaced database must not survive it"); + } +} diff --git a/crates/sqlite-store/src/db_management/errors.rs b/crates/sqlite-store/src/db_management/errors.rs index 3282694c97..3dac4e4311 100644 --- a/crates/sqlite-store/src/db_management/errors.rs +++ b/crates/sqlite-store/src/db_management/errors.rs @@ -26,6 +26,24 @@ pub enum SqliteStoreError { "store is at schema version {found}, which is newer than the highest version this client supports ({supported})" )] SchemaTooNew { found: u32, supported: u32 }, + #[error( + "migrating to schema version {version} produced a schema this client does not expect (expected {expected}, found {actual})" + )] + MigratedSchemaMismatch { + version: u32, + expected: String, + actual: String, + }, + #[error( + "the database is not empty and does not record a schema version, so it was not created by this client and will not be migrated into a store" + )] + NotAClientStore, + #[error("failed to back up the store to {backup} before migrating it: {reason}")] + BackupFailed { backup: String, reason: String }, + #[error( + "migrating the store failed and it could not be restored from its backup at {backup}: {reason}. The backup holds the store as it was before migrating" + )] + BackupRestoreFailed { backup: String, reason: String }, } impl From for SqliteStoreError { @@ -35,26 +53,23 @@ impl From for SqliteStoreError { } impl From for SqliteStoreError { + /// Renders a migration failure without reproducing the migration script. fn from(err: MigrationError) -> Self { - SqliteStoreError::Migration(describe_migration_error(&err)) - } -} - -/// Renders a migration failure without reproducing the migration script. -pub fn describe_migration_error(err: &MigrationError) -> String { - match err { - MigrationError::RusqliteError { err, .. } => describe_sqlite_error(err), - MigrationError::ForeignKeyCheck(violations) => { - format!("{} foreign key violation(s) after applying the migration", violations.len()) - }, - other => other.to_string(), - } -} + let message = match &err { + MigrationError::RusqliteError { + err: RusqliteError::SqlInputError { msg, .. }, + .. + } => msg.clone(), + MigrationError::RusqliteError { err, .. } => err.to_string(), + MigrationError::ForeignKeyCheck(violations) => { + format!( + "{} foreign key violation(s) after applying the migration", + violations.len() + ) + }, + other => other.to_string(), + }; -/// Renders a `SQLite` failure without reproducing the statement that caused it. -fn describe_sqlite_error(err: &RusqliteError) -> String { - match err { - RusqliteError::SqlInputError { msg, .. } => msg.clone(), - other => other.to_string(), + SqliteStoreError::Migration(message) } } diff --git a/crates/sqlite-store/src/db_management/mod.rs b/crates/sqlite-store/src/db_management/mod.rs index 886e502007..4cb1b3747a 100644 --- a/crates/sqlite-store/src/db_management/mod.rs +++ b/crates/sqlite-store/src/db_management/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod backup; pub(crate) mod errors; pub(crate) mod pool_manager; pub(crate) mod utils; diff --git a/crates/sqlite-store/src/db_management/utils.rs b/crates/sqlite-store/src/db_management/utils.rs index 2d81069a4c..d50f53e467 100644 --- a/crates/sqlite-store/src/db_management/utils.rs +++ b/crates/sqlite-store/src/db_management/utils.rs @@ -64,28 +64,44 @@ const SCHEMA_HASH_DOMAIN: &[u8] = b"miden-client-sqlite-schema-v1"; /// The migrations that build the store schema, in the order they are applied. const MIGRATION_SCRIPTS: [&str; 1] = [include_str!("../migrations/0001_init.sql")]; -/// The schema fingerprint each migration in [`MIGRATION_SCRIPTS`] produced when it was released. -const PINNED_SCHEMA_HASHES: [&str; MIGRATION_SCRIPTS.len()] = - ["0x6300110a9f3efa3476fac4e736f94c33e07935ab7eedf357b38a50f55cabf140"]; - static MIGRATIONS: LazyLock = LazyLock::new(prepare_migrations); +/// The schema fingerprint each migration in [`MIGRATION_SCRIPTS`] produces, obtained by replaying +/// the migrations rather than by trusting a recorded value. +static EXPECTED_SCHEMA_HASHES: LazyLock> = LazyLock::new(compute_expected_schema_hashes); + fn up(s: &'static str) -> M<'static> { M::up(s).foreign_key_check() } +/// Returns whether the database holds a schema that is behind the latest version. +/// +/// A database with no schema at all is not behind: there is nothing in it to preserve, so opening +/// it builds the latest schema directly. +pub fn has_pending_migrations(conn: &Connection) -> Result { + match MIGRATIONS.current_version(conn)? { + SchemaVersion::Inside(ver) => Ok(ver.get() < MIGRATION_SCRIPTS.len()), + // A version beyond the last migration is rejected when migrating, not backed up. + SchemaVersion::NoneSet | SchemaVersion::Outside(_) => Ok(false), + } +} + /// Brings the database up to the latest schema version, creating it if it is empty. pub fn apply_migrations(conn: &mut Connection) -> Result<(), SqliteStoreError> { match MIGRATIONS.current_version(conn)? { - SchemaVersion::NoneSet => {}, + SchemaVersion::NoneSet => { + if !is_empty_database(conn)? { + return Err(SqliteStoreError::NotAClientStore); + } + }, SchemaVersion::Inside(ver) => { - let expected = PINNED_SCHEMA_HASHES[ver.get() - 1]; - let actual = String::from(schema_hash(conn)?); + let expected = EXPECTED_SCHEMA_HASHES[ver.get() - 1]; + let actual = schema_hash(conn)?; if actual != expected { return Err(SqliteStoreError::SchemaDrift { version: schema_version(ver.get()), - expected: expected.to_string(), - actual, + expected: String::from(expected), + actual: String::from(actual), }); } }, @@ -99,6 +115,33 @@ pub fn apply_migrations(conn: &mut Connection) -> Result<(), SqliteStoreError> { MIGRATIONS.to_latest(conn)?; + verify_migrated_schema(conn, MIGRATION_SCRIPTS.len()) +} + +/// Returns whether the database holds no objects of its own. +fn is_empty_database(conn: &Connection) -> Result { + let objects: u32 = conn.query_row( + "SELECT COUNT(*) FROM sqlite_schema WHERE name NOT GLOB 'sqlite_*'", + [], + |row| row.get(0), + )?; + + Ok(objects == 0) +} + +/// Checks that migrating to `version` built the schema that version is defined to build. +fn verify_migrated_schema(conn: &Connection, version: usize) -> Result<(), SqliteStoreError> { + let expected = EXPECTED_SCHEMA_HASHES[version - 1]; + let actual = schema_hash(conn)?; + + if actual != expected { + return Err(SqliteStoreError::MigratedSchemaMismatch { + version: schema_version(version), + expected: String::from(expected), + actual: String::from(actual), + }); + } + Ok(()) } @@ -116,7 +159,6 @@ fn prepare_migrations() -> Migrations<'static> { /// Computes the schema fingerprint each migration produces by replaying the migrations on an /// in-memory database. -#[cfg(test)] fn compute_expected_schema_hashes() -> Vec { let mut conn = Connection::open_in_memory().expect("in-memory database creation should not fail"); @@ -170,83 +212,14 @@ fn push_field(buf: &mut Vec, field: &[u8]) { buf.extend_from_slice(field); } -/// Rewrites the SQL text stored for a schema object into a form that ignores differences `SQLite` -/// itself ignores, so cosmetic edits do not change the fingerprint. +/// Collapses runs of whitespace to single spaces and trims a trailing semicolon so cosmetic +/// differences in stored SQL text do not change the fingerprint. fn normalize_sql(sql: &str) -> String { - let mut out = String::with_capacity(sql.len()); - let mut chars = sql.chars().peekable(); - - while let Some(ch) = chars.next() { - match ch { - // A doubled quote inside a quoted region escapes itself and does not close it. - '\'' | '"' | '`' => { - out.push(ch); - while let Some(inner) = chars.next() { - out.push(inner); - if inner == ch { - if chars.peek() == Some(&ch) { - out.push(ch); - chars.next(); - } else { - break; - } - } - } - }, - // Bracketed identifiers do not nest and have no escape sequence. - '[' => { - out.push(ch); - for inner in chars.by_ref() { - out.push(inner); - if inner == ']' { - break; - } - } - }, - // A comment collapses to a separator rather than to nothing, because `SQLite` does not - // require whitespace before `--` and fusing the tokens on either side of it would - // change what the text means. - '-' if chars.peek() == Some(&'-') => { - chars.next(); - while chars.peek().is_some_and(|&inner| inner != '\n') { - chars.next(); - } - push_separator(&mut out); - }, - '/' if chars.peek() == Some(&'*') => { - chars.next(); - let mut prev = '\0'; - for inner in chars.by_ref() { - if prev == '*' && inner == '/' { - break; - } - prev = inner; - } - push_separator(&mut out); - }, - _ if is_sql_whitespace(ch) => push_separator(&mut out), - _ => out.push(ch), - } - } - - let normalized = out.trim_end().trim_end_matches(';').trim(); - normalized.to_string() -} - -/// Appends a single space unless one is already there, so adjacent separators do not stack up. -fn push_separator(out: &mut String) { - if !out.is_empty() && !out.ends_with(' ') { - out.push(' '); - } -} - -/// Returns whether `ch` separates tokens for `SQLite`. -/// -/// This is deliberately narrower than [`char::is_whitespace`]. `SQLite` treats every byte above -/// the ASCII range as part of an identifier, so a Unicode space between two tokens makes them one -/// token and must not be normalized away. -fn is_sql_whitespace(ch: char) -> bool { - matches!(ch, ' ' | '\t' | '\n' | '\r' | '\u{0c}') + sql.trim_end() + .trim_end_matches(';') + .split_whitespace() + .collect::>() + .join(" ") } pub fn get_setting(conn: &mut Connection, name: &str) -> Result, StoreError> { @@ -292,19 +265,16 @@ mod tests { use rusqlite::Connection; use super::{ + EXPECTED_SCHEMA_HASHES, MIGRATION_SCRIPTS, - PINNED_SCHEMA_HASHES, apply_migrations, - compute_expected_schema_hashes, schema_hash, + verify_migrated_schema, }; use crate::db_management::errors::SqliteStoreError; - fn hash_of(schema: &str) -> super::Hash { - let conn = Connection::open_in_memory().unwrap(); - conn.execute_batch(schema).unwrap(); - schema_hash(&conn).unwrap() - } + const PINNED_SCHEMA_HASHES: [&str; MIGRATION_SCRIPTS.len()] = + ["0x749fba4988cae911b43dd2a3efef634ce5f514515ae26687f791fb17612c5b7a"]; #[test] fn honest_database_reopens_without_error() { @@ -315,6 +285,41 @@ mod tests { apply_migrations(&mut conn).unwrap(); } + #[test] + fn fresh_database_is_built_to_the_latest_version() { + let mut conn = Connection::open_in_memory().unwrap(); + apply_migrations(&mut conn).unwrap(); + + let version: usize = conn.query_row("PRAGMA user_version", [], |row| row.get(0)).unwrap(); + assert_eq!(version, MIGRATION_SCRIPTS.len()); + assert_eq!(schema_hash(&conn).unwrap(), EXPECTED_SCHEMA_HASHES[version - 1]); + } + + #[test] + fn unversioned_database_with_contents_is_rejected() { + let mut conn = Connection::open_in_memory().unwrap(); + // A database that is not a store, named by mistake. It records no version, which is what + // an empty file also looks like. + conn.execute_batch("CREATE TABLE somebody_elses (id INTEGER PRIMARY KEY);") + .unwrap(); + + let err = apply_migrations(&mut conn).unwrap_err(); + assert!( + matches!(err, SqliteStoreError::NotAClientStore), + "a foreign database should not be migrated into a store, got {err:?}" + ); + + // Refusing must leave the database alone. + let version: usize = conn.query_row("PRAGMA user_version", [], |row| row.get(0)).unwrap(); + assert_eq!(version, 0); + let tables: u32 = conn + .query_row("SELECT COUNT(*) FROM sqlite_schema WHERE name = 'input_notes'", [], |row| { + row.get(0) + }) + .unwrap(); + assert_eq!(tables, 0); + } + #[test] fn schema_drift_is_rejected() { let mut conn = Connection::open_in_memory().unwrap(); @@ -368,12 +373,27 @@ mod tests { assert_eq!(schema_hash(&left).unwrap(), schema_hash(&right).unwrap()); } + #[test] + fn migrated_schema_is_verified() { + let mut conn = Connection::open_in_memory().unwrap(); + apply_migrations(&mut conn).unwrap(); + + // Migrating cannot be made to build the wrong schema without a broken migration, so the + // schema is changed under the check instead, which is what such a migration would leave + // behind. + conn.execute("DROP TABLE input_notes", []).unwrap(); + + let err = verify_migrated_schema(&conn, MIGRATION_SCRIPTS.len()).unwrap_err(); + let SqliteStoreError::MigratedSchemaMismatch { version, expected, actual } = err else { + panic!("an unexpected migrated schema should be reported as a mismatch, got {err:?}"); + }; + assert_eq!(version as usize, MIGRATION_SCRIPTS.len()); + assert_ne!(expected, actual); + } + #[test] fn migration_schema_hashes_are_stable() { - let replayed = compute_expected_schema_hashes() - .into_iter() - .map(String::from) - .collect::>(); + let replayed = EXPECTED_SCHEMA_HASHES.iter().copied().map(String::from).collect::>(); let pinned = PINNED_SCHEMA_HASHES.map(str::to_string).to_vec(); assert_eq!( @@ -383,23 +403,4 @@ mod tests { migration, append its hash rather than rewriting the entries before it." ); } - - #[test] - fn schema_hash_ignores_comment_edits() { - let documented = hash_of( - "CREATE TABLE items ( - id INTEGER PRIMARY KEY, -- the identifier - /* the payload */ - value TEXT - );", - ); - let reworded = hash_of( - "CREATE TABLE items ( - id INTEGER PRIMARY KEY, -- a completely different explanation - value TEXT - );", - ); - - assert_eq!(documented, reworded); - } } diff --git a/crates/sqlite-store/src/lib.rs b/crates/sqlite-store/src/lib.rs index 0fa3344534..3df7f27db0 100644 --- a/crates/sqlite-store/src/lib.rs +++ b/crates/sqlite-store/src/lib.rs @@ -6,15 +6,18 @@ use std::boxed::Box; use std::collections::{BTreeMap, BTreeSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::string::{String, ToString}; use std::sync::{Arc, RwLock}; use std::vec::Vec; +use db_management::backup::{backup_path, create_backup, discard_backup, restore_backup}; +use db_management::errors::SqliteStoreError; use db_management::pool_manager::{Pool, SqlitePoolManager}; use db_management::utils::{ apply_migrations, get_setting, + has_pending_migrations, list_setting_keys, remove_setting, set_setting, @@ -91,17 +94,12 @@ impl SqliteStore { /// Returns a new instance of [Store] instantiated with the specified configuration options. pub async fn new(database_filepath: PathBuf) -> Result { let database_filepath_str = database_filepath.to_string_lossy().into_owned(); - let sqlite_pool_manager = SqlitePoolManager::new(database_filepath); + let sqlite_pool_manager = SqlitePoolManager::new(database_filepath.clone()); let pool = Pool::builder(sqlite_pool_manager) .build() .map_err(|e| StoreError::DatabaseError(e.to_string()))?; - let conn = pool.get().await.map_err(|e| StoreError::DatabaseError(e.to_string()))?; - - conn.interact(apply_migrations) - .await - .map_err(|e| StoreError::DatabaseError(e.to_string()))? - .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + Self::migrate(&pool, &database_filepath).await?; let store = SqliteStore { pool, @@ -126,6 +124,59 @@ impl SqliteStore { Ok(store) } + /// Brings the database at `database_filepath` up to the latest schema version. + async fn migrate(pool: &Pool, database_filepath: &Path) -> Result<(), StoreError> { + Self::migrate_with(pool, database_filepath, has_pending_migrations, apply_migrations).await + } + + /// [`Self::migrate`] with its two schema steps injected, so that tests can drive the paths a + /// single migration cannot reach on its own. + async fn migrate_with( + pool: &Pool, + database_filepath: &Path, + pending: fn(&Connection) -> Result, + apply: fn(&mut Connection) -> Result<(), SqliteStoreError>, + ) -> Result<(), StoreError> { + let conn = pool.get().await.map_err(|e| StoreError::DatabaseError(e.to_string()))?; + + let upgrading = conn + .interact(move |conn| pending(conn)) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))? + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + + let backup = backup_path(database_filepath); + if upgrading { + let backup = backup.clone(); + conn.interact(move |conn| create_backup(conn, &backup)) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))? + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + } + + let migrated = conn + .interact(apply) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + + // The database file cannot be replaced while anything is still reading it, so the + // connection goes back to the pool and the pool closes before the backup is put back. + drop(conn); + + let Err(migration_error) = migrated else { + discard_backup(&backup).map_err(|e| StoreError::DatabaseError(e.to_string()))?; + return Ok(()); + }; + + if upgrading { + pool.close(); + restore_backup(database_filepath, &backup) + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + } + + Err(StoreError::DatabaseError(migration_error.to_string())) + } + /// Interacts with the database by executing the provided function on a connection from the /// pool. /// @@ -617,7 +668,32 @@ pub mod tests { use miden_client::store::Store; use miden_client::testing::common::create_test_store_path; - use super::SqliteStore; + use super::db_management::pool_manager::SqlitePoolManager; + use super::{Pool, SqliteStore, SqliteStoreError, String, backup_path}; + + /// Stands in for a migration that damages the store before failing, which is what the backup + /// exists to undo. + fn failing_migration(conn: &mut rusqlite::Connection) -> Result<(), SqliteStoreError> { + conn.execute_batch("DROP TABLE input_notes;")?; + Err(SqliteStoreError::Migration(String::from("migration failed"))) + } + + #[tokio::test] + async fn failed_migration_leaves_the_store_as_it_was() { + let database_filepath = create_test_store_path(); + drop(SqliteStore::new(database_filepath.clone()).await.unwrap()); + + let pool = Pool::builder(SqlitePoolManager::new(database_filepath.clone())) + .build() + .unwrap(); + SqliteStore::migrate_with(&pool, &database_filepath, |_| Ok(true), failing_migration) + .await + .unwrap_err(); + + assert!(!backup_path(&database_filepath).exists(), "the backup should be consumed"); + // Reopening verifies the schema, so it only succeeds if the dropped table came back. + SqliteStore::new(database_filepath).await.unwrap(); + } fn assert_send_sync() {} From 4f4dd5dfd2410719fe95d79e17c6d465d9dad33e Mon Sep 17 00:00:00 2001 From: Juan Munoz Date: Fri, 7 Aug 2026 16:20:53 -0300 Subject: [PATCH 3/3] chore: address PR comments --- crates/sqlite-store/src/db_management/backup.rs | 4 ++++ scripts/check-migrations.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/sqlite-store/src/db_management/backup.rs b/crates/sqlite-store/src/db_management/backup.rs index fd0f67dd24..0e5223cad1 100644 --- a/crates/sqlite-store/src/db_management/backup.rs +++ b/crates/sqlite-store/src/db_management/backup.rs @@ -91,6 +91,10 @@ fn sidecar_path(database_filepath: &Path, suffix: &str) -> PathBuf { #[cfg(test)] mod tests { + // `tempfile` rather than `create_test_store_path`: `TempDir` removes the database and any + // backup left behind on drop, so repeated runs do not accumulate files in the system temp + // directory. + use rusqlite::Connection; use super::{backup_path, create_backup, restore_backup, sidecar_path}; diff --git a/scripts/check-migrations.sh b/scripts/check-migrations.sh index 866a8df2ec..4b360c278d 100755 --- a/scripts/check-migrations.sh +++ b/scripts/check-migrations.sh @@ -21,7 +21,7 @@ if [ -z "${CHANGED}" ]; then exit 0 fi ->&2 echo "The following released migrations were modified, renamed or deleted:" +>&2 echo "The following merged migrations were modified, renamed or deleted:" >&2 echo "${CHANGED}" >&2 echo "" >&2 echo "Migrations are append-only. Add a new file under \"${MIGRATIONS_DIR}\" with the next