Merge branch 'fix-on-train-method-in-mr' into 'master'

Fix AutoMergeProcessWorker raises an error for merged MRs

Closes #121989

See merge request gitlab-org/gitlab!22262

changelogs/unreleased/fix-on-train-method-in-mr.yml

+---
+title: Fix RefreshMergeRequestsService raises an exception and unnecessary sidekiq retry
+merge_request: 22262
+author:
+type: fixed

ee/app/models/ee/merge_request.rb

@@ -81,7 +81,7 @@ module EE
     end
 
     def on_train?
-      merge_train.present?
+      merge_train&.active?
     end
 
     def allows_multiple_assignees?

ee/app/models/merge_train.rb

@@ -3,6 +3,8 @@
 class MergeTrain < ApplicationRecord
   include AfterCommitQueue
 
+  ACTIVE_STATUSES = %w[created stale fresh].freeze
+
   belongs_to :target_project, class_name: "Project"
   belongs_to :merge_request, inverse_of: :merge_train
   belongs_to :user
@@ -59,7 +61,7 @@ class MergeTrain < ApplicationRecord
     end
 
     def active_statuses
-      statuses.values_at(:created, :stale, :fresh)
+      statuses.values_at(*ACTIVE_STATUSES)
     end
 
     def merged_statuses
@@ -117,6 +119,10 @@ class MergeTrain < ApplicationRecord
     merge_request.cleanup_refs(only: :train)
   end
 
+  def active?
+    ACTIVE_STATUSES.include?(status)
+  end
+
   private
 
   def refresh_async

ee/app/services/merge_trains/refresh_merge_requests_service.rb

@@ -33,8 +33,9 @@ module MergeTrains
       end
 
       if result[:status] == :finished && result[:new_items].present?
-        first_merge_request = MergeTrain.first_in_train_from(result[:new_items])
-        AutoMergeProcessWorker.perform_async(first_merge_request.id)
+        MergeTrain.first_in_train_from(result[:new_items]).try do |merge_request|
+          AutoMergeProcessWorker.perform_async(merge_request.id)
+        end
       end
     end
 

ee/spec/factories/merge_requests.rb

@@ -11,6 +11,7 @@ FactoryBot.modify do
     trait :on_train do
       transient do
         train_creator { author }
+        status { 'created' }
       end
 
       auto_merge_enabled { true }
@@ -18,7 +19,8 @@ FactoryBot.modify do
       merge_user { train_creator }
 
       after :create do |merge_request, evaluator|
-        merge_request.create_merge_train(user: evaluator.train_creator,
+        merge_request.create_merge_train(status: evaluator.status,
+                                         user: evaluator.train_creator,
                                          target_project: merge_request.target_project,
                                          target_branch: merge_request.target_branch)
       end

ee/spec/models/merge_request_spec.rb

@@ -745,6 +745,14 @@ describe MergeRequest do
       it { is_expected.to be_truthy }
     end
 
+    context 'when the merge request was on a merge train' do
+      let(:merge_request) do
+        create(:merge_request, :on_train, status: 'merged', source_project: project, target_project: project)
+      end
+
+      it { is_expected.to be_falsy }
+    end
+
     context 'when the merge request is not on a merge train' do
       let(:merge_request) do
         create(:merge_request, source_project: project, target_project: project)

ee/spec/models/merge_train_spec.rb

@@ -569,6 +569,22 @@ describe MergeTrain do
     end
   end
 
+  describe '#active?' do
+    subject { merge_train.active? }
+
+    context 'when status is created' do
+      let(:merge_train) { create(:merge_train, :created) }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'when status is merged' do
+      let(:merge_train) { create(:merge_train, :merged) }
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
   def create_merge_request_on_train(target_project: project, target_branch: 'master', source_project: project, source_branch: 'feature')
     create(:merge_request,
       :on_train,

ee/spec/services/merge_trains/refresh_merge_requests_service_spec.rb

@@ -112,6 +112,19 @@ describe MergeTrains::RefreshMergeRequestsService do
         end
       end
 
+      context 'when merge request 1 was on a merge train' do
+        before do
+          allow(merge_request_1.merge_train).to receive(:cleanup_ref)
+          merge_request_1.merge_train.merged!
+        end
+
+        it 'does not refresh' do
+          expect(refresh_service_1).not_to receive(:execute).with(merge_request_1)
+
+          subject
+        end
+      end
+
       context 'when the other thread has already been processing the merge train' do
         let(:lock_key) { "batch_pop_queueing:lock:merge_trains:#{merge_request.target_project_id}:#{merge_request.target_branch}" }
 
@@ -177,6 +190,19 @@ describe MergeTrains::RefreshMergeRequestsService do
 
           subject
         end
+
+        context 'when merge request 1 has already been merged' do
+          before do
+            allow(merge_request_1.merge_train).to receive(:cleanup_ref)
+            merge_request_1.merge_train.merged!
+          end
+
+          it 'does not refresh the merge request 1' do
+            expect(AutoMergeProcessWorker).not_to receive(:perform_async).with(merge_request_1.id)
+
+            subject
+          end
+        end
       end
     end
   end