Merge branch 'fix-merge-request-pipeline-exist-method' into 'master'

Fix duplicate merge request pipelines created by Sidekiq worker retry

See merge request gitlab-org/gitlab-ce!26643

11 files changed, 34 insertions, 33 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index c2a1487fc6e..df162e4844c 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -66,7 +66,7 @@ class MergeRequest < ApplicationRecord
     dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
-  has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
+  has_many :pipelines_for_merge_request, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
   has_many :suggestions, through: :notes
 
   has_many :merge_request_assignees
@@ -1157,10 +1157,6 @@ class MergeRequest < ApplicationRecord
     end
   end
 
-  def merge_request_pipeline_exists?
-    merge_request_pipelines.exists?(sha: diff_head_sha)
-  end
-
   def has_test_reports?
     actual_head_pipeline&.has_reports?(Ci::JobArtifact.test_reports)
   end
@@ -1379,12 +1375,12 @@ class MergeRequest < ApplicationRecord
     source_project.repository.squash_in_progress?(id)
   end
 
-  private
-
   def find_actual_head_pipeline
     all_pipelines.for_sha_or_source_sha(diff_head_sha).first
   end
 
+  private
+
   def source_project_variables
     Gitlab::Ci::Variables::Collection.new.tap do |variables|
       break variables unless source_project
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index a9dd26c02ad..bb9062e9b40 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -81,7 +81,7 @@ module MergeRequests
       ##
       # UpdateMergeRequestsWorker could be retried by an exception.
       # pipelines for merge request should not be recreated in such case.
-      return false if merge_request.merge_request_pipeline_exists?
+      return false if merge_request.find_actual_head_pipeline&.triggered_by_merge_request?
       return false if merge_request.has_no_commits?
 
       true
diff --git a/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml b/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml
new file mode 100644
index 00000000000..294a665ff3e
--- /dev/null
+++ b/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml
@@ -0,0 +1,5 @@
+---
+title: Fix duplicate merge request pipelines created by Sidekiq worker retry
+merge_request: 26643
+author:
+type: fixed
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index abf0e6bccb7..e8df5094b83 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -119,7 +119,7 @@ FactoryBot.define do
 
     trait :with_legacy_detached_merge_request_pipeline do
       after(:create) do |merge_request|
-        merge_request.merge_request_pipelines << create(:ci_pipeline,
+        merge_request.pipelines_for_merge_request << create(:ci_pipeline,
           source: :merge_request_event,
           merge_request: merge_request,
           project: merge_request.source_project,
@@ -130,7 +130,7 @@ FactoryBot.define do
 
     trait :with_detached_merge_request_pipeline do
       after(:create) do |merge_request|
-        merge_request.merge_request_pipelines << create(:ci_pipeline,
+        merge_request.pipelines_for_merge_request << create(:ci_pipeline,
           source: :merge_request_event,
           merge_request: merge_request,
           project: merge_request.source_project,
@@ -147,7 +147,7 @@ FactoryBot.define do
       end
 
       after(:create) do |merge_request, evaluator|
-        merge_request.merge_request_pipelines << create(:ci_pipeline,
+        merge_request.pipelines_for_merge_request << create(:ci_pipeline,
           source: :merge_request_event,
           merge_request: merge_request,
           project: merge_request.source_project,
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 482e9c05da8..2242543daad 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -99,7 +99,7 @@ merge_requests:
 - timelogs
 - head_pipeline
 - latest_merge_request_diff
-- merge_request_pipelines
+- pipelines_for_merge_request
 - merge_request_assignees
 - suggestions
 - assignees
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 8c73f37bd32..9b489baf163 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2817,7 +2817,7 @@ describe Ci::Build do
 
     context 'when ref is merge request' do
       let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
-      let(:pipeline) { merge_request.merge_request_pipelines.first }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
       let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
 
       context 'when ref is protected' do
@@ -2875,7 +2875,7 @@ describe Ci::Build do
 
     context 'when ref is merge request' do
       let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
-      let(:pipeline) { merge_request.merge_request_pipelines.first }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
       let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
 
       context 'when ref is protected' do
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index af455a72f50..a0319b3eb0a 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -466,7 +466,7 @@ describe Ci::Pipeline, :mailer do
           target_branch: 'master')
       end
 
-      let(:pipeline) { merge_request.merge_request_pipelines.first }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
 
       it 'does not return the pipeline' do
         is_expected.to be_empty
diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb
index 6805731fed3..66b25c77430 100644
--- a/spec/models/concerns/has_ref_spec.rb
+++ b/spec/models/concerns/has_ref_spec.rb
@@ -19,7 +19,7 @@ describe HasRef do
 
       context 'when it was triggered by merge request' do
         let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
-        let(:pipeline) { merge_request.merge_request_pipelines.first }
+        let(:pipeline) { merge_request.pipelines_for_merge_request.first }
         let(:build) { create(:ci_build, pipeline: pipeline) }
 
         it 'returns false' do
@@ -68,7 +68,7 @@ describe HasRef do
 
     context 'when it is triggered by a merge request' do
       let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
-      let(:pipeline) { merge_request.merge_request_pipelines.first }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
       let(:build) { create(:ci_build, tag: false, pipeline: pipeline) }
 
       it 'returns nil' do
diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb
index dba7fd91747..47f767ae4ab 100644
--- a/spec/serializers/pipeline_entity_spec.rb
+++ b/spec/serializers/pipeline_entity_spec.rb
@@ -137,7 +137,7 @@ describe PipelineEntity do
     context 'when pipeline is detached merge request pipeline' do
       let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
       let(:project) { merge_request.target_project }
-      let(:pipeline) { merge_request.merge_request_pipelines.first }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
 
       it 'makes detached flag true' do
         expect(subject[:flags][:detached_merge_request_pipeline]).to be_truthy
@@ -185,7 +185,7 @@ describe PipelineEntity do
     context 'when pipeline is merge request pipeline' do
       let(:merge_request) { create(:merge_request, :with_merge_request_pipeline, merge_sha: 'abc') }
       let(:project) { merge_request.target_project }
-      let(:pipeline) { merge_request.merge_request_pipelines.first }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
 
       it 'makes detached flag false' do
         expect(subject[:flags][:detached_merge_request_pipeline]).to be_falsy
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index c795176a1e4..ed48f4b1e44 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -195,7 +195,7 @@ describe MergeRequests::CreateService do
             expect(merge_request).to be_persisted
 
             merge_request.reload
-            expect(merge_request.merge_request_pipelines.count).to eq(1)
+            expect(merge_request.pipelines_for_merge_request.count).to eq(1)
             expect(merge_request.actual_head_pipeline).to be_detached_merge_request_pipeline
           end
 
@@ -247,7 +247,7 @@ describe MergeRequests::CreateService do
               expect(merge_request).to be_persisted
 
               merge_request.reload
-              expect(merge_request.merge_request_pipelines.count).to eq(0)
+              expect(merge_request.pipelines_for_merge_request.count).to eq(0)
             end
           end
 
@@ -281,7 +281,7 @@ describe MergeRequests::CreateService do
             expect(merge_request).to be_persisted
 
             merge_request.reload
-            expect(merge_request.merge_request_pipelines.count).to eq(0)
+            expect(merge_request.pipelines_for_merge_request.count).to eq(0)
           end
         end
       end
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index d20b2d81763..7258428589f 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -166,8 +166,8 @@ describe MergeRequests::RefreshService do
 
         it 'create detached merge request pipeline with commits' do
           expect { subject }
-            .to change { @merge_request.merge_request_pipelines.count }.by(1)
-            .and change { @another_merge_request.merge_request_pipelines.count }.by(0)
+            .to change { @merge_request.pipelines_for_merge_request.count }.by(1)
+            .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0)
 
           expect(@merge_request.has_commits?).to be_truthy
           expect(@another_merge_request.has_commits?).to be_falsy
@@ -175,13 +175,13 @@ describe MergeRequests::RefreshService do
 
         it 'does not create detached merge request pipeline for forked project' do
           expect { subject }
-            .not_to change { @fork_merge_request.merge_request_pipelines.count }
+            .not_to change { @fork_merge_request.pipelines_for_merge_request.count }
         end
 
         it 'create detached merge request pipeline for non-fork merge request' do
           subject
 
-          expect(@merge_request.merge_request_pipelines.first)
+          expect(@merge_request.pipelines_for_merge_request.first)
             .to be_detached_merge_request_pipeline
         end
 
@@ -190,7 +190,7 @@ describe MergeRequests::RefreshService do
 
           it 'does not create detached merge request pipeline' do
             expect { subject }
-              .not_to change { @merge_request.merge_request_pipelines.count }
+              .not_to change { @merge_request.pipelines_for_merge_request.count }
           end
         end
 
@@ -199,9 +199,9 @@ describe MergeRequests::RefreshService do
 
           it 'creates legacy detached merge request pipeline for fork merge request' do
             expect { subject }
-              .to change { @fork_merge_request.merge_request_pipelines.count }.by(1)
+              .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1)
 
-            expect(@fork_merge_request.merge_request_pipelines.first)
+            expect(@fork_merge_request.pipelines_for_merge_request.first)
               .to be_legacy_detached_merge_request_pipeline
           end
         end
@@ -214,7 +214,7 @@ describe MergeRequests::RefreshService do
           it 'create legacy detached merge request pipeline for non-fork merge request' do
             subject
 
-            expect(@merge_request.merge_request_pipelines.first)
+            expect(@merge_request.pipelines_for_merge_request.first)
               .to be_legacy_detached_merge_request_pipeline
           end
         end
@@ -245,11 +245,11 @@ describe MergeRequests::RefreshService do
           it 'does not re-create a duplicate detached merge request pipeline' do
             expect do
               service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master')
-            end.to change { @merge_request.merge_request_pipelines.count }.by(1)
+            end.to change { @merge_request.pipelines_for_merge_request.count }.by(1)
 
             expect do
               service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master')
-            end.not_to change { @merge_request.merge_request_pipelines.count }
+            end.not_to change { @merge_request.pipelines_for_merge_request.count }
           end
         end
       end
@@ -266,7 +266,7 @@ describe MergeRequests::RefreshService do
 
         it 'does not create a detached merge request pipeline' do
           expect { subject }
-            .not_to change { @merge_request.merge_request_pipelines.count }
+            .not_to change { @merge_request.pipelines_for_merge_request.count }
         end
       end
     end