Jake
05d823bf05
Every multi-statement migration and its matching schema_version insert now execute on the same sqlx Transaction. A failure anywhere — a bad statement, the version insert, or the commit itself — rolls the database back to its previous state, so the next startup replays the migration against a clean schema rather than a half-mutated one. Extracted run_migrations_slice(pool, migrations) as the single apply path. run_migrations delegates to it with MIGRATIONS; the test helper run_migrations_up_to now filters MIGRATIONS by target and delegates to the same code, eliminating the duplicated loop that previously lived in the test module. Regression test multi_statement_migration_rolls_back_on_failure injects a poisoned v9 migration (valid CREATE followed by a bogus function call) and asserts neither the partial schema change nor the schema_version row persists after the failure. SQLite DDL participates in transactions, so this is sufficient. Any future migration that needs an implicitly-committing statement (VACUUM / REINDEX / ATTACH — none today) must be its own non-transactional migration; that's a reviewer responsibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
51 lines
2.9 KiB
Markdown
51 lines
2.9 KiB
Markdown
# RB-02 CRITICAL: multi-statement migrations can half-apply
|
|
|
|
**Severity:** CRITICAL
|
|
**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
|
|
|
|
`run_migrations` executes each statement individually and only records the schema version after the full migration succeeds. If a multi-statement migration (v5, v6, v8 — any containing more than one `CREATE` / `ALTER` / `UPDATE`) fails mid-run, or the process is killed between statements, the schema can end up partially changed while still appearing unapplied. The next startup replays the same migration against the mutated database, which can fail in confusing ways or corrupt data further.
|
|
|
|
## Acceptance
|
|
|
|
- Every migration runs inside a single `BEGIN` / `COMMIT` transaction.
|
|
- The version row update happens inside the same transaction — atomic success or no change.
|
|
- Regression test: a migration that panics partway through leaves the database at the previous schema version with no partial changes visible on restart.
|
|
|
|
## Fix scope
|
|
|
|
Medium. Wrap each migration in `pool.begin()` / `tx.commit()`. The version update and the migration statements all execute on the same `Transaction` handle. Needs careful review of any migration that uses implicit commits (SQLite `VACUUM`, `REINDEX`, `ATTACH` — none of which Kon currently uses, but the review pattern should guard against future additions).
|
|
|
|
## Dependencies
|
|
|
|
- Coupled with RB-03 (any v9 migration adding the transcript-profile FK must itself be transactional — this fix is a prerequisite).
|