Fixes cross-db modification for Ci::JobArtifacts::DestroyBatchService

Split out main, and geo DB out of CI transaction

We should not run main, and geo DB operations in the context of a CI
transaction. We split it out into a pre-, and post- set of DB
operations.

At this point it should be OK for the security_finding to be deleted,
regardless of whether the Ci::JobArtifact are successfully delete or
not. If the Ci::JobArtifact deletion fails, another worker will try
again.

Changelog: changed

app/services/ci/job_artifacts/destroy_batch_service.rb

@@ -26,12 +26,15 @@ module Ci
       def execute(update_stats: true)
         return success(destroyed_artifacts_count: 0, statistics_updates: {}) if @job_artifacts.empty?
 
+        destroy_related_records(@job_artifacts)
+
         Ci::DeletedObject.transaction do
           Ci::DeletedObject.bulk_import(@job_artifacts, @pick_up_at)
           Ci::JobArtifact.id_in(@job_artifacts.map(&:id)).delete_all
-          destroy_related_records(@job_artifacts)
         end
 
+        after_batch_destroy_hook(@job_artifacts)
+
         # This is executed outside of the transaction because it depends on Redis
         update_project_statistics! if update_stats
         increment_monitoring_statistics(artifacts_count, artifacts_bytes)
@@ -43,9 +46,12 @@ module Ci
 
       private
 
-      # This method is implemented in EE and it must do only database work
+      # Overriden in EE
       def destroy_related_records(artifacts); end
 
+      # Overriden in EE
+      def after_batch_destroy_hook(artifacts); end
+
       # using ! here since this can't be called inside a transaction
       def update_project_statistics!
         affected_project_statistics.each do |project, delta|

ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb

@@ -11,6 +11,10 @@ module EE
         override :destroy_related_records
         def destroy_related_records(artifacts)
           destroy_security_findings(artifacts)
+        end
+
+        override :after_batch_destroy_hook
+        def after_batch_destroy_hook(artifacts)
           insert_geo_event_records(artifacts)
         end
 

ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb

@@ -66,9 +66,9 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
 
       let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) }
 
-      it 'raises an exception and stop destroying' do
+      it 'raises an exception but destroys the security_finding object regardless' do
         expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed)
-                          .and not_change { Security::Finding.count }
+          .and change { Security::Finding.count }.by(-1)
       end
     end
 

ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb

@@ -23,12 +23,29 @@ RSpec.describe Ci::JobArtifacts::DestroyBatchService do
       let_it_be(:primary) { create(:geo_node, :primary) }
       let_it_be(:secondary) { create(:geo_node) }
 
-      it 'creates a JobArtifactDeletedEvent' do
+      before do
         stub_current_geo_node(primary)
         create(:ee_ci_job_artifact, :archive)
+      end
 
+      it 'creates a JobArtifactDeletedEvent' do
         expect { subject }.to change { Geo::JobArtifactDeletedEvent.count }.by(1)
       end
+
+      context 'JobArtifact batch destroy fails' do
+        before do
+          expect(Ci::DeletedObject)
+            .to receive(:bulk_import)
+            .once
+            .and_raise(ActiveRecord::RecordInvalid)
+        end
+
+        it 'does not create a JobArtifactDeletedEvent' do
+          expect { subject }.to raise_error(ActiveRecord::RecordInvalid)
+                            .and not_change { Ci::JobArtifact.count }
+                            .and not_change { Geo::JobArtifactDeletedEvent.count }
+        end
+      end
     end
   end
 end

spec/support/database/cross-database-modification-allowlist.yml

@@ -13,7 +13,6 @@
 - "./ee/spec/services/ci/subscribe_bridge_service_spec.rb"
 - "./ee/spec/services/deployments/auto_rollback_service_spec.rb"
 - "./ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb"
-- "./ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb"
 - "./ee/spec/services/ee/users/destroy_service_spec.rb"
 - "./ee/spec/services/projects/transfer_service_spec.rb"
 - "./ee/spec/services/security/security_orchestration_policies/rule_schedule_service_spec.rb"
@@ -76,7 +75,6 @@
 - "./spec/services/ci/expire_pipeline_cache_service_spec.rb"
 - "./spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb"
 - "./spec/services/ci/job_artifacts/destroy_associations_service_spec.rb"
-- "./spec/services/ci/job_artifacts/destroy_batch_service_spec.rb"
 - "./spec/services/ci/pipeline_bridge_status_service_spec.rb"
 - "./spec/services/ci/pipelines/add_job_service_spec.rb"
 - "./spec/services/ci/retry_build_service_spec.rb"