From 3f5a46d2741a6124b394e7f629e8fd39b2287bbb Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Mon, 15 Dec 2025 14:01:16 +0100 Subject: [PATCH 1/4] Remove split_refresh_worker_web_hooks feature flag The split_refresh_worker_web_hooks feature flag has been enabled by default and is stable. This commit removes the feature flag and keeps the enabled behavior. Changelog: changed --- app/services/merge_requests/refresh_service.rb | 16 +++++++--------- .../beta/split_refresh_worker_web_hooks.yml | 10 ---------- .../merge_requests/refresh_service_spec.rb | 5 ++--- 3 files changed, 9 insertions(+), 22 deletions(-) delete mode 100644 config/feature_flags/beta/split_refresh_worker_web_hooks.yml diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 6be974a1a0437c..e85a9e6ffbbdab 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -359,15 +359,13 @@ def schedule_duo_code_review(merge_request) end def execute_async_workers - if Feature.enabled?(:split_refresh_worker_web_hooks, @current_user) - MergeRequests::Refresh::WebHooksWorker.perform_async( - @project.id, - @current_user.id, - @push.oldrev, - @push.newrev, - @push.ref - ) - end + MergeRequests::Refresh::WebHooksWorker.perform_async( + @project.id, + @current_user.id, + @push.oldrev, + @push.newrev, + @push.ref + ) if Feature.enabled?(:split_refresh_worker_pipeline, @current_user) MergeRequests::Refresh::PipelineWorker.perform_async( diff --git a/config/feature_flags/beta/split_refresh_worker_web_hooks.yml b/config/feature_flags/beta/split_refresh_worker_web_hooks.yml deleted file mode 100644 index d26dac26d8198c..00000000000000 --- a/config/feature_flags/beta/split_refresh_worker_web_hooks.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: split_refresh_worker_web_hooks -description: Split web hooks execution from MergeRequests::RefreshService into separate worker -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/554081 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/203215 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/569485 -milestone: '18.4' -group: group::code review -type: beta -default_enabled: true diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index d993abc2689963..b38baa1887d0b8 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -671,11 +671,10 @@ def refresh refresh end - it 'executes hooks with update action' do + it 'does not execute the web hooks' do refresh - expect(refresh_service).to have_received(:execute_hooks) - .with(@fork_merge_request, 'update', old_rev: @oldrev) + expect(refresh_service).not_to have_received(:execute_hooks) expect(@merge_request.notes).to be_empty expect(@merge_request).to be_open -- GitLab From 3389e976166d7d760efe76e51e8d5bd2eea93316 Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Mon, 15 Dec 2025 14:10:49 +0100 Subject: [PATCH 2/4] Remove split_refresh_worker_web_hooks feature flag from service --- app/services/merge_requests/refresh_service.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index e85a9e6ffbbdab..3e838f2ca3d757 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -45,9 +45,6 @@ def refresh_merge_requests! notify_about_push(mr) mark_mr_as_draft_from_commits(mr) - # Call merge request webhook with update branches - execute_mr_web_hooks(mr) if Feature.disabled?(:split_refresh_worker_web_hooks, @current_user) - if Feature.disabled?(:split_refresh_worker_pipeline, @current_user) && !@push.branch_removed? # Run at the end of the loop to avoid any potential contention on the MR object refresh_pipelines_on_merge_requests(mr) -- GitLab From 733cf282c14023bbceb6be97bb7872ab14e09927 Mon Sep 17 00:00:00 2001 From: marc_shaw Date: Mon, 15 Dec 2025 14:24:46 +0100 Subject: [PATCH 3/4] Remove other references --- .../merge_requests/refresh_service_spec.rb | 73 +++++-------------- 1 file changed, 17 insertions(+), 56 deletions(-) diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index b38baa1887d0b8..2d9f923a06e4e6 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -10,13 +10,11 @@ let(:user) { create(:user) } let(:service) { described_class } - let(:webhook_ff) { false } let(:pipeline_ff) { false } describe '#execute' do before do stub_feature_flags( - split_refresh_worker_web_hooks: webhook_ff, split_refresh_worker_pipeline: pipeline_ff ) @@ -86,7 +84,6 @@ let(:notification_service) { spy('notification_service') } before do - allow(refresh_service).to receive(:execute_hooks) allow(NotificationService).to receive(:new) { notification_service } end @@ -105,13 +102,10 @@ end end - it 'executes hooks with update action' do + it 'executes hooks with update action', :sidekiq_inline do refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs - expect(refresh_service).to have_received(:execute_hooks) - .with(@merge_request, 'update', old_rev: @oldrev) - expect(notification_service).to have_received(:push_to_merge_request) .with(@merge_request, @user, new_commits: anything, existing_commits: anything) expect(notification_service).to have_received(:push_to_merge_request) @@ -127,27 +121,25 @@ expect(@fork_build_failed_todo).to be_done end - context 'when split_refresh_worker_web_hooks is on' do - let(:webhook_ff) { true } + it 'calls the web hooks worker async' do + expect(MergeRequests::Refresh::WebHooksWorker).to receive(:perform_async) + .with( + @project.id, + @user.id, + @oldrev, + @newrev, + 'refs/heads/master' + ) - it 'calls the web hooks worker async' do - expect(MergeRequests::Refresh::WebHooksWorker).to receive(:perform_async) - .with( - @project.id, - @user.id, - @oldrev, - @newrev, - 'refs/heads/master' - ) + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') + end - refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') - end + it 'does not execute the web hooks' do + allow(refresh_service).to receive(:execute_hooks) - it 'does not execute the web hooks' do - refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') - expect(refresh_service).not_to have_received(:execute_hooks) - end + expect(refresh_service).not_to have_received(:execute_hooks) end it 'triggers mergeRequestMergeStatusUpdated GraphQL subscription conditionally' do @@ -509,15 +501,12 @@ let(:notification_service) { spy('notification_service') } before do - allow(refresh_service).to receive(:execute_hooks) allow(NotificationService).to receive(:new) { notification_service } refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end - it 'executes hooks with update action' do - expect(refresh_service).to have_received(:execute_hooks) - .with(@merge_request, 'update', old_rev: @oldrev) + it 'executes hooks with update action', :sidekiq_inline do expect(notification_service).to have_received(:push_to_merge_request) .with(@merge_request, @user, new_commits: anything, existing_commits: anything) expect(notification_service).to have_received(:push_to_merge_request) @@ -657,7 +646,6 @@ let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } def refresh - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end @@ -671,19 +659,6 @@ def refresh refresh end - it 'does not execute the web hooks' do - refresh - - expect(refresh_service).not_to have_received(:execute_hooks) - - expect(@merge_request.notes).to be_empty - expect(@merge_request).to be_open - expect(@fork_merge_request.notes.last.note).to include('added 28 commits') - expect(@fork_merge_request).to be_open - expect(@build_failed_todo).to be_pending - expect(@fork_build_failed_todo).to be_pending - end - it 'outdates opened forked MR suggestions' do expect_next_instance_of(Suggestions::OutdateService) do |service| expect(service).to receive(:execute).with(@fork_merge_request).and_call_original @@ -698,12 +673,6 @@ def refresh @fork_merge_request.close! end - it 'do not execute hooks with update action' do - refresh - - expect(refresh_service).not_to have_received(:execute_hooks) - end - it 'updates merge request to closed state' do refresh @@ -842,8 +811,6 @@ def refresh let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } it 'refreshes the merge request' do - expect(refresh_service).to receive(:execute_hooks) - .with(@fork_merge_request, 'update', old_rev: Gitlab::Git::SHA1_BLANK_SHA) allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev) refresh_service.execute(Gitlab::Git::SHA1_BLANK_SHA, @newrev, 'refs/heads/master') @@ -891,7 +858,6 @@ def refresh ) refresh_service = service.new(project: project, current_user: user) - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') expect(MergeRequestsClosingIssues.where(merge_request: merge_request)).to contain_exactly( @@ -917,7 +883,6 @@ def refresh ) refresh_service = service.new(project: forked_project, current_user: user) - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') expect(MergeRequestsClosingIssues.where(merge_request: merge_request)).to contain_exactly( @@ -930,10 +895,6 @@ def refresh context 'marking the merge request as draft' do let(:refresh_service) { service.new(project: @project, current_user: @user) } - before do - allow(refresh_service).to receive(:execute_hooks) - end - it 'marks the merge request as draft from fixup commits' do fixup_merge_request = create( :merge_request, -- GitLab From def370c056904a79aa1808830e7db7efdc0ec8b7 Mon Sep 17 00:00:00 2001 From: marc_shaw Date: Mon, 15 Dec 2025 17:18:53 +0100 Subject: [PATCH 4/4] Fix specs --- .../merge_requests/refresh_service_spec.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 2d9f923a06e4e6..4d06bd53648486 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -102,7 +102,7 @@ end end - it 'executes hooks with update action', :sidekiq_inline do + it 'refreshes the merge requests' do refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs @@ -506,7 +506,7 @@ reload_mrs end - it 'executes hooks with update action', :sidekiq_inline do + it 'refreshes the merge requests' do expect(notification_service).to have_received(:push_to_merge_request) .with(@merge_request, @user, new_commits: anything, existing_commits: anything) expect(notification_service).to have_received(:push_to_merge_request) @@ -659,6 +659,17 @@ def refresh refresh end + it 'refreshes the merge requests' do + refresh + + expect(@merge_request.notes).to be_empty + expect(@merge_request).to be_open + expect(@fork_merge_request.notes.last.note).to include('added 28 commits') + expect(@fork_merge_request).to be_open + expect(@build_failed_todo).to be_pending + expect(@fork_build_failed_todo).to be_pending + end + it 'outdates opened forked MR suggestions' do expect_next_instance_of(Suggestions::OutdateService) do |service| expect(service).to receive(:execute).with(@fork_merge_request).and_call_original -- GitLab