Proactively marking MRs in train as stale

This commit Proactively marking MRs in train as stale

doc/ci/merge_request_pipelines/pipelines_for_merged_results/merge_trains/index.md

@@ -6,6 +6,7 @@ last_update: 2019-07-03
 # Merge Trains **(PREMIUM)**
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/9186) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.0.
+> [Squash and merge](../../../../user/project/merge_requests/squash_and_merge.md) support [introduced](https://gitlab.com/gitlab-org/gitlab/issues/13001) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.6.
 
 [Pipelines for merged results](../index.md#pipelines-for-merged-results-premium) introduces
 running a build on the result of the merged code prior to merging, as a way to keep master green.
@@ -36,7 +37,6 @@ Merge trains have the following requirements and limitations:
   If more than twenty merge requests are added to the merge train, the merge requests
   will be queued until a slot in the merge train is free. There is no limit to the
   number of merge requests that can be queued.
-- This feature does not support [squash and merge](../../../../user/project/merge_requests/squash_and_merge.md).
 
 <i class="fa fa-youtube-play youtube" aria-hidden="true"></i>
 Watch this video for a demonstration on [how parallel execution

ee/app/models/merge_train.rb

@@ -9,12 +9,13 @@ class MergeTrain < ApplicationRecord
   belongs_to :pipeline, class_name: 'Ci::Pipeline'
 
   after_commit :cleanup_ref, if: -> { saved_change_to_status? && merged? }
+  after_commit :refresh_async, if: -> { saved_change_to_status? && stale? }
 
   after_destroy do |merge_train|
     run_after_commit { merge_train.cleanup_ref }
   end
 
-  enum status: %i[created merged]
+  enum status: %i[created merged stale fresh]
 
   scope :active, -> { where(status: active_statuses) }
   scope :merged, -> { where(status: merged_statuses) }
@@ -22,14 +23,14 @@ class MergeTrain < ApplicationRecord
   scope :by_id, -> { order('merge_trains.id ASC') }
 
   class << self
-    def all_active_mrs_in_train(merge_request)
+    def all_active_mrs_in_train(target_project_id, target_branch)
       MergeRequest.joins(:merge_train).merge(
-        MergeTrain.active.for_target(merge_request.target_project_id, merge_request.target_branch).by_id
+        MergeTrain.active.for_target(target_project_id, target_branch).by_id
       )
     end
 
-    def first_in_train(merge_request)
-      all_active_mrs_in_train(merge_request).first
+    def first_in_train(target_project_id, target_branch)
+      all_active_mrs_in_train(target_project_id, target_branch).first
     end
 
     def first_in_trains(project)
@@ -38,19 +39,24 @@ class MergeTrain < ApplicationRecord
 
     def first_in_train_from(merge_request_ids)
       merge_request = MergeRequest.find(merge_request_ids.first)
-      all_active_mrs_in_train(merge_request).where(id: merge_request_ids).first
+      all_active_mrs_in_train(merge_request.target_project_id, merge_request.target_branch).where(id: merge_request_ids).first
     end
 
     def last_merged_mr_in_train(target_project_id, target_branch)
       MergeRequest.where(id: last_merged_merge_train(target_project_id, target_branch)).take
     end
 
+    def sha_exists_in_history?(target_project_id, target_branch, newrev, limit: 20)
+      MergeRequest.exists?(id: merged_merge_trains(target_project_id, target_branch, limit: limit),
+                           merge_commit_sha: newrev)
+    end
+
     def total_count_in_train(merge_request)
-      all_active_mrs_in_train(merge_request).count
+      all_active_mrs_in_train(merge_request.target_project_id, merge_request.target_branch).count
     end
 
     def active_statuses
-      statuses.values_at(:created)
+      statuses.values_at(:created, :stale, :fresh)
     end
 
     def merged_statuses
@@ -67,17 +73,21 @@ class MergeTrain < ApplicationRecord
     end
 
     def last_merged_merge_train(target_project_id, target_branch)
+      merged_merge_trains(target_project_id, target_branch, limit: 1)
+    end
+
+    def merged_merge_trains(target_project_id, target_branch, limit:)
       MergeTrain.for_target(target_project_id, target_branch)
-        .merged.order(id: :desc).select(:merge_request_id).limit(1)
+        .merged.order(id: :desc).select(:merge_request_id).limit(limit)
     end
   end
 
   def all_next
-    self.class.all_active_mrs_in_train(merge_request).where('merge_trains.id > ?', id)
+    self.class.all_active_mrs_in_train(target_project_id, target_branch).where('merge_trains.id > ?', id)
   end
 
   def all_prev
-    self.class.all_active_mrs_in_train(merge_request).where('merge_trains.id < ?', id)
+    self.class.all_active_mrs_in_train(target_project_id, target_branch).where('merge_trains.id < ?', id)
   end
 
   def next
@@ -103,4 +113,10 @@ class MergeTrain < ApplicationRecord
   def cleanup_ref
     merge_request.cleanup_refs(only: :train)
   end
+
+  private
+
+  def refresh_async
+    AutoMergeProcessWorker.perform_async(merge_request_id)
+  end
 end

ee/app/services/auto_merge/merge_train_service.rb

@@ -26,7 +26,7 @@ module AutoMerge
       super do
         if merge_request.merge_train&.destroy
           SystemNoteService.cancel_merge_train(merge_request, project, current_user)
-          AutoMergeProcessWorker.perform_async(next_merge_request.id) if next_merge_request
+          next_merge_request.merge_train.stale! if next_merge_request
         end
       end
     end
@@ -39,7 +39,7 @@ module AutoMerge
       super(merge_request, reason) do
         if merge_request.merge_train&.destroy
           SystemNoteService.abort_merge_train(merge_request, project, current_user, reason)
-          AutoMergeProcessWorker.perform_async(next_merge_request.id) if next_merge_request && process_next
+          next_merge_request.merge_train.stale! if next_merge_request && process_next
         end
       end
     end

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -11,6 +11,7 @@ module EE
       def refresh_merge_requests!
         update_approvers
         reset_approvals_for_merge_requests(push.ref, push.newrev)
+        check_merge_train_status
 
         super
       end
@@ -44,6 +45,13 @@ module EE
           ::MergeRequests::SyncReportApproverApprovalRules.new(merge_request).execute if project.feature_available?(:report_approver_rules)
         end
       end
+
+      # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      def check_merge_train_status
+        MergeTrains::CheckStatusService.new(project, current_user)
+          .execute(project, @push.branch_name, @push.newrev)
+      end
+      # rubocop:enable Gitlab/ModuleWithInstanceVariables
     end
   end
 end

ee/app/services/merge_trains/check_status_service.rb

+# frozen_string_literal: true
+
+module MergeTrains
+  class CheckStatusService < BaseService
+    def execute(target_project, target_branch, newrev)
+      return unless target_project.merge_trains_enabled?
+
+      # If the new revision doesn't exist in the merge train history,
+      # that means there was an unexpected commit came from out of merge train cycle.
+      unless MergeTrain.sha_exists_in_history?(target_project.id, target_branch, newrev)
+        merge_request = MergeTrain.first_in_train(target_project.id, target_branch)
+        merge_request.merge_train.stale! if merge_request
+      end
+    end
+  end
+end

ee/app/services/merge_trains/refresh_merge_request_service.rb

@@ -33,7 +33,7 @@ module MergeTrains
         raise ProcessError, 'merge request is not on a merge train'
       end
 
-      if merge_request.squash?
+      if merge_request.squash? && Feature.disabled?(:merge_train_new_stale_check, project, default_enabled: true)
         raise ProcessError, 'merge train does not support squash merge'
       end
 
@@ -80,9 +80,17 @@ module MergeTrains
       raise ProcessError, "failed to merge. #{merge_request.merge_error}" unless merge_request.merged?
     end
 
+    def stale_pipeline?
+      if Feature.enabled?(:merge_train_new_stale_check, project, default_enabled: true)
+        merge_train.stale?
+      else
+        legacy_stale_pipeline?
+      end
+    end
+
     # NOTE: This method works for both no-ff-merge and ff-merge, however,
     #       it doesn't work for squash and merge option.
-    def stale_pipeline?
+    def legacy_stale_pipeline?
       return true unless pipeline_for_merge_train.source_sha == merge_request.diff_head_sha
       return false if pipeline_for_merge_train.target_sha == previous_ref_sha
 
@@ -120,7 +128,8 @@ module MergeTrains
     end
 
     def update_pipeline_for_merge_train(pipeline)
-      merge_train.update!(pipeline: pipeline)
+      merge_train.pipeline = pipeline
+      merge_train.fresh!
     end
 
     def merge_user

ee/spec/factories/merge_trains.rb

@@ -8,8 +8,20 @@ FactoryBot.define do
     user
     pipeline factory: :ci_pipeline
 
+    trait :created do
+      status { :created }
+    end
+
     trait :merged do
       status { :merged }
     end
+
+    trait :stale do
+      status { :stale }
+    end
+
+    trait :fresh do
+      status { :fresh }
+    end
   end
 end

ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb

@@ -155,7 +155,7 @@ describe 'Two merge requests on a merge train' do
     end
   end
 
-  context 'when merge request 1 is canceled by a user' do
+  context 'when merge request 1 is canceled by a user', :sidekiq_inline do
     before do
       AutoMergeService.new(project, maintainer_1).cancel(merge_request_1)
 
@@ -217,11 +217,10 @@ describe 'Two merge requests on a merge train' do
     end
   end
 
-  context 'when master got a new commit and pipeline for merge request 1 finished', :sidekiq_might_not_need_inline do
+  context 'when master got a new commit', :sidekiq_inline do
     before do
-      create_file_in_repo(project, 'master', 'master', 'test.txt', 'This is test')
+      push_commit_to_master
 
-      merge_request_1.merge_train.pipeline.succeed!
       merge_request_1.reload
       merge_request_2.reload
     end
@@ -238,6 +237,11 @@ describe 'Two merge requests on a merge train' do
         .to eq(project.repository.commit(merge_request_1.train_ref_path).sha)
     end
 
+    it 'does not recreate pipeline when merge request 1 refreshed again' do
+      expect { merge_request_1.merge_train.send(:refresh_async) }
+        .not_to change { merge_request_1.all_pipelines.count }
+    end
+
     context 'when the pipeline for merge request 1 succeeded' do
       before do
         merge_request_1.merge_train.pipeline.succeed!
@@ -263,5 +267,12 @@ describe 'Two merge requests on a merge train' do
         end
       end
     end
+
+    def push_commit_to_master
+      create_file_in_repo(project, 'master', 'master', 'test.txt', 'This is test')
+      changes = Base64.encode64("123456 789012 refs/heads/master")
+      key_id = create(:key, user: project.owner).shell_id
+      PostReceive.new.perform("project-#{project.id}", key_id, changes)
+    end
   end
 end

ee/spec/models/merge_train_spec.rb

@@ -15,22 +15,27 @@ describe MergeTrain do
     allow(AutoMergeProcessWorker).to receive(:perform_async)
   end
 
+  shared_context 'various merge trains' do
+    let_it_be(:merge_train_created) { create(:merge_train, :created) }
+    let_it_be(:merge_train_stale) { create(:merge_train, :stale) }
+    let_it_be(:merge_train_fresh) { create(:merge_train, :fresh) }
+    let_it_be(:merge_train_merged) { create(:merge_train, :merged) }
+  end
+
   describe '.active' do
     subject { described_class.active }
 
-    let!(:merge_train_created) { create(:merge_train) }
-    let!(:merge_train_merged) { create(:merge_train, :merged) }
+    include_context 'various merge trains'
 
     it 'returns only active merge trains' do
-      is_expected.to contain_exactly(merge_train_created)
+      is_expected.to contain_exactly(merge_train_created, merge_train_stale, merge_train_fresh)
     end
   end
 
   describe '.merged' do
     subject { described_class.merged }
 
-    let!(:merge_train_created) { create(:merge_train) }
-    let!(:merge_train_merged) { create(:merge_train, :merged) }
+    include_context 'various merge trains'
 
     it 'returns only merged merge trains' do
       is_expected.to contain_exactly(merge_train_merged)
@@ -74,8 +79,10 @@ describe MergeTrain do
   end
 
   describe '.all_active_mrs_in_train' do
-    subject { described_class.all_active_mrs_in_train(merge_request) }
+    subject { described_class.all_active_mrs_in_train(target_project_id, target_branch) }
 
+    let(:target_project_id) { merge_request.target_project_id }
+    let(:target_branch) { merge_request.target_branch }
     let!(:merge_request) { create_merge_request_on_train }
 
     it 'returns the merge request' do
@@ -108,8 +115,10 @@ describe MergeTrain do
   end
 
   describe '.first_in_train' do
-    subject { described_class.first_in_train(merge_request) }
+    subject { described_class.first_in_train(target_project_id, target_branch) }
 
+    let(:target_project_id) { merge_request.target_project_id }
+    let(:target_branch) { merge_request.target_branch }
     let!(:merge_request) { create_merge_request_on_train }
 
     it 'returns the merge request' do
@@ -250,6 +259,58 @@ describe MergeTrain do
     end
   end
 
+  describe '.sha_exists_in_history?' do
+    subject { described_class.sha_exists_in_history?(target_project_id, target_branch, target_sha, limit: limit) }
+
+    let(:target_project_id) { project.id }
+    let(:target_branch) { 'master' }
+    let(:target_sha) { '' }
+    let(:limit) { 20 }
+
+    context 'when there is a merge request on train' do
+      let!(:merge_request_1) { create_merge_request_on_train }
+      let(:merge_commit_sha_1) { Digest::SHA1.hexdigest 'test-1' }
+      let(:target_sha) { merge_commit_sha_1 }
+
+      context 'when the merge request has already been merged' do
+        before do
+          merge_request_1.merge_train.merged!
+          merge_request_1.update_column(:merge_commit_sha, merge_commit_sha_1)
+        end
+
+        it { is_expected.to eq(true) }
+      end
+
+      context 'when there is another merge request on train and it has been merged' do
+        let!(:merge_request_2) { create_merge_request_on_train(source_branch: 'improve/awesome') }
+        let(:merge_commit_sha_2) { Digest::SHA1.hexdigest 'test-2' }
+        let(:target_sha) { merge_commit_sha_2 }
+
+        before do
+          merge_request_2.merge_train.merged!
+          merge_request_2.update_column(:merge_commit_sha, merge_commit_sha_2)
+        end
+
+        it { is_expected.to eq(true) }
+
+        context 'when limit is 1' do
+          let(:limit) { 1 }
+          let(:target_sha) { merge_commit_sha_1 }
+
+          it { is_expected.to eq(false) }
+        end
+      end
+
+      context 'when the merge request has not been merged yet' do
+        it { is_expected.to eq(false) }
+      end
+    end
+
+    context 'when there are no merge requests on train' do
+      it { is_expected.to eq(false) }
+    end
+  end
+
   describe '.total_count_in_train' do
     subject { described_class.total_count_in_train(merge_request) }
 
@@ -439,6 +500,34 @@ describe MergeTrain do
           merge_train.merged!
         end
       end
+
+      context 'and transits to stale' do
+        it 'refreshes asynchronously' do
+          expect(merge_train).to receive(:refresh_async).once
+
+          merge_train.stale!
+        end
+      end
+    end
+
+    context 'when status is fresh' do
+      let(:merge_train) { create(:merge_train, :fresh) }
+
+      context 'and transits to merged' do
+        it 'cleanup ref' do
+          expect(merge_train).to receive(:cleanup_ref).once
+
+          merge_train.merged!
+        end
+      end
+
+      context 'and transits to stale' do
+        it 'refreshes asynchronously' do
+          expect(merge_train).to receive(:refresh_async).once
+
+          merge_train.stale!
+        end
+      end
     end
 
     context 'when status is merged' do

ee/spec/services/auto_merge/merge_train_service_spec.rb

@@ -154,6 +154,8 @@ describe AutoMerge::MergeTrainService do
         expect(AutoMergeProcessWorker).to receive(:perform_async).with(merge_request_2.id)
 
         subject
+
+        expect(merge_request_2.reset.merge_train).to be_stale
       end
     end
   end
@@ -200,6 +202,8 @@ describe AutoMerge::MergeTrainService do
         expect(AutoMergeProcessWorker).to receive(:perform_async).with(merge_request_2.id)
 
         subject
+
+        expect(merge_request_2.reset.merge_train).to be_stale
       end
 
       context 'when process_next is false' do

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -36,10 +36,20 @@ describe MergeRequests::RefreshService do
   end
   let(:oldrev) { TestEnv::BRANCH_SHA[source_branch] }
   let(:newrev) { TestEnv::BRANCH_SHA['after-create-delete-modify-move'] } # Pretend source_branch is now updated
+  let(:service) { described_class.new(project, current_user) }
+  let(:current_user) { merge_request.author }
 
   subject { service.execute(oldrev, newrev, "refs/heads/#{source_branch}") }
 
   describe '#execute' do
+    it 'checks merge train status' do
+      expect_next_instance_of(MergeTrains::CheckStatusService, project, current_user) do |service|
+        expect(service).to receive(:execute).with(project, source_branch, newrev)
+      end
+
+      subject
+    end
+
     context '#update_approvers' do
       let(:owner) { create(:user) }
       let(:current_user) { merge_request.author }

ee/spec/services/merge_trains/check_status_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeTrains::CheckStatusService do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u)} }
+  let(:service) { described_class.new(project, maintainer) }
+  let(:previous_ref) { 'refs/heads/master' }
+
+  before do
+    stub_licensed_features(merge_pipelines: true, merge_trains: true)
+    project.update!(merge_pipelines_enabled: true)
+  end
+
+  describe '#execute' do
+    subject { service.execute(target_project, target_branch, newrev) }
+
+    let(:target_project) { project }
+    let(:target_branch) { 'master' }
+    let(:newrev) { Digest::SHA1.hexdigest 'test' }
+
+    context 'when there is at least one merge request on the train' do
+      let!(:merged_merge_request) do
+        create(:merge_request, :on_train, train_creator: maintainer,
+          source_branch: 'feature', source_project: project,
+          target_branch: 'master', target_project: project,
+          merge_status: 'unchecked')
+      end
+
+      let!(:active_merge_request) do
+        create(:merge_request, :on_train, train_creator: maintainer,
+          source_branch: 'improve/awesome', source_project: project,
+          target_branch: 'master', target_project: project,
+          merge_status: 'unchecked')
+      end
+
+      before do
+        merged_merge_request.mark_as_merged!
+        merged_merge_request.update_column(:merge_commit_sha, merge_commit_sha_1)
+      end
+
+      context 'when new revision is included in merge train history' do
+        let!(:merge_commit_sha_1) { Digest::SHA1.hexdigest 'test' }
+
+        it 'does not mark merge train as stale' do
+          expect(MergeTrain).to receive(:sha_exists_in_history?).and_return(true).and_call_original
+          expect_any_instance_of(MergeTrain).not_to receive(:stale!)
+
+          subject
+        end
+      end
+
+      context 'when new revision is not included in merge train history' do
+        let!(:merge_commit_sha_1) { Digest::SHA1.hexdigest 'other' }
+
+        it 'marks the merge train as stale' do
+          expect(MergeTrain).to receive(:sha_exists_in_history?).and_return(false).and_call_original
+          expect_any_instance_of(MergeTrain).to receive(:stale!)
+
+          subject
+        end
+      end
+    end
+
+    context 'when there are no merge requests on train' do
+      it 'does not raise error' do
+        expect(MergeTrain).to receive(:sha_exists_in_history?).and_return(false).and_call_original
+
+        expect { subject }.not_to raise_error
+      end
+    end
+
+    context 'when merge train is disabled on the project' do
+      before do
+        project.update!(merge_pipelines_enabled: false)
+      end
+
+      it 'does not check history' do
+        expect(MergeTrain).not_to receive(:sha_exists_in_history?)
+
+        subject
+      end
+    end
+  end
+end

ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb

@@ -47,6 +47,7 @@ describe MergeTrains::RefreshMergeRequestService do
         result = subject
         expect(result[:status]).to eq(:success)
         expect(result[:pipeline_created]).to eq(true)
+        expect(merge_request.merge_train).to be_fresh
       end
     end
 
@@ -121,8 +122,20 @@ describe MergeTrains::RefreshMergeRequestService do
         merge_request.update!(squash: true)
       end
 
-      it_behaves_like 'drops the merge request from the merge train' do
-        let(:expected_reason) { 'merge train does not support squash merge' }
+      context 'when merge_train_new_stale_check feature flag is enabled' do
+        let(:pipeline) { create(:ci_pipeline) }
+
+        it_behaves_like 'creates a pipeline for merge train'
+      end
+
+      context 'when merge_train_new_stale_check feature flag is disabled' do
+        before do
+          stub_feature_flags(merge_train_new_stale_check: false)
+        end
+
+        it_behaves_like 'drops the merge request from the merge train' do
+          let(:expected_reason) { 'merge train does not support squash merge' }
+        end
       end
     end
 
@@ -169,9 +182,23 @@ describe MergeTrains::RefreshMergeRequestService do
       end
 
       context 'when the pipeline is stale' do
-        let(:previous_ref_sha) { project.repository.commits('refs/heads/master', limit: 2).last.sha }
+        context 'when merge_train_new_stale_check feature flag is enabled' do
+          before do
+            merge_request.merge_train.update_column(:status, :stale)
+          end
 
-        it_behaves_like 'cancels and recreates a pipeline for the merge train'
+          it_behaves_like 'cancels and recreates a pipeline for the merge train'
+        end
+
+        context 'when merge_train_new_stale_check feature flag is disabled' do
+          before do
+            stub_feature_flags(merge_train_new_stale_check: false)
+          end
+
+          let(:previous_ref_sha) { project.repository.commits('refs/heads/master', limit: 2).last.sha }
+
+          it_behaves_like 'cancels and recreates a pipeline for the merge train'
+        end
       end
 
       context 'when the pipeline is required to be recreated' do