Fix off-by-one error in background migration retriesfix/gb/process-scheduled-background-migrations

2 files changed, 25 insertions, 14 deletions

diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb
index cb0ac9221a0..3586c6a2560 100644
--- a/lib/gitlab/background_migration.rb
+++ b/lib/gitlab/background_migration.rb
@@ -48,12 +48,17 @@ module Gitlab
     #
     # arguments - The arguments to pass to the background migration's "perform"
     #             method.
-    def self.perform(class_name, arguments, retries: 1)
+    def self.perform(class_name, arguments, retries: 0)
       const_get(class_name).new.perform(*arguments)
-    rescue => e
-      Rails.logger.warn("Retrying background migration #{class_name} " \
-                        "with #{arguments}")
-      (retries -= 1) > 0 ? retry : raise
+    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 9c6b07f02cc..64f59c72cec 100644
--- a/spec/lib/gitlab/background_migration_spec.rb
+++ b/spec/lib/gitlab/background_migration_spec.rb
@@ -63,14 +63,10 @@ describe Gitlab::BackgroundMigration do
         end
 
         context 'when standard error is being raised' do
-          before do
-            allow(migration).to receive(:perform).with(10, 20)
-              .and_raise(StandardError, 'Migration error')
-          end
-
           it 'recovers from an exception and retries the migration' do
             expect(migration).to receive(:perform).with(10, 20)
-              .exactly(3).times.ordered
+              .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)
@@ -148,7 +144,17 @@ describe Gitlab::BackgroundMigration do
         expect(migration).to receive(:perform).with(10, 20)
           .and_raise(StandardError).once
 
-        expect {  described_class.perform('Foo', [10, 20], retries: 0) }
+        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
@@ -156,9 +162,9 @@ describe Gitlab::BackgroundMigration do
     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(3).times
+          .and_raise(StandardError).exactly(4).times
 
-        expect {  described_class.perform('Foo', [10, 20], retries: 3) }
+        expect { described_class.perform('Foo', [10, 20], retries: 3) }
           .to raise_error(StandardError)
       end
     end