From 76e5f067c857cf3b3a440becfb0503c4a6efd32b Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Thu, 27 Jan 2022 14:57:38 +0800 Subject: [PATCH] Ensure mergeability check runs on specific cases We call `MergeRequest#check_mergeability` on MR page load to trigger mergeability check. Other than that, we rely on calls for `MergeRequest#mergeable?` to eventually call it as well. This can cause stale `MergeRequest#merge_status` when: 1. Race condition occurred between the MR page load and the time `NewMergeRequestWorker` runs. 2. The `MergeRequest#merge_status` gets updated after the MR page has already loaded (e.g. new changes were pushed to the MR's target branch). As a fix, we are calling `MergeRequest#check_mergeability` in more places to ensure we check for it: 1. We now call it on `MergeRequests::AfterCreateService` to prevent race condition issue. This service is called by the `NewMergeRequestWorker`. This way, we ensure that we check for MR's mergeability after it was created. 2. `MergeRequestPollCachedWidgetEntity#merge_status` has been updated so whenever the `MergeRequest#merge_status` gets updated and the MR widget polls for updated status, we will recheck for mergeability as well. On next poll, it should show the updated `merge_status`. 3. The `check_mergeability_async_in_widget` FF has also been removed since it doesn't seem to be used at all. We also need to keep the call for `MergeRequest#check_mergeability` on the show action so we won't need to wait for poll in case the merge status gets updated and the user decides to refresh the page. Changelog: fixed --- .../projects/merge_requests_controller.rb | 5 +---- ...merge_request_poll_cached_widget_entity.rb | 6 +++++- .../merge_request_poll_widget_entity.rb | 2 -- .../merge_requests/after_create_service.rb | 13 ++++++++---- .../check_mergeability_async_in_widget.yml | 8 -------- .../merge_requests_controller_spec.rb | 16 +++++---------- ..._request_poll_cached_widget_entity_spec.rb | 20 +++++++++++++------ .../merge_request_poll_widget_entity_spec.rb | 10 ---------- .../after_create_service_spec.rb | 10 +++++----- 9 files changed, 39 insertions(+), 51 deletions(-) delete mode 100644 config/feature_flags/development/check_mergeability_async_in_widget.yml diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b6d5ce6e39382d..bb951a54aebf8f 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -100,10 +100,7 @@ def index # rubocop:disable Metrics/AbcSize def show close_merge_request_if_no_source_project - - if Feature.disabled?(:check_mergeability_async_in_widget, @project, default_enabled: :yaml) - @merge_request.check_mergeability(async: true) - end + @merge_request.check_mergeability(async: true) respond_to do |format| format.html do diff --git a/app/serializers/merge_request_poll_cached_widget_entity.rb b/app/serializers/merge_request_poll_cached_widget_entity.rb index 5bf02c93c99fee..9d001d18aa6c0d 100644 --- a/app/serializers/merge_request_poll_cached_widget_entity.rb +++ b/app/serializers/merge_request_poll_cached_widget_entity.rb @@ -8,7 +8,6 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity expose :merged_commit_sha expose :short_merged_commit_sha expose :merge_error - expose :public_merge_status, as: :merge_status expose :merge_user_id expose :source_branch expose :source_project_id @@ -26,6 +25,11 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity expose :source_branch_exists?, as: :source_branch_exists expose :branch_missing?, as: :branch_missing + expose :merge_status do |merge_request| + merge_request.check_mergeability(async: true) + merge_request.public_merge_status + end + expose :default_squash_commit_message do |merge_request| merge_request.default_squash_commit_message(user: request.current_user) end diff --git a/app/serializers/merge_request_poll_widget_entity.rb b/app/serializers/merge_request_poll_widget_entity.rb index f68477e82c9819..12998d70a22378 100644 --- a/app/serializers/merge_request_poll_widget_entity.rb +++ b/app/serializers/merge_request_poll_widget_entity.rb @@ -24,8 +24,6 @@ class MergeRequestPollWidgetEntity < Grape::Entity end expose :mergeable do |merge_request, options| - next merge_request.mergeable? if Feature.disabled?(:check_mergeability_async_in_widget, merge_request.project, default_enabled: :yaml) - merge_request.mergeable? end diff --git a/app/services/merge_requests/after_create_service.rb b/app/services/merge_requests/after_create_service.rb index d2c83f82ff8953..83525899e98bdc 100644 --- a/app/services/merge_requests/after_create_service.rb +++ b/app/services/merge_requests/after_create_service.rb @@ -7,7 +7,7 @@ class AfterCreateService < MergeRequests::BaseService def execute(merge_request) prepare_for_mergeability(merge_request) if early_prepare_for_mergeability?(merge_request) prepare_merge_request(merge_request) - mark_as_unchecked(merge_request) unless early_prepare_for_mergeability?(merge_request) + check_mergeability(merge_request) unless early_prepare_for_mergeability?(merge_request) end private @@ -15,7 +15,7 @@ def execute(merge_request) def prepare_for_mergeability(merge_request) create_pipeline_for(merge_request, current_user) merge_request.update_head_pipeline - mark_as_unchecked(merge_request) + check_mergeability(merge_request) end def prepare_merge_request(merge_request) @@ -55,8 +55,13 @@ def early_prepare_for_mergeability?(merge_request) end end - def mark_as_unchecked(merge_request) - merge_request.mark_as_unchecked if merge_request.preparing? + def check_mergeability(merge_request) + return unless merge_request.preparing? + + # Need to set to unchecked to be able to check for mergeability or else + # it'll be a no-op. + merge_request.mark_as_unchecked + merge_request.check_mergeability(async: true) end end end diff --git a/config/feature_flags/development/check_mergeability_async_in_widget.yml b/config/feature_flags/development/check_mergeability_async_in_widget.yml deleted file mode 100644 index ff8116c3a65045..00000000000000 --- a/config/feature_flags/development/check_mergeability_async_in_widget.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: check_mergeability_async_in_widget -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58178 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326567 -milestone: '13.11' -type: development -group: group::source code -default_enabled: false diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index d20bb2b16e5105..3ac59d8e43ae72 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -57,19 +57,13 @@ def go(extra_params = {}) merge_request.mark_as_unchecked! end - context 'check_mergeability_async_in_widget feature flag is disabled' do - before do - stub_feature_flags(check_mergeability_async_in_widget: false) + 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 - 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 + go end end diff --git a/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb b/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb index ecc93219b53585..e9c1fe23855733 100644 --- a/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb @@ -21,12 +21,6 @@ is_expected.to include(:target_branch_sha) end - it 'has public_merge_status as merge_status' do - expect(resource).to receive(:public_merge_status).and_return('checking') - - expect(subject[:merge_status]).to eq 'checking' - end - it 'has blob path data' do allow(resource).to receive_messages( base_pipeline: pipeline, @@ -38,6 +32,20 @@ expect(subject[:blob_path]).to include(:head_path) end + describe 'merge_status' do + it 'calls for MergeRequest#check_mergeability' do + expect(resource).to receive(:check_mergeability).with(async: true) + + subject[:merge_status] + end + + it 'has public_merge_status as merge_status' do + expect(resource).to receive(:public_merge_status).and_return('checking') + + expect(subject[:merge_status]).to eq 'checking' + end + end + describe 'diverged_commits_count' do context 'when MR open and its diverging' do it 'returns diverged commits count' do diff --git a/spec/serializers/merge_request_poll_widget_entity_spec.rb b/spec/serializers/merge_request_poll_widget_entity_spec.rb index 3aebe16438ce9f..581efd331ef42c 100644 --- a/spec/serializers/merge_request_poll_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_widget_entity_spec.rb @@ -180,16 +180,6 @@ it 'calculates mergeability and returns true' do expect(subject[:mergeable]).to eq(true) end - - context 'when check_mergeability_async_in_widget is disabled' do - before do - stub_feature_flags(check_mergeability_async_in_widget: false) - end - - it 'calculates mergeability and returns true' do - expect(subject[:mergeable]).to eq(true) - end - end end end end diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 781be57d7096a1..ff6ccc7524486e 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -89,10 +89,10 @@ merge_request.mark_as_preparing! end - it 'marks the merge request as unchecked' do - execute_service + it 'checks for mergeability' do + expect(merge_request).to receive(:check_mergeability).with(async: true) - expect(merge_request.reload).to be_unchecked + execute_service end context 'when preparing for mergeability fails' do @@ -130,9 +130,9 @@ .and_raise(StandardError) end - it 'still marks the merge request as unchecked' do + it 'still checks for mergeability' do + expect(merge_request).to receive(:check_mergeability).with(async: true) expect { execute_service }.to raise_error(StandardError) - expect(merge_request.reload).to be_unchecked end context 'when early_prepare_for_mergeability feature flag is disabled' do -- GitLab