From 69e90206adf474fac10bfa560714951c04552627 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Tue, 28 Feb 2017 19:23:35 +0100
Subject: Fix concurrent access on builds/register

---
 app/services/ci/register_build_service.rb       | 36 +++++++++++++-------
 spec/services/ci/register_build_service_spec.rb | 45 +++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_build_service.rb
index 6f03bf2be13..5b52a0425de 100644
--- a/app/services/ci/register_build_service.rb
+++ b/app/services/ci/register_build_service.rb
@@ -20,21 +20,33 @@ module Ci
           builds_for_specific_runner
         end
 
-      build = builds.find do |build|
-        runner.can_pick?(build)
-      end
+      valid = true
 
-      if build
-        # In case when 2 runners try to assign the same build, second runner will be declined
-        # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
-        build.runner_id = runner.id
-        build.run!
-      end
+      builds.find do |build|
+        next unless runner.can_pick?(build)
+
+        begin
+          # In case when 2 runners try to assign the same build, second runner will be declined
+          # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
+          build.runner_id = runner.id
+          build.run!
 
-      Result.new(build, true)
+          return Result.new(build, true)
+        rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
+          # We are looping to find another build that is not conflicting
+          # It also indicates that this build can be picked and passed to runner.
+          # If we don't do it, basically a bunch of runners would be competing for a build
+          # and thus we will generate a lot of 409. This will increase
+          # the number of generated requests, also will reduce significantly
+          # how many builds can be picked by runner in a unit of time.
+          # In case we hit the concurrency-access lock,
+          # we still have to return 409 in the end,
+          # to make sure that this is properly handled by runner.
+          valid = false
+        end
+      end
 
-    rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
-      Result.new(build, false)
+      Result.new(nil, valid)
     end
 
     private
diff --git a/spec/services/ci/register_build_service_spec.rb b/spec/services/ci/register_build_service_spec.rb
index d9f774a1095..cd7dd53025c 100644
--- a/spec/services/ci/register_build_service_spec.rb
+++ b/spec/services/ci/register_build_service_spec.rb
@@ -170,6 +170,51 @@ module Ci
         end
       end
 
+      context 'when first build is stalled' do
+        before do
+          pending_build.lock_version = 10
+        end
+
+        subject { described_class.new(specific_runner).execute }
+
+        context 'with multiple builds are in queue' do
+          let!(:other_build) { create :ci_build, pipeline: pipeline }
+
+          before do
+            allow_any_instance_of(Ci::RegisterBuildService).to receive(:builds_for_specific_runner)
+              .and_return([pending_build, other_build])
+          end
+
+          it "receives second build from the queue" do
+            expect(subject).to be_valid
+            expect(subject.build).to eq(other_build)
+          end
+        end
+
+        context 'when single build is in queue' do
+          before do
+            allow_any_instance_of(Ci::RegisterBuildService).to receive(:builds_for_specific_runner)
+              .and_return([pending_build])
+          end
+
+          it "does not receive any valid result" do
+            expect(subject).not_to be_valid
+          end
+        end
+
+        context 'when there is no build in queue' do
+          before do
+            allow_any_instance_of(Ci::RegisterBuildService).to receive(:builds_for_specific_runner)
+              .and_return([])
+          end
+
+          it "does not receive builds but result is valid" do
+            expect(subject).to be_valid
+            expect(subject.build).to be_nil
+          end
+        end
+      end
+
       def execute(runner)
         described_class.new(runner).execute.build
       end
-- 
cgit v1.2.1