Improves queries required to push build to a queue

This also ensures that we handle errors in a better way, if we detect
duplicate records. This commit also adds additional tests to cover
multiple failure scenarios related to managing builds queue.

app/models/ci/build.rb

@@ -305,7 +305,6 @@ module Ci
       end
 
       after_transition pending: any do |build, transition|
-        # TODO ensure that there is no race condition here, add test for this
         Ci::UpdateBuildQueueService.new.pop(build, transition)
       end
       # rubocop:enable CodeReuse/ServiceClass

app/services/ci/update_build_queue_service.rb

@@ -19,8 +19,14 @@ module Ci
       raise InvalidQueueTransition unless transition.to == 'pending'
 
       transition.within_transaction do
-        ::Ci::PendingBuild.create!(build: build, project: build.project).tap do
+        attributes = { build_id: build.id, project_id: build.project.id }
+
+        ::Ci::PendingBuild.upsert(attributes).then do |result|
+          raise ArgumentError if result.length > 1
+
           metrics.increment_queue_operation(:build_queue_push)
+
+          result.rows.dig(0, 0)
         end
       end
     end
@@ -34,13 +40,13 @@ module Ci
       raise InvalidQueueTransition unless transition.from == 'pending'
 
       transition.within_transaction do
-        ::Ci::PendingBuild.find_by(build_id: build.id).tap do |queued| # rubocop:disable CodeReuse/ActiveRecord
-          next unless queued.present?
-
-          queued.destroy!
-
+        ::Ci::PendingBuild.where(build_id: build.id).delete_all.then do |removed| # rubocop:disable CodeReuse/ActiveRecord
+          if removed > 0
             metrics.increment_queue_operation(:build_queue_pop)
           end
+
+          removed > 0
+        end
       end
     end
 

spec/models/ci/build_spec.rb

@@ -353,7 +353,7 @@ RSpec.describe Ci::Build do
         expect(build).to be_pending
       end
 
-      it 'pushed build to a queue' do
+      it 'pushes build to a queue' do
         build.enqueue
 
         expect(build.queuing_entry).to be_present
@@ -365,12 +365,35 @@ RSpec.describe Ci::Build do
         end
 
         it 'does not push build to a queue' do
-          expect { build.enqueue }
+          expect { build.enqueue! }
             .to raise_error(ActiveRecord::StaleObjectError)
 
           expect(build.queuing_entry).not_to be_present
         end
       end
+
+      context 'when there is a queuing entry already present' do
+        before do
+          ::Ci::PendingBuild.create!(build: build, project: build.project)
+        end
+
+        it 'does not raise an error' do
+          expect { build.enqueue! }.not_to raise_error
+          expect(build.reload.queuing_entry).to be_present
+        end
+      end
+
+      context 'when both failure scenario happen at the same time' do
+        before do
+          ::Ci::Build.find(build.id).update_column(:lock_version, 100)
+          ::Ci::PendingBuild.create!(build: build, project: build.project)
+        end
+
+        it 'raises stale object error exception' do
+          expect { build.enqueue! }
+            .to raise_error(ActiveRecord::StaleObjectError)
+        end
+      end
     end
   end
 
@@ -429,6 +452,7 @@ RSpec.describe Ci::Build do
   end
 
   describe '#run' do
+    context 'when build has been just created' do
       let(:build) { create(:ci_build, :created) }
 
       it 'creates queuing entry and then removes it' do
@@ -440,6 +464,23 @@ RSpec.describe Ci::Build do
       end
     end
 
+    context 'when build status transition fails' do
+      let(:build) { create(:ci_build, :pending) }
+
+      before do
+        ::Ci::PendingBuild.create!(build: build, project: build.project)
+        ::Ci::Build.find(build.id).update_column(:lock_version, 100)
+      end
+
+      it 'does not remove build from a queue' do
+        expect { build.run! }
+          .to raise_error(ActiveRecord::StaleObjectError)
+
+        expect(build.queuing_entry).to be_present
+      end
+    end
+  end
+
   describe '#schedulable?' do
     subject { build.schedulable? }
 

spec/services/ci/update_build_queue_service_spec.rb

@@ -19,8 +19,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
       it 'creates a new pending build in transaction' do
         queued = subject.push(build, transition)
 
-        expect(queued.build).to eq build
-        expect(queued.project).to eq project
+        expect(queued).to eq build.id
       end
 
       it 'increments queue push metric' do
@@ -42,6 +41,18 @@ RSpec.describe Ci::UpdateBuildQueueService do
           .to raise_error(described_class::InvalidQueueTransition)
       end
     end
+
+    context 'when duplicate entry exists' do
+      before do
+        ::Ci::PendingBuild.create(build: build, project: project)
+      end
+
+      it 'does nothing and returns updated build id' do
+        queued = subject.push(build, transition)
+
+        expect(queued).to eq build.id
+      end
+    end
   end
 
   describe '#pop' do
@@ -60,9 +71,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
       it 'removes pending build in a transaction' do
         dequeued = subject.pop(build, transition)
 
-        expect(dequeued.build).to eq build
-        expect(dequeued.project).to eq project
-        expect(dequeued).to be_destroyed
+        expect(dequeued).to be true
       end
 
       it 'increments queue pop metric' do
@@ -80,7 +89,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
       it 'does nothing if there is no pending build to remove' do
         dequeued = subject.pop(build, transition)
 
-        expect(dequeued).to be_nil
+        expect(dequeued).to be false
       end
     end