Catch exceptions when stealing background migrations

2 files changed, 49 insertions, 19 deletions

diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb
index 73d2611e51f..64ddae12f89 100644
--- a/lib/gitlab/background_migration.rb
+++ b/lib/gitlab/background_migration.rb
@@ -19,7 +19,12 @@ module Gitlab
           next unless job.queue == self.queue
           next unless migration_class == steal_class
 
-          perform(migration_class, migration_args) if job.delete
+          begin
+            perform(migration_class, migration_args) if job.delete
+          rescue => e
+            Logger.new($stdout).warn(e.message)
+            next
+          end
         end
       end
     end
diff --git a/spec/lib/gitlab/background_migration_spec.rb b/spec/lib/gitlab/background_migration_spec.rb
index c12040cba14..b28329792eb 100644
--- a/spec/lib/gitlab/background_migration_spec.rb
+++ b/spec/lib/gitlab/background_migration_spec.rb
@@ -10,40 +10,65 @@ describe Gitlab::BackgroundMigration do
 
   describe '.steal' do
     context 'when there are enqueued jobs present' do
-      let(:job) { double(:job, args: ['Foo', [10, 20]]) }
-      let(:queue) { [job] }
+      let(:queue) do
+        [double(args: ['Foo', [10, 20]], queue: described_class.queue)]
+      end
 
       before do
         allow(Sidekiq::Queue).to receive(:new)
           .with(described_class.queue)
           .and_return(queue)
-
-        allow(job).to receive(:queue)
-          .and_return(described_class.queue)
       end
 
-      it 'steals jobs from a queue' do
-        expect(queue[0]).to receive(:delete).and_return(true)
+      context 'when queue contains unprocessed jobs' do
+        it 'steals jobs from a queue' do
+          expect(queue[0]).to receive(:delete).and_return(true)
 
-        expect(described_class).to receive(:perform).with('Foo', [10, 20])
+          expect(described_class).to receive(:perform).with('Foo', [10, 20])
 
-        described_class.steal('Foo')
-      end
+          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 job that has already been taken' do
-        expect(queue[0]).to receive(:delete).and_return(false)
+        it 'does not steal jobs for a different migration' do
+          expect(described_class).not_to receive(:perform)
 
-        expect(described_class).not_to receive(:perform).with('Foo', [10, 20])
+          expect(queue[0]).not_to receive(:delete)
 
-        described_class.steal('Foo')
+          described_class.steal('Bar')
+        end
       end
 
-      it 'does not steal jobs for a different migration' do
-        expect(described_class).not_to receive(:perform)
+      context 'when one of the jobs raises an error' do
+        let(:migration) { spy(:migration) }
+
+        let(:queue) do
+          [double(args: ['Foo', [10, 20]], queue: described_class.queue),
+           double(args: ['Foo', [20, 30]], queue: described_class.queue)]
+        end
+
+        before do
+          stub_const("#{described_class}::Foo", migration)
 
-        expect(queue[0]).not_to receive(:delete)
+          allow(migration).to receive(:perform).with(10, 20)
+            .and_raise(StandardError, 'Migration error')
 
-        described_class.steal('Bar')
+          allow(queue[0]).to receive(:delete).and_return(true)
+          allow(queue[1]).to receive(:delete).and_return(true)
+        end
+
+        it 'recovers from an exceptions and continues' do
+          expect(migration).to receive(:perform).twice
+          expect { described_class.steal('Foo') }
+            .to output(/Migration error/).to_stdout
+        end
       end
     end