From b535f1ff45c8d661ba1c9818b26f5a41b32ae5ad Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Wed, 2 Feb 2022 15:33:25 +0800
Subject: [PATCH] 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
---
 .../mergeability_check_service.rb              |  2 +-
 .../mergeability_check_service_spec.rb         | 18 ++++++------------
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index 3e294aeaa07705..30531fcc17bb6b 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -14,8 +14,8 @@ def initialize(merge_request)
 
     def async_execute
       return service_error if service_error
-      return unless merge_request.mark_as_checking
 
+      merge_request.mark_as_checking
       MergeRequestMergeabilityCheckWorker.perform_async(merge_request.id)
     end
 
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
index 65599b7e046592..c24b83e21a60ba 100644
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -73,12 +73,10 @@
   let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
 
   describe '#async_execute' do
-    shared_examples_for 'no job is enqueued' do
-      it 'does not enqueue MergeRequestMergeabilityCheckWorker' do
-        expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
+    it 'updates merge status to checking' do
+      described_class.new(merge_request).async_execute
 
-        described_class.new(merge_request).async_execute
-      end
+      expect(merge_request).to be_checking
     end
 
     it 'enqueues MergeRequestMergeabilityCheckWorker' do
@@ -92,15 +90,11 @@
         allow(Gitlab::Database).to receive(:read_only?) { true }
       end
 
-      it_behaves_like 'no job is enqueued'
-    end
+      it 'does not enqueue MergeRequestMergeabilityCheckWorker' do
+        expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
 
-    context 'when merge_status is already checking' do
-      before do
-        merge_request.mark_as_checking
+        described_class.new(merge_request).async_execute
       end
-
-      it_behaves_like 'no job is enqueued'
     end
   end
 
-- 
GitLab