diff --git a/app/services/ci/job_artifacts/destroy_batch_service.rb b/app/services/ci/job_artifacts/destroy_batch_service.rb index 8faecfbd4eeacfed4b55d5d1c00c4d2490e99c67..866b40c32d835676116388e71f3af2a68ad0bfdf 100644 --- a/app/services/ci/job_artifacts/destroy_batch_service.rb +++ b/app/services/ci/job_artifacts/destroy_batch_service.rb @@ -26,12 +26,15 @@ def initialize(job_artifacts, pick_up_at: nil) 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 @@ def execute(update_stats: true) 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| diff --git a/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb b/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb index 3aaf32026e55c8e6aeba0542949a84922a08fed3..52c78c313adf43ac0464f1309e36275448d7996c 100644 --- a/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb +++ b/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb @@ -11,6 +11,10 @@ module DestroyBatchService 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 diff --git a/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb b/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb index ded0a9919bc5da92b1aaa484719728cc6d7250ce..4c1386a7c23b1bb7ea6937cdea1933885c3acce7 100644 --- a/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb +++ b/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb @@ -66,9 +66,9 @@ 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 diff --git a/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb b/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb index 41d84c461c6898722c96cb9985e6067a066768e9..1f1888edaff9dbea0c2a650c31851cd1a6d8b726 100644 --- a/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb +++ b/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb @@ -23,12 +23,29 @@ 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 diff --git a/spec/support/database/cross-database-modification-allowlist.yml b/spec/support/database/cross-database-modification-allowlist.yml index e0166f2ae3f7e5c46f90350031e5aa1627c94391..2a5b9e66185c517af9347ae8912142a41a5801df 100644 --- a/spec/support/database/cross-database-modification-allowlist.yml +++ b/spec/support/database/cross-database-modification-allowlist.yml @@ -21,7 +21,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" @@ -85,7 +84,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"