Merge branch 'fix/gb/do-not-add-todo-before-build-auto-retry' into 'master'

Do not modify todos inside a failed build transaction

See merge request gitlab-org/gitlab!77972

app/models/ci/build.rb

@@ -397,6 +397,10 @@ module Ci
       auto_retry.allowed?
     end
 
+    def auto_retry_expected?
+      failed? && auto_retry_allowed?
+    end
+
     def detailed_status(current_user)
       Gitlab::Ci::Status::Build::Factory
         .new(self.present, current_user)
@@ -1069,12 +1073,7 @@ module Ci
     end
 
     def drop_with_exit_code!(failure_reason, exit_code)
-      ::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/348495') do
-        transaction do
-          conditionally_allow_failure!(exit_code)
-          drop!(failure_reason)
-        end
-      end
+      drop!(::Gitlab::Ci::Build::Status::Reason.new(self, failure_reason, exit_code))
     end
 
     def exit_codes_defined?
@@ -1117,6 +1116,13 @@ module Ci
       end
     end
 
+    def allowed_to_fail_with_code?(exit_code)
+      options
+        .dig(:allow_failure_criteria, :exit_codes)
+        .to_a
+        .include?(exit_code)
+    end
+
     protected
 
     def run_status_commit_hooks!
@@ -1207,21 +1213,6 @@ module Ci
       end
     end
 
-    def conditionally_allow_failure!(exit_code)
-      return unless exit_code
-
-      if allowed_to_fail_with_code?(exit_code)
-        update_columns(allow_failure: true)
-      end
-    end
-
-    def allowed_to_fail_with_code?(exit_code)
-      options
-        .dig(:allow_failure_criteria, :exit_codes)
-        .to_a
-        .include?(exit_code)
-    end
-
     def cache_for_online_runners(&block)
       Rails.cache.fetch(
         ['has-online-runners', id],

app/models/commit_status.rb

@@ -170,8 +170,11 @@ class CommitStatus < Ci::ApplicationRecord
     end
 
     before_transition any => :failed do |commit_status, transition|
-      failure_reason = transition.args.first
-      commit_status.failure_reason = CommitStatus.failure_reasons[failure_reason]
+      reason = ::Gitlab::Ci::Build::Status::Reason
+        .fabricate(commit_status, transition.args.first)
+
+      commit_status.failure_reason = reason.failure_reason_enum
+      commit_status.allow_failure = true if reason.force_allow_failure?
     end
 
     before_transition [:skipped, :manual] => :created do |commit_status, transition|

app/services/ci/retry_build_service.rb

@@ -25,10 +25,6 @@ module Ci
 
         Gitlab::OptimisticLocking.retry_lock(new_build, name: 'retry_build', &:enqueue)
         AfterRequeueJobService.new(project, current_user).execute(build)
-
-        ::MergeRequests::AddTodoWhenBuildFailsService
-          .new(project: project, current_user: current_user)
-          .close(new_build)
       end
     end
 
@@ -43,6 +39,12 @@ module Ci
 
       new_build = clone_build(build)
 
+      new_build.run_after_commit do
+        ::MergeRequests::AddTodoWhenBuildFailsService
+          .new(project: project)
+          .close(new_build)
+      end
+
       if create_deployment_in_separate_transaction?
         new_build.run_after_commit do |new_build|
           ::Deployments::CreateForBuildService.new.execute(new_build)

app/services/merge_requests/add_todo_when_build_fails_service.rb

@@ -16,9 +16,7 @@ module MergeRequests
     # build is retried
     #
     def close(commit_status)
-      pipeline_merge_requests(commit_status.pipeline) do |merge_request|
-        todo_service.merge_request_build_retried(merge_request)
-      end
+      close_all(commit_status.pipeline)
     end
 
     def close_all(pipeline)

app/workers/ci/build_finished_worker.rb

@@ -40,7 +40,7 @@ module Ci
       BuildHooksWorker.perform_async(build.id)
       ChatNotificationWorker.perform_async(build.id) if build.pipeline.chat?
 
-      if build.failed?
+      if build.failed? && !build.auto_retry_expected?
         ::Ci::MergeRequests::AddTodoWhenBuildFailsWorker.perform_async(build.id)
       end
 

lib/gitlab/ci/build/status/reason.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Build
+      module Status
+        class Reason
+          attr_reader :build, :failure_reason, :exit_code
+
+          def initialize(build, failure_reason, exit_code = nil)
+            @build = build
+            @failure_reason = failure_reason
+            @exit_code = exit_code
+          end
+
+          def failure_reason_enum
+            ::CommitStatus.failure_reasons[failure_reason]
+          end
+
+          def force_allow_failure?
+            return false if exit_code.nil?
+
+            !build.allow_failure? && build.allowed_to_fail_with_code?(exit_code)
+          end
+
+          def self.fabricate(build, reason)
+            if reason.is_a?(self)
+              new(build, reason.failure_reason, reason.exit_code)
+            else
+              new(build, reason)
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/build/status/reason_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Build::Status::Reason do
+  let(:build) { double('build') }
+
+  describe '.fabricate' do
+    context 'when failure symbol reason is being passed' do
+      it 'correctly fabricates a status reason object' do
+        reason = described_class.fabricate(build, :script_failure)
+
+        expect(reason.failure_reason_enum).to eq 1
+      end
+    end
+
+    context 'when another status reason object is being passed' do
+      it 'correctly fabricates a status reason object' do
+        reason = described_class.fabricate(build, :script_failure)
+
+        new_reason = described_class.fabricate(build, reason)
+
+        expect(new_reason.failure_reason_enum).to eq 1
+      end
+    end
+  end
+
+  describe '#failure_reason_enum' do
+    it 'exposes a failure reason enum' do
+      reason = described_class.fabricate(build, :script_failure)
+
+      enum = ::CommitStatus.failure_reasons[:script_failure]
+
+      expect(reason.failure_reason_enum).to eq enum
+    end
+  end
+
+  describe '#force_allow_failure?' do
+    context 'when build is not allowed to fail' do
+      context 'when build is allowed to fail with a given exit code' do
+        it 'returns true' do
+          reason = described_class.new(build, :script_failure, 11)
+
+          allow(build).to receive(:allow_failure?).and_return(false)
+          allow(build).to receive(:allowed_to_fail_with_code?)
+            .with(11)
+            .and_return(true)
+
+          expect(reason.force_allow_failure?).to be true
+        end
+      end
+
+      context 'when build is not allowed to fail regardless of an exit code' do
+        it 'returns false' do
+          reason = described_class.new(build, :script_failure, 11)
+
+          allow(build).to receive(:allow_failure?).and_return(false)
+          allow(build).to receive(:allowed_to_fail_with_code?)
+            .with(11)
+            .and_return(false)
+
+          expect(reason.force_allow_failure?).to be false
+        end
+      end
+
+      context 'when an exit code is not specified' do
+        it 'returns false' do
+          reason = described_class.new(build, :script_failure)
+
+          expect(reason.force_allow_failure?).to be false
+        end
+      end
+    end
+  end
+end

spec/models/ci/build_spec.rb

@@ -565,6 +565,26 @@ RSpec.describe Ci::Build do
         expect(build.reload.runtime_metadata).not_to be_present
       end
     end
+
+    context 'when a failure reason is provided' do
+      context 'when a failure reason is a symbol' do
+        it 'correctly sets a failure reason' do
+          build.drop!(:script_failure)
+
+          expect(build.failure_reason).to eq 'script_failure'
+        end
+      end
+
+      context 'when a failure reason is an object' do
+        it 'correctly sets a failure reason' do
+          reason = ::Gitlab::Ci::Build::Status::Reason.new(build, :script_failure)
+
+          build.drop!(reason)
+
+          expect(build.failure_reason).to eq 'script_failure'
+        end
+      end
+    end
   end
 
   describe '#schedulable?' do
@@ -2187,6 +2207,28 @@ RSpec.describe Ci::Build do
     end
   end
 
+  describe '#auto_retry_expected?' do
+    subject { create(:ci_build, :failed) }
+
+    context 'when build is failed and auto retry is configured' do
+      before do
+        allow(subject)
+          .to receive(:auto_retry_allowed?)
+          .and_return(true)
+      end
+
+      it 'expects auto-retry to happen' do
+        expect(subject.auto_retry_expected?).to be true
+      end
+    end
+
+    context 'when build failed by auto retry is not configured' do
+      it 'does not expect auto-retry to happen' do
+        expect(subject.auto_retry_expected?).to be false
+      end
+    end
+  end
+
   describe '#artifacts_file_for_type' do
     let(:build) { create(:ci_build, :artifacts) }
     let(:file_type) { :archive }
@@ -5279,19 +5321,6 @@ RSpec.describe Ci::Build do
           .to change { build.reload.failed? }
       end
 
-      it 'is executed inside a transaction' do
-        expect(build).to receive(:drop!)
-          .with(:unknown_failure)
-          .and_raise(ActiveRecord::Rollback)
-
-        expect(build).to receive(:conditionally_allow_failure!)
-          .with(1)
-          .and_call_original
-
-        expect { drop_with_exit_code }
-          .not_to change { build.reload.allow_failure }
-      end
-
       context 'when exit_code is nil' do
         let(:exit_code) {}
 

spec/models/commit_status_spec.rb

@@ -773,6 +773,26 @@ RSpec.describe CommitStatus do
         expect { commit_status.drop! }.to change { commit_status.status }.from('manual').to('failed')
       end
     end
+
+    context 'when a failure reason is provided' do
+      context 'when a failure reason is a symbol' do
+        it 'correctly sets a failure reason' do
+          commit_status.drop!(:script_failure)
+
+          expect(commit_status).to be_script_failure
+        end
+      end
+
+      context 'when a failure reason is an object' do
+        it 'correctly sets a failure reason' do
+          reason = ::Gitlab::Ci::Build::Status::Reason.new(commit_status, :script_failure)
+
+          commit_status.drop!(reason)
+
+          expect(commit_status).to be_script_failure
+        end
+      end
+    end
   end
 
   describe 'ensure stage assignment' do

spec/workers/ci/build_finished_worker_spec.rb

@@ -50,6 +50,21 @@ RSpec.describe Ci::BuildFinishedWorker do
 
           subject
         end
+
+        context 'when a build can be auto-retried' do
+          before do
+            allow(build)
+              .to receive(:auto_retry_allowed?)
+              .and_return(true)
+          end
+
+          it 'does not add a todo' do
+            expect(::Ci::MergeRequests::AddTodoWhenBuildFailsWorker)
+              .not_to receive(:perform_async)
+
+            subject
+          end
+        end
       end
 
       context 'when build has a chat' do