diff --git a/crates/storage/src/migrations.rs b/crates/storage/src/migrations.rs index 85db42c..18a3ed9 100644 --- a/crates/storage/src/migrations.rs +++ b/crates/storage/src/migrations.rs @@ -261,6 +261,30 @@ fn split_statements(sql: &str) -> Vec { /// Ensure the schema_version table exists and run any pending migrations. pub async fn run_migrations(pool: &SqlitePool) -> Result<()> { + run_migrations_slice(pool, MIGRATIONS).await +} + +/// Apply the pending prefix of `migrations`, each inside its own +/// transaction along with the matching `schema_version` row insert. +/// +/// Atomicity was added in response to the 2026-04-22 review (RB-02): +/// the previous implementation executed statements individually against +/// the pool and only recorded the new version after all statements had +/// succeeded. A multi-statement migration that failed midway therefore +/// left the schema partially changed but still appearing unapplied — +/// the next startup would replay the migration against a mutated DB +/// and fail in surprising ways. +/// +/// Wrapping both the statements and the version record in a single +/// `Transaction` is sufficient for SQLite (DDL participates in +/// transactions there). If a future migration needs an operation that +/// implicitly commits (`VACUUM`, `REINDEX`, `ATTACH`), it must be split +/// out into its own non-transactional migration — reviewer's job to +/// flag. +async fn run_migrations_slice( + pool: &SqlitePool, + migrations: &[(i64, &str, &str)], +) -> Result<()> { sqlx::query( "CREATE TABLE IF NOT EXISTS schema_version ( version INTEGER PRIMARY KEY, @@ -277,27 +301,36 @@ pub async fn run_migrations(pool: &SqlitePool) -> Result<()> { .await .map_err(|e| KonError::StorageError(format!("Schema version query failed: {e}")))?; - for (version, description, sql) in MIGRATIONS { + for (version, description, sql) in migrations { if *version > current { log::info!("Running migration {}: {}", version, description); - let statements = split_statements(sql); + let mut tx = pool.begin().await.map_err(|e| { + KonError::StorageError(format!("Migration {} tx begin failed: {e}", version)) + })?; - for statement in &statements { - sqlx::query(statement).execute(pool).await.map_err(|e| { - KonError::StorageError(format!("Migration {} failed: {e}", version)) - })?; + for statement in split_statements(sql) { + sqlx::query(&statement) + .execute(&mut *tx) + .await + .map_err(|e| { + KonError::StorageError(format!("Migration {} failed: {e}", version)) + })?; } sqlx::query("INSERT INTO schema_version (version, description) VALUES (?, ?)") .bind(version) .bind(description) - .execute(pool) + .execute(&mut *tx) .await .map_err(|e| { KonError::StorageError(format!("Migration version record failed: {e}")) })?; + tx.commit().await.map_err(|e| { + KonError::StorageError(format!("Migration {} commit failed: {e}", version)) + })?; + log::info!("Migration {} complete", version); } } @@ -463,47 +496,15 @@ mod tests { } /// Test-only helper: run migrations only up to (and including) `target_version`. - /// Mirrors `run_migrations` but stops early — used by the v6 upgrade-path test - /// to seed a v5 schema with dictionary rows before applying v6. + /// Used by the v6 upgrade-path test to seed a v5 schema with + /// dictionary rows before applying v6. async fn run_migrations_up_to(pool: &SqlitePool, target_version: i64) -> Result<()> { - sqlx::query( - "CREATE TABLE IF NOT EXISTS schema_version ( - version INTEGER PRIMARY KEY, - description TEXT NOT NULL, - applied_at TEXT NOT NULL DEFAULT (datetime('now')) - )", - ) - .execute(pool) - .await - .map_err(|e| { - KonError::StorageError(format!("Schema version table creation failed: {e}")) - })?; - - let current: i64 = - sqlx::query_scalar("SELECT COALESCE(MAX(version), 0) FROM schema_version") - .fetch_one(pool) - .await - .map_err(|e| KonError::StorageError(format!("Schema version query failed: {e}")))?; - - for (version, description, sql) in MIGRATIONS { - if *version > current && *version <= target_version { - let statements = split_statements(sql); - for statement in &statements { - sqlx::query(statement).execute(pool).await.map_err(|e| { - KonError::StorageError(format!("Migration {} failed: {e}", version)) - })?; - } - sqlx::query("INSERT INTO schema_version (version, description) VALUES (?, ?)") - .bind(version) - .bind(description) - .execute(pool) - .await - .map_err(|e| { - KonError::StorageError(format!("Migration version record failed: {e}")) - })?; - } - } - Ok(()) + let filtered: Vec<(i64, &str, &str)> = MIGRATIONS + .iter() + .filter(|(v, _, _)| *v <= target_version) + .copied() + .collect(); + run_migrations_slice(pool, &filtered).await } #[tokio::test] @@ -632,4 +633,63 @@ mod tests { let err = result.unwrap_err().to_string().to_lowercase(); assert!(err.contains("no such table"), "got: {err}"); } + + // RB-02 regression: a multi-statement migration that fails part-way + // through must leave no trace on disk — the transaction rolls back + // both the partial schema change and (implicitly) the `schema_version` + // row that the pre-fix implementation would have recorded after + // statement-level success. + // + // The poisoned migration below first creates `poison_marker` + // (syntactically valid, would succeed against any SQLite) and then + // runs a guaranteed-invalid function call. Under the new atomic + // implementation, neither `poison_marker` nor the v9 row should + // survive the failed call. + #[tokio::test] + async fn multi_statement_migration_rolls_back_on_failure() { + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .expect("pool"); + + run_migrations(&pool).await.expect("baseline migrate"); + + const POISON: &[(i64, &str, &str)] = &[( + 9, + "rb-02 atomicity poison", + r#" + CREATE TABLE poison_marker (id INTEGER PRIMARY KEY); + SELECT this_function_does_not_exist(); + "#, + )]; + + let result = run_migrations_slice(&pool, POISON).await; + assert!( + result.is_err(), + "poisoned migration must return Err, got: {result:?}" + ); + + // `poison_marker` must not be on disk — transaction rolled back. + let marker: std::result::Result = + sqlx::query_scalar("SELECT COUNT(*) FROM poison_marker") + .fetch_one(&pool) + .await; + assert!( + marker.is_err(), + "poison_marker must not exist; got: {marker:?}" + ); + + // `schema_version` must not include v9 — version insert is part + // of the same transaction that rolled back. + let max: i64 = + sqlx::query_scalar("SELECT COALESCE(MAX(version), 0) FROM schema_version") + .fetch_one(&pool) + .await + .expect("read schema_version"); + assert_eq!( + max, 8, + "schema_version must not advance past the failed migration" + ); + } } diff --git a/docs/issues/README.md b/docs/issues/README.md index 3abdef4..416c0dc 100644 --- a/docs/issues/README.md +++ b/docs/issues/README.md @@ -11,12 +11,11 @@ Issues here must land before Kon v0.1 ships. Each is sourced from `docs/code-review-2026-04-22.md`. When `gh` CLI is available, these should be mirrored as real GitHub issues on `jakejars/kon`. -## CRITICAL (3) +## CRITICAL (2 open, 1 resolved) | # | File | Area | Fix scope | |---|---|---|---| | RB-01 | [c1-live-session-race.md](c1-live-session-race.md) | `src-tauri/commands/live.rs` | large | -| RB-02 | [c3-migrations-atomicity.md](c3-migrations-atomicity.md) | `crates/storage/migrations.rs` | medium | | RB-03 | [c4-transcript-profile-fk.md](c4-transcript-profile-fk.md) | `crates/storage/migrations.rs` + `database.rs` | large | ## MAJOR (7 open, 2 resolved) @@ -35,6 +34,7 @@ should be mirrored as real GitHub issues on `jakejars/kon`. | # | File | Area | Resolution | |---|---|---|---| +| RB-02 | [c3-migrations-atomicity.md](c3-migrations-atomicity.md) | `crates/storage/migrations.rs` | Each migration now runs inside a `pool.begin()` / `tx.commit()` transaction alongside its `schema_version` insert. Regression test injects a poisoned v9 migration and asserts neither the partial schema change nor the version row persists. DRY'd `run_migrations_up_to` test helper onto the same code path. | | RB-09 | [decoder-partial-audio-on-error.md](decoder-partial-audio-on-error.md) | `crates/audio/decode.rs` | Packet-loop now propagates all non-EOF `SymphoniaError`s as `AudioDecodeFailed`; per-packet decode errors bubble via `?`. Mock-`MediaSource` regression test confirms mid-stream I/O errors surface instead of returning partial audio. | | RB-12 | [hotkey-linux-device-filter.md](hotkey-linux-device-filter.md) | `crates/hotkey/linux.rs` | Extracted `device_supports_combo` helper; `try_attach_device` now reads the configured `HotkeyCombo` from the watch channel and checks support for that trigger key. Four regression tests land in `linux::tests`. | diff --git a/docs/issues/c3-migrations-atomicity.md b/docs/issues/c3-migrations-atomicity.md index 444a5aa..427e82b 100644 --- a/docs/issues/c3-migrations-atomicity.md +++ b/docs/issues/c3-migrations-atomicity.md @@ -4,6 +4,32 @@ **Path:** `crates/storage/src/migrations.rs:263-299` **Source:** [2026-04-22 code review](../code-review-2026-04-22.md#c3--multi-statement-migrations-can-half-apply) **Labels:** release-blocker, critical, data-integrity, storage +**Status:** RESOLVED (2026-04-22) + +## Resolution + +Extracted `run_migrations_slice(pool, migrations)` as the single code +path that applies pending migrations. For each pending version it +opens a `Transaction` via `pool.begin()`, applies every split statement +on that transaction, records the `schema_version` row inside the same +transaction, and finally `tx.commit()`s. A failure anywhere in the +sequence — statement, version insert, commit — rolls the whole +migration back. + +`run_migrations` delegates to `run_migrations_slice(pool, MIGRATIONS)` +and the test helper `run_migrations_up_to` to a filtered subset, so +only one version of the apply logic exists. + +Regression test `multi_statement_migration_rolls_back_on_failure` +feeds a poisoned v9 migration (`CREATE TABLE poison_marker; SELECT +this_function_does_not_exist()`) through `run_migrations_slice`. The +call returns `Err`, and post-call `SELECT COUNT(*) FROM poison_marker` +fails with "no such table" while `MAX(schema_version)` remains at 8. + +SQLite DDL participates in transactions, so this is sufficient for the +Kon schema. If any future migration needs a statement that implicitly +commits (`VACUUM`, `REINDEX`, `ATTACH`) — none do today — it must be +split into its own non-transactional migration. Reviewer's job to flag. ## Problem