From fe53377e3b6d4df607f9a0e1d3e277125dc1de07 Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sun, 20 Sep 2026 04:09:31 +0300 Subject: [PATCH 1/6] fix(database): the migrator was not atomic, and could record nothing at all Three faults, found while adopting the migrator in a consumer app. Migrator.run applied and recorded each migration in turn with no transaction anywhere, so a failure part way left that migration's earlier statements applied and its ledger row absent. The next launch re-ran it from its first statement, met the table it had already created, and failed identically. A host that migrates inside Magic.init before runApp has no UI to report that from, and the only repair is deleting the database. The whole run is one transaction now rather than each migration: a ledger recording one and not the next describes a schema nobody designed, and the host cannot learn which half it has. A caller that already opened a transaction is not nested into, because sqlite refuses a nested BEGIN and opening one unconditionally would break every host that wraps the call itself, which is what a host had to do before this. CommonDatabase.autocommit is false exactly while a transaction is open and is the only thing that can tell the two cases apart. The tracking table stays outside the transaction so a failed first run leaves somewhere to record the retry. Second fault, latent rather than observed: _ensureMigrationsTable creates the ledger with a raw execute that DatabaseManager never hears about, and getColumns caches the EMPTY answer a missing table gives. _recordMigration filters its keys against that cache through QueryBuilder, and an empty filter makes insert return 0 without inserting and without throwing. So anything that read the ledger's columns first left every migration re-running on every launch for ever. One clearSchemaCache after the create, with a test that reproduces it by reading the columns deliberately. Third, documentation: void up() async compiles and is recorded complete at its first suspension, because run cannot await a void. The contract now says so and names the synchronous alternatives. --- CHANGELOG.md | 10 + doc/database/migrations.md | 23 +++ lib/src/database/migrations/migration.dart | 13 ++ lib/src/database/migrations/migrator.dart | 65 ++++-- skills/magic-framework/SKILL.md | 2 +- .../references/eloquent-orm.md | 15 +- test/database/migrator_atomicity_test.dart | 187 ++++++++++++++++++ 7 files changed, 302 insertions(+), 13 deletions(-) create mode 100644 test/database/migrator_atomicity_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 432400e..d812b52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,16 @@ All notable changes to this project will be documented in this file. ### Fixed +- **`Migrator.run` was not atomic, and the state that produced is a host that never boots again.** It applied and recorded each migration in turn with no transaction anywhere, so a failure part way left that migration's earlier statements applied and its ledger row absent. The next launch re-ran it from its first statement, met the table it had already created, and failed identically. A host that migrates inside `Magic.init` before `runApp` has no UI to report that from, and the only repair is deleting the database. + + The whole run is one transaction now, not each migration: a ledger recording one migration and not the next describes a schema nobody designed, and the host cannot learn which half it has. SQLite rolls DDL back like anything else, so this is one `BEGIN`. + + **A caller that already opened a transaction is not nested into.** sqlite refuses a nested `BEGIN`, so opening one unconditionally would break every host that wraps the call itself, which is what a host had to do before this landed. `CommonDatabase.autocommit` is false exactly while a transaction is open and is the only thing that can tell the two cases apart. The tracking table is still created outside the transaction, so a failed first run leaves somewhere to record the retry. + +- **Every migration could be applied and silently never recorded.** `_ensureMigrationsTable` creates the ledger with a raw `execute`, which `DatabaseManager` never hears about, and `getColumns` caches the EMPTY answer a missing table gives. `_recordMigration` goes through `QueryBuilder`, which filters every key against that cache, and an empty filter makes `insert` return 0 without inserting and without throwing. So anything that read the ledger's columns before it existed left every migration re-running on every launch for ever. One `clearSchemaCache` after the create. Latent rather than observed: no caller in the wild was found reaching it. + +- **`Migration.up()` and `down()` are documented as synchronous**, because `void up() async` compiles and is a silent defect: `run` cannot await a `void`, so an async body is recorded complete the moment it reaches its first suspension. The doc block names the synchronous alternatives and says why `DatabaseManager().hasColumn` must not be called from a migration. + - **A translation catalogue loaded nothing, silently, whenever no `log` service was bound.** `JsonAssetLoader._loadJson` opened with `Log.info('Loading translation file [...]')` before it read anything, and `Log` resolves `log` through the container, which throws for an unbound key (`foundation/application.dart:269-274`). `load`'s own catch then turned that throw into an empty map, so every key rendered as itself with nothing anywhere to read. Reported from a consumer app whose test suite could not assert a single translated sentence. Measured there: `rootBundle` reads the asset fine (19,345 bytes), `Translator.load` reports `loaded: true` for the right locale, and the loader still answers zero keys. diff --git a/doc/database/migrations.md b/doc/database/migrations.md index 3972883..11c8b88 100644 --- a/doc/database/migrations.md +++ b/doc/database/migrations.md @@ -6,6 +6,8 @@ Migrations are version-controlled schema definitions that let you create, modify - [Generating Migrations](#generating-migrations) - [Migration Structure](#migration-structure) - [Running Migrations](#running-migrations) + - [The run is atomic](#the-run-is-atomic) + - [`up()` and `down()` are synchronous](#up-and-down-are-synchronous) - [Creating Tables](#creating-tables) - [Available Column Types](#available-column-types) - [Column Modifiers](#column-modifiers) @@ -102,6 +104,27 @@ void main() async { The `Migrator` keeps track of which migrations have already run, so calling `run()` multiple times is safe. +### The run is atomic + +Every pending migration in one `run()` is applied inside a single transaction. If any of them throws, all of them are rolled back and the ledger records none, so the next launch retries the whole run from a clean schema rather than meeting half-applied work it cannot recognise. + +That matters most when migrations run before your UI exists. A host that migrates inside `Magic.init` and then calls `runApp` has nowhere to report a failure from, and a migration that was half-applied and unrecorded would fail identically on every later launch with no way out but deleting the database. + +If you have already opened a transaction yourself, `run()` uses yours rather than opening a second one, because SQLite refuses a nested `BEGIN`. Either shape works: + +```dart +await Migrator().run([...]); // the migrator's transaction +await DB.transaction(() => Migrator().run([...])); // yours +``` + +The tracking table is created outside the transaction, so a failed first run still leaves somewhere to record the retry. + +### `up()` and `down()` are synchronous + +Writing `void up() async` compiles and is a silent defect: `run()` cannot await a `void`, so an async body is recorded complete the moment it reaches its first suspension. Use `DB.statement` and `DB.select`, both synchronous. `DatabaseManager().getColumns` and `hasColumn` answer futures and must not be called from a migration. + +A migration with no honest rollback should throw `UnsupportedError` from `down()` rather than doing nothing, or `rollback()` will delete the ledger row for a migration that is still applied. + ## Creating Tables diff --git a/lib/src/database/migrations/migration.dart b/lib/src/database/migrations/migration.dart index debde4c..c38d10e 100644 --- a/lib/src/database/migrations/migration.dart +++ b/lib/src/database/migrations/migration.dart @@ -47,10 +47,23 @@ abstract class Migration { /// /// Define your schema changes here using [Schema.create], [Schema.drop], /// or raw SQL via [DB]. + /// + /// **Synchronous, and writing `void up() async` is a silent defect.** + /// `Migrator.run` calls this without awaiting, because a `void` cannot be + /// awaited, so an async body is recorded as complete the moment it reaches + /// its first suspension and the migrator commits over the top of work that + /// has not happened. Everything a migration needs has a synchronous form: + /// use `DB.statement` and `DB.select`, never `DatabaseManager().getColumns` + /// or `hasColumn`, which answer futures. void up(); /// Reverse the migration. /// /// Define how to undo the changes made in [up]. + /// + /// Synchronous for the same reason as [up]. A migration with no honest + /// rollback should throw `UnsupportedError` rather than do nothing: a + /// `down` that silently succeeds tells `Migrator.rollback` to delete the + /// ledger row for a migration that is still applied. void down(); } diff --git a/lib/src/database/migrations/migrator.dart b/lib/src/database/migrations/migrator.dart index 98e51b4..fe9a2b3 100644 --- a/lib/src/database/migrations/migrator.dart +++ b/lib/src/database/migrations/migrator.dart @@ -68,6 +68,35 @@ class Migrator { /// CreatePostsTable(), /// ]); /// ``` + /// Runs every pending migration, or none of them. + /// + /// ### The whole run is one transaction + /// + /// It was none, and the state that produced is a host that never boots + /// again. Each migration was applied and recorded in turn, so a failure part + /// way left that migration's earlier statements applied and its ledger row + /// absent. The next launch re-ran it from its first statement, met the table + /// it had already created, and failed identically. A host that migrates + /// inside `Magic.init` before `runApp` has no UI to report that from, and + /// the only repair is deleting the database. + /// + /// SQLite rolls DDL back like anything else, so one `BEGIN` is the whole + /// fix. The run is the unit rather than each migration: a ledger recording + /// one migration and not the next describes a schema nobody designed, and + /// the host has no way to learn which half it has. + /// + /// ### Why it defers to a caller that already opened one + /// + /// sqlite refuses a nested `BEGIN` with `cannot start a transaction within a + /// transaction`, so opening one unconditionally would break every host that + /// wraps this call itself, which is what a host had to do before this + /// landed. [CommonDatabase.autocommit] is false exactly while a transaction + /// is open, and is the only thing that can tell the two cases apart. + /// + /// The tracking table is created OUTSIDE the transaction, deliberately. A + /// database with a ledger and no rows is what a fresh install has anyway, so + /// there is nothing to roll back about it, and creating it inside would mean + /// a failed first run left no way to record the retry. Future> run(List migrations) async { // Ensure migrations table exists await _ensureMigrationsTable(); @@ -87,25 +116,28 @@ class Migrator { // Get next batch number _batch = await _getNextBatchNumber(); - // Run each pending migration + final owned = _db.connection.autocommit; + if (owned) _db.connection.execute('BEGIN'); + final ranMigrations = []; - for (final migration in pending) { - try { - // Execute the up method + try { + for (final migration in pending) { migration.up(); - - // Record it await _recordMigration(migration.name); - ranMigrations.add(migration.name); - } catch (e) { - // Log the error but continue with other migrations - // In production, you might want to stop here - rethrow; } + } catch (_) { + // Only unwind what this call started. A caller that owns the + // transaction gets the throw and rolls back its own, which is what the + // `outer` case in `migrator_atomicity_test.dart` asserts. + if (owned) _db.connection.execute('ROLLBACK'); + + rethrow; } + if (owned) _db.connection.execute('COMMIT'); + // Clear schema cache after migrations _db.clearSchemaCache(); @@ -202,6 +234,15 @@ class Migrator { // --------------------------------------------------------------------------- /// Create the migrations tracking table if it doesn't exist. + /// + /// The cache line is not housekeeping. A raw `execute` changes the schema + /// behind [DatabaseManager]'s back, and `getColumns` caches the EMPTY answer + /// a missing table gives. [_recordMigration] goes through `QueryBuilder`, + /// which filters every key against that cache, and an empty filter makes + /// `insert` return 0 without inserting and without throwing + /// (`query_builder.dart:278-280`). So anything that read this table's + /// columns before it existed would leave every migration applied and never + /// recorded, and re-applied on every launch for ever. Future _ensureMigrationsTable() async { _db.connection.execute(''' CREATE TABLE IF NOT EXISTS $_table ( @@ -210,6 +251,8 @@ class Migrator { batch INTEGER NOT NULL ) '''); + + _db.clearSchemaCache(_table); } /// Get list of executed migration names. diff --git a/skills/magic-framework/SKILL.md b/skills/magic-framework/SKILL.md index 8331a3d..77a1f8d 100644 --- a/skills/magic-framework/SKILL.md +++ b/skills/magic-framework/SKILL.md @@ -2,7 +2,7 @@ name: magic-framework description: "Write correct, idiomatic code in a Flutter app that depends on the `magic` framework (Laravel-inspired: IoC container, 18 facades, Eloquent-style ORM, service providers, reactive controllers, GoRouter routing, validation, auth, broadcasting). Use whenever code imports `package:magic/magic.dart` or `package:magic/testing.dart`, or the work touches Magic.init, MagicApp, a facade (Auth/Http/Cache/DB/Echo/Event/Gate/Config/Lang/Launch/Log/Pick/MagicRoute/Schema/Session/Storage/Vault/Crypt), a Model, MagicController, a MagicView, MagicFormData, FormRequest, a ServiceProvider, a migration, or the artisan make:* CLI. UI styling is Wind (separate wind-ui skill). Do NOT use for plain Flutter or Wind-only work with no magic import." when_to_use: "Use proactively when editing or scaffolding a magic app: Magic.init / a facade / a Model / a MagicController or MagicView / a form (MagicFormData, FormRequest, Validator) / a ServiceProvider / a route or MagicMiddleware / a migration / MagicStateMixin + RxStatus + fetchList / Session flash + old() + trans() / testing with MagicTest + Http.fake/Auth.fake / the artisan make:* CLI / the magic_deeplink, magic_notifications, magic_social_auth, magic_starter, magic_payments, or magic_devtools plugins. Trigger even when the user does not say the word 'magic'. Do NOT trigger for plain Flutter or Wind-only UI with no package:magic import." -version: 0.1.32 +version: 0.1.33 --- diff --git a/skills/magic-framework/references/eloquent-orm.md b/skills/magic-framework/references/eloquent-orm.md index 19e93a5..6150b5e 100644 --- a/skills/magic-framework/references/eloquent-orm.md +++ b/skills/magic-framework/references/eloquent-orm.md @@ -332,7 +332,20 @@ class CreateMonitorsTable extends Migration { } ``` -`up()` and `down()` are synchronous `void`. Register migrations via `Migrator().run([...])`. +`up()` and `down()` are synchronous `void`, and that is load-bearing rather than incidental: `Migrator.run` cannot await a `void`, so writing `void up() async` compiles and is recorded complete the moment the body reaches its first suspension. Use `DB.statement` and `DB.select`; `DatabaseManager().getColumns` and `hasColumn` answer futures and must not be called from a migration. A migration with no honest rollback throws `UnsupportedError` from `down()` rather than doing nothing, or `rollback()` deletes the ledger row for a migration that is still applied. + +Register migrations via `Migrator().run([...])`. The whole run is one transaction: if any migration throws, all of them roll back and the ledger records none, so the retry starts from a clean schema. If you already opened a transaction, `run()` uses yours rather than nesting, which sqlite would refuse. + +A migration that changes a table an earlier version already created has to sense the schema rather than trust the ledger, because a baseline migration records both shapes identically: + +```dart +final bool present = DB.select('PRAGMA table_info(users)') + .any((Map row) => row['name'] == 'avatar'); +if (present) return; +DB.statement("ALTER TABLE users ADD COLUMN avatar TEXT NOT NULL DEFAULT ''"); +``` + +`ADD COLUMN ... NOT NULL` with no default succeeds on an empty table and throws on one with rows, so omitting the default passes in development and fails on exactly the install it exists to repair. ### Blueprint Column Methods diff --git a/test/database/migrator_atomicity_test.dart b/test/database/migrator_atomicity_test.dart new file mode 100644 index 0000000..7ea2d79 --- /dev/null +++ b/test/database/migrator_atomicity_test.dart @@ -0,0 +1,187 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:magic/magic.dart'; +import 'package:sqlite3/sqlite3.dart'; + +/// A migration whose `up` does two statements and can be told to fail between +/// them, which is the shape that makes a partial application visible. +class _TwoStepMigration extends Migration { + _TwoStepMigration(this.name, {this.throwsAfterFirst = false}); + + @override + final String name; + + /// Whether to throw once the first statement is applied. + final bool throwsAfterFirst; + + @override + void up() { + DB.statement('CREATE TABLE IF NOT EXISTS ${name}_a (x TEXT)'); + + if (throwsAfterFirst) throw StateError('migration $name failed part way'); + + DB.statement('CREATE TABLE IF NOT EXISTS ${name}_b (x TEXT)'); + } + + @override + void down() { + DB.statement('DROP TABLE IF EXISTS ${name}_b'); + DB.statement('DROP TABLE IF EXISTS ${name}_a'); + } +} + +/// A migration that fails is rolled back, and a run that fails leaves nothing. +/// +/// **The state this prevents is a permanent boot with no UI.** `run` applied +/// and recorded each migration in turn with no transaction anywhere, so a +/// failure part way left that migration's earlier statements applied and its +/// ledger row absent. Every later launch re-ran it from its first statement, +/// which was already applied, and failed identically. A host that runs +/// migrations inside `Magic.init` before `runApp` never renders again, and the +/// only repair is deleting the database. +/// +/// SQLite rolls DDL back like anything else, which is what makes the fix one +/// `BEGIN`. Verified: `CREATE TABLE a(x); BEGIN; CREATE TABLE b(y); ROLLBACK;` +/// leaves only `a`. +void main() { + setUp(() { + MagicApp.reset(); + Magic.flush(); + DatabaseManager().setConnection(sqlite3.openInMemory()); + }); + + tearDown(DatabaseManager().dispose); + + /// Whether a table exists right now. + bool exists(String table) => DB.select( + "SELECT name FROM sqlite_master WHERE type = 'table' AND name = ?", + [table], + ).isNotEmpty; + + test('a migration that throws part way applies none of itself', () async { + await expectLater( + Migrator().run([ + _TwoStepMigration('half', throwsAfterFirst: true), + ]), + throwsA(isA()), + ); + + expect( + exists('half_a'), + isFalse, + reason: 'the first statement survived a failed migration', + ); + expect(exists('half_b'), isFalse); + }); + + test('an earlier migration in the same run is rolled back too', () async { + // The whole run is one unit, not each migration. A ledger that records + // one and not the next describes a schema nobody designed, and the host + // has no way to know which half it has. + await expectLater( + Migrator().run([ + _TwoStepMigration('first'), + _TwoStepMigration('second', throwsAfterFirst: true), + ]), + throwsA(isA()), + ); + + expect(exists('first_a'), isFalse); + expect(exists('second_a'), isFalse); + }); + + test('the ledger records nothing for a run that failed', () async { + await expectLater( + Migrator().run([ + _TwoStepMigration('kept'), + _TwoStepMigration('broken', throwsAfterFirst: true), + ]), + throwsA(isA()), + ); + + // The tracking table itself survives: it is created outside the + // transaction on purpose, because a database with no ledger and no + // migrations is the state a fresh install is in anyway. + expect(exists('magic_migrations'), isTrue); + expect(DB.select('SELECT migration FROM magic_migrations'), isEmpty); + }); + + test('a retry after a failure succeeds rather than repeating it', () async { + // The point of the rollback. Before it, the second attempt met a table + // that already existed and threw again, for ever. + await expectLater( + Migrator().run([ + _TwoStepMigration('retry', throwsAfterFirst: true), + ]), + throwsA(isA()), + ); + + await Migrator().run([_TwoStepMigration('retry')]); + + expect(exists('retry_a'), isTrue); + expect(exists('retry_b'), isTrue); + expect(DB.select('SELECT migration FROM magic_migrations'), hasLength(1)); + }); + + test('a successful run still commits', () async { + final List ran = await Migrator().run([ + _TwoStepMigration('one'), + _TwoStepMigration('two'), + ]); + + expect(ran, ['one', 'two']); + expect(exists('one_b'), isTrue); + expect(exists('two_b'), isTrue); + }); + + test('a caller that opened its own transaction is not nested into', () async { + // sqlite refuses a nested BEGIN with `cannot start a transaction within a + // transaction`, so a migrator that opened one unconditionally would break + // every host that already wraps the call. `autocommit` is false exactly + // when a transaction is open, which is the only thing that can tell them + // apart. + await DB.transaction( + () => Migrator().run([_TwoStepMigration('wrapped')]), + ); + + expect(exists('wrapped_b'), isTrue); + }); + + test( + 'a stale schema cache for the ledger does not silence the record', + () async { + // The ledger is created with a raw `execute`, which no cache hears about. + // Reading its columns first caches the empty answer a missing table gives, + // and `QueryBuilder.insert` filters every key against that cache: an empty + // filter returns 0 WITHOUT inserting and without throwing + // (`query_builder.dart:278-280`). So every migration would be applied and + // never recorded, and re-applied on every launch for ever. + expect(await DatabaseManager().getColumns('magic_migrations'), isEmpty); + + await Migrator().run([_TwoStepMigration('cached')]); + + expect( + DB.select('SELECT migration FROM magic_migrations'), + hasLength(1), + reason: 'the migration ran and was not recorded, so it will run again', + ); + }, + ); + + test( + 'a failure inside a caller-owned transaction still rolls back', + () async { + // The caller's transaction does the work here rather than the migrator's, + // which is the point of deferring to it. + await expectLater( + DB.transaction( + () => Migrator().run([ + _TwoStepMigration('outer', throwsAfterFirst: true), + ]), + ), + throwsA(isA()), + ); + + expect(exists('outer_a'), isFalse); + }, + ); +} From 6636e1b65b09777f61c93ada2e88a6140963e8f0 Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sun, 20 Sep 2026 04:11:20 +0300 Subject: [PATCH 2/6] docs(database): the two new sections needed explicit anchors This repo's TOC check requires an tag rather than relying on GitHub's generated slug, and CI caught both new entries. --- doc/database/migrations.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/database/migrations.md b/doc/database/migrations.md index 11c8b88..01805dc 100644 --- a/doc/database/migrations.md +++ b/doc/database/migrations.md @@ -104,6 +104,7 @@ void main() async { The `Migrator` keeps track of which migrations have already run, so calling `run()` multiple times is safe. + ### The run is atomic Every pending migration in one `run()` is applied inside a single transaction. If any of them throws, all of them are rolled back and the ledger records none, so the next launch retries the whole run from a clean schema rather than meeting half-applied work it cannot recognise. @@ -119,6 +120,7 @@ await DB.transaction(() => Migrator().run([...])); // yours The tracking table is created outside the transaction, so a failed first run still leaves somewhere to record the retry. + ### `up()` and `down()` are synchronous Writing `void up() async` compiles and is a silent defect: `run()` cannot await a `void`, so an async body is recorded complete the moment it reaches its first suspension. Use `DB.statement` and `DB.select`, both synchronous. `DatabaseManager().getColumns` and `hasColumn` answer futures and must not be called from a migration. From f315d1bef71279cdd52e69ac68aac24d1a3fcc0c Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sun, 20 Sep 2026 04:21:48 +0300 Subject: [PATCH 3/6] fix(database): a migration that committed made a successful run report failure Kodizm review round 1, and the Major is a real regression the first version of this PR introduced. DB.beginTransaction, commit and rollback are a documented pattern (doc/database/getting-started.md), so a migration written that way was legitimate before run opened a transaction of its own. With the BEGIN it was not, and the failure shape was the worst possible one: a commit() inside up() closed the migrator's transaction, so every later migration ran unprotected and the closing COMMIT threw AFTER every migration had succeeded and committed its ledger row. The caller saw a failure from a run that fully worked, and the retry found nothing pending. Two changes. A SAVEPOINT rather than a BEGIN, which nests, so the autocommit branch is gone and the same code path runs whether or not the host wrapped the call. Verified in all three shapes: no transaction open, inside one, and unwinding through ROLLBACK TO. And a guard after each up(): autocommit is false while a savepoint is open, so a true means the migration ended the transaction under us. It throws naming the migration rather than failing late and confusingly, and stops the loop there rather than carrying on unprotected. Three minors, all correct. The schema cache is cleared on the rollback path too. The 'tracking table outside the transaction' claim only held for an owned run and now says so. The doc named DatabaseManager's futures but not Schema.hasTable / hasColumn / getColumns, which are what a migration author would actually reach for, and offered no Schema.create or Schema.table as the synchronous alternatives. Also merged run's two dartdoc openings, so the real summary is the one dartdoc takes. --- CHANGELOG.md | 6 +- doc/database/migrations.md | 21 +++- lib/src/database/migrations/migrator.dart | 102 ++++++++++++------ .../references/eloquent-orm.md | 2 +- test/database/migrator_atomicity_test.dart | 73 +++++++++++++ 5 files changed, 164 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d812b52..c2a08e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,11 @@ All notable changes to this project will be documented in this file. The whole run is one transaction now, not each migration: a ledger recording one migration and not the next describes a schema nobody designed, and the host cannot learn which half it has. SQLite rolls DDL back like anything else, so this is one `BEGIN`. - **A caller that already opened a transaction is not nested into.** sqlite refuses a nested `BEGIN`, so opening one unconditionally would break every host that wraps the call itself, which is what a host had to do before this landed. `CommonDatabase.autocommit` is false exactly while a transaction is open and is the only thing that can tell the two cases apart. The tracking table is still created outside the transaction, so a failed first run leaves somewhere to record the retry. + **A `SAVEPOINT` rather than a `BEGIN`**, which is what lets it nest inside a transaction the host opened itself. Verified in all three shapes: with no transaction open, inside one, and unwinding through `ROLLBACK TO`. + + **BREAKING: a migration may no longer manage its own transaction.** `DB.beginTransaction`, `commit` and `rollback` are a documented pattern elsewhere, so a migration written that way was legitimate before this. A `commit()` inside `up()` closes the migrator's savepoint, which meant every later migration ran unprotected and the closing `RELEASE` threw AFTER every migration had succeeded and committed its ledger row: the caller saw a failure from a run that fully worked, and the retry found nothing pending. `run` detects it and throws naming the migration that broke the contract, rather than failing late and confusingly. + + The tracking table is created before the savepoint, so a run that owns its transaction and fails still leaves somewhere to record the retry. A host that wrapped the call itself and rolls back takes the table with it, which is harmless because every entry point creates it again. The schema cache is cleared on the rollback path too, so nothing cached during an undone migration survives to describe schema that no longer exists. - **Every migration could be applied and silently never recorded.** `_ensureMigrationsTable` creates the ledger with a raw `execute`, which `DatabaseManager` never hears about, and `getColumns` caches the EMPTY answer a missing table gives. `_recordMigration` goes through `QueryBuilder`, which filters every key against that cache, and an empty filter makes `insert` return 0 without inserting and without throwing. So anything that read the ledger's columns before it existed left every migration re-running on every launch for ever. One `clearSchemaCache` after the create. Latent rather than observed: no caller in the wild was found reaching it. diff --git a/doc/database/migrations.md b/doc/database/migrations.md index 01805dc..91c51ec 100644 --- a/doc/database/migrations.md +++ b/doc/database/migrations.md @@ -111,19 +111,30 @@ Every pending migration in one `run()` is applied inside a single transaction. I That matters most when migrations run before your UI exists. A host that migrates inside `Magic.init` and then calls `runApp` has nowhere to report a failure from, and a migration that was half-applied and unrecorded would fail identically on every later launch with no way out but deleting the database. -If you have already opened a transaction yourself, `run()` uses yours rather than opening a second one, because SQLite refuses a nested `BEGIN`. Either shape works: +It is a `SAVEPOINT` rather than a `BEGIN`, which is what lets it nest inside a transaction you opened yourself. Either shape works: ```dart -await Migrator().run([...]); // the migrator's transaction -await DB.transaction(() => Migrator().run([...])); // yours +await Migrator().run([...]); // the migrator's savepoint alone +await DB.transaction(() => Migrator().run([...])); // nested in yours ``` -The tracking table is created outside the transaction, so a failed first run still leaves somewhere to record the retry. +**A migration must not manage its own transaction.** `DB.beginTransaction`, `DB.commit` and `DB.rollback` are a supported pattern elsewhere and are not available inside `up()`: the run is already one unit. A `commit()` there closes the migrator's savepoint, which used to mean every later migration ran unprotected and the run reported a failure after fully succeeding. `run()` detects it now and throws naming the migration. + +The tracking table is created before the savepoint, so a run that owns its own transaction and fails still leaves somewhere to record the retry. A host that wrapped the call in its own transaction and rolls back takes the table with it, which is harmless: every entry point creates it again. ### `up()` and `down()` are synchronous -Writing `void up() async` compiles and is a silent defect: `run()` cannot await a `void`, so an async body is recorded complete the moment it reaches its first suspension. Use `DB.statement` and `DB.select`, both synchronous. `DatabaseManager().getColumns` and `hasColumn` answer futures and must not be called from a migration. +Writing `void up() async` compiles and is a silent defect: `run()` cannot await a `void`, so an async body is recorded complete the moment it reaches its first suspension. + +The synchronous half of the schema API is what a migration uses: `Schema.create`, `Schema.table`, `Schema.drop`, `Schema.dropIfExists`, `Schema.rename`, plus `DB.statement` and `DB.select`. + +The introspection helpers all answer futures and must not be called from a migration: `Schema.hasTable`, `Schema.hasColumn`, `Schema.getColumns`, and `DatabaseManager().getColumns` / `hasColumn`. To sense a schema synchronously, read the pragma directly: + +```dart +final bool present = DB.select('PRAGMA table_info(users)') + .any((Map row) => row['name'] == 'avatar'); +``` A migration with no honest rollback should throw `UnsupportedError` from `down()` rather than doing nothing, or `rollback()` will delete the ledger row for a migration that is still applied. diff --git a/lib/src/database/migrations/migrator.dart b/lib/src/database/migrations/migrator.dart index fe9a2b3..4dfa0b7 100644 --- a/lib/src/database/migrations/migrator.dart +++ b/lib/src/database/migrations/migrator.dart @@ -57,10 +57,13 @@ class Migrator { /// Current batch number. int _batch = 0; - /// Run all pending migrations. + /// The savepoint name [run] wraps itself in. + static const String _savepoint = 'magic_migrator'; + + /// Runs every pending migration, or none of them. /// - /// [migrations] should be an ordered list of all migration classes. - /// Only migrations that haven't been executed yet will run. + /// [migrations] is an ordered list of every migration class; the ones + /// already recorded are skipped. /// /// ```dart /// await Migrator().run([ @@ -68,35 +71,50 @@ class Migrator { /// CreatePostsTable(), /// ]); /// ``` - /// Runs every pending migration, or none of them. /// - /// ### The whole run is one transaction + /// ### The whole run is one unit + /// + /// It was not, and the state that produced is a host that never boots again. + /// Each migration was applied and recorded in turn with nothing around them, + /// so a failure part way left that migration's earlier statements applied + /// and its ledger row absent. The next launch re-ran it from its first + /// statement, met the table it had already created, and failed identically. + /// A host that migrates inside `Magic.init` before `runApp` has no UI to + /// report that from, and the only repair is deleting the database. + /// + /// The run is the unit rather than each migration: a ledger recording one + /// migration and not the next describes a schema nobody designed, and the + /// host has no way to learn which half it has. + /// + /// ### A SAVEPOINT rather than a BEGIN /// - /// It was none, and the state that produced is a host that never boots - /// again. Each migration was applied and recorded in turn, so a failure part - /// way left that migration's earlier statements applied and its ledger row - /// absent. The next launch re-ran it from its first statement, met the table - /// it had already created, and failed identically. A host that migrates - /// inside `Magic.init` before `runApp` has no UI to report that from, and - /// the only repair is deleting the database. + /// A savepoint nests and a `BEGIN` does not, so this composes with a host + /// that wraps the call in its own transaction instead of throwing `cannot + /// start a transaction within a transaction` at it. Verified in all three + /// shapes: with no transaction open, inside one, and unwinding through + /// `ROLLBACK TO`. The alternative was branching on + /// [CommonDatabase.autocommit], which works and leaves the host's shape + /// deciding which code path runs. /// - /// SQLite rolls DDL back like anything else, so one `BEGIN` is the whole - /// fix. The run is the unit rather than each migration: a ledger recording - /// one migration and not the next describes a schema nobody designed, and - /// the host has no way to learn which half it has. + /// ### A migration must not manage its own transaction /// - /// ### Why it defers to a caller that already opened one + /// `DB.beginTransaction`, `commit` and `rollback` are a documented pattern + /// (`doc/database/getting-started.md`), so a migration written that way was + /// legitimate before this. It is not now, and the guard below is there + /// because failing silently was the alternative: a `COMMIT` inside `up` + /// closes this savepoint, so every later migration runs unprotected and the + /// `RELEASE` throws `no such savepoint` AFTER every migration has succeeded + /// and committed its ledger row. The caller would see a failure from a run + /// that fully worked, and the retry would find nothing pending. /// - /// sqlite refuses a nested `BEGIN` with `cannot start a transaction within a - /// transaction`, so opening one unconditionally would break every host that - /// wraps this call itself, which is what a host had to do before this - /// landed. [CommonDatabase.autocommit] is false exactly while a transaction - /// is open, and is the only thing that can tell the two cases apart. + /// The check names the migration that broke the contract and stops the loop + /// there. A `DB.beginTransaction` inside `up` throws from sqlite instead, + /// which is self-describing and unwinds through the same rollback. /// - /// The tracking table is created OUTSIDE the transaction, deliberately. A - /// database with a ledger and no rows is what a fresh install has anyway, so - /// there is nothing to roll back about it, and creating it inside would mean - /// a failed first run left no way to record the retry. + /// The tracking table is created BEFORE the savepoint, so an owned run that + /// fails still leaves somewhere to record the retry. A host that wrapped the + /// call in its own transaction and rolls back takes the table with it; that + /// is harmless, because every entry point creates it again. Future> run(List migrations) async { // Ensure migrations table exists await _ensureMigrationsTable(); @@ -116,27 +134,45 @@ class Migrator { // Get next batch number _batch = await _getNextBatchNumber(); - final owned = _db.connection.autocommit; - if (owned) _db.connection.execute('BEGIN'); + _db.connection.execute('SAVEPOINT $_savepoint'); final ranMigrations = []; try { for (final migration in pending) { migration.up(); + + // `autocommit` is false while a savepoint is open, so a true here + // means this migration ended the transaction under us. + if (_db.connection.autocommit) { + throw StateError( + 'Migration [${migration.name}] committed or rolled back the ' + 'transaction the migrator opened. A migration must not call ' + 'DB.beginTransaction, DB.commit or DB.rollback: the whole run is ' + 'already one unit.', + ); + } + await _recordMigration(migration.name); ranMigrations.add(migration.name); } } catch (_) { - // Only unwind what this call started. A caller that owns the - // transaction gets the throw and rolls back its own, which is what the - // `outer` case in `migrator_atomicity_test.dart` asserts. - if (owned) _db.connection.execute('ROLLBACK'); + // `ROLLBACK TO` leaves the savepoint in place, so the `RELEASE` after it + // is what actually discards it. Guarded on `autocommit` because the one + // failure this cannot unwind is a migration that already closed it. + if (!_db.connection.autocommit) { + _db.connection.execute('ROLLBACK TO $_savepoint'); + _db.connection.execute('RELEASE $_savepoint'); + } + + // Anything cached during the undone migrations describes schema that no + // longer exists. + _db.clearSchemaCache(); rethrow; } - if (owned) _db.connection.execute('COMMIT'); + _db.connection.execute('RELEASE $_savepoint'); // Clear schema cache after migrations _db.clearSchemaCache(); diff --git a/skills/magic-framework/references/eloquent-orm.md b/skills/magic-framework/references/eloquent-orm.md index 6150b5e..94a48ff 100644 --- a/skills/magic-framework/references/eloquent-orm.md +++ b/skills/magic-framework/references/eloquent-orm.md @@ -334,7 +334,7 @@ class CreateMonitorsTable extends Migration { `up()` and `down()` are synchronous `void`, and that is load-bearing rather than incidental: `Migrator.run` cannot await a `void`, so writing `void up() async` compiles and is recorded complete the moment the body reaches its first suspension. Use `DB.statement` and `DB.select`; `DatabaseManager().getColumns` and `hasColumn` answer futures and must not be called from a migration. A migration with no honest rollback throws `UnsupportedError` from `down()` rather than doing nothing, or `rollback()` deletes the ledger row for a migration that is still applied. -Register migrations via `Migrator().run([...])`. The whole run is one transaction: if any migration throws, all of them roll back and the ledger records none, so the retry starts from a clean schema. If you already opened a transaction, `run()` uses yours rather than nesting, which sqlite would refuse. +Register migrations via `Migrator().run([...])`. The whole run is one unit: if any migration throws, all of them roll back and the ledger records none, so the retry starts from a clean schema. It is a SAVEPOINT rather than a BEGIN, so it nests inside a transaction you opened yourself. A migration must NOT call `DB.beginTransaction`, `DB.commit` or `DB.rollback`; the run is already one unit and closing it from inside throws. A migration that changes a table an earlier version already created has to sense the schema rather than trust the ledger, because a baseline migration records both shapes identically: diff --git a/test/database/migrator_atomicity_test.dart b/test/database/migrator_atomicity_test.dart index 7ea2d79..3dfb830 100644 --- a/test/database/migrator_atomicity_test.dart +++ b/test/database/migrator_atomicity_test.dart @@ -29,6 +29,27 @@ class _TwoStepMigration extends Migration { } } +/// A migration that closes the migrator's own transaction from inside `up`. +/// +/// `DB.beginTransaction` / `commit` / `rollback` are a documented pattern +/// (`doc/database/getting-started.md:195`), so a migration written this way was +/// legitimate before `run` opened a transaction of its own. +class _CommittingMigration extends Migration { + _CommittingMigration(this.name); + + @override + final String name; + + @override + void up() { + DB.statement('CREATE TABLE IF NOT EXISTS ${name}_a (x TEXT)'); + DB.commit(); + } + + @override + void down() {} +} + /// A migration that fails is rolled back, and a run that fails leaves nothing. /// /// **The state this prevents is a permanent boot with no UI.** `run` applied @@ -184,4 +205,56 @@ void main() { expect(exists('outer_a'), isFalse); }, ); + + group('a migration that manages its own transaction', () { + test( + 'is named in the error rather than reported as a late failure', + () async { + // The shape this replaces was the worst possible one. A `COMMIT` inside + // `up` closed the migrator's own transaction, so every later migration + // ran unprotected and the migrator's closing statement threw AFTER every + // migration had succeeded and its ledger row had been committed. The + // caller saw a failure from a run that had fully worked, and the retry + // found nothing pending. + await expectLater( + Migrator().run([_CommittingMigration('selfcommit')]), + throwsA( + isA().having( + (StateError e) => e.message, + 'message', + allOf(contains('selfcommit'), contains('transaction')), + ), + ), + ); + }, + ); + + test('does not leave a later migration running unprotected', () async { + await expectLater( + Migrator().run([ + _CommittingMigration('selfcommit'), + _TwoStepMigration('after', throwsAfterFirst: true), + ]), + throwsA(isA()), + ); + + // `after` must never have run at all: the guard stops the loop on the + // migration that broke the contract rather than carrying on. + expect(exists('after_a'), isFalse); + }); + }); + + test('a rollback clears the schema cache too', () async { + // It ran only on the success path, so a column list cached during an + // undone migration stayed in the manager describing schema that no longer + // exists. + await expectLater( + Migrator().run([ + _TwoStepMigration('cachefail', throwsAfterFirst: true), + ]), + throwsA(isA()), + ); + + expect(await DatabaseManager().getColumns('cachefail_a'), isEmpty); + }); } From 7acfd1c4a99cafdc86d1d4d35003e6cc6643f539 Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sun, 20 Sep 2026 11:41:59 +0300 Subject: [PATCH 4/6] fix(database): DB.transaction replaced a callback's error with its own Kodizm review round 2. Three minors, all correct. The one that matters: in the wrapped shape this PR tells hosts they may use, the new guard's diagnostic was destroyed. A migration calling DB.commit() closes the HOST's transaction too, so DB.transaction's catch found nothing to roll back and sqlite threw 'cannot rollback - no transaction is active' over the StateError naming the migration. The caller saw a message about transactions instead of the cause. Fixed in DB.transaction rather than in the migrator, because it is not a migrator problem: any callback that closes the transaction itself had its own error replaced, and one that committed and succeeded had the mirror of it on the other branch. Both are guarded on autocommit now. Closing it from inside is still a mistake; the guard stops that mistake from hiding the next one. Second: my 'a rollback clears the schema cache too' test was vacuous. It passed with the clearSchemaCache deleted, because nothing in that run ever populates the cache for that table. I did not mutate it, which is exactly the rule I hold everything else to. Deleted rather than repaired: there is no reachable case today, since up() is synchronous while every cache-populating API is a future, so the only entry the cache can hold mid-run is magic_migrations, which survives the rollback anyway. The line stays with the reasoning in a comment that says it is deliberately untested. Third: the guard is early and named but NOT atomic, and 'all of them, or none' does not hold for it. By the time the guard sees anything the offending migration's statements and every earlier ledger row are committed and there is nothing to unwind. The dartdoc, the doc page and the CHANGELOG all say so now rather than leaving the headline overclaiming. --- CHANGELOG.md | 4 ++- doc/database/migrations.md | 4 ++- lib/src/database/migrations/migrator.dart | 18 ++++++++-- lib/src/facades/db.dart | 17 ++++++++-- test/database/migrator_atomicity_test.dart | 39 ++++++++++++++-------- 5 files changed, 62 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2a08e1..6ba9de8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,9 @@ All notable changes to this project will be documented in this file. **A `SAVEPOINT` rather than a `BEGIN`**, which is what lets it nest inside a transaction the host opened itself. Verified in all three shapes: with no transaction open, inside one, and unwinding through `ROLLBACK TO`. - **BREAKING: a migration may no longer manage its own transaction.** `DB.beginTransaction`, `commit` and `rollback` are a documented pattern elsewhere, so a migration written that way was legitimate before this. A `commit()` inside `up()` closes the migrator's savepoint, which meant every later migration ran unprotected and the closing `RELEASE` threw AFTER every migration had succeeded and committed its ledger row: the caller saw a failure from a run that fully worked, and the retry found nothing pending. `run` detects it and throws naming the migration that broke the contract, rather than failing late and confusingly. + **BREAKING: a migration may no longer manage its own transaction.** `DB.beginTransaction`, `commit` and `rollback` are a documented pattern elsewhere, so a migration written that way was legitimate before this. A `commit()` inside `up()` closes the migrator's savepoint, which meant every later migration ran unprotected and the closing `RELEASE` threw AFTER every migration had succeeded and committed its ledger row: the caller saw a failure from a run that fully worked, and the retry found nothing pending. `run` detects it and throws naming the migration that broke the contract, rather than failing late and confusingly. That case is the one exception to "all of them, or none": the offending migration's statements and every earlier ledger row are already committed by the time the guard sees anything. + +- **`DB.transaction` no longer replaces a callback's own error with a transaction one.** Its `commit` and `rollback` are guarded on `autocommit` now. A callback that closed the transaction itself had `rollback()` find nothing to unwind, and sqlite threw `cannot rollback - no transaction is active` over whatever actually went wrong, so the caller saw a message about transactions in place of the real cause. Closing it from inside is still a mistake; the guard stops that mistake from hiding the next one. The tracking table is created before the savepoint, so a run that owns its transaction and fails still leaves somewhere to record the retry. A host that wrapped the call itself and rolls back takes the table with it, which is harmless because every entry point creates it again. The schema cache is cleared on the rollback path too, so nothing cached during an undone migration survives to describe schema that no longer exists. diff --git a/doc/database/migrations.md b/doc/database/migrations.md index 91c51ec..00ace88 100644 --- a/doc/database/migrations.md +++ b/doc/database/migrations.md @@ -118,7 +118,9 @@ await Migrator().run([...]); // the migrator's savepoin await DB.transaction(() => Migrator().run([...])); // nested in yours ``` -**A migration must not manage its own transaction.** `DB.beginTransaction`, `DB.commit` and `DB.rollback` are a supported pattern elsewhere and are not available inside `up()`: the run is already one unit. A `commit()` there closes the migrator's savepoint, which used to mean every later migration ran unprotected and the run reported a failure after fully succeeding. `run()` detects it now and throws naming the migration. +**A migration must not manage its own transaction.** `DB.beginTransaction`, `DB.commit` and `DB.rollback` are a supported pattern elsewhere and are not available inside `up()`: the run is already one unit. A `commit()` there closes the migrator's savepoint, which used to mean every later migration ran unprotected and the run reported a failure after fully succeeding. `run()` detects it and throws naming the migration. + +That one case is the exception to "all of them, or none": by the time the guard sees anything, the offending migration's statements and every earlier ledger row are already committed and there is nothing left to unwind. Nothing can recover that, which is why the rule exists rather than a workaround. The tracking table is created before the savepoint, so a run that owns its own transaction and fails still leaves somewhere to record the retry. A host that wrapped the call in its own transaction and rolls back takes the table with it, which is harmless: every entry point creates it again. diff --git a/lib/src/database/migrations/migrator.dart b/lib/src/database/migrations/migrator.dart index 4dfa0b7..40d5552 100644 --- a/lib/src/database/migrations/migrator.dart +++ b/lib/src/database/migrations/migrator.dart @@ -111,6 +111,14 @@ class Migrator { /// there. A `DB.beginTransaction` inside `up` throws from sqlite instead, /// which is self-describing and unwinds through the same rollback. /// + /// **That one case is early and named but NOT atomic**, and the headline + /// above does not hold for it: the offending migration's own statements and + /// every earlier migration's ledger row are already committed by the time + /// the guard sees anything, so there is nothing left to unwind. Nothing can + /// recover that, which is the whole reason a migration must not do it. A + /// migration in this shape that is not written with `IF NOT EXISTS` will + /// still boot-loop on retry. + /// /// The tracking table is created BEFORE the savepoint, so an owned run that /// fails still leaves somewhere to record the retry. A host that wrapped the /// call in its own transaction and rolls back takes the table with it; that @@ -165,8 +173,14 @@ class Migrator { _db.connection.execute('RELEASE $_savepoint'); } - // Anything cached during the undone migrations describes schema that no - // longer exists. + // Anything cached during the undone migrations would describe schema + // that no longer exists. No case reaches it today and the line stays + // anyway: `up()` is synchronous while every cache-populating API is a + // future, so the only entry the cache can hold mid-run is + // `magic_migrations`, which survives the rollback. It costs one map + // clear and stops being a no-op the day a synchronous introspection + // helper lands. Deliberately untested rather than tested vacuously: a + // test for it passes with the line deleted. _db.clearSchemaCache(); rethrow; diff --git a/lib/src/facades/db.dart b/lib/src/facades/db.dart index 722f1d6..0c38cca 100644 --- a/lib/src/facades/db.dart +++ b/lib/src/facades/db.dart @@ -180,14 +180,27 @@ class DB { /// await DB.table('profiles').insert({...}); /// }); /// ``` + /// Both the commit and the rollback are guarded on [CommonDatabase.autocommit], + /// which is true exactly when no transaction is open. + /// + /// A callback that closed the transaction itself used to have its own error + /// replaced: `rollback()` found nothing to unwind and sqlite threw `cannot + /// rollback - no transaction is active` over whatever actually went wrong. + /// The caller then saw a message about transactions in place of the real + /// cause, which is the one thing an error path must not do. A callback that + /// committed and then succeeded had the mirror of it on the other branch. + /// + /// Closing the transaction from inside the callback is still a mistake, and + /// the guard does not make it one less. It stops that mistake from hiding + /// the next one. static Future transaction(Future Function() callback) async { beginTransaction(); try { final result = await callback(); - commit(); + if (!_db.connection.autocommit) commit(); return result; } catch (e) { - rollback(); + if (!_db.connection.autocommit) rollback(); rethrow; } } diff --git a/test/database/migrator_atomicity_test.dart b/test/database/migrator_atomicity_test.dart index 3dfb830..a713047 100644 --- a/test/database/migrator_atomicity_test.dart +++ b/test/database/migrator_atomicity_test.dart @@ -229,6 +229,31 @@ void main() { }, ); + test( + 'is named even when the host wrapped the call in its own transaction', + () async { + // The diagnostic used to be destroyed in exactly the shape this PR + // tells hosts they may use. The migration's `commit()` closes the + // HOST's transaction too, so `DB.transaction`'s catch found nothing to + // roll back and sqlite threw `cannot rollback - no transaction is + // active` over the StateError naming the migration. + await expectLater( + DB.transaction( + () => Migrator().run([ + _CommittingMigration('wrappedcommit'), + ]), + ), + throwsA( + isA().having( + (StateError e) => e.message, + 'message', + contains('wrappedcommit'), + ), + ), + ); + }, + ); + test('does not leave a later migration running unprotected', () async { await expectLater( Migrator().run([ @@ -243,18 +268,4 @@ void main() { expect(exists('after_a'), isFalse); }); }); - - test('a rollback clears the schema cache too', () async { - // It ran only on the success path, so a column list cached during an - // undone migration stayed in the manager describing schema that no longer - // exists. - await expectLater( - Migrator().run([ - _TwoStepMigration('cachefail', throwsAfterFirst: true), - ]), - throwsA(isA()), - ); - - expect(await DatabaseManager().getColumns('cachefail_a'), isEmpty); - }); } From 1ffa383b25dbadf13591739519c447bae2bd4e83 Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sun, 20 Sep 2026 12:43:59 +0300 Subject: [PATCH 5/6] fix(database): the transaction guard was asymmetric the wrong way Kodizm review round 3. The Major is right and I missed it. Guarding the catch branch protects the callback's real error, which is what round 2 asked for. Guarding the success branch the same way does the opposite: there is no error to protect there, so it converts a loud failure into silence. A callback that commits half way and keeps writing ran everything after that point outside any transaction, and returning normally told the caller the block was atomic when it was not. That is the class of defect the rest of this PR exists to remove, and Migrator.run already treats the identical situation as an error worth naming the culprit for. The success branch throws now, naming what happened and what to do instead. The catch branch still skips, for the reason it was added. Two minors, both correct. doc/database/getting-started.md still said the entire transaction is rolled back on any exception, which is now conditional, and its Manual Transaction Control section is exactly the pattern that produces the guarded case. And [CommonDatabase.autocommit] could not resolve as a dartdoc link because the symbol is not in scope in either file. Three new tests: the self-committing callback is an error, an ordinary callback still commits and returns its value, an ordinary failure still rolls back. The mutation that never throws turns the first red. Cross-checked against the consumer this came from: its migration suite is green against this branch. --- CHANGELOG.md | 6 ++- doc/database/getting-started.md | 9 +++- lib/src/database/migrations/migrator.dart | 2 +- lib/src/facades/db.dart | 41 ++++++++++++------ test/database/migrator_atomicity_test.dart | 50 ++++++++++++++++++++++ 5 files changed, 93 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ba9de8..e800e4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,11 @@ All notable changes to this project will be documented in this file. **BREAKING: a migration may no longer manage its own transaction.** `DB.beginTransaction`, `commit` and `rollback` are a documented pattern elsewhere, so a migration written that way was legitimate before this. A `commit()` inside `up()` closes the migrator's savepoint, which meant every later migration ran unprotected and the closing `RELEASE` threw AFTER every migration had succeeded and committed its ledger row: the caller saw a failure from a run that fully worked, and the retry found nothing pending. `run` detects it and throws naming the migration that broke the contract, rather than failing late and confusingly. That case is the one exception to "all of them, or none": the offending migration's statements and every earlier ledger row are already committed by the time the guard sees anything. -- **`DB.transaction` no longer replaces a callback's own error with a transaction one.** Its `commit` and `rollback` are guarded on `autocommit` now. A callback that closed the transaction itself had `rollback()` find nothing to unwind, and sqlite threw `cannot rollback - no transaction is active` over whatever actually went wrong, so the caller saw a message about transactions in place of the real cause. Closing it from inside is still a mistake; the guard stops that mistake from hiding the next one. +- **BREAKING: `DB.transaction` answers a callback that closes the transaction itself, instead of failing confusingly or quietly.** The two branches differ on purpose. + + On failure the rollback is skipped. There is a real error in flight and the only thing that matters is that it reaches the caller: `rollback()` would find nothing to unwind and throw `cannot rollback - no transaction is active` over the top, so the caller read a message about transactions in place of the cause. + + On success it now throws, naming what happened. Skipping the commit the same way would be the worse bug: there is no error to protect on that branch, so silence buys nothing and costs the signal. A callback that commits half way and keeps writing ran everything after that point outside any transaction, and returning normally tells the caller the block was atomic when it was not. The tracking table is created before the savepoint, so a run that owns its transaction and fails still leaves somewhere to record the retry. A host that wrapped the call itself and rolls back takes the table with it, which is harmless because every entry point creates it again. The schema cache is cleared on the rollback path too, so nothing cached during an undone migration survives to describe schema that no longer exists. diff --git a/doc/database/getting-started.md b/doc/database/getting-started.md index f396e00..d8ecbd3 100644 --- a/doc/database/getting-started.md +++ b/doc/database/getting-started.md @@ -189,10 +189,17 @@ await DB.transaction(() async { If any operation throws an exception, the entire transaction is rolled back. +**Do not close the transaction from inside the callback.** `DB.transaction` owns it, and a `DB.commit()` or `DB.rollback()` in there means everything written afterwards ran outside any transaction. The two cases are answered differently and neither is silent: + +- On failure the rollback is skipped, so your error reaches you rather than being replaced by `cannot rollback - no transaction is active`. +- On success it throws, naming what happened. There is no error to protect on that branch, so staying quiet would only cost you the signal: the block was not atomic and returning normally would say it was. + +If you need that control, use the manual API below throughout rather than mixing the two. + ### Manual Transaction Control -For cases where you need finer control over transaction boundaries, call `DB.beginTransaction()`, `DB.commit()`, and `DB.rollback()` directly. This is useful when you need to interleave non-database work between statements or handle multiple error branches differently. +For cases where you need finer control over transaction boundaries, call `DB.beginTransaction()`, `DB.commit()`, and `DB.rollback()` directly. This is useful when you need to interleave non-database work between statements or handle multiple error branches differently. It is an alternative to `DB.transaction`, not something to use inside one. ```dart DB.beginTransaction(); diff --git a/lib/src/database/migrations/migrator.dart b/lib/src/database/migrations/migrator.dart index 40d5552..32fc386 100644 --- a/lib/src/database/migrations/migrator.dart +++ b/lib/src/database/migrations/migrator.dart @@ -93,7 +93,7 @@ class Migrator { /// start a transaction within a transaction` at it. Verified in all three /// shapes: with no transaction open, inside one, and unwinding through /// `ROLLBACK TO`. The alternative was branching on - /// [CommonDatabase.autocommit], which works and leaves the host's shape + /// the connection's `autocommit`, which works and leaves the host's shape /// deciding which code path runs. /// /// ### A migration must not manage its own transaction diff --git a/lib/src/facades/db.dart b/lib/src/facades/db.dart index 0c38cca..c10e542 100644 --- a/lib/src/facades/db.dart +++ b/lib/src/facades/db.dart @@ -180,24 +180,41 @@ class DB { /// await DB.table('profiles').insert({...}); /// }); /// ``` - /// Both the commit and the rollback are guarded on [CommonDatabase.autocommit], - /// which is true exactly when no transaction is open. + /// **A callback must not close the transaction itself**, and the two branches + /// answer that differently on purpose. /// - /// A callback that closed the transaction itself used to have its own error - /// replaced: `rollback()` found nothing to unwind and sqlite threw `cannot - /// rollback - no transaction is active` over whatever actually went wrong. - /// The caller then saw a message about transactions in place of the real - /// cause, which is the one thing an error path must not do. A callback that - /// committed and then succeeded had the mirror of it on the other branch. + /// On failure the rollback is skipped. There is a real error in flight and + /// the only thing that matters is that it reaches the caller: `rollback()` + /// would find nothing to unwind and throw `cannot rollback - no transaction + /// is active` over the top, so the caller would read a message about + /// transactions in place of the cause. /// - /// Closing the transaction from inside the callback is still a mistake, and - /// the guard does not make it one less. It stops that mistake from hiding - /// the next one. + /// On success it throws instead, and skipping the commit the same way would + /// be the worse bug. There is no error to protect here, so silence buys + /// nothing and costs the signal: a callback that commits half way and keeps + /// writing ran everything after that point outside any transaction, and + /// returning normally tells the caller the block was atomic when it was not. + /// `Migrator.run` treats the identical situation as an error worth naming + /// the culprit for, and this is the same situation one layer down. + /// + /// `autocommit` on the connection is true exactly when no transaction is + /// open, which is the only thing that can tell either case apart. static Future transaction(Future Function() callback) async { beginTransaction(); try { final result = await callback(); - if (!_db.connection.autocommit) commit(); + + if (_db.connection.autocommit) { + throw StateError( + 'The transaction callback committed or rolled back the transaction ' + 'itself. Anything it wrote afterwards ran outside a transaction and ' + 'is not covered by this block. Remove the DB.commit or DB.rollback, ' + 'or stop using DB.transaction and manage it manually throughout.', + ); + } + + commit(); + return result; } catch (e) { if (!_db.connection.autocommit) rollback(); diff --git a/test/database/migrator_atomicity_test.dart b/test/database/migrator_atomicity_test.dart index a713047..1794656 100644 --- a/test/database/migrator_atomicity_test.dart +++ b/test/database/migrator_atomicity_test.dart @@ -268,4 +268,54 @@ void main() { expect(exists('after_a'), isFalse); }); }); + + group('DB.transaction and a callback that closes the transaction itself', () { + test( + 'a callback that commits half way is an error, not a silent success', + () async { + // The asymmetry this closes. Guarding the catch branch protects the + // callback's real error, which is what round 2 asked for. Guarding the + // success branch the same way turns a loud failure into silence: the + // writes after that `commit()` ran outside any transaction and the + // caller would be told the block was atomic. + await expectLater( + DB.transaction(() async { + DB.statement('CREATE TABLE IF NOT EXISTS inside_a (x TEXT)'); + DB.commit(); + DB.statement('CREATE TABLE IF NOT EXISTS inside_b (x TEXT)'); + }), + throwsA( + isA().having( + (StateError e) => e.message, + 'message', + contains('transaction'), + ), + ), + ); + }, + ); + + test('an ordinary callback still commits and returns its value', () async { + final int answer = await DB.transaction(() async { + DB.statement('CREATE TABLE IF NOT EXISTS ordinary (x TEXT)'); + + return 7; + }); + + expect(answer, 7); + expect(exists('ordinary'), isTrue); + }); + + test('an ordinary failure still rolls back', () async { + await expectLater( + DB.transaction(() async { + DB.statement('CREATE TABLE IF NOT EXISTS undone (x TEXT)'); + throw StateError('nope'); + }), + throwsA(isA()), + ); + + expect(exists('undone'), isFalse); + }); + }); } From 6fa49206b0c14083c64a3f3cf65573c003d3bf43 Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sun, 20 Sep 2026 12:54:56 +0300 Subject: [PATCH 6/6] docs(database): the skills mirror still described the old DB.transaction Kodizm round 4, one minor and it is a sync step I owed rather than a judgement call. facades-api.md said 'Auto-commit on success, auto-rollback on error', which is now wrong in both directions: on success a callback that closed the transaction gets a StateError rather than a commit, and on failure the rollback is conditional. CLAUDE.md's post-change sync asks for the skills mirror when a facade API changes; round 3 reached doc/database but not this. SKILL.md bumped with it. --- skills/magic-framework/SKILL.md | 2 +- skills/magic-framework/references/facades-api.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/skills/magic-framework/SKILL.md b/skills/magic-framework/SKILL.md index 77a1f8d..d587123 100644 --- a/skills/magic-framework/SKILL.md +++ b/skills/magic-framework/SKILL.md @@ -2,7 +2,7 @@ name: magic-framework description: "Write correct, idiomatic code in a Flutter app that depends on the `magic` framework (Laravel-inspired: IoC container, 18 facades, Eloquent-style ORM, service providers, reactive controllers, GoRouter routing, validation, auth, broadcasting). Use whenever code imports `package:magic/magic.dart` or `package:magic/testing.dart`, or the work touches Magic.init, MagicApp, a facade (Auth/Http/Cache/DB/Echo/Event/Gate/Config/Lang/Launch/Log/Pick/MagicRoute/Schema/Session/Storage/Vault/Crypt), a Model, MagicController, a MagicView, MagicFormData, FormRequest, a ServiceProvider, a migration, or the artisan make:* CLI. UI styling is Wind (separate wind-ui skill). Do NOT use for plain Flutter or Wind-only work with no magic import." when_to_use: "Use proactively when editing or scaffolding a magic app: Magic.init / a facade / a Model / a MagicController or MagicView / a form (MagicFormData, FormRequest, Validator) / a ServiceProvider / a route or MagicMiddleware / a migration / MagicStateMixin + RxStatus + fetchList / Session flash + old() + trans() / testing with MagicTest + Http.fake/Auth.fake / the artisan make:* CLI / the magic_deeplink, magic_notifications, magic_social_auth, magic_starter, magic_payments, or magic_devtools plugins. Trigger even when the user does not say the word 'magic'. Do NOT trigger for plain Flutter or Wind-only UI with no package:magic import." -version: 0.1.33 +version: 0.1.34 --- diff --git a/skills/magic-framework/references/facades-api.md b/skills/magic-framework/references/facades-api.md index 2cdc310..04d38a0 100644 --- a/skills/magic-framework/references/facades-api.md +++ b/skills/magic-framework/references/facades-api.md @@ -185,7 +185,7 @@ Resolves `Magic.make('db')`. | `DB.beginTransaction()` | `void` | Begin a manual transaction. | | `DB.commit()` | `void` | Commit the current transaction. | | `DB.rollback()` | `void` | Roll back the current transaction. | -| `DB.transaction(Future Function() callback)` | `Future` | Auto-commit on success, auto-rollback on error. | +| `DB.transaction(Future Function() callback)` | `Future` | Commits on success, rolls back on error. The callback must NOT call `DB.commit` / `DB.rollback` itself: on success that throws a `StateError` (the writes after it were not in the transaction), and on failure the rollback is skipped so the callback's own error still reaches you. | ```dart import 'package:magic/magic.dart';