Fixed #29721 -- Ensured migrations are applied and recorded atomically.

django/db/migrations/executor.py

@@ -230,6 +230,7 @@ class MigrationExecutor:
 
     def apply_migration(self, state, migration, fake=False, fake_initial=False):
         """Run a migration forwards."""
+        migration_recorded = False
         if self.progress_callback:
             self.progress_callback("apply_start", migration, fake)
         if not fake:
@@ -242,16 +243,22 @@ class MigrationExecutor:
                 # Alright, do it normally
                 with self.connection.schema_editor(atomic=migration.atomic) as schema_editor:
                     state = migration.apply(state, schema_editor)
+                    self.record_migration(migration)
+                    migration_recorded = True
+        if not migration_recorded:
+            self.record_migration(migration)
+        # Report progress
+        if self.progress_callback:
+            self.progress_callback("apply_success", migration, fake)
+        return state
+
+    def record_migration(self, migration):
         # For replacement migrations, record individual statuses
         if migration.replaces:
             for app_label, name in migration.replaces:
                 self.recorder.record_applied(app_label, name)
         else:
             self.recorder.record_applied(migration.app_label, migration.name)
-        # Report progress
-        if self.progress_callback:
-            self.progress_callback("apply_success", migration, fake)
-        return state
 
     def unapply_migration(self, state, migration, fake=False):
         """Run a migration backwards."""

tests/migrations/test_executor.py

@@ -1,3 +1,5 @@
+from unittest import mock
+
 from django.apps.registry import apps as global_apps
 from django.db import connection
 from django.db.migrations.exceptions import InvalidMigrationPlan
@@ -5,7 +7,9 @@ from django.db.migrations.executor import MigrationExecutor
 from django.db.migrations.graph import MigrationGraph
 from django.db.migrations.recorder import MigrationRecorder
 from django.db.utils import DatabaseError
-from django.test import TestCase, modify_settings, override_settings
+from django.test import (
+    TestCase, modify_settings, override_settings, skipUnlessDBFeature,
+)
 
 from .test_base import MigrationTestBase
 
@@ -649,6 +653,22 @@ class ExecutorTests(MigrationTestBase):
             recorder.applied_migrations(),
         )
 
+    # When the feature is False, the operation and the record won't be
+    # performed in a transaction and the test will systematically pass.
+    @skipUnlessDBFeature('can_rollback_ddl')
+    @override_settings(MIGRATION_MODULES={'migrations': 'migrations.test_migrations'})
+    def test_migrations_applied_and_recorded_atomically(self):
+        """Migrations are applied and recorded atomically."""
+        executor = MigrationExecutor(connection)
+        with mock.patch('django.db.migrations.executor.MigrationExecutor.record_migration') as record_migration:
+            record_migration.side_effect = RuntimeError('Recording migration failed.')
+            with self.assertRaisesMessage(RuntimeError, 'Recording migration failed.'):
+                executor.migrate([('migrations', '0001_initial')])
+        # The migration isn't recorded as applied since it failed.
+        migration_recorder = MigrationRecorder(connection)
+        self.assertFalse(migration_recorder.migration_qs.filter(app='migrations', name='0001_initial').exists())
+        self.assertTableNotExists('migrations_author')
+
 
 class FakeLoader:
     def __init__(self, graph, applied):