From 29248d1f94beb6641b573502b151b11c3e214a53 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 5 Dec 2019 18:49:37 +0800
Subject: [PATCH 1/6] Check mergeability of MR asynchronously

Executing `MergeRequests::MergeabilityCheckService#execute` can be
time consuming and can likely cause a request to timeout.

Moving its execution asynchronously via `#async_execute` will help
in ensuring page and API requests that needs it can load faster.
---
 .../components/states/mr_widget_checking.vue  |  2 +-
 .../stores/get_state_key.js                   |  2 +-
 app/models/merge_request.rb                   | 33 +++++++--
 app/services/merge_requests/merge_service.rb  |  2 +-
 .../mergeability_check_service.rb             | 19 +++++
 app/workers/all_queues.yml                    |  1 +
 ...merge_request_mergeability_check_worker.rb | 20 +++++
 ...984-asynchronous-mr-mergeability-check.yml |  5 ++
 config/sidekiq_queues.yml                     |  1 +
 ee/app/models/ee/merge_request.rb             |  2 +-
 locale/gitlab.pot                             |  2 +-
 .../user_sees_merge_widget_spec.rb            |  2 +-
 .../user_squashes_merge_request_spec.rb       |  4 +-
 .../states/mr_widget_checking_spec.js         |  2 +-
 spec/models/merge_request_spec.rb             | 73 ++++++++++++++-----
 .../mergeability_check_service_spec.rb        | 38 +++++++++-
 ..._request_mergeability_check_worker_spec.rb | 29 ++++++++
 17 files changed, 200 insertions(+), 37 deletions(-)
 create mode 100644 app/workers/merge_request_mergeability_check_worker.rb
 create mode 100644 changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml
 create mode 100644 spec/workers/merge_request_mergeability_check_worker_spec.rb

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue
index cf26003d03813f..77336b63c2c1c8 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue
@@ -12,7 +12,7 @@ export default {
   <div class="mr-widget-body media">
     <status-icon :show-disabled-button="true" status="loading" />
     <div class="media-body space-children">
-      <span class="bold"> {{ s__('mrWidget|Checking ability to merge automatically') }} </span>
+      <span class="bold"> {{ s__('mrWidget|Checking ability to merge automatically...') }} </span>
     </div>
   </div>
 </template>
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js
index 3ab229567f6f33..a298331c1fccdd 100644
--- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js
+++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js
@@ -7,7 +7,7 @@ export default function deviseState(data) {
     return stateKey.missingBranch;
   } else if (!data.commits_count) {
     return stateKey.nothingToMerge;
-  } else if (this.mergeStatus === 'unchecked') {
+  } else if (this.mergeStatus === 'unchecked' || this.mergeStatus === 'checking') {
     return stateKey.checking;
   } else if (data.has_conflicts) {
     return stateKey.conflicts;
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7162ba08a7632b..817ad26a2b2178 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -160,20 +160,32 @@ class << self
 
   state_machine :merge_status, initial: :unchecked do
     event :mark_as_unchecked do
-      transition [:can_be_merged, :unchecked] => :unchecked
+      transition [:can_be_merged, :checking, :unchecked] => :unchecked
       transition [:cannot_be_merged, :cannot_be_merged_recheck] => :cannot_be_merged_recheck
     end
 
+    event :mark_as_checking do
+      transition [:unchecked, :cannot_be_merged_recheck, :checking] => :checking
+    end
+
+    # TODO: Remove the :unchecked and :cannot_be_merged_recheck statuses for the
+    # following transitions (mark_as_mergeable and mark_as_unmergeable) in a
+    # future release.
+    #
+    # It's still needed now because it's possible that once this is released
+    # or deployed, the merge_status may still be :unchecked or :cannot_be_merged_recheck
+    # when these transitions happen.
     event :mark_as_mergeable do
-      transition [:unchecked, :cannot_be_merged_recheck] => :can_be_merged
+      transition [:unchecked, :cannot_be_merged_recheck, :checking] => :can_be_merged
     end
 
     event :mark_as_unmergeable do
-      transition [:unchecked, :cannot_be_merged_recheck] => :cannot_be_merged
+      transition [:unchecked, :cannot_be_merged_recheck, :checking] => :cannot_be_merged
     end
 
     state :unchecked
     state :cannot_be_merged_recheck
+    state :checking
     state :can_be_merged
     state :cannot_be_merged
 
@@ -812,10 +824,17 @@ def reload_diff(current_user = nil)
     MergeRequests::ReloadDiffsService.new(self, current_user).execute
   end
 
-  def check_mergeability
+  def check_mergeability(async: true)
     return if Feature.enabled?(:merge_requests_conditional_mergeability_check, default_enabled: true) && !recheck_merge_status?
 
-    MergeRequests::MergeabilityCheckService.new(self).execute(retry_lease: false)
+    check_service = MergeRequests::MergeabilityCheckService.new(self)
+    service_params = { retry_lease: false }
+
+    if async && Feature.enabled?(:async_merge_request_check_mergeability, default_enabled: true)
+      check_service.async_execute(service_params)
+    else
+      check_service.execute(service_params)
+    end
   end
   # rubocop: enable CodeReuse/ServiceClass
 
@@ -845,10 +864,10 @@ def wip_title
     self.class.wip_title(self.title)
   end
 
-  def mergeable?(skip_ci_check: false)
+  def mergeable?(skip_ci_check: false, async_check_mergeability: true)
     return false unless mergeable_state?(skip_ci_check: skip_ci_check)
 
-    check_mergeability
+    check_mergeability(async: async_check_mergeability)
 
     can_be_merged? && !should_be_rebased?
   end
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 4a109fe4e164b0..f7029976fc5595 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -54,7 +54,7 @@ def error_check!
       error =
         if @merge_request.should_be_rebased?
           'Only fast-forward merge is allowed for your project. Please update your source branch'
-        elsif !@merge_request.mergeable?
+        elsif !@merge_request.mergeable?(async_check_mergeability: false)
           'Merge request is not mergeable'
         end
 
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index 962e2327b3ed10..486942c47b882c 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -12,6 +12,16 @@ def initialize(merge_request)
       @merge_request = merge_request
     end
 
+    def async_execute(recheck: false, retry_lease: true)
+      return if checking_mergeability?
+
+      MergeRequestMergeabilityCheckWorker.perform_async(
+        merge_request.id,
+        recheck: recheck,
+        retry_lease: retry_lease
+      )
+    end
+
     # Updates the MR merge_status. Whenever it switches to a can_be_merged state,
     # the merge-ref is refreshed.
     #
@@ -54,6 +64,10 @@ def check_mergeability(recheck)
       recheck! if recheck
       update_merge_status
 
+      if checking_mergeability?
+        return ServiceResponse.error(message: 'Merge request mergeability already being checked')
+      end
+
       unless merge_request.can_be_merged?
         return ServiceResponse.error(message: 'Merge request is not mergeable')
       end
@@ -109,6 +123,7 @@ def merge_ref_head_payload
 
     def update_merge_status
       return unless merge_request.recheck_merge_status?
+      return unless merge_request.mark_as_checking
 
       if can_git_merge? && merge_to_ref
         merge_request.mark_as_mergeable
@@ -155,5 +170,9 @@ def merge_ref_auto_sync_enabled?
     def merge_ref_auto_sync_lock_enabled?
       Feature.enabled?(:merge_ref_auto_sync_lock, project, default_enabled: true)
     end
+
+    def checking_mergeability?
+      merge_request.merge_status.to_sym == :checking
+    end
   end
 end
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 502e7976ef2db7..f26a2201550f76 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -191,3 +191,4 @@
 - group_export
 - self_monitoring_project_create
 - self_monitoring_project_delete
+- merge_request_mergeability_check
diff --git a/app/workers/merge_request_mergeability_check_worker.rb b/app/workers/merge_request_mergeability_check_worker.rb
new file mode 100644
index 00000000000000..b3c8f447078269
--- /dev/null
+++ b/app/workers/merge_request_mergeability_check_worker.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class MergeRequestMergeabilityCheckWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+
+  def perform(merge_request_id, params = {})
+    merge_request = MergeRequest.find_by_id(merge_request_id)
+
+    return unless merge_request
+
+    result =
+      ::MergeRequests::MergeabilityCheckService
+        .new(merge_request)
+        .execute(params.symbolize_keys)
+
+    logger.error("Failed to check mergeability of merge request (#{merge_request_id}): #{result.message}") if result.error?
+  end
+end
diff --git a/changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml b/changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml
new file mode 100644
index 00000000000000..c1192390dcef28
--- /dev/null
+++ b/changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml
@@ -0,0 +1,5 @@
+---
+title: Check mergeability of MR asynchronously
+merge_request: 21026
+author:
+type: performance
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 06f7f01b83d457..dba8b68033ea8d 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -102,6 +102,7 @@
   - [self_monitoring_project_create, 2]
   - [self_monitoring_project_delete, 2]
   - [error_tracking_issue_link, 2]
+  - [merge_request_mergeability_check, 3]
 
   # EE-specific queues
   - [analytics, 1]
diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 963ea89868af71..9f9b82eeb215b6 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -94,7 +94,7 @@ def grouping_columns(sort)
     end
 
     override :mergeable?
-    def mergeable?(skip_ci_check: false)
+    def mergeable?(skip_ci_check: false, async_check_mergeability: true)
       return false unless approved?
       return false if merge_blocked_by_other_mrs?
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 942c81aafd5edd..c66bdb50f7cceb 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -22436,7 +22436,7 @@ msgstr ""
 msgid "mrWidget|Check out branch"
 msgstr ""
 
-msgid "mrWidget|Checking ability to merge automatically"
+msgid "mrWidget|Checking ability to merge automatically..."
 msgstr ""
 
 msgid "mrWidget|Cherry-pick"
diff --git a/spec/features/merge_request/user_sees_merge_widget_spec.rb b/spec/features/merge_request/user_sees_merge_widget_spec.rb
index 098f41f120d33e..17754400b9176c 100644
--- a/spec/features/merge_request/user_sees_merge_widget_spec.rb
+++ b/spec/features/merge_request/user_sees_merge_widget_spec.rb
@@ -19,7 +19,7 @@
     sign_in(user)
   end
 
-  context 'new merge request' do
+  context 'new merge request', :sidekiq_might_not_need_inline do
     before do
       visit project_new_merge_request_path(
         project,
diff --git a/spec/features/merge_requests/user_squashes_merge_request_spec.rb b/spec/features/merge_requests/user_squashes_merge_request_spec.rb
index a9b96c5bbf571b..4115907d074da4 100644
--- a/spec/features/merge_requests/user_squashes_merge_request_spec.rb
+++ b/spec/features/merge_requests/user_squashes_merge_request_spec.rb
@@ -67,7 +67,7 @@ def accept_mr
     end
   end
 
-  context 'when squash is enabled on merge request creation' do
+  context 'when squash is enabled on merge request creation', :sidekiq_might_not_need_inline do
     before do
       visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: source_branch })
       check 'merge_request[squash]'
@@ -97,7 +97,7 @@ def accept_mr
     end
   end
 
-  context 'when squash is not enabled on merge request creation' do
+  context 'when squash is not enabled on merge request creation', :sidekiq_might_not_need_inline do
     before do
       visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: source_branch })
       click_on 'Submit merge request'
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js
index 4f6451473e8cd8..98542a957343cb 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js
@@ -25,7 +25,7 @@ describe('MRWidgetChecking', () => {
 
   it('renders information about merging', () => {
     expect(vm.$el.querySelector('.media-body').textContent.trim()).toEqual(
-      'Checking ability to merge automatically',
+      'Checking ability to merge automatically...',
     );
   });
 });
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c6894c04385fa6..8519ef583644bc 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2072,12 +2072,25 @@ def set_compare(merge_request)
       expect(subject.mergeable?).to be_falsey
     end
 
-    it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do
-      allow(subject).to receive(:mergeable_state?) { true }
-      expect(subject).to receive(:check_mergeability)
-      expect(subject).to receive(:can_be_merged?) { true }
+    context 'when #mergeable_state? is true' do
+      before do
+        allow(subject).to receive(:mergeable_state?) { true }
+      end
 
-      expect(subject.mergeable?).to be_truthy
+      it 'return true if the MR #can_be_merged? is true' do
+        expect(subject).to receive(:check_mergeability).with(async: true)
+        expect(subject).to receive(:can_be_merged?) { true }
+
+        expect(subject.mergeable?).to be_truthy
+      end
+
+      context 'and async_check_mergeability is false' do
+        it 'calls #check_mergeability with async as false' do
+          expect(subject).to receive(:check_mergeability).with(async: false)
+
+          subject.mergeable?(async_check_mergeability: false)
+        end
+      end
     end
   end
 
@@ -2090,37 +2103,59 @@ def set_compare(merge_request)
       end
     end
 
+    shared_examples_for 'method that executes MergeabilityCheckService' do
+      context 'and async_merge_request_check_mergeability feature flag is enabled' do
+        it 'executes MergeabilityCheckService asynchronously' do
+          expect(mergeability_service).to receive(:async_execute)
+
+          subject.check_mergeability
+        end
+
+        context 'and async is false' do
+          it 'executes MergeabilityCheckService' do
+            expect(mergeability_service).to receive(:execute)
+
+            subject.check_mergeability(async: false)
+          end
+        end
+      end
+
+      context 'and async_merge_request_check_mergeability feature flag is disabled' do
+        before do
+          stub_feature_flags(async_merge_request_check_mergeability: false)
+        end
+
+        it 'executes MergeabilityCheckService' do
+          expect(mergeability_service).to receive(:execute)
+
+          subject.check_mergeability
+        end
+      end
+    end
+
     context 'if the merge status is unchecked' do
       before do
         subject.mark_as_unchecked!
       end
 
-      it 'executes MergeabilityCheckService' do
-        expect(mergeability_service).to receive(:execute)
-
-        subject.check_mergeability
-      end
+      it_behaves_like 'method that executes MergeabilityCheckService'
     end
 
     context 'if the merge status is checked' do
-      context 'and feature flag is enabled' do
-        it 'executes MergeabilityCheckService' do
-          expect(mergeability_service).not_to receive(:execute)
+      context 'and merge_requests_conditional_mergeability_check feature flag is enabled' do
+        it 'does not call MergeabilityCheckService' do
+          expect(MergeRequests::MergeabilityCheckService).not_to receive(:new)
 
           subject.check_mergeability
         end
       end
 
-      context 'and feature flag is disabled' do
+      context 'and merge_requests_conditional_mergeability_check feature flag is disabled' do
         before do
           stub_feature_flags(merge_requests_conditional_mergeability_check: false)
         end
 
-        it 'does not execute MergeabilityCheckService' do
-          expect(mergeability_service).to receive(:execute)
-
-          subject.check_mergeability
-        end
+        it_behaves_like 'method that executes MergeabilityCheckService'
       end
     end
   end
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
index a864da0a6fba07..cb0f4abc4be5a3 100644
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -53,9 +53,30 @@
     end
   end
 
+  let(:project) { create(:project, :repository) }
+  let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
+
+  describe '#async_execute' do
+    it 'enqueues MergeRequestMergeabilityCheckWorker' do
+      expect(MergeRequestMergeabilityCheckWorker).to receive(:perform_async)
+
+      described_class.new(merge_request).async_execute
+    end
+
+    context 'when merge_status is already checking' do
+      before do
+        merge_request.mark_as_checking
+      end
+
+      it 'doesn not enqueue MergeRequestMergeabilityCheckWorker' do
+        expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
+
+        described_class.new(merge_request).async_execute
+      end
+    end
+  end
+
   describe '#execute' do
-    let(:project) { create(:project, :repository) }
-    let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
     let(:repo) { project.repository }
 
     subject { described_class.new(merge_request).execute }
@@ -332,5 +353,18 @@ def execute_within_threads(amount:, retry_lease: true)
         end
       end
     end
+
+    context 'when merge_status is already checking' do
+      before do
+        merge_request.mark_as_checking
+      end
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_error
+        expect(result.message).to eq('Merge request mergeability already being checked')
+      end
+    end
   end
 end
diff --git a/spec/workers/merge_request_mergeability_check_worker_spec.rb b/spec/workers/merge_request_mergeability_check_worker_spec.rb
new file mode 100644
index 00000000000000..2331664215f2e5
--- /dev/null
+++ b/spec/workers/merge_request_mergeability_check_worker_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestMergeabilityCheckWorker do
+  subject { described_class.new }
+
+  describe '#perform' do
+    context 'when merge request does not exist' do
+      it 'does not execute MergeabilityCheckService' do
+        expect(MergeRequests::MergeabilityCheckService).not_to receive(:new)
+
+        subject.perform(1)
+      end
+    end
+
+    context 'when merge request exists' do
+      let(:merge_request) { create(:merge_request) }
+
+      it 'executes MergeabilityCheckService' do
+        expect_next_instance_of(MergeRequests::MergeabilityCheckService, merge_request) do |service|
+          expect(service).to receive(:execute).and_return(double(error?: false))
+        end
+
+        subject.perform(merge_request.id)
+      end
+    end
+  end
+end
-- 
GitLab


From 65a563d97491ba46bd29ab5e4d0a5465a230db8c Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 12 Dec 2019 12:55:22 +0800
Subject: [PATCH 2/6] Update API doc about change in behavior

---
 doc/api/merge_requests.md | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index d85310de1598d9..3a00e862094d00 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -61,6 +61,12 @@ Parameters:
 | `in`                | string         | no       | Modify the scope of the `search` attribute. `title`, `description`, or a string joining them with comma. Default is `title,description` |
 | `wip`               | string         | no       | Filter merge requests against their `wip` status. `yes` to return *only* WIP merge requests, `no` to return *non* WIP merge requests |
 
+NOTE: **Note:**
+[Starting in GitLab 12.8](https://gitlab.com/gitlab-org/gitlab/issues/29984),
+the mergeability (`merge_status`) of each merge request will be checked
+asynchronously when a request is made to this endpoint. Poll this API endpoint
+to get updated status.
+
 ```json
 [
   {
@@ -526,6 +532,12 @@ Parameters:
 - `include_diverged_commits_count` (optional) - If `true` response includes the commits behind the target branch
 - `include_rebase_in_progress` (optional) - If `true` response includes whether a rebase operation is in progress
 
+NOTE: **Note:**
+[Starting in GitLab 12.8](https://gitlab.com/gitlab-org/gitlab/issues/29984),
+the mergeability (`merge_status`) of a merge request will be checked
+asynchronously when a request is made to this endpoint. Poll this API endpoint
+to get updated status.
+
 ```json
 {
   "id": 1,
-- 
GitLab


From b2379c7932cd264fbe874d27d3202bda5982d9e4 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Thu, 12 Dec 2019 10:38:51 -0700
Subject: [PATCH 3/6] Switch message to use ellipsis instead of periods

---
 .../components/states/mr_widget_checking.vue                    | 2 +-
 locale/gitlab.pot                                               | 2 +-
 .../vue_mr_widget/components/states/mr_widget_checking_spec.js  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue
index 77336b63c2c1c8..a5e3115397acdd 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue
@@ -12,7 +12,7 @@ export default {
   <div class="mr-widget-body media">
     <status-icon :show-disabled-button="true" status="loading" />
     <div class="media-body space-children">
-      <span class="bold"> {{ s__('mrWidget|Checking ability to merge automatically...') }} </span>
+      <span class="bold"> {{ s__('mrWidget|Checking ability to merge automatically…') }} </span>
     </div>
   </div>
 </template>
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index c66bdb50f7cceb..eb5baf3ffdd4cf 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -22436,7 +22436,7 @@ msgstr ""
 msgid "mrWidget|Check out branch"
 msgstr ""
 
-msgid "mrWidget|Checking ability to merge automatically..."
+msgid "mrWidget|Checking ability to merge automatically…"
 msgstr ""
 
 msgid "mrWidget|Cherry-pick"
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js
index 98542a957343cb..efccd507fe2d83 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js
@@ -25,7 +25,7 @@ describe('MRWidgetChecking', () => {
 
   it('renders information about merging', () => {
     expect(vm.$el.querySelector('.media-body').textContent.trim()).toEqual(
-      'Checking ability to merge automatically...',
+      'Checking ability to merge automatically…',
     );
   });
 });
-- 
GitLab


From 62f5064d18cf3d554d2e448ad931d0bc5e26599e Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Fri, 13 Dec 2019 23:22:19 +0800
Subject: [PATCH 4/6] Set the merge_status to checking before enqueueing

Increase the priority to highest as well
---
 app/models/merge_request.rb                   | 16 +++------
 .../mergeability_check_service.rb             | 18 +++-------
 config/sidekiq_queues.yml                     |  2 +-
 spec/models/merge_request_spec.rb             | 15 ++++++--
 .../mergeability_check_service_spec.rb        | 35 +++++++++----------
 5 files changed, 40 insertions(+), 46 deletions(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 817ad26a2b2178..cacd0e4aa60eef 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -165,16 +165,9 @@ class << self
     end
 
     event :mark_as_checking do
-      transition [:unchecked, :cannot_be_merged_recheck, :checking] => :checking
+      transition [:unchecked, :cannot_be_merged_recheck] => :checking
     end
 
-    # TODO: Remove the :unchecked and :cannot_be_merged_recheck statuses for the
-    # following transitions (mark_as_mergeable and mark_as_unmergeable) in a
-    # future release.
-    #
-    # It's still needed now because it's possible that once this is released
-    # or deployed, the merge_status may still be :unchecked or :cannot_be_merged_recheck
-    # when these transitions happen.
     event :mark_as_mergeable do
       transition [:unchecked, :cannot_be_merged_recheck, :checking] => :can_be_merged
     end
@@ -203,7 +196,7 @@ class << self
     # rubocop: enable CodeReuse/ServiceClass
 
     def check_state?(merge_status)
-      [:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym)
+      [:unchecked, :cannot_be_merged_recheck, :checking].include?(merge_status.to_sym)
     end
   end
 
@@ -828,12 +821,11 @@ def check_mergeability(async: true)
     return if Feature.enabled?(:merge_requests_conditional_mergeability_check, default_enabled: true) && !recheck_merge_status?
 
     check_service = MergeRequests::MergeabilityCheckService.new(self)
-    service_params = { retry_lease: false }
 
     if async && Feature.enabled?(:async_merge_request_check_mergeability, default_enabled: true)
-      check_service.async_execute(service_params)
+      check_service.async_execute
     else
-      check_service.execute(service_params)
+      check_service.execute(retry_lease: false)
     end
   end
   # rubocop: enable CodeReuse/ServiceClass
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index 486942c47b882c..a51c853d7dfbc7 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -12,13 +12,14 @@ def initialize(merge_request)
       @merge_request = merge_request
     end
 
-    def async_execute(recheck: false, retry_lease: true)
-      return if checking_mergeability?
+    def async_execute
+      return if Gitlab::Database.read_only?
+      return unless merge_request.mark_as_checking
 
       MergeRequestMergeabilityCheckWorker.perform_async(
         merge_request.id,
-        recheck: recheck,
-        retry_lease: retry_lease
+        recheck: false,
+        retry_lease: false
       )
     end
 
@@ -64,10 +65,6 @@ def check_mergeability(recheck)
       recheck! if recheck
       update_merge_status
 
-      if checking_mergeability?
-        return ServiceResponse.error(message: 'Merge request mergeability already being checked')
-      end
-
       unless merge_request.can_be_merged?
         return ServiceResponse.error(message: 'Merge request is not mergeable')
       end
@@ -123,7 +120,6 @@ def merge_ref_head_payload
 
     def update_merge_status
       return unless merge_request.recheck_merge_status?
-      return unless merge_request.mark_as_checking
 
       if can_git_merge? && merge_to_ref
         merge_request.mark_as_mergeable
@@ -170,9 +166,5 @@ def merge_ref_auto_sync_enabled?
     def merge_ref_auto_sync_lock_enabled?
       Feature.enabled?(:merge_ref_auto_sync_lock, project, default_enabled: true)
     end
-
-    def checking_mergeability?
-      merge_request.merge_status.to_sym == :checking
-    end
   end
 end
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index dba8b68033ea8d..e341c91899b0ce 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -102,7 +102,7 @@
   - [self_monitoring_project_create, 2]
   - [self_monitoring_project_delete, 2]
   - [error_tracking_issue_link, 2]
-  - [merge_request_mergeability_check, 3]
+  - [merge_request_mergeability_check, 5]
 
   # EE-specific queues
   - [analytics, 1]
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 8519ef583644bc..5e1ebaf791a72b 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -277,6 +277,7 @@
 
   describe 'respond to' do
     it { is_expected.to respond_to(:unchecked?) }
+    it { is_expected.to respond_to(:checking?) }
     it { is_expected.to respond_to(:can_be_merged?) }
     it { is_expected.to respond_to(:cannot_be_merged?) }
     it { is_expected.to respond_to(:merge_params) }
@@ -2097,6 +2098,8 @@ def set_compare(merge_request)
   describe '#check_mergeability' do
     let(:mergeability_service) { double }
 
+    subject { create(:merge_request, merge_status: 'unchecked') }
+
     before do
       allow(MergeRequests::MergeabilityCheckService).to receive(:new) do
         mergeability_service
@@ -2134,14 +2137,22 @@ def set_compare(merge_request)
     end
 
     context 'if the merge status is unchecked' do
+      it_behaves_like 'method that executes MergeabilityCheckService'
+    end
+
+    context 'if the merge status is checking' do
       before do
-        subject.mark_as_unchecked!
+        subject.mark_as_checking!
       end
 
       it_behaves_like 'method that executes MergeabilityCheckService'
     end
 
     context 'if the merge status is checked' do
+      before do
+        subject.mark_as_mergeable!
+      end
+
       context 'and merge_requests_conditional_mergeability_check feature flag is enabled' do
         it 'does not call MergeabilityCheckService' do
           expect(MergeRequests::MergeabilityCheckService).not_to receive(:new)
@@ -3180,7 +3191,7 @@ def create_build(pipeline, coverage, name)
     describe 'check_state?' do
       it 'indicates whether MR is still checking for mergeability' do
         state_machine = described_class.state_machines[:merge_status]
-        check_states = [:unchecked, :cannot_be_merged_recheck]
+        check_states = [:unchecked, :cannot_be_merged_recheck, :checking]
 
         check_states.each do |merge_status|
           expect(state_machine.check_state?(merge_status)).to be true
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
index cb0f4abc4be5a3..8f17e8083e3968 100644
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -57,22 +57,34 @@
   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)
+
+        described_class.new(merge_request).async_execute
+      end
+    end
+
     it 'enqueues MergeRequestMergeabilityCheckWorker' do
       expect(MergeRequestMergeabilityCheckWorker).to receive(:perform_async)
 
       described_class.new(merge_request).async_execute
     end
 
-    context 'when merge_status is already checking' do
+    context 'when read only DB' do
       before do
-        merge_request.mark_as_checking
+        allow(Gitlab::Database).to receive(:read_only?) { true }
       end
 
-      it 'doesn not enqueue MergeRequestMergeabilityCheckWorker' do
-        expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
+      it_behaves_like 'no job is enqueued'
+    end
 
-        described_class.new(merge_request).async_execute
+    context 'when merge_status is already checking' do
+      before do
+        merge_request.mark_as_checking
       end
+
+      it_behaves_like 'no job is enqueued'
     end
   end
 
@@ -353,18 +365,5 @@ def execute_within_threads(amount:, retry_lease: true)
         end
       end
     end
-
-    context 'when merge_status is already checking' do
-      before do
-        merge_request.mark_as_checking
-      end
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_error
-        expect(result.message).to eq('Merge request mergeability already being checked')
-      end
-    end
   end
 end
-- 
GitLab


From c6734a120a44fac7f4725cf7550ad1d9946f98ca Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 19 Dec 2019 13:38:46 +0800
Subject: [PATCH 5/6] Disable the feature flag by default

---
 app/models/merge_request.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index cacd0e4aa60eef..8195dac7193dc4 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -822,7 +822,7 @@ def check_mergeability(async: true)
 
     check_service = MergeRequests::MergeabilityCheckService.new(self)
 
-    if async && Feature.enabled?(:async_merge_request_check_mergeability, default_enabled: true)
+    if async && Feature.enabled?(:async_merge_request_check_mergeability, project)
       check_service.async_execute
     else
       check_service.execute(retry_lease: false)
-- 
GitLab


From ca5e38273a18a243bb537f2d94a78f2660b025de Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 2 Jan 2020 11:39:05 +0800
Subject: [PATCH 6/6] Simplify MergeRequest#mergeable? method

Keep the mergeability check synchronous when it is called.
---
 .../projects/merge_requests_controller.rb     |  2 +-
 app/models/merge_request.rb                   |  6 +-
 app/services/merge_requests/merge_service.rb  |  2 +-
 .../mergeability_check_service.rb             | 21 ++++---
 ...merge_request_mergeability_check_worker.rb |  9 ++-
 ee/app/models/ee/merge_request.rb             |  2 +-
 lib/api/entities.rb                           |  2 +-
 .../merge_requests_controller_spec.rb         | 15 +++++
 spec/models/merge_request_spec.rb             | 57 +++++++------------
 spec/requests/api/merge_requests_spec.rb      | 30 ++++++++++
 10 files changed, 93 insertions(+), 53 deletions(-)

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 170256704880ca..22de8dd4109297 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -45,7 +45,7 @@ def index
 
   def show
     close_merge_request_if_no_source_project
-    @merge_request.check_mergeability
+    @merge_request.check_mergeability(async: true)
 
     respond_to do |format|
       format.html do
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 8195dac7193dc4..4ed73d979ce3e8 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -817,7 +817,7 @@ def reload_diff(current_user = nil)
     MergeRequests::ReloadDiffsService.new(self, current_user).execute
   end
 
-  def check_mergeability(async: true)
+  def check_mergeability(async: false)
     return if Feature.enabled?(:merge_requests_conditional_mergeability_check, default_enabled: true) && !recheck_merge_status?
 
     check_service = MergeRequests::MergeabilityCheckService.new(self)
@@ -856,10 +856,10 @@ def wip_title
     self.class.wip_title(self.title)
   end
 
-  def mergeable?(skip_ci_check: false, async_check_mergeability: true)
+  def mergeable?(skip_ci_check: false)
     return false unless mergeable_state?(skip_ci_check: skip_ci_check)
 
-    check_mergeability(async: async_check_mergeability)
+    check_mergeability
 
     can_be_merged? && !should_be_rebased?
   end
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index f7029976fc5595..4a109fe4e164b0 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -54,7 +54,7 @@ def error_check!
       error =
         if @merge_request.should_be_rebased?
           'Only fast-forward merge is allowed for your project. Please update your source branch'
-        elsif !@merge_request.mergeable?(async_check_mergeability: false)
+        elsif !@merge_request.mergeable?
           'Merge request is not mergeable'
         end
 
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index a51c853d7dfbc7..5b79e4d01f2fc6 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -13,14 +13,10 @@ def initialize(merge_request)
     end
 
     def async_execute
-      return if Gitlab::Database.read_only?
+      return service_error if service_error
       return unless merge_request.mark_as_checking
 
-      MergeRequestMergeabilityCheckWorker.perform_async(
-        merge_request.id,
-        recheck: false,
-        retry_lease: false
-      )
+      MergeRequestMergeabilityCheckWorker.perform_async(merge_request.id)
     end
 
     # Updates the MR merge_status. Whenever it switches to a can_be_merged state,
@@ -41,8 +37,7 @@ def async_execute
     # and the merge-ref is synced. Success in case of being/becoming mergeable,
     # error otherwise.
     def execute(recheck: false, retry_lease: true)
-      return ServiceResponse.error(message: 'Invalid argument') unless merge_request
-      return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
+      return service_error if service_error
       return check_mergeability(recheck) unless merge_ref_auto_sync_lock_enabled?
 
       in_write_lock(retry_lease: retry_lease) do |retried|
@@ -166,5 +161,15 @@ def merge_ref_auto_sync_enabled?
     def merge_ref_auto_sync_lock_enabled?
       Feature.enabled?(:merge_ref_auto_sync_lock, project, default_enabled: true)
     end
+
+    def service_error
+      strong_memoize(:service_error) do
+        if !merge_request
+          ServiceResponse.error(message: 'Invalid argument')
+        elsif Gitlab::Database.read_only?
+          ServiceResponse.error(message: 'Unsupported operation')
+        end
+      end
+    end
   end
 end
diff --git a/app/workers/merge_request_mergeability_check_worker.rb b/app/workers/merge_request_mergeability_check_worker.rb
index b3c8f447078269..ed35284b66c436 100644
--- a/app/workers/merge_request_mergeability_check_worker.rb
+++ b/app/workers/merge_request_mergeability_check_worker.rb
@@ -5,15 +5,18 @@ class MergeRequestMergeabilityCheckWorker
 
   feature_category :source_code_management
 
-  def perform(merge_request_id, params = {})
+  def perform(merge_request_id)
     merge_request = MergeRequest.find_by_id(merge_request_id)
 
-    return unless merge_request
+    unless merge_request
+      logger.error("Failed to find merge request with ID: #{merge_request_id}")
+      return
+    end
 
     result =
       ::MergeRequests::MergeabilityCheckService
         .new(merge_request)
-        .execute(params.symbolize_keys)
+        .execute(recheck: false, retry_lease: false)
 
     logger.error("Failed to check mergeability of merge request (#{merge_request_id}): #{result.message}") if result.error?
   end
diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 9f9b82eeb215b6..963ea89868af71 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -94,7 +94,7 @@ def grouping_columns(sort)
     end
 
     override :mergeable?
-    def mergeable?(skip_ci_check: false, async_check_mergeability: true)
+    def mergeable?(skip_ci_check: false)
       return false unless approved?
       return false if merge_blocked_by_other_mrs?
 
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index dfd0e676586dd1..97be90030c8ad0 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -811,7 +811,7 @@ class MergeRequestBasic < IssuableEntity
       # See https://gitlab.com/gitlab-org/gitlab-foss/issues/42344 for more
       # information.
       expose :merge_status do |merge_request|
-        merge_request.check_mergeability
+        merge_request.check_mergeability(async: true)
         merge_request.merge_status
       end
       expose :diff_head_sha, as: :sha
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index d5b1bfe0ac4263..df2727d13cfb34 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -44,6 +44,21 @@ def go(extra_params = {})
       get :show, params: params.merge(extra_params)
     end
 
+    context 'when merge request is unchecked' do
+      before do
+        merge_request.mark_as_unchecked!
+      end
+
+      it 'checks mergeability asynchronously' do
+        expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
+          expect(service).not_to receive(:execute)
+          expect(service).to receive(:async_execute)
+        end
+
+        go
+      end
+    end
+
     describe 'as html' do
       context 'when diff files were cleaned' do
         render_views
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 5e1ebaf791a72b..9f2567192484e9 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2073,25 +2073,12 @@ def set_compare(merge_request)
       expect(subject.mergeable?).to be_falsey
     end
 
-    context 'when #mergeable_state? is true' do
-      before do
-        allow(subject).to receive(:mergeable_state?) { true }
-      end
-
-      it 'return true if the MR #can_be_merged? is true' do
-        expect(subject).to receive(:check_mergeability).with(async: true)
-        expect(subject).to receive(:can_be_merged?) { true }
-
-        expect(subject.mergeable?).to be_truthy
-      end
-
-      context 'and async_check_mergeability is false' do
-        it 'calls #check_mergeability with async as false' do
-          expect(subject).to receive(:check_mergeability).with(async: false)
+    it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do
+      allow(subject).to receive(:mergeable_state?) { true }
+      expect(subject).to receive(:check_mergeability)
+      expect(subject).to receive(:can_be_merged?) { true }
 
-          subject.mergeable?(async_check_mergeability: false)
-        end
-      end
+      expect(subject.mergeable?).to be_truthy
     end
   end
 
@@ -2107,31 +2094,31 @@ def set_compare(merge_request)
     end
 
     shared_examples_for 'method that executes MergeabilityCheckService' do
-      context 'and async_merge_request_check_mergeability feature flag is enabled' do
-        it 'executes MergeabilityCheckService asynchronously' do
-          expect(mergeability_service).to receive(:async_execute)
+      it 'executes MergeabilityCheckService' do
+        expect(mergeability_service).to receive(:execute)
 
-          subject.check_mergeability
-        end
+        subject.check_mergeability
+      end
 
-        context 'and async is false' do
-          it 'executes MergeabilityCheckService' do
-            expect(mergeability_service).to receive(:execute)
+      context 'when async is true' do
+        context 'and async_merge_request_check_mergeability feature flag is enabled' do
+          it 'executes MergeabilityCheckService asynchronously' do
+            expect(mergeability_service).to receive(:async_execute)
 
-            subject.check_mergeability(async: false)
+            subject.check_mergeability(async: true)
           end
         end
-      end
 
-      context 'and async_merge_request_check_mergeability feature flag is disabled' do
-        before do
-          stub_feature_flags(async_merge_request_check_mergeability: false)
-        end
+        context 'and async_merge_request_check_mergeability feature flag is disabled' do
+          before do
+            stub_feature_flags(async_merge_request_check_mergeability: false)
+          end
 
-        it 'executes MergeabilityCheckService' do
-          expect(mergeability_service).to receive(:execute)
+          it 'executes MergeabilityCheckService' do
+            expect(mergeability_service).to receive(:execute)
 
-          subject.check_mergeability
+            subject.check_mergeability(async: true)
+          end
         end
       end
     end
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index ae0596bea98c13..c3d92a90d116d6 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -65,6 +65,21 @@
         end.not_to exceed_query_limit(control)
       end
 
+      context 'when merge request is unchecked' do
+        before do
+          merge_request.mark_as_unchecked!
+        end
+
+        it 'checks mergeability asynchronously' do
+          expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
+            expect(service).not_to receive(:execute)
+            expect(service).to receive(:async_execute)
+          end
+
+          get api(endpoint_path, user)
+        end
+      end
+
       context 'with labels' do
         include_context 'with labels'
 
@@ -1003,6 +1018,21 @@
 
       expect(json_response['user']['can_merge']).to be_falsy
     end
+
+    context 'when merge request is unchecked' do
+      before do
+        merge_request.mark_as_unchecked!
+      end
+
+      it 'checks mergeability asynchronously' do
+        expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
+          expect(service).not_to receive(:execute)
+          expect(service).to receive(:async_execute)
+        end
+
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user)
+      end
+    end
   end
 
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/participants' do
-- 
GitLab