Reraise errors when running background migrations inline

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## master (unreleased)
 
+- Reraise errors when running background migrations inline
 - Add `remove_background_migration` migration helper
 - Allow adding bigint foreign keys referencing integer primary keys
 - Fix `add_reference_concurrently` to check for mismatched key types

lib/online_migrations/background_migrations/delete_associated_records.rb

@@ -21,7 +21,7 @@ def relation
       end
 
       def process_batch(relation)
-        relation.delete_all(:delete_all)
+        relation.delete_all
       end
     end
   end

lib/online_migrations/background_migrations/migration_helpers.rb

@@ -372,8 +372,7 @@ def perform_action_on_relation_in_background(model_name, conditions, action, upd
       def enqueue_background_migration(migration_name, *arguments, **options)
         migration = create_background_migration(migration_name, *arguments, **options)
 
-        run_inline = OnlineMigrations.config.run_background_migrations_inline
-        if run_inline && run_inline.call
+        if Utils.run_background_migrations_inline?
           runner = MigrationRunner.new(migration)
           runner.run_all_migration_jobs
         end

lib/online_migrations/background_migrations/migration_job_runner.rb

@@ -46,6 +46,7 @@ def run
         )
 
         ::OnlineMigrations.config.background_migrations.error_handler.call(e, migration_job)
+        raise if Utils.run_background_migrations_inline?
       end
 
       private

lib/online_migrations/utils.rb

@@ -151,6 +151,11 @@ def multiple_databases?
         db_config = ActiveRecord::Base.configurations.configs_for(env_name: env)
         db_config.reject(&:replica?).size > 1
       end
+
+      def run_background_migrations_inline?
+        run_inline = OnlineMigrations.config.run_background_migrations_inline
+        run_inline && run_inline.call
+      end
     end
   end
 end

test/background_migrations/delete_associated_records_test.rb

@@ -5,7 +5,7 @@
 module BackgroundMigrations
   class DeleteAssociatedRecordsTest < Minitest::Test
     class Link < ActiveRecord::Base
-      has_many :clicks
+      has_many :clicks, dependent: :delete_all
     end
 
     class Click < ActiveRecord::Base

test/background_migrations/migration_job_runner_test.rb

@@ -41,7 +41,10 @@ def test_run_saves_error_when_failed
 
       job = create_migration_job(migration_name: "EachBatchFails")
 
-      run_migration_job(job)
+      error = assert_raises(RuntimeError) do
+        run_migration_job(job)
+      end
+      assert_equal "Boom!", error.message
 
       assert job.failed?
       assert job.finished_at.present?
@@ -63,14 +66,43 @@ def test_run_calls_error_handler_when_failed
         handled_job = errored_job
       end
 
-      run_migration_job(job)
+      error = assert_raises(RuntimeError) do
+        run_migration_job(job)
+      end
+      assert_equal "Boom!", error.message
 
       assert_instance_of RuntimeError, handled_error
       assert_equal job, handled_job
     ensure
       OnlineMigrations.config.background_migrations.error_handler = previous_error_handler
     end
 
+    def test_run_reraises_error_when_running_background_migrations_inline
+      _user = User.create!
+      job = create_migration_job(migration_name: "EachBatchFails")
+
+      prev = OnlineMigrations.config.run_background_migrations_inline
+      OnlineMigrations.config.run_background_migrations_inline = -> { true }
+
+      error = assert_raises(RuntimeError) do
+        run_migration_job(job)
+      end
+      assert_equal "Boom!", error.message
+    ensure
+      OnlineMigrations.config.run_background_migrations_inline = prev
+    end
+
+    def test_run_do_not_reraise_error_when_running_background_migrations_in_background
+      _user = User.create!
+      job = create_migration_job(migration_name: "EachBatchFails")
+
+      OnlineMigrations.config.stub(:run_background_migrations_inline, nil) do
+        assert_nothing_raised do
+          run_migration_job(job)
+        end
+      end
+    end
+
     def test_active_support_instrumentation
       2.times { User.create! }
       job = create_migration_job(migration_name: "FailingBatch", batch_size: 1, sub_batch_size: 1)
@@ -87,7 +119,11 @@ def test_active_support_instrumentation
         assert_kind_of OnlineMigrations::BackgroundMigrations::MigrationJob, payload[:background_migration_job]
       end
 
-      run_migration_job(job)
+      error = assert_raises(RuntimeError) do
+        run_migration_job(job)
+      end
+      assert_equal "Boom!", error.message
+
       assert_equal 0, retry_called
       assert_equal 1, process_batch_called
 

test/background_migrations/migration_job_test.rb

@@ -59,7 +59,10 @@ def test_copies_settings_from_background_migration
     def test_retry
       User.create!
       m = create_migration(migration_name: "EachBatchFails")
-      run_migration_job(m)
+      error = assert_raises(RuntimeError) do
+        run_migration_job(m)
+      end
+      assert_equal "Boom!", error.message
       j = m.last_job
 
       j.retry

test/schema_statements/misc_test.rb

@@ -9,7 +9,7 @@ class User < ActiveRecord::Base
     end
 
     class Invoice < ActiveRecord::Base
-      belongs_to :user
+      belongs_to :user, counter_cache: true
     end
 
     def setup
@@ -19,6 +19,7 @@ def setup
         t.string :status
         t.boolean :admin
         t.boolean :banned
+        t.integer :invoices_count
         t.bigint :id_for_type_change
         t.string :name_for_type_change
       end
@@ -128,10 +129,10 @@ def test_copy_columns_in_background_raises_for_multiple_dbs_when_no_model_name
     end
 
     def test_reset_counters_in_background
-      m = @connection.reset_counters_in_background(User.name, :projects, :friends, touch: true)
+      m = @connection.reset_counters_in_background(User.name, :invoices, touch: true)
 
       assert_equal "ResetCounters", m.migration_name
-      assert_equal [User.name, ["projects", "friends"], { "touch" => true }], m.arguments
+      assert_equal [User.name, ["invoices"], { "touch" => true }], m.arguments
     end
 
     def test_delete_orphaned_records_in_background
@@ -195,12 +196,14 @@ def test_run_background_migrations_inline_configured_to_custom_proc
       user = User.create!
       assert_nil user.admin
 
-      m = OnlineMigrations.config.stub(:run_background_migrations_inline, -> { false }) do
-        @connection.enqueue_background_migration("MakeAllNonAdmins")
-      end
+      prev = OnlineMigrations.config.run_background_migrations_inline
+      OnlineMigrations.config.run_background_migrations_inline = -> { false }
 
+      m = @connection.enqueue_background_migration("MakeAllNonAdmins")
       assert m.enqueued?
       assert_nil user.reload.admin
+    ensure
+      OnlineMigrations.config.run_background_migrations_inline = prev
     end
 
     def test_remove_non_existing_background_migration