Fix transactions-related race condition in stages code
This commit is contained in:
parent
b5c69ce3ab
commit
538ad92a1c
2 changed files with 57 additions and 1 deletions
|
@ -7,6 +7,8 @@ module Ci
|
||||||
# stage.
|
# stage.
|
||||||
#
|
#
|
||||||
class EnsureStageService < BaseService
|
class EnsureStageService < BaseService
|
||||||
|
PipelineStageError = Class.new(StandardError)
|
||||||
|
|
||||||
def execute(build)
|
def execute(build)
|
||||||
@build = build
|
@build = build
|
||||||
|
|
||||||
|
@ -22,8 +24,11 @@ module Ci
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def ensure_stage
|
def ensure_stage(attempts: 2)
|
||||||
find_stage || create_stage
|
find_stage || create_stage
|
||||||
|
rescue ActiveRecord::RecordNotUnique
|
||||||
|
retry if (attempts -= 1) > 0
|
||||||
|
raise PipelineStageError, 'Fix me!'
|
||||||
end
|
end
|
||||||
|
|
||||||
def find_stage
|
def find_stage
|
||||||
|
|
51
spec/services/ci/ensure_stage_service_spec.rb
Normal file
51
spec/services/ci/ensure_stage_service_spec.rb
Normal file
|
@ -0,0 +1,51 @@
|
||||||
|
require 'spec_helper'
|
||||||
|
|
||||||
|
describe Ci::EnsureStageService, '#execute' do
|
||||||
|
set(:project) { create(:project) }
|
||||||
|
set(:user) { create(:user) }
|
||||||
|
|
||||||
|
let(:stage) { create(:ci_stage_entity) }
|
||||||
|
let(:job) { build(:ci_build) }
|
||||||
|
|
||||||
|
let(:service) { described_class.new(project, user) }
|
||||||
|
|
||||||
|
context 'when build has a stage assigned' do
|
||||||
|
it 'does not create a new stage' do
|
||||||
|
job.assign_attributes(stage_id: stage.id)
|
||||||
|
|
||||||
|
expect { service.execute(job) }.not_to change { Ci::Stage.count }
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when build does not have a stage assigned' do
|
||||||
|
it 'creates a new stage' do
|
||||||
|
job.assign_attributes(stage_id: nil, stage: 'test')
|
||||||
|
|
||||||
|
expect { service.execute(job) }.to change { Ci::Stage.count }.by(1)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when build is invalid' do
|
||||||
|
it 'does not create a new stage' do
|
||||||
|
job.assign_attributes(stage_id: nil, ref: nil)
|
||||||
|
|
||||||
|
expect { service.execute(job) }.not_to change { Ci::Stage.count }
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when new stage can not be created because of an exception' do
|
||||||
|
before do
|
||||||
|
allow(Ci::Stage).to receive(:create!)
|
||||||
|
.and_raise(ActiveRecord::RecordNotUnique.new('Duplicates!'))
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'retries up to two times' do
|
||||||
|
job.assign_attributes(stage_id: nil)
|
||||||
|
|
||||||
|
expect(service).to receive(:find_stage).exactly(2).times
|
||||||
|
|
||||||
|
expect { service.execute(job) }
|
||||||
|
.to raise_error(Ci::EnsureStageService::PipelineStageError)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
Loading…
Reference in a new issue