Use fast_destroy_all for deleting artifacts

Changes the artifact deletion method to fast_destroy_all to optimize
project and build deletion.

10 files changed, 61 insertions, 20 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 7930bef5cf2..77df24f47cc 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -13,6 +13,7 @@ module Ci
     include Importable
     include IgnorableColumn
     include Gitlab::Utils::StrongMemoize
+    include FastDestroyAll::Helpers
     include Deployable
     include HasRef
 
@@ -40,7 +41,7 @@ module Ci
     has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id
     has_many :needs, class_name: 'Ci::BuildNeed', foreign_key: :build_id, inverse_of: :build
 
-    has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy, inverse_of: :job # rubocop:disable Cop/ActiveRecordDependent
+    has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, inverse_of: :job
     has_many :job_variables, class_name: 'Ci::JobVariable', foreign_key: :job_id
 
     Ci::JobArtifact.file_types.each do |key, value|
@@ -152,6 +153,8 @@ module Ci
     before_save :ensure_token
     before_destroy { unscoped_project }
 
+    use_fast_destroy :job_artifacts
+
     after_create unless: :importing? do |build|
       run_after_commit { BuildHooksWorker.perform_async(build.id) }
     end
@@ -633,13 +636,13 @@ module Ci
 
     # and use that for `ExpireBuildInstanceArtifactsWorker`?
     def erase_erasable_artifacts!
-      job_artifacts.erasable.destroy_all # rubocop: disable DestroyAll
+      job_artifacts.erasable.fast_destroy_all
     end
 
     def erase(opts = {})
       return false unless erasable?
 
-      job_artifacts.destroy_all # rubocop: disable DestroyAll
+      job_artifacts.fast_destroy_all
       erase_trace!
       update_erased!(opts[:erased_by])
     end
diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index b4497d8af09..6b0b1ed5ecc 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -5,6 +5,7 @@ module Ci
     include AfterCommitQueue
     include ObjectStorage::BackgroundMove
     include UpdateProjectStatistics
+    include FastDestroyAll
     extend Gitlab::Ci::Model
 
     NotSupportedAdapterError = Class.new(StandardError)
@@ -85,6 +86,7 @@ module Ci
       where(file_type: types)
     end
 
+    # This is a very expensive query, .limit is a way to scope it down as much as possible
     scope :expired, -> (limit) { where('expire_at < ?', Time.now).limit(limit) }
 
     scope :scoped_project, -> { where('ci_job_artifacts.project_id = projects.id') }
@@ -143,8 +145,23 @@ module Ci
       self.update_column(:file_store, file.object_store)
     end
 
-    def self.artifacts_size_for(project)
-      self.where(project: project).sum(:size)
+    class << self
+      def artifacts_size_for(project)
+        where(project: project).sum(:size)
+      end
+
+      def begin_fast_destroy
+        preload(:project).to_a.group_by(&:project).transform_values { |artifacts| artifacts.map(&:file) }
+      end
+
+      def finalize_fast_destroy(params)
+        params.each do |project, artifact_files|
+          delta = artifact_files.sum(&:size)
+          artifact_files.each(&:remove!)
+
+          update_project_statistics!(project, :build_artifacts_size, -delta)
+        end
+      end
     end
 
     def local_store?
diff --git a/app/models/concerns/fast_destroy_all.rb b/app/models/concerns/fast_destroy_all.rb
index f862031bce0..bcfb163d69e 100644
--- a/app/models/concerns/fast_destroy_all.rb
+++ b/app/models/concerns/fast_destroy_all.rb
@@ -44,11 +44,14 @@ module FastDestroyAll
     #
     # This method can replace `destroy` and `destroy_all` without having `after_destroy` hook
     def fast_destroy_all
-      params = begin_fast_destroy
+      items_ids = ids
+      return if items_ids.empty?
 
-      delete_all
+      items = self.limit(nil).where(primary_key => items_ids)
 
-      finalize_fast_destroy(params)
+      params = items.begin_fast_destroy
+      items.delete_all
+      items.finalize_fast_destroy(params)
     end
 
     ##
@@ -84,6 +87,11 @@ module FastDestroyAll
     end
 
     def perform_fast_destroy(subject)
+      items_ids = subject.ids
+      return if items_ids.empty?
+
+      subject = subject.limit(nil).where(self.class.primary_key => items_ids)
+
       params = subject.begin_fast_destroy
 
       run_after_commit do
diff --git a/app/models/concerns/update_project_statistics.rb b/app/models/concerns/update_project_statistics.rb
index 869b3490f3f..baa87e86566 100644
--- a/app/models/concerns/update_project_statistics.rb
+++ b/app/models/concerns/update_project_statistics.rb
@@ -39,6 +39,20 @@ module UpdateProjectStatistics
       after_destroy(:update_project_statistics_after_destroy, unless: :project_destroyed?)
     end
 
+    # Update a projects statistics directly
+    #
+    # Useful when you want to update project statistics outside of an
+    # ActiveRecord model.
+    def update_project_statistics!(project, statistics_name, delta)
+      unless project.pending_delete?
+        ProjectStatistics.increment_statistic(project.id, statistics_name, delta)
+
+        if Feature.enabled?(:update_statistics_namespace, project.root_ancestor)
+          Namespaces::ScheduleAggregationWorker.perform_async(project.namespace_id)
+        end
+      end
+    end
+
     private :update_project_statistics
   end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index c67c5c7bc8c..cd7a691d9ac 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -105,6 +105,7 @@ class Project < ApplicationRecord
   before_destroy :remove_private_deploy_keys
 
   use_fast_destroy :build_trace_chunks
+  use_fast_destroy :job_artifacts
 
   after_destroy -> { run_after_commit { remove_pages } }
   after_destroy :remove_exports
@@ -268,6 +269,7 @@ class Project < ApplicationRecord
   has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName'
   has_many :build_trace_chunks, class_name: 'Ci::BuildTraceChunk', through: :builds, source: :trace_chunks
+  has_many :job_artifacts, class_name: 'Ci::JobArtifact', through: :builds
   has_many :runner_projects, class_name: 'Ci::RunnerProject', inverse_of: :project
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, class_name: 'Ci::Variable'
diff --git a/app/services/ci/destroy_expired_job_artifacts_service.rb b/app/services/ci/destroy_expired_job_artifacts_service.rb
index 7d2f5d33fed..69e9f9be33a 100644
--- a/app/services/ci/destroy_expired_job_artifacts_service.rb
+++ b/app/services/ci/destroy_expired_job_artifacts_service.rb
@@ -28,11 +28,7 @@ module Ci
     private
 
     def destroy_batch
-      artifacts = Ci::JobArtifact.expired(BATCH_SIZE).to_a
-
-      return false if artifacts.empty?
-
-      artifacts.each(&:destroy!)
+      Ci::JobArtifact.expired(BATCH_SIZE).fast_destroy_all
     end
   end
 end
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
index 9550bc6d39c..a4fa8a9defa 100644
--- a/lib/gitlab/ci/trace.rb
+++ b/lib/gitlab/ci/trace.rb
@@ -101,7 +101,7 @@ module Gitlab
       def erase!
         ##
         # Erase the archived trace
-        trace_artifact&.destroy!
+        job.job_artifacts.trace.fast_destroy_all
 
         ##
         # Erase the live trace
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index ec4a6ef05b9..c11ff67f641 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -339,6 +339,7 @@ project:
 - members_and_requesters
 - build_trace_section_names
 - build_trace_chunks
+- job_artifacts
 - root_of_fork_network
 - fork_network_member
 - fork_network
diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
index fc5450ab33d..cc6516655c5 100644
--- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
+++ b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
@@ -35,13 +35,13 @@ describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state
       before do
         stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10)
 
-        allow_any_instance_of(Ci::JobArtifact)
-          .to receive(:destroy!)
-          .and_raise(ActiveRecord::RecordNotDestroyed)
+        allow_any_instance_of(JobArtifactUploader)
+          .to receive(:remove!)
+          .and_raise(StandardError)
       end
 
       it 'raises an exception and stop destroying' do
-        expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed)
+        expect { subject }.to raise_error(StandardError)
       end
     end
 
diff --git a/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb b/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb
index e03435cafe8..7fb69ebf4ee 100644
--- a/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb
+++ b/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb
@@ -88,7 +88,7 @@ shared_examples_for 'UpdateProjectStatistics' do
         .to receive(:increment_statistic)
         .and_call_original
 
-      expect { subject.destroy! }
+      expect { subject.job.destroy! }
         .to change { reload_stat }
         .by(delta)
     end
@@ -97,7 +97,7 @@ shared_examples_for 'UpdateProjectStatistics' do
       expect(Namespaces::ScheduleAggregationWorker)
         .to receive(:perform_async).once
 
-      subject.destroy!
+      subject.job.destroy!
     end
 
     context 'when it is destroyed from the project level' do