Commit b535f1ff authored by Patrick Bajao's avatar Patrick Bajao

Allow mergeability check when merge_status is already checking

We previously don't perform `MergeRequestMergeabilityCheckWorker`
when MR `merge_status` is already `checking`. This is to reduce
the number of workers getting enqueued.

However, this can cause a MR getting stuck in `checking` state
in case the worker failed to update the status (e.g. service failed
to obtain a lock). The worker won't be retried because we handle
and log this type of errors.

Since the worker is idempotent, it's already deduplicated. We also
have a lease mechanism in `MergeRequests::MergeabilityCheckService`
that will obtain a lock and keep it for a minute.

We can rely on these mechanisms instead so we can still allow
performing a mergeability check when the `merge_status` is already
`checking`.

Changelog: fixed
parent 04f2c3b6
...@@ -14,8 +14,8 @@ module MergeRequests ...@@ -14,8 +14,8 @@ module MergeRequests
def async_execute def async_execute
return service_error if service_error return service_error if service_error
return unless merge_request.mark_as_checking
merge_request.mark_as_checking
MergeRequestMergeabilityCheckWorker.perform_async(merge_request.id) MergeRequestMergeabilityCheckWorker.perform_async(merge_request.id)
end end
......
...@@ -73,12 +73,10 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar ...@@ -73,12 +73,10 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) } let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
describe '#async_execute' do describe '#async_execute' do
shared_examples_for 'no job is enqueued' do it 'updates merge status to checking' do
it 'does not enqueue MergeRequestMergeabilityCheckWorker' do described_class.new(merge_request).async_execute
expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
described_class.new(merge_request).async_execute expect(merge_request).to be_checking
end
end end
it 'enqueues MergeRequestMergeabilityCheckWorker' do it 'enqueues MergeRequestMergeabilityCheckWorker' do
...@@ -92,15 +90,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar ...@@ -92,15 +90,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
allow(Gitlab::Database).to receive(:read_only?) { true } allow(Gitlab::Database).to receive(:read_only?) { true }
end end
it_behaves_like 'no job is enqueued' it 'does not enqueue MergeRequestMergeabilityCheckWorker' do
end expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
context 'when merge_status is already checking' do described_class.new(merge_request).async_execute
before do
merge_request.mark_as_checking
end end
it_behaves_like 'no job is enqueued'
end end
end 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