Loosen rule to detect cyclical pipelines

The rule to detect whether upstream/downstream
pipelines chain had an infinite loop was too
strict.
To soften this rule we distinguish
pipelines also by their source and add another
level of depth to avoid false positives.

Changelog: changed

app/services/ci/create_downstream_pipeline_service.rb

@@ -125,7 +125,9 @@ module Ci
           config_checksum(pipeline) unless pipeline.child?
         end
 
-        pipeline_checksums.uniq.length != pipeline_checksums.length
+        # To avoid false positives we allow 1 cycle in the ancestry and
+        # fail when 2 cycles are detected: A -> B -> A -> B -> A
+        pipeline_checksums.tally.any? { |_checksum, occurrences| occurrences > 2 }
       end
     end
 
@@ -137,7 +139,7 @@ module Ci
     end
 
     def config_checksum(pipeline)
-      [pipeline.project_id, pipeline.ref].hash
+      [pipeline.project_id, pipeline.ref, pipeline.source].hash
     end
   end
 end

spec/services/ci/create_downstream_pipeline_service_spec.rb

@@ -441,17 +441,8 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
       end
     end
 
-    context 'when relationship between pipelines is cyclical' do
-      before do
-        pipeline_a = create(:ci_pipeline, project: upstream_project)
-        pipeline_b = create(:ci_pipeline, project: downstream_project)
-        pipeline_c = create(:ci_pipeline, project: upstream_project)
-
-        create_source_pipeline(pipeline_a, pipeline_b)
-        create_source_pipeline(pipeline_b, pipeline_c)
-        create_source_pipeline(pipeline_c, upstream_pipeline)
-      end
-
+    describe 'cyclical dependency detection' do
+      shared_examples 'detects cyclical pipelines' do
         it 'does not create a new pipeline' do
           expect { service.execute(bridge) }
             .not_to change { Ci::Pipeline.count }
@@ -463,12 +454,9 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
           expect(bridge.reload).to be_failed
           expect(bridge.failure_reason).to eq 'pipeline_loop_detected'
         end
-
-      context 'when ci_drop_cyclical_triggered_pipelines is not enabled' do
-        before do
-          stub_feature_flags(ci_drop_cyclical_triggered_pipelines: false)
       end
 
+      shared_examples 'passes cyclical pipeline precondition' do
         it 'creates a new pipeline' do
           expect { service.execute(bridge) }
             .to change { Ci::Pipeline.count }
@@ -480,6 +468,73 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
           expect(bridge.reload).not_to be_failed
         end
       end
+
+      context 'when pipeline ancestry contains 2 cycles of dependencies' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(push on master) ->
+          #   B(pipeline on master) -> A(push on master)
+          pipeline_1 = create(:ci_pipeline, project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, project: downstream_project, source: :pipeline)
+          pipeline_3 = create(:ci_pipeline, project: upstream_project, source: :push)
+          pipeline_4 = create(:ci_pipeline, project: downstream_project, source: :pipeline)
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, pipeline_3)
+          create_source_pipeline(pipeline_3, pipeline_4)
+          create_source_pipeline(pipeline_4, upstream_pipeline)
+        end
+
+        it_behaves_like 'detects cyclical pipelines'
+
+        context 'when ci_drop_cyclical_triggered_pipelines is not enabled' do
+          before do
+            stub_feature_flags(ci_drop_cyclical_triggered_pipelines: false)
+          end
+
+          it_behaves_like 'passes cyclical pipeline precondition'
+        end
+      end
+
+      context 'when source in the ancestry differ' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(pipeline on master)
+          pipeline_1 = create(:ci_pipeline, project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, project: downstream_project, source: :pipeline)
+          upstream_pipeline.update!(source: :pipeline)
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, upstream_pipeline)
+        end
+
+        it_behaves_like 'passes cyclical pipeline precondition'
+      end
+
+      context 'when ref in the ancestry differ' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(push on feature-1)
+          pipeline_1 = create(:ci_pipeline, ref: 'master', project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, ref: 'master', project: downstream_project, source: :pipeline)
+          upstream_pipeline.update!(ref: 'feature-1')
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, upstream_pipeline)
+        end
+
+        it_behaves_like 'passes cyclical pipeline precondition'
+      end
+
+      context 'when only 1 cycle is detected' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(push on master)
+          pipeline_1 = create(:ci_pipeline, ref: 'master', project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, ref: 'master', project: downstream_project, source: :pipeline)
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, upstream_pipeline)
+        end
+
+        it_behaves_like 'passes cyclical pipeline precondition'
+      end
     end
 
     context 'when downstream pipeline creation errors out' do