Avoid race condition when stealing a background migration

We first pop a job from the Sidekiq queue / scheduled set and only if
this has been successfully deleted we process the job. This makes it
possible to minimize a possibility of a race condition happening.

2 files changed, 10 insertions, 4 deletions

diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb
index e0325c0138e..73d2611e51f 100644
--- a/lib/gitlab/background_migration.rb
+++ b/lib/gitlab/background_migration.rb
@@ -19,9 +19,7 @@ module Gitlab
           next unless job.queue == self.queue
           next unless migration_class == steal_class
 
-          perform(migration_class, migration_args)
-
-          job.delete
+          perform(migration_class, migration_args) if job.delete
         end
       end
     end
diff --git a/spec/lib/gitlab/background_migration_spec.rb b/spec/lib/gitlab/background_migration_spec.rb
index 2f3e7aba1f4..c12040cba14 100644
--- a/spec/lib/gitlab/background_migration_spec.rb
+++ b/spec/lib/gitlab/background_migration_spec.rb
@@ -23,13 +23,21 @@ describe Gitlab::BackgroundMigration do
       end
 
       it 'steals jobs from a queue' do
-        expect(queue[0]).to receive(:delete)
+        expect(queue[0]).to receive(:delete).and_return(true)
 
         expect(described_class).to receive(:perform).with('Foo', [10, 20])
 
         described_class.steal('Foo')
       end
 
+      it 'does not steal job that has already been taken' do
+        expect(queue[0]).to receive(:delete).and_return(false)
+
+        expect(described_class).not_to receive(:perform).with('Foo', [10, 20])
+
+        described_class.steal('Foo')
+      end
+
       it 'does not steal jobs for a different migration' do
         expect(described_class).not_to receive(:perform)