Update ProjectStatistics#build_artifacts_size synchronously without summing (#41059)

Previously we scheduled a worker to just some this but we were running
into performance issues when the build table was getting too large. So
now we've updated the code such that this column is updated immediately
and incremented/decremented by the correct amount whenever artifacts are
created or deleted.

We've also added the performance optimization that we do not update this
statistic if a project is deleted because it could result in many
updates for a project with many builds.

3 files changed, 113 insertions, 75 deletions

diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index a12717835b0..c5e4e9c464f 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1384,29 +1384,51 @@ describe Ci::Build do
     end
   end
 
-  describe '#update_project_statistics' do
-    let!(:build) { create(:ci_build, artifacts_size: 23) }
-
-    it 'updates project statistics when the artifact size changes' do
-      expect(ProjectCacheWorker).to receive(:perform_async)
-        .with(build.project_id, [], [:build_artifacts_size])
+  context 'when updating the build' do
+    let(:build) { create(:ci_build, artifacts_size: 23) }
 
+    it 'updates project statistics' do
       build.artifacts_size = 42
-      build.save!
+
+      expect(build).to receive(:update_project_statistics_after_save).and_call_original
+
+      expect { build.save! }
+        .to change { build.project.statistics.reload.build_artifacts_size }
+        .by(19)
     end
 
-    it 'does not update project statistics when the artifact size stays the same' do
-      expect(ProjectCacheWorker).not_to receive(:perform_async)
+    context 'when the artifact size stays the same' do
+      it 'does not update project statistics' do
+        build.name = 'changed'
 
-      build.name = 'changed'
-      build.save!
+        expect(build).not_to receive(:update_project_statistics_after_save)
+
+        build.save!
+      end
     end
+  end
 
-    it 'updates project statistics when the build is destroyed' do
-      expect(ProjectCacheWorker).to receive(:perform_async)
-        .with(build.project_id, [], [:build_artifacts_size])
+  context 'when destroying the build' do
+    let!(:build) { create(:ci_build, artifacts_size: 23) }
+
+    it 'updates project statistics' do
+      expect(ProjectStatistics)
+        .to receive(:increment_statistic)
+        .and_call_original
+
+      expect { build.destroy! }
+        .to change { build.project.statistics.reload.build_artifacts_size }
+        .by(-23)
+    end
+
+    context 'when the build is destroyed due to the project being destroyed' do
+      it 'does not update the project statistics' do
+        expect(ProjectStatistics)
+          .not_to receive(:increment_statistic)
 
-      build.destroy
+        build.project.update_attributes(pending_delete: true)
+        build.project.destroy!
+      end
     end
   end
 
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
index 1aa28434879..aad0026aa29 100644
--- a/spec/models/ci/job_artifact_spec.rb
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Ci::JobArtifact do
-  set(:artifact) { create(:ci_job_artifact, :archive) }
+  let(:artifact) { create(:ci_job_artifact, :archive) }
 
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
@@ -59,10 +59,32 @@ describe Ci::JobArtifact do
     end
   end
 
-  describe '#set_size' do
-    it 'sets the size' do
+  context 'creating the artifact' do
+    let(:project) { create(:project) }
+    let(:artifact) { create(:ci_job_artifact, :archive, project: project) }
+
+    it 'sets the size from the file size' do
       expect(artifact.size).to eq(106365)
     end
+
+    it 'updates the project statistics' do
+      expect { artifact }
+        .to change { project.statistics.reload.build_artifacts_size }
+        .by(106365)
+    end
+  end
+
+  context 'updating the artifact file' do
+    it 'updates the artifact size' do
+      artifact.update!(file: fixture_file_upload(File.join(Rails.root, 'spec/fixtures/dk.png')))
+      expect(artifact.size).to eq(1062)
+    end
+
+    it 'updates the project statistics' do
+      expect { artifact.update!(file: fixture_file_upload(File.join(Rails.root, 'spec/fixtures/dk.png'))) }
+        .to change { artifact.project.statistics.reload.build_artifacts_size }
+        .by(1062 - 106365)
+    end
   end
 
   describe '#file' do
@@ -118,4 +140,32 @@ describe Ci::JobArtifact do
       is_expected.to be_nil
     end
   end
+
+  context 'when destroying the artifact' do
+    let(:project) { create(:project, :repository) }
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let!(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
+
+    it 'updates the project statistics' do
+      artifact = build.job_artifacts.first
+
+      expect(ProjectStatistics)
+        .to receive(:increment_statistic)
+        .and_call_original
+
+      expect { artifact.destroy }
+        .to change { project.statistics.reload.build_artifacts_size }
+        .by(-106365)
+    end
+
+    context 'when it is destroyed from the project level' do
+      it 'does not update the project statistics' do
+        expect(ProjectStatistics)
+          .not_to receive(:increment_statistic)
+
+        project.update_attributes(pending_delete: true)
+        project.destroy!
+      end
+    end
+  end
 end
diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb
index 5cff2af4aca..38a3590ad12 100644
--- a/spec/models/project_statistics_spec.rb
+++ b/spec/models/project_statistics_spec.rb
@@ -4,26 +4,6 @@ describe ProjectStatistics do
   let(:project) { create :project }
   let(:statistics) { project.statistics }
 
-  describe 'constants' do
-    describe 'STORAGE_COLUMNS' do
-      it 'is an array of symbols' do
-        expect(described_class::STORAGE_COLUMNS).to be_kind_of Array
-        expect(described_class::STORAGE_COLUMNS.map(&:class).uniq).to eq [Symbol]
-      end
-    end
-
-    describe 'STATISTICS_COLUMNS' do
-      it 'is an array of symbols' do
-        expect(described_class::STATISTICS_COLUMNS).to be_kind_of Array
-        expect(described_class::STATISTICS_COLUMNS.map(&:class).uniq).to eq [Symbol]
-      end
-
-      it 'includes all storage columns' do
-        expect(described_class::STATISTICS_COLUMNS & described_class::STORAGE_COLUMNS).to eq described_class::STORAGE_COLUMNS
-      end
-    end
-  end
-
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:namespace) }
@@ -63,7 +43,6 @@ describe ProjectStatistics do
       allow(statistics).to receive(:update_commit_count)
       allow(statistics).to receive(:update_repository_size)
       allow(statistics).to receive(:update_lfs_objects_size)
-      allow(statistics).to receive(:update_build_artifacts_size)
       allow(statistics).to receive(:update_storage_size)
     end
 
@@ -76,7 +55,6 @@ describe ProjectStatistics do
         expect(statistics).to have_received(:update_commit_count)
         expect(statistics).to have_received(:update_repository_size)
         expect(statistics).to have_received(:update_lfs_objects_size)
-        expect(statistics).to have_received(:update_build_artifacts_size)
       end
     end
 
@@ -89,7 +67,6 @@ describe ProjectStatistics do
         expect(statistics).to have_received(:update_lfs_objects_size)
         expect(statistics).not_to have_received(:update_commit_count)
         expect(statistics).not_to have_received(:update_repository_size)
-        expect(statistics).not_to have_received(:update_build_artifacts_size)
       end
     end
   end
@@ -131,40 +108,6 @@ describe ProjectStatistics do
     end
   end
 
-  describe '#update_build_artifacts_size' do
-    let!(:pipeline) { create(:ci_pipeline, project: project) }
-
-    context 'when new job artifacts are calculated' do
-      let(:ci_build) { create(:ci_build, pipeline: pipeline) }
-
-      before do
-        create(:ci_job_artifact, :archive, project: pipeline.project, job: ci_build)
-      end
-
-      it "stores the size of related build artifacts" do
-        statistics.update_build_artifacts_size
-
-        expect(statistics.build_artifacts_size).to be(106365)
-      end
-
-      it 'calculates related build artifacts by project' do
-        expect(Ci::JobArtifact).to receive(:artifacts_size_for).with(project) { 0 }
-
-        statistics.update_build_artifacts_size
-      end
-    end
-
-    context 'when legacy artifacts are used' do
-      let!(:ci_build) { create(:ci_build, pipeline: pipeline, artifacts_size: 10.megabytes) }
-
-      it "stores the size of related build artifacts" do
-        statistics.update_build_artifacts_size
-
-        expect(statistics.build_artifacts_size).to eq(10.megabytes)
-      end
-    end
-  end
-
   describe '#update_storage_size' do
     it "sums all storage counters" do
       statistics.update!(
@@ -177,4 +120,27 @@ describe ProjectStatistics do
       expect(statistics.storage_size).to eq 5
     end
   end
+
+  describe '.increment_statistic' do
+    it 'increases the statistic by that amount' do
+      expect { described_class.increment_statistic(project.id, :build_artifacts_size, 13) }
+        .to change { statistics.reload.build_artifacts_size }
+        .by(13)
+    end
+
+    context 'when the amount is 0' do
+      it 'does not execute a query' do
+        project
+        expect { described_class.increment_statistic(project.id, :build_artifacts_size, 0) }
+          .not_to exceed_query_limit(0)
+      end
+    end
+
+    context 'when using an invalid column' do
+      it 'raises an error' do
+        expect { described_class.increment_statistic(project.id, :id, 13) }
+          .to raise_error(ArgumentError, "Cannot increment attribute: id")
+      end
+    end
+  end
 end