The migrator was not atomic, and could record nothing at all

CHANGELOG.md

@@ -33,6 +33,26 @@ 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 `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. 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.
+
+- **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.
+
+- **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.

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.
+
 <a name="manual-transaction-control"></a>
 ### 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();

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,42 @@ void main() async {
 
 The `Migrator` keeps track of which migrations have already run, so calling `run()` multiple times is safe.
 
+<a name="the-run-is-atomic"></a>
+### 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.
+
+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 savepoint alone
+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 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.
+
+<a name="up-and-down-are-synchronous"></a>
+### `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.
+
+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<String, dynamic> 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.
+
 <a name="creating-tables"></a>
 ## Creating Tables
 

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();
 }

lib/src/database/migrations/migrator.dart

@@ -57,17 +57,72 @@ 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([
   ///   CreateUsersTable(),
   ///   CreatePostsTable(),
   /// ]);
   /// ```
+  ///
+  /// ### 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
+  ///
+  /// 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
+  /// 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
+  ///
+  /// `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.
+  ///
+  /// 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.
+  ///
+  /// **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
+  /// is harmless, because every entry point creates it again.
   Future<List<String>> run(List<Migration> migrations) async {
     // Ensure migrations table exists
     await _ensureMigrationsTable();
@@ -87,25 +142,52 @@ class Migrator {
     // Get next batch number
     _batch = await _getNextBatchNumber();
 
-    // Run each pending migration
+    _db.connection.execute('SAVEPOINT $_savepoint');
+
     final ranMigrations = <String>[];
 
-    for (final migration in pending) {
-      try {
-        // Execute the up method
+    try {
+      for (final migration in pending) {
         migration.up();
 
-        // Record it
-        await _recordMigration(migration.name);
+        // `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 (e) {
-        // Log the error but continue with other migrations
-        // In production, you might want to stop here
-        rethrow;
       }
+    } catch (_) {
+      // `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 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;
     }
 
+    _db.connection.execute('RELEASE $_savepoint');
+
     // Clear schema cache after migrations
     _db.clearSchemaCache();
 
@@ -202,6 +284,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<void> _ensureMigrationsTable() async {
     _db.connection.execute('''
       CREATE TABLE IF NOT EXISTS $_table (
@@ -210,6 +301,8 @@ class Migrator {
         batch INTEGER NOT NULL
       )
     ''');
+
+    _db.clearSchemaCache(_table);
   }
 
   /// Get list of executed migration names.