Merge branch 'fix-pipeline-creation-race-conditions' into 'master'

Make status transition resilient for downstream pipeline creation

See merge request gitlab-org/gitlab!25706

app/models/ci/bridge.rb

@@ -62,6 +62,10 @@ module Ci
       end
     end
 
+    def has_downstream_pipeline?
+      sourced_pipelines.exists?
+    end
+
     def downstream_pipeline_params
       return child_params if triggers_child_pipeline?
       return cross_project_params if downstream_project.present?

app/models/ci/pipeline.rb

@@ -227,6 +227,7 @@ module Ci
       end
 
       after_transition created: :pending do |pipeline|
+        next if Feature.enabled?(:ci_drop_bridge_on_downstream_errors, pipeline.project, default_enabled: true)
         next unless pipeline.bridge_triggered?
         next if pipeline.bridge_waiting?
 
@@ -756,6 +757,8 @@ module Ci
       raise BridgeStatusError unless source_bridge.active?
 
       source_bridge.success!
+    rescue => e
+      Gitlab::ErrorTracking.track_exception(e, pipeline_id: id)
     end
 
     def bridge_triggered?
@@ -774,6 +777,10 @@ module Ci
       child_pipelines.exists?
     end
 
+    def created_successfully?
+      persisted? && failure_reason.blank?
+    end
+
     def detailed_status(current_user)
       Gitlab::Ci::Status::Pipeline::Factory
         .new(self, current_user)

app/services/ci/create_cross_project_pipeline_service.rb

@@ -5,9 +5,19 @@ module Ci
   class CreateCrossProjectPipelineService < ::BaseService
     include Gitlab::Utils::StrongMemoize
 
+    DuplicateDownstreamPipelineError = Class.new(StandardError)
+
     def execute(bridge)
       @bridge = bridge
 
+      if bridge.has_downstream_pipeline?
+        Gitlab::ErrorTracking.track_exception(
+          DuplicateDownstreamPipelineError.new,
+          bridge_id: @bridge.id, project_id: @bridge.project_id
+        )
+        return
+      end
+
       pipeline_params = @bridge.downstream_pipeline_params
       target_ref = pipeline_params.dig(:target_revision, :ref)
 
@@ -18,14 +28,32 @@ module Ci
         current_user,
         pipeline_params.fetch(:target_revision))
 
-      service.execute(
+      downstream_pipeline = service.execute(
         pipeline_params.fetch(:source), pipeline_params[:execute_params]) do |pipeline|
           pipeline.variables.build(@bridge.downstream_variables)
         end
+
+      downstream_pipeline.tap do |pipeline|
+        next if Feature.disabled?(:ci_drop_bridge_on_downstream_errors, project, default_enabled: true)
+
+        update_bridge_status!(@bridge, pipeline)
+      end
     end
 
     private
 
+    def update_bridge_status!(bridge, pipeline)
+      Gitlab::OptimisticLocking.retry_lock(bridge) do |subject|
+        if pipeline.created_successfully?
+          # If bridge uses `strategy:depend` we leave it running
+          # and update the status when the downstream pipeline completes.
+          subject.success! unless subject.dependent?
+        else
+          subject.drop!(:downstream_pipeline_creation_failed)
+        end
+      end
+    end
+
     def ensure_preconditions!(target_ref)
       unless downstream_project_accessible?
         @bridge.drop!(:downstream_bridge_project_not_found)

changelogs/unreleased/fix-pipeline-creation-race-conditions.yml

+---
+title: Drop bridge if downstream pipeline has errors
+merge_request: 25706
+author:
+type: fixed

spec/models/ci/pipeline_spec.rb

@@ -2813,6 +2813,30 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#created_successfully?' do
+    subject { pipeline.created_successfully? }
+
+    context 'when pipeline is not persisted' do
+      let(:pipeline) { build(:ci_pipeline) }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when pipeline is persisted' do
+      context 'when pipeline has failure reasons' do
+        let(:pipeline) { create(:ci_pipeline, failure_reason: :config_error) }
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when pipeline has no failure reasons' do
+        let(:pipeline) { create(:ci_pipeline, failure_reason: nil) }
+
+        it { is_expected.to be_truthy }
+      end
+    end
+  end
+
   describe '#parent_pipeline' do
     let(:project) { create(:project) }
     let(:pipeline) { create(:ci_pipeline, project: project) }
@@ -2960,8 +2984,7 @@ describe Ci::Pipeline, :mailer do
           it 'can not update bridge status if is not active' do
             bridge.success!
 
-            expect { pipeline.update_bridge_status! }
-              .to raise_error Ci::Pipeline::BridgeStatusError
+            expect { pipeline.update_bridge_status! }.not_to change { bridge.status }
           end
         end
       end
@@ -2992,9 +3015,12 @@ describe Ci::Pipeline, :mailer do
         end
 
         describe '#update_bridge_status!' do
-          it 'can not update upstream job status' do
-            expect { pipeline.update_bridge_status! }
-              .to raise_error ArgumentError
+          it 'tracks an ArgumentError and does not update upstream job status' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:track_exception)
+              .with(instance_of(ArgumentError), pipeline_id: pipeline.id)
+
+            pipeline.update_bridge_status!
           end
         end
       end

spec/services/ci/create_cross_project_pipeline_service_spec.rb

@@ -116,6 +116,28 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
       expect(bridge.reload).to be_success
     end
 
+    context 'when bridge job has already any downstream pipelines' do
+      before do
+        bridge.sourced_pipelines.create!(
+          source_pipeline: bridge.pipeline,
+          source_project: bridge.project,
+          project: bridge.project,
+          pipeline: create(:ci_pipeline, project: bridge.project)
+        )
+      end
+
+      it 'logs an error and exits' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception)
+          .with(
+            instance_of(Ci::CreateCrossProjectPipelineService::DuplicateDownstreamPipelineError),
+            bridge_id: bridge.id, project_id: bridge.project.id)
+          .and_call_original
+        expect(Ci::CreatePipelineService).not_to receive(:new)
+        expect(service.execute(bridge)).to be_nil
+      end
+    end
+
     context 'when target ref is not specified' do
       let(:trigger) do
         { trigger: { project: downstream_project.full_path } }
@@ -149,13 +171,11 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
         expect(pipeline.source_bridge).to be_a ::Ci::Bridge
       end
 
-      it 'does not update bridge status when downstream pipeline gets processed' do
+      it 'updates the bridge status when downstream pipeline gets processed' do
         pipeline = service.execute(bridge)
 
         expect(pipeline.reload).to be_failed
-        # TODO: This should change to failed once #198354 gets fixed.
-        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25706
-        expect(bridge.reload).to be_pending
+        expect(bridge.reload).to be_failed
       end
     end
 
@@ -242,6 +262,22 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
 
         it_behaves_like 'creates a child pipeline'
 
+        it 'updates the bridge job to success' do
+          expect { service.execute(bridge) }.to change { bridge.status }.to 'success'
+        end
+
+        context 'when bridge uses "depend" strategy' do
+          let(:trigger) do
+            {
+              trigger: { include: 'child-pipeline.yml', strategy: 'depend' }
+            }
+          end
+
+          it 'does not update the bridge job status' do
+            expect { service.execute(bridge) }.not_to change { bridge.status }
+          end
+        end
+
         context 'when latest sha for the ref changed in the meantime' do
           before do
             upstream_project.repository.create_file(
@@ -298,6 +334,34 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
       end
     end
 
+    context 'when downstream pipeline creation errors out' do
+      let(:stub_config) { false }
+
+      before do
+        stub_ci_pipeline_yaml_file(YAML.dump(invalid: { yaml: 'error' }))
+      end
+
+      it 'creates only one new pipeline' do
+        expect { service.execute(bridge) }
+          .to change { Ci::Pipeline.count }.by(1)
+      end
+
+      it 'creates a new pipeline in the downstream project' do
+        pipeline = service.execute(bridge)
+
+        expect(pipeline.user).to eq bridge.user
+        expect(pipeline.project).to eq downstream_project
+      end
+
+      it 'drops the bridge' do
+        pipeline = service.execute(bridge)
+
+        expect(pipeline.reload).to be_failed
+        expect(bridge.reload).to be_failed
+        expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed')
+      end
+    end
+
     context 'when bridge job has YAML variables defined' do
       before do
         bridge.yaml_variables = [{ key: 'BRIDGE', value: 'var', public: true }]