Commit ec100a20 authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Stan Hu

Fix concurrent access on builds/register

parent ad00b9bc
......@@ -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
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment