Commit 76e5f067 authored by Patrick Bajao's avatar Patrick Bajao

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
parent c7c5c096
...@@ -100,10 +100,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -100,10 +100,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
# rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/AbcSize
def show def show
close_merge_request_if_no_source_project 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) @merge_request.check_mergeability(async: true)
end
respond_to do |format| respond_to do |format|
format.html do format.html do
......
...@@ -8,7 +8,6 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity ...@@ -8,7 +8,6 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
expose :merged_commit_sha expose :merged_commit_sha
expose :short_merged_commit_sha expose :short_merged_commit_sha
expose :merge_error expose :merge_error
expose :public_merge_status, as: :merge_status
expose :merge_user_id expose :merge_user_id
expose :source_branch expose :source_branch
expose :source_project_id expose :source_project_id
...@@ -26,6 +25,11 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity ...@@ -26,6 +25,11 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
expose :source_branch_exists?, as: :source_branch_exists expose :source_branch_exists?, as: :source_branch_exists
expose :branch_missing?, as: :branch_missing 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| expose :default_squash_commit_message do |merge_request|
merge_request.default_squash_commit_message(user: request.current_user) merge_request.default_squash_commit_message(user: request.current_user)
end end
......
...@@ -24,8 +24,6 @@ class MergeRequestPollWidgetEntity < Grape::Entity ...@@ -24,8 +24,6 @@ class MergeRequestPollWidgetEntity < Grape::Entity
end end
expose :mergeable do |merge_request, options| 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? merge_request.mergeable?
end end
......
...@@ -7,7 +7,7 @@ module MergeRequests ...@@ -7,7 +7,7 @@ module MergeRequests
def execute(merge_request) def execute(merge_request)
prepare_for_mergeability(merge_request) if early_prepare_for_mergeability?(merge_request) prepare_for_mergeability(merge_request) if early_prepare_for_mergeability?(merge_request)
prepare_merge_request(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 end
private private
...@@ -15,7 +15,7 @@ module MergeRequests ...@@ -15,7 +15,7 @@ module MergeRequests
def prepare_for_mergeability(merge_request) def prepare_for_mergeability(merge_request)
create_pipeline_for(merge_request, current_user) create_pipeline_for(merge_request, current_user)
merge_request.update_head_pipeline merge_request.update_head_pipeline
mark_as_unchecked(merge_request) check_mergeability(merge_request)
end end
def prepare_merge_request(merge_request) def prepare_merge_request(merge_request)
...@@ -55,8 +55,13 @@ module MergeRequests ...@@ -55,8 +55,13 @@ module MergeRequests
end end
end end
def mark_as_unchecked(merge_request) def check_mergeability(merge_request)
merge_request.mark_as_unchecked if merge_request.preparing? 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 end
end end
......
---
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
...@@ -57,11 +57,6 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -57,11 +57,6 @@ RSpec.describe Projects::MergeRequestsController do
merge_request.mark_as_unchecked! merge_request.mark_as_unchecked!
end end
context 'check_mergeability_async_in_widget feature flag is disabled' do
before do
stub_feature_flags(check_mergeability_async_in_widget: false)
end
it 'checks mergeability asynchronously' do it 'checks mergeability asynchronously' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service| expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
expect(service).not_to receive(:execute) expect(service).not_to receive(:execute)
...@@ -71,7 +66,6 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -71,7 +66,6 @@ RSpec.describe Projects::MergeRequestsController do
go go
end end
end end
end
describe 'as html' do describe 'as html' do
context 'when diff files were cleaned' do context 'when diff files were cleaned' do
......
...@@ -21,12 +21,6 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do ...@@ -21,12 +21,6 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do
is_expected.to include(:target_branch_sha) is_expected.to include(:target_branch_sha)
end 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 it 'has blob path data' do
allow(resource).to receive_messages( allow(resource).to receive_messages(
base_pipeline: pipeline, base_pipeline: pipeline,
...@@ -38,6 +32,20 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do ...@@ -38,6 +32,20 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do
expect(subject[:blob_path]).to include(:head_path) expect(subject[:blob_path]).to include(:head_path)
end 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 describe 'diverged_commits_count' do
context 'when MR open and its diverging' do context 'when MR open and its diverging' do
it 'returns diverged commits count' do it 'returns diverged commits count' do
......
...@@ -180,16 +180,6 @@ RSpec.describe MergeRequestPollWidgetEntity do ...@@ -180,16 +180,6 @@ RSpec.describe MergeRequestPollWidgetEntity do
it 'calculates mergeability and returns true' do it 'calculates mergeability and returns true' do
expect(subject[:mergeable]).to eq(true) expect(subject[:mergeable]).to eq(true)
end 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 end
end end
...@@ -89,10 +89,10 @@ RSpec.describe MergeRequests::AfterCreateService do ...@@ -89,10 +89,10 @@ RSpec.describe MergeRequests::AfterCreateService do
merge_request.mark_as_preparing! merge_request.mark_as_preparing!
end end
it 'marks the merge request as unchecked' do it 'checks for mergeability' do
execute_service expect(merge_request).to receive(:check_mergeability).with(async: true)
expect(merge_request.reload).to be_unchecked execute_service
end end
context 'when preparing for mergeability fails' do context 'when preparing for mergeability fails' do
...@@ -130,9 +130,9 @@ RSpec.describe MergeRequests::AfterCreateService do ...@@ -130,9 +130,9 @@ RSpec.describe MergeRequests::AfterCreateService do
.and_raise(StandardError) .and_raise(StandardError)
end 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 { execute_service }.to raise_error(StandardError)
expect(merge_request.reload).to be_unchecked
end end
context 'when early_prepare_for_mergeability feature flag is disabled' do context 'when early_prepare_for_mergeability feature flag is disabled' do
......
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