Log artifact deletion if undergoing stats refresh

To track if ever we miss any entrypoints that may cause
artifact deletion while project is undergoing refresh, we
added some log calls.

app/models/ci/build.rb

@@ -826,12 +826,26 @@ def artifacts_metadata_entry(path, **options)
     end
     def erase_erasable_artifacts!
+      if project.refreshing_build_artifacts_size?
+        Gitlab::ProjectStatsRefreshConflictsLogger.warn_artifact_deletion_during_stats_refresh(
+          method: 'Ci::Build#erase_erasable_artifacts!',
+          project_id: project_id
+        )
+      end
+
       job_artifacts.erasable.destroy_all # rubocop: disable Cop/DestroyAll
     end
     def erase(opts = {})
       return false unless erasable?
+      if project.refreshing_build_artifacts_size?
+        Gitlab::ProjectStatsRefreshConflictsLogger.warn_artifact_deletion_during_stats_refresh(
+          method: 'Ci::Build#erase',
+          project_id: project_id
+        )
+      end
+
       job_artifacts.destroy_all # rubocop: disable Cop/DestroyAll
       erase_trace!
       update_erased!(opts[:erased_by])

app/models/ci/job_artifact.rb

@@ -187,7 +187,7 @@ class JobArtifact < Ci::ApplicationRecord
     scope :downloadable, -> { where(file_type: DOWNLOADABLE_TYPES) }
     scope :unlocked, -> { joins(job: :pipeline).merge(::Ci::Pipeline.unlocked) }
     scope :order_expired_asc, -> { order(expire_at: :asc) }
-    scope :with_destroy_preloads, -> { includes(project: [:route, :statistics]) }
+    scope :with_destroy_preloads, -> { includes(project: [:route, :statistics, :build_artifacts_size_refresh]) }
     scope :for_project, ->(project) { where(project_id: project) }
     scope :created_in_time_range, ->(from: nil, to: nil) { where(created_at: from..to) }

app/models/project.rb

@@ -418,6 +418,8 @@ def self.integration_association_name(name)
   has_one :ci_project_mirror, class_name: 'Ci::ProjectMirror'
   has_many :sync_events, class_name: 'Projects::SyncEvent'
+  has_one :build_artifacts_size_refresh, class_name: 'Projects::BuildArtifactsSizeRefresh'
+
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :project_feature, update_only: true
   accepts_nested_attributes_for :project_setting, update_only: true
@@ -2900,6 +2902,10 @@ def inactive?
       last_activity_at < ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months.months.ago
   end
+  def refreshing_build_artifacts_size?
+    build_artifacts_size_refresh&.started?
+  end
+
   private
   # overridden in EE

app/models/projects/build_artifacts_size_refresh.rb

@@ -87,5 +87,9 @@ def next_batch(limit:)
         .order(:created_at)
         .limit(limit)
     end
+
+    def started?
+      !created?
+    end
   end
 end

app/services/ci/job_artifacts/destroy_batch_service.rb

@@ -25,6 +25,8 @@ def initialize(job_artifacts, pick_up_at: nil, fix_expire_at: fix_expire_at?)
       # rubocop: disable CodeReuse/ActiveRecord
       def execute(update_stats: true)
+        track_artifacts_undergoing_stats_refresh
+
         # Detect and fix artifacts that had `expire_at` wrongly backfilled by migration
         # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/47723
         detect_and_fix_wrongly_expired_artifacts
@@ -154,6 +156,19 @@ def remove_expire_at(artifacts)
         Gitlab::AppLogger.info(message: "Fixed expire_at from artifacts.", fixed_artifacts_expire_at_count: artifacts.count)
       end
+
+      def track_artifacts_undergoing_stats_refresh
+        project_ids = @job_artifacts.find_all do |artifact|
+          artifact.project.refreshing_build_artifacts_size?
+        end.map(&:project_id).uniq
+
+        project_ids.each do |project_id|
+          Gitlab::ProjectStatsRefreshConflictsLogger.warn_artifact_deletion_during_stats_refresh(
+            method: 'Ci::JobArtifacts::DestroyBatchService#execute',
+            project_id: project_id
+          )
+        end
+      end
     end
   end
 end

app/services/projects/destroy_service.rb

@@ -191,6 +191,10 @@ def destroy_mr_diff_commits!
     # rubocop: enable CodeReuse/ActiveRecord
     def destroy_ci_records!
+      # Make sure to destroy this first just in case the project is undergoing stats refresh.
+      # This is to avoid logging the artifact deletion in Ci::JobArtifacts::DestroyBatchService.
+      project.build_artifacts_size_refresh&.destroy
+
       project.all_pipelines.find_each(batch_size: BATCH_SIZE) do |pipeline| # rubocop: disable CodeReuse/ActiveRecord
         # Destroy artifacts, then builds, then pipelines
         # All builds have already been dropped by Ci::AbortPipelinesService,

lib/gitlab/project_stats_refresh_conflicts_logger.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class ProjectStatsRefreshConflictsLogger
+    def self.warn_artifact_deletion_during_stats_refresh(project_id:, method:)
+      Gitlab::AppLogger.warn(
+        message: 'Deleted artifacts undergoing refresh',
+        method: method,
+        project_id: project_id
+      )
+    end
+  end
+end

spec/lib/gitlab/project_stats_refresh_conflicts_logger_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::ProjectStatsRefreshConflictsLogger do
+  describe '.warn_artifact_deletion_during_stats_refresh' do
+    it 'logs a warning about artifacts being deleted while the project is undergoing stats refresh' do
+      project_id = 123
+      method = 'Foo#action'
+
+      expect(Gitlab::AppLogger).to receive(:warn).with(
+        message: 'Deleted artifacts undergoing refresh',
+        method: method,
+        project_id: project_id
+      )
+
+      described_class.warn_artifact_deletion_during_stats_refresh(project_id: project_id, method: method)
+    end
+  end
+end

spec/models/ci/build_spec.rb

@@ -1830,6 +1830,27 @@
     end
     context 'build is erasable' do
+      context 'when project is undergoing stats refresh' do
+        let!(:build) { create(:ci_build, :test_reports, :trace_artifact, :success, :artifacts) }
+
+        describe '#erase' do
+          before do
+            allow(build.project).to receive(:refreshing_build_artifacts_size?).and_return(true)
+          end
+
+          it 'logs and continues with deleting the artifacts' do
+            expect(Gitlab::ProjectStatsRefreshConflictsLogger).to receive(:warn_artifact_deletion_during_stats_refresh).with(
+              method: 'Ci::Build#erase',
+              project_id: build.project.id
+            )
+
+            build.erase
+
+            expect(build.job_artifacts.count).to eq(0)
+          end
+        end
+      end
+
       context 'new artifacts' do
         let!(:build) { create(:ci_build, :test_reports, :trace_artifact, :success, :artifacts) }
@@ -1924,6 +1945,23 @@
         expect(build.send("job_artifacts_#{file_type}")).not_to be_nil
       end
     end
+
+    context 'when the project is undergoing stats refresh' do
+      before do
+        allow(build.project).to receive(:refreshing_build_artifacts_size?).and_return(true)
+      end
+
+      it 'logs and continues with deleting the artifacts' do
+        expect(Gitlab::ProjectStatsRefreshConflictsLogger).to receive(:warn_artifact_deletion_during_stats_refresh).with(
+          method: 'Ci::Build#erase_erasable_artifacts!',
+          project_id: build.project.id
+        )
+
+        subject
+
+        expect(build.job_artifacts.erasable).to be_empty
+      end
+    end
   end
   describe '#first_pending' do

spec/models/project_spec.rb

@@ -147,6 +147,7 @@
     it { is_expected.to have_many(:job_artifacts).dependent(:restrict_with_error) }
     it { is_expected.to have_many(:build_trace_chunks).through(:builds).dependent(:restrict_with_error) }
     it { is_expected.to have_many(:secure_files).class_name('Ci::SecureFile').dependent(:restrict_with_error) }
+    it { is_expected.to have_one(:build_artifacts_size_refresh).class_name('Projects::BuildArtifactsSizeRefresh') }
     # GitLab Pages
     it { is_expected.to have_many(:pages_domains) }
@@ -8290,6 +8291,38 @@ def has_external_wiki
     end
   end
+  describe "#refreshing_build_artifacts_size?" do
+    let_it_be(:project) { create(:project) }
+
+    subject { project.refreshing_build_artifacts_size? }
+
+    context 'when project has no existing refresh record' do
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when project has existing refresh record' do
+      context 'and refresh has not yet started' do
+        before do
+          allow(project)
+            .to receive_message_chain(:build_artifacts_size_refresh, :started?)
+            .and_return(false)
+        end
+
+        it { is_expected.to eq(false) }
+      end
+
+      context 'and refresh has started' do
+        before do
+          allow(project)
+            .to receive_message_chain(:build_artifacts_size_refresh, :started?)
+            .and_return(true)
+        end
+
+        it { is_expected.to eq(true) }
+      end
+    end
+  end
+
   private
   def finish_job(export_job)

spec/models/projects/build_artifacts_size_refresh_spec.rb

@@ -224,4 +224,26 @@
       expect(batch).to eq([artifact_2, artifact_3])
     end
   end
+
+  describe '#started?' do
+    using RSpec::Parameterized::TableSyntax
+
+    let_it_be(:project) { create(:project) }
+
+    subject { refresh.started? }
+
+    where(:refresh_state, :result) do
+      :created | false
+      :pending | true
+      :running | true
+    end
+
+    with_them do
+      let(:refresh) do
+        create(:project_build_artifacts_size_refresh, refresh_state, project: project)
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
 end

spec/services/ci/job_artifacts/destroy_batch_service_spec.rb

@@ -52,6 +52,45 @@
         .and not_change { Ci::JobArtifact.exists?(trace_artifact.id) }
     end
+    context 'when artifact belongs to a project that is undergoing stats refresh' do
+      let!(:artifact_under_refresh_1) do
+        create(:ci_job_artifact, :zip)
+      end
+
+      let!(:artifact_under_refresh_2) do
+        create(:ci_job_artifact, :zip)
+      end
+
+      let!(:artifact_under_refresh_3) do
+        create(:ci_job_artifact, :zip, project: artifact_under_refresh_2.project)
+      end
+
+      let(:artifacts) do
+        Ci::JobArtifact.where(id: [artifact_with_file.id, artifact_under_refresh_1.id, artifact_under_refresh_2.id,
+                                   artifact_under_refresh_3.id])
+      end
+
+      before do
+        create(:project_build_artifacts_size_refresh, :created, project: artifact_with_file.project)
+        create(:project_build_artifacts_size_refresh, :pending, project: artifact_under_refresh_1.project)
+        create(:project_build_artifacts_size_refresh, :running, project: artifact_under_refresh_2.project)
+      end
+
+      it 'logs the artifacts undergoing refresh and continues with the delete' do
+        expect(Gitlab::ProjectStatsRefreshConflictsLogger).to receive(:warn_artifact_deletion_during_stats_refresh).with(
+          method: 'Ci::JobArtifacts::DestroyBatchService#execute',
+          project_id: artifact_under_refresh_1.project.id
+        ).once
+
+        expect(Gitlab::ProjectStatsRefreshConflictsLogger).to receive(:warn_artifact_deletion_during_stats_refresh).with(
+          method: 'Ci::JobArtifacts::DestroyBatchService#execute',
+          project_id: artifact_under_refresh_2.project.id
+        ).once
+
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-4)
+      end
+    end
+
     context 'ProjectStatistics' do
       it 'resets project statistics' do
         expect(ProjectStatistics).to receive(:increment_statistic).once

spec/services/projects/destroy_service_spec.rb

@@ -73,6 +73,18 @@
       end
       it_behaves_like 'deleting the project'
+
+      context 'when project is undergoing refresh' do
+        let!(:build_artifacts_size_refresh) { create(:project_build_artifacts_size_refresh, :pending, project: project) }
+
+        it 'does not log about artifact deletion but continues to delete artifacts' do
+          expect(Gitlab::ProjectStatsRefreshConflictsLogger).not_to receive(:warn_artifact_deletion_during_stats_refresh)
+
+          expect { destroy_project(project, user, {}) }
+            .to change { Ci::JobArtifact.count }.by(-2)
+            .and change { Projects::BuildArtifactsSizeRefresh.count }.by(-1)
+        end
+      end
     end
   end