From 39b96f02dca3d6edac40b94bf003e6735c4c2524 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 14 Jul 2017 12:48:11 +0200 Subject: [PATCH] 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. --- lib/gitlab/background_migration.rb | 4 +--- spec/lib/gitlab/background_migration_spec.rb | 10 +++++++++- 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)