Simplify background migrations stealing code

Simply re-raise an exception when it occurs, but guarantee that no
background migration is lost in the process.

2 files changed, 12 insertions, 74 deletions

diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb
index 3586c6a2560..b0741b1fba7 100644
--- a/lib/gitlab/background_migration.rb
+++ b/lib/gitlab/background_migration.rb
@@ -27,9 +27,7 @@ module Gitlab
 
           begin
             perform(migration_class, migration_args, retries: 3) if job.delete
-          rescue StandardError
-            next
-          rescue Exception
+          rescue Exception # rubocop:disable Lint/RescueException
             BackgroundMigrationWorker # enqueue this migration again
               .perform_async(migration_class, migration_args)
 
@@ -40,25 +38,15 @@ module Gitlab
     end
 
     ##
-    # Performs a background migration. In case of `StandardError` being caught
-    # this will retry a migration up to three times.
+    # Performs a background migration.
     #
     # class_name - The name of the background migration class as defined in the
     #              Gitlab::BackgroundMigration namespace.
     #
     # arguments - The arguments to pass to the background migration's "perform"
     #             method.
-    def self.perform(class_name, arguments, retries: 0)
+    def self.perform(class_name, arguments)
       const_get(class_name).new.perform(*arguments)
-    rescue StandardError
-      if retries > 0
-        Rails.logger.warn("Retrying background migration #{class_name} " \
-                          "with #{arguments}")
-        retries -= 1
-        retry
-      else
-        raise
-      end
     end
   end
 end
diff --git a/spec/lib/gitlab/background_migration_spec.rb b/spec/lib/gitlab/background_migration_spec.rb
index 64f59c72cec..cfa59280139 100644
--- a/spec/lib/gitlab/background_migration_spec.rb
+++ b/spec/lib/gitlab/background_migration_spec.rb
@@ -62,32 +62,14 @@ describe Gitlab::BackgroundMigration do
           allow(queue[1]).to receive(:delete).and_return(true)
         end
 
-        context 'when standard error is being raised' do
-          it 'recovers from an exception and retries the migration' do
-            expect(migration).to receive(:perform).with(10, 20)
-              .and_raise(StandardError, 'Migration error')
-              .exactly(4).times.ordered
-            expect(migration).to receive(:perform).with(20, 30)
-              .once.ordered
-            expect(Rails.logger).to receive(:warn)
-              .with(/Retrying background migration/).exactly(3).times
-
-            described_class.steal('Foo')
-          end
-        end
-
-        context 'when top level exception is being raised' do
-          it 'enqueues the migration again and reraises the error' do
-            allow(migration).to receive(:perform).with(10, 20)
-              .and_raise(Exception, 'Migration error').once
+        it 'enqueues the migration again and re-raises the error' do
+          allow(migration).to receive(:perform).with(10, 20)
+            .and_raise(Exception, 'Migration error').once
 
-            expect(BackgroundMigrationWorker).to receive(:perform_async)
-              .with('Foo', [10, 20]).once
+          expect(BackgroundMigrationWorker).to receive(:perform_async)
+            .with('Foo', [10, 20]).once
 
-            expect(Rails.logger).not_to receive(:warn)
-            expect { described_class.steal('Foo') }
-              .to raise_error(Exception)
-          end
+          expect { described_class.steal('Foo') }.to raise_error(Exception)
         end
       end
     end
@@ -131,42 +113,10 @@ describe Gitlab::BackgroundMigration do
       stub_const("#{described_class.name}::Foo", migration)
     end
 
-    context 'when retries count is not specified' do
-      it 'performs a background migration' do
-        expect(migration).to receive(:perform).with(10, 20).once
-
-        described_class.perform('Foo', [10, 20])
-      end
-    end
-
-    context 'when retries count is zero' do
-      it 'perform a background migration only once' do
-        expect(migration).to receive(:perform).with(10, 20)
-          .and_raise(StandardError).once
+    it 'performs a background migration' do
+      expect(migration).to receive(:perform).with(10, 20).once
 
-        expect { described_class.perform('Foo', [10, 20], retries: 0) }
-          .to raise_error(StandardError)
-      end
-    end
-
-    context 'when retries count is one' do
-      it 'retries a background migration when needed' do
-        expect(migration).to receive(:perform).with(10, 20)
-          .and_raise(StandardError).twice
-
-        expect { described_class.perform('Foo', [10, 20], retries: 1) }
-          .to raise_error(StandardError)
-      end
-    end
-
-    context 'when retries count is larger than zero' do
-      it 'retries a background migration when needed' do
-        expect(migration).to receive(:perform).with(10, 20)
-          .and_raise(StandardError).exactly(4).times
-
-        expect { described_class.perform('Foo', [10, 20], retries: 3) }
-          .to raise_error(StandardError)
-      end
+      described_class.perform('Foo', [10, 20])
     end
   end
 end