Merge branch 'fix-ci-requests-concurrency' into 'master'

Fix CI requests concurrency

See merge request !8760

5 files changed, 38 insertions, 12 deletions

diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_build_service.rb
index cd548b3c8d5..6f03bf2be13 100644
--- a/app/services/ci/register_build_service.rb
+++ b/app/services/ci/register_build_service.rb
@@ -6,6 +6,8 @@ module Ci
 
     attr_reader :runner
 
+    Result = Struct.new(:build, :valid?)
+
     def initialize(runner)
       @runner = runner
     end
@@ -29,10 +31,10 @@ module Ci
         build.run!
       end
 
-      build
+      Result.new(build, true)
 
     rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
-      nil
+      Result.new(build, false)
     end
 
     private
diff --git a/changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml b/changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml
new file mode 100644
index 00000000000..075d74b9cb8
--- /dev/null
+++ b/changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml
@@ -0,0 +1,3 @@
+---
+title: 'Fix CI requests concurrency for newer runners that prevents from picking pending builds (from 1.9.0-rc5)'
+merge_request: 8760
diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index a9da8ea7eeb..8b939663ffd 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -18,24 +18,31 @@ module Ci
 
           if current_runner.is_runner_queue_value_latest?(params[:last_update])
             header 'X-GitLab-Last-Update', params[:last_update]
+            Gitlab::Metrics.add_event(:build_not_found_cached)
             return build_not_found!
           end
 
           new_update = current_runner.ensure_runner_queue_value
 
-          build = Ci::RegisterBuildService.new(current_runner).execute
+          result = Ci::RegisterBuildService.new(current_runner).execute
 
-          if build
-            Gitlab::Metrics.add_event(:build_found,
-                                      project: build.project.path_with_namespace)
+          if result.valid?
+            if result.build
+              Gitlab::Metrics.add_event(:build_found,
+                                        project: result.build.project.path_with_namespace)
 
-            present build, with: Entities::BuildDetails
-          else
-            Gitlab::Metrics.add_event(:build_not_found)
+              present result.build, with: Entities::BuildDetails
+            else
+              Gitlab::Metrics.add_event(:build_not_found)
 
-            header 'X-GitLab-Last-Update', new_update
+              header 'X-GitLab-Last-Update', new_update
 
-            build_not_found!
+              build_not_found!
+            end
+          else
+            # We received build that is invalid due to concurrency conflict
+            Gitlab::Metrics.add_event(:build_invalid)
+            conflict!
           end
         end
 
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 270c23e3f19..8dbe5f0b025 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -91,6 +91,20 @@ describe Ci::API::Builds do
           expect { register_builds }.to change { runner.reload.contacted_at }
         end
 
+        context 'when concurrently updating build' do
+          before do
+            expect_any_instance_of(Ci::Build).to receive(:run!).
+              and_raise(ActiveRecord::StaleObjectError.new(nil, nil))
+          end
+
+          it 'returns a conflict' do
+            register_builds info: { platform: :darwin }
+
+            expect(response).to have_http_status(409)
+            expect(response.headers).not_to have_key('X-GitLab-Last-Update')
+          end
+        end
+
         context 'registry credentials' do
           let(:registry_credentials) do
             { 'type' => 'registry',
diff --git a/spec/services/ci/register_build_service_spec.rb b/spec/services/ci/register_build_service_spec.rb
index 27d7853bbdd..d9f774a1095 100644
--- a/spec/services/ci/register_build_service_spec.rb
+++ b/spec/services/ci/register_build_service_spec.rb
@@ -171,7 +171,7 @@ module Ci
       end
 
       def execute(runner)
-        described_class.new(runner).execute
+        described_class.new(runner).execute.build
       end
     end
   end