Refactor artifact FastDestroy implementation

Refactors the artifact FastDestroy implementation to collect artifacts
in batches and delete them one by one.

6 files changed, 91 insertions, 36 deletions

diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index 62d1432926d..fd33dee682f 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -60,7 +60,7 @@ module Ci
     validate :valid_file_format?, unless: :trace?, on: :create
     before_save :set_size, if: :file_changed?
 
-    update_project_statistics project_statistics_name: :build_artifacts_size
+    update_project_statistics project_statistics_name: :build_artifacts_size, update_after_destroy: false
 
     after_save :update_file_store, if: :saved_change_to_file?
 
@@ -151,18 +151,19 @@ module Ci
       end
 
       def begin_fast_destroy
-        preload(:project).to_a.group_by(&:project)
+        preload(:project).find_each.map do |artifact|
+          [artifact.project_id, artifact.store_path, artifact.local_store?, artifact.size]
+        end
       end
 
       def finalize_fast_destroy(params)
-        params.each do |project, artifacts|
-          delta = artifacts.sum(&:size)
-
-          artifacts.each do |artifact|
-            Ci::DeleteStoredArtifactsWorker.perform_async(artifact.store_path, artifact.local_store?)
-          end
-
-          update_project_statistics!(project, :build_artifacts_size, -delta)
+        params.each do |artifact_info|
+          Ci::DeleteStoredArtifactsWorker.perform_async(
+            artifact_info[0], # project_id
+            artifact_info[1], # store_path
+            artifact_info[2], # local_store
+            artifact_info[3]  # size
+          )
         end
       end
     end
diff --git a/app/models/concerns/update_project_statistics.rb b/app/models/concerns/update_project_statistics.rb
index 8c2b75748f1..8eeab292d85 100644
--- a/app/models/concerns/update_project_statistics.rb
+++ b/app/models/concerns/update_project_statistics.rb
@@ -31,12 +31,12 @@ module UpdateProjectStatistics
     #
     # - project_statistics_name: A column of `ProjectStatistics` to update
     # - statistic_attribute: An attribute of the current model, default to `size`
-    def update_project_statistics(project_statistics_name:, statistic_attribute: :size)
+    def update_project_statistics(project_statistics_name:, statistic_attribute: :size, update_after_save: true, update_after_destroy: true)
       @project_statistics_name = project_statistics_name
       @statistic_attribute = statistic_attribute
 
-      after_save(:update_project_statistics_after_save, if: :update_project_statistics_attribute_changed?)
-      after_destroy(:update_project_statistics_after_destroy, unless: :project_destroyed?)
+      after_save(:update_project_statistics_after_save, if: :update_project_statistics_attribute_changed?) if update_after_save
+      after_destroy(:update_project_statistics_after_destroy, unless: :project_destroyed?) if update_after_destroy
     end
 
     # Update a projects statistics directly
diff --git a/app/workers/ci/delete_stored_artifacts_worker.rb b/app/workers/ci/delete_stored_artifacts_worker.rb
index 2e1dffd24cf..ea816349430 100644
--- a/app/workers/ci/delete_stored_artifacts_worker.rb
+++ b/app/workers/ci/delete_stored_artifacts_worker.rb
@@ -4,8 +4,10 @@ module Ci
   class DeleteStoredArtifactsWorker
     include ApplicationWorker
 
-    def perform(artifact_store_path, local)
-      local ? delete_local_file(artifact_store_path) : delete_remote_file(artifact_store_path)
+    def perform(project_id, store_path, local_store, size)
+      local_store ? delete_local_file(store_path) : delete_remote_file(store_path)
+
+      UpdateProjectStatistics.update_project_statistics!(Project.find(project_id), :build_artifacts_size, -size)
     end
 
     private
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
index dbdc7d2319d..3c0a40a4299 100644
--- a/spec/models/ci/job_artifact_spec.rb
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -19,8 +19,15 @@ describe Ci::JobArtifact do
 
   it_behaves_like 'having unique enum values'
 
-  it_behaves_like 'UpdateProjectStatistics' do
+  describe 'UpdateProjectStatistics' do
+    before do
+      allow(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async)
+    end
+
     subject { build(:ci_job_artifact, :archive, size: 106365) }
+
+    it_behaves_like 'UpdateProjectStatisticsAfterCreate'
+    it_behaves_like 'UpdateProjectStatisticsAfterUpdate'
   end
 
   describe '.with_reports' do
@@ -95,36 +102,42 @@ describe Ci::JobArtifact do
     end
   end
 
+  describe '.begin_fast_destroy' do
+    before do
+      stub_artifacts_object_storage
+
+      create(:ci_job_artifact, :metadata, size: 100)
+      create(:ci_job_artifact, :codequality, :remote_store, size: 100)
+    end
+
+    subject { described_class.begin_fast_destroy }
+
+    it 'collects all artifacts' do
+      expect(subject.count).to eq(Ci::JobArtifact.count)
+    end
+  end
+
   describe '.finalize_fast_destroy' do
-    let(:project) { create(:project ) }
-    let(:local_artifacts) { create_list(:ci_job_artifact, 2, project: project, size: 100) }
-    let(:remote_artifacts) { create_list(:ci_job_artifact, 2, :remote_store, project: project, size: 100) }
+    let(:project) { create(:project) }
 
     let(:artifact_list) do
-      {
-        project => local_artifacts + remote_artifacts
-      }
+      Ci::JobArtifact.all.map do |artifact|
+        [artifact.project_id, artifact.store_path, artifact.local_store?, artifact.size]
+      end
     end
 
     before do
       stub_artifacts_object_storage
+
+      create_list(:ci_job_artifact, 2, project: project, size: 100)
+      create_list(:ci_job_artifact, 2, :remote_store, project: project, size: 100)
     end
 
     subject { described_class.finalize_fast_destroy(artifact_list) }
 
     it 'calls the async deletion worker' do
-      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(instance_of(String), true).exactly(2).times
-      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(instance_of(String), false).exactly(2).times
-
-      subject
-    end
-
-    it 'updates project statistics' do
-      allow(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async)
-
-      delta = local_artifacts.sum(&:size) + remote_artifacts.sum(&:size)
-
-      expect(described_class).to receive(:update_project_statistics!).with(project, :build_artifacts_size, -delta)
+      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(project.id, instance_of(String), true, instance_of(Integer)).exactly(2).times
+      expect(Ci::DeleteStoredArtifactsWorker).to receive(:perform_async).with(project.id, instance_of(String), false, instance_of(Integer)).exactly(2).times
 
       subject
     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 7fb69ebf4ee..8c896357350 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
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-shared_examples_for 'UpdateProjectStatistics' do
+shared_context 'UpdateProjectStatisticsContext' do
   let(:project) { subject.project }
   let(:project_statistics_name) { described_class.project_statistics_name }
   let(:statistic_attribute) { described_class.statistic_attribute }
@@ -16,6 +16,10 @@ shared_examples_for 'UpdateProjectStatistics' do
   end
 
   it { is_expected.to be_new_record }
+end
+
+shared_examples_for 'UpdateProjectStatisticsAfterCreate' do
+  include_context 'UpdateProjectStatisticsContext'
 
   context 'when creating' do
     it 'updates the project statistics' do
@@ -33,6 +37,10 @@ shared_examples_for 'UpdateProjectStatistics' do
       subject.save!
     end
   end
+end
+
+shared_examples_for 'UpdateProjectStatisticsAfterUpdate' do
+  include_context 'UpdateProjectStatisticsContext'
 
   context 'when updating' do
     let(:delta) { 42 }
@@ -75,6 +83,10 @@ shared_examples_for 'UpdateProjectStatistics' do
       end.not_to exceed_query_limit(control_count)
     end
   end
+end
+
+shared_examples_for 'UpdateProjectStatisticsAfterDestroy' do
+  include_context 'UpdateProjectStatisticsContext'
 
   context 'when destroying' do
     before do
@@ -119,3 +131,11 @@ shared_examples_for 'UpdateProjectStatistics' do
     end
   end
 end
+
+shared_examples_for 'UpdateProjectStatistics' do
+  include_context 'UpdateProjectStatisticsContext'
+
+  it_behaves_like 'UpdateProjectStatisticsAfterCreate'
+  it_behaves_like 'UpdateProjectStatisticsAfterUpdate'
+  it_behaves_like 'UpdateProjectStatisticsAfterDestroy'
+end
diff --git a/spec/workers/ci/delete_stored_artifacts_worker_spec.rb b/spec/workers/ci/delete_stored_artifacts_worker_spec.rb
index dc1e5da854d..ff786844726 100644
--- a/spec/workers/ci/delete_stored_artifacts_worker_spec.rb
+++ b/spec/workers/ci/delete_stored_artifacts_worker_spec.rb
@@ -4,9 +4,14 @@ require 'spec_helper'
 
 describe Ci::DeleteStoredArtifactsWorker do
   describe '#perform' do
+    let(:project) { create(:project) }
     let(:worker) { described_class.new }
 
-    subject { worker.perform(artifact_store_path, local) }
+    subject { worker.perform(project.id, artifact_store_path, local, 10) }
+
+    before do
+      allow(UpdateProjectStatistics).to receive(:update_project_statistics!)
+    end
 
     context 'with a local artifact' do
       let(:artifact_store_path) { 'local_file_path' }
@@ -22,6 +27,13 @@ describe Ci::DeleteStoredArtifactsWorker do
 
         subject
       end
+
+      it 'updates the project statistics' do
+        allow(File).to receive(:delete).with(full_path)
+        expect(UpdateProjectStatistics).to receive(:update_project_statistics!)
+
+        subject
+      end
     end
 
     context 'with a remote artifact' do
@@ -40,6 +52,13 @@ describe Ci::DeleteStoredArtifactsWorker do
 
         subject
       end
+
+      it 'updates the project statistics' do
+        allow(file_double).to receive(:destroy)
+        expect(UpdateProjectStatistics).to receive(:update_project_statistics!)
+
+        subject
+      end
     end
   end
 end