Commit 85c29981 authored by Furkan Ayhan's avatar Furkan Ayhan

Fix retry-lock problem on pipeline cancel

Having the "retries" parameter nil causes an error on
Gitlab::OptimisticLocking when comparing retry_attempts < max_retries.
So we need to change its default value. We can make it `0`,
but I think it's better to have `1` to have a "secure" cancel mechanism.

Changelog: fixed
parent b324db38
...@@ -577,11 +577,11 @@ module Ci ...@@ -577,11 +577,11 @@ module Ci
canceled? && auto_canceled_by_id? canceled? && auto_canceled_by_id?
end end
def cancel_running(retries: nil) def cancel_running(retries: 1)
commit_status_relations = [:project, :pipeline] commit_status_relations = [:project, :pipeline]
ci_build_relations = [:deployment, :taggings] ci_build_relations = [:deployment, :taggings]
retry_optimistic_lock(cancelable_statuses, retries, name: 'ci_pipeline_cancel_running') do |cancelables| retry_lock(cancelable_statuses, retries, name: 'ci_pipeline_cancel_running') do |cancelables|
cancelables.find_in_batches do |batch| cancelables.find_in_batches do |batch|
ActiveRecord::Associations::Preloader.new.preload(batch, commit_status_relations) ActiveRecord::Associations::Preloader.new.preload(batch, commit_status_relations)
ActiveRecord::Associations::Preloader.new.preload(batch.select { |job| job.is_a?(Ci::Build) }, ci_build_relations) ActiveRecord::Associations::Preloader.new.preload(batch.select { |job| job.is_a?(Ci::Build) }, ci_build_relations)
...@@ -594,7 +594,7 @@ module Ci ...@@ -594,7 +594,7 @@ module Ci
end end
end end
def auto_cancel_running(pipeline, retries: nil) def auto_cancel_running(pipeline, retries: 1)
update(auto_canceled_by: pipeline) update(auto_canceled_by: pipeline)
cancel_running(retries: retries) do |job| cancel_running(retries: retries) do |job|
......
...@@ -2768,6 +2768,41 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -2768,6 +2768,41 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
expect(control2.count).to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) expect(control2.count).to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries)
end end
end end
context 'when the first try cannot get an exclusive lock' do
let(:retries) { 1 }
subject(:cancel_running) { pipeline.cancel_running(retries: retries) }
before do
build = create(:ci_build, :running, pipeline: pipeline)
allow(pipeline.cancelable_statuses).to receive(:find_in_batches).and_yield([build])
call_count = 0
allow(build).to receive(:cancel).and_wrap_original do |original, *args|
call_count >= retries ? raise(ActiveRecord::StaleObjectError) : original.call(*args)
call_count += 1
end
end
it 'retries again and cancels the build' do
cancel_running
expect(latest_status).to contain_exactly('canceled')
end
context 'when the retries parameter is 0' do
let(:retries) { 0 }
it 'raises error' do
expect do
cancel_running
end.to raise_error(ActiveRecord::StaleObjectError)
end
end
end
end end
describe '#retry_failed' do describe '#retry_failed' do
......
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