Merge branch '29984-asynchronous-mr-mergeability-check' into 'master'

Check mergeability of MR asynchronously

See merge request gitlab-org/gitlab!21026

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>

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;

app/controllers/projects/merge_requests_controller.rb

@@ -45,7 +45,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   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

app/models/merge_request.rb

@@ -160,20 +160,25 @@ class MergeRequest < ApplicationRecord
 
   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
+    end
+
     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
 
@@ -191,7 +196,7 @@ class MergeRequest < ApplicationRecord
     # 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
 
@@ -812,10 +817,16 @@ class MergeRequest < ApplicationRecord
     MergeRequests::ReloadDiffsService.new(self, current_user).execute
   end
 
-  def check_mergeability
+  def check_mergeability(async: false)
     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)
+
+    if async && Feature.enabled?(:async_merge_request_check_mergeability, project)
+      check_service.async_execute
+    else
+      check_service.execute(retry_lease: false)
+    end
   end
   # rubocop: enable CodeReuse/ServiceClass
 

app/services/merge_requests/mergeability_check_service.rb

@@ -12,6 +12,13 @@ module MergeRequests
       @merge_request = merge_request
     end
 
+    def async_execute
+      return service_error if service_error
+      return unless merge_request.mark_as_checking
+
+      MergeRequestMergeabilityCheckWorker.perform_async(merge_request.id)
+    end
+
     # Updates the MR merge_status. Whenever it switches to a can_be_merged state,
     # the merge-ref is refreshed.
     #
@@ -30,8 +37,7 @@ module MergeRequests
     # 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|
@@ -155,5 +161,15 @@ module MergeRequests
     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

app/workers/all_queues.yml

@@ -191,3 +191,4 @@
 - group_export
 - self_monitoring_project_create
 - self_monitoring_project_delete
+- merge_request_mergeability_check

app/workers/merge_request_mergeability_check_worker.rb

+# frozen_string_literal: true
+
+class MergeRequestMergeabilityCheckWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+
+  def perform(merge_request_id)
+    merge_request = MergeRequest.find_by_id(merge_request_id)
+
+    unless merge_request
+      logger.error("Failed to find merge request with ID: #{merge_request_id}")
+      return
+    end
+
+    result =
+      ::MergeRequests::MergeabilityCheckService
+        .new(merge_request)
+        .execute(recheck: false, retry_lease: false)
+
+    logger.error("Failed to check mergeability of merge request (#{merge_request_id}): #{result.message}") if result.error?
+  end
+end

changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml

+---
+title: Check mergeability of MR asynchronously
+merge_request: 21026
+author:
+type: performance

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, 5]
 
   # EE-specific queues
   - [analytics, 1]

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,

lib/api/entities.rb

@@ -811,7 +811,7 @@ module API
       # 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

locale/gitlab.pot

@@ -22448,7 +22448,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"

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -44,6 +44,21 @@ describe Projects::MergeRequestsController do
       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

spec/features/merge_request/user_sees_merge_widget_spec.rb

@@ -19,7 +19,7 @@ describe 'Merge request > User sees merge widget', :js do
     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,

spec/features/merge_requests/user_squashes_merge_request_spec.rb

@@ -67,7 +67,7 @@ describe 'User squashes a merge request', :js do
     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 @@ describe 'User squashes a merge request', :js do
     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'

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…',
     );
   });
 });

spec/models/merge_request_spec.rb

@@ -277,6 +277,7 @@ describe MergeRequest do
 
   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) }
@@ -2084,43 +2085,75 @@ describe MergeRequest do
   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
       end
     end
 
-    context 'if the merge status is unchecked' do
-      before do
-        subject.mark_as_unchecked!
-      end
-
+    shared_examples_for 'method that executes MergeabilityCheckService' do
       it 'executes MergeabilityCheckService' do
         expect(mergeability_service).to receive(:execute)
 
         subject.check_mergeability
       end
+
+      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: true)
+          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(async: true)
+          end
+        end
+      end
+    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_checking!
+      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)
+      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)
 
           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
@@ -3145,7 +3178,7 @@ describe MergeRequest do
     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

spec/requests/api/merge_requests_spec.rb

@@ -65,6 +65,21 @@ describe API::MergeRequests do
         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 @@ describe API::MergeRequests do
 
       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

spec/services/merge_requests/mergeability_check_service_spec.rb

@@ -53,9 +53,42 @@ describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shared_sta
     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
+    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 read only DB' do
+      before do
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+      end
+
+      it_behaves_like 'no job is enqueued'
+    end
+
+    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
+
   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 }

spec/workers/merge_request_mergeability_check_worker_spec.rb

+# 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