Destroy project-related CI records in separate operations

By moving the destruction into separate calls, the worker will make
real progress even if it times out, and start from where it left
off on retries instead of starting from the beginning.

app/models/ci/build.rb

@@ -42,6 +42,10 @@ module Ci
     has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id, inverse_of: :build
     has_many :report_results, class_name: 'Ci::BuildReportResult', inverse_of: :build
 
+    # Projects::DestroyService destroys Ci::Pipelines, which use_fast_destroy on :job_artifacts
+    # before we delete builds. By doing this, the relation should be empty and not fire any
+    # DELETE queries when the Ci::Build is destroyed. The next step is to remove `dependent: :destroy`.
+    # Details: https://gitlab.com/gitlab-org/gitlab/-/issues/24644#note_689472685
     has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy, inverse_of: :job # rubocop:disable Cop/ActiveRecordDependent
     has_many :job_variables, class_name: 'Ci::JobVariable', foreign_key: :job_id
     has_many :sourced_pipelines, class_name: 'Ci::Sources::Pipeline', foreign_key: :source_job_id

app/services/ci/destroy_pipeline_service.rb

@@ -9,6 +9,9 @@ module Ci
 
       pipeline.cancel_running if pipeline.cancelable?
 
+      # Ci::Pipeline#destroy triggers `use_fast_destroy :job_artifacts` and
+      # ci_builds has ON DELETE CASCADE to ci_pipelines. The pipeline, the builds,
+      # job and pipeline artifacts all get destroyed here.
       pipeline.reset.destroy!
 
       ServiceResponse.success(message: 'Pipeline not found')

app/services/projects/destroy_service.rb

@@ -5,6 +5,7 @@ module Projects
     include Gitlab::ShellAdapter
 
     DestroyError = Class.new(StandardError)
+    BATCH_SIZE = 100
 
     def async_execute
       project.update_attribute(:pending_delete, true)
@@ -119,6 +120,12 @@ module Projects
       destroy_web_hooks!
       destroy_project_bots!
 
+      if ::Feature.enabled?(:ci_optimize_project_records_destruction, project, default_enabled: :yaml) &&
+        Feature.enabled?(:abort_deleted_project_pipelines, default_enabled: :yaml)
+
+        destroy_ci_records!
+      end
+
       # Rails attempts to load all related records into memory before
       # destroying: https://github.com/rails/rails/issues/22510
       # This ensures we delete records in batches.
@@ -133,6 +140,23 @@ module Projects
       log_info("Attempting to destroy #{project.full_path} (#{project.id})")
     end
 
+    def destroy_ci_records!
+      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,
+        # so no Ci::Build-instantiating cancellations happen here.
+        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71342#note_691523196
+
+        ::Ci::DestroyPipelineService.new(project, current_user).execute(pipeline)
+      end
+
+      deleted_count = project.commit_statuses.delete_all
+
+      if deleted_count > 0
+        Gitlab::AppLogger.info "Projects::DestroyService - Project #{project.id} - #{deleted_count} leftover commit statuses"
+      end
+    end
+
     # The project can have multiple webhooks with hundreds of thousands of web_hook_logs.
     # By default, they are removed with "DELETE CASCADE" option defined via foreign_key.
     # But such queries can exceed the statement_timeout limit and fail to delete the project.

config/feature_flags/development/ci_optimize_project_records_destruction.yml

+---
+name: ci_optimize_project_records_destruction
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71342
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/341936
+milestone: '14.4'
+type: development
+group: group::pipeline execution
+default_enabled: false

spec/services/projects/destroy_service_spec.rb

@@ -39,12 +39,15 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
       let!(:job_variables) { create(:ci_job_variable, job: build) }
       let!(:report_result) { create(:ci_build_report_result, build: build) }
       let!(:pending_state) { create(:ci_build_pending_state, build: build) }
+      let!(:pipeline_artifact) { create(:ci_pipeline_artifact, pipeline: pipeline) }
 
-      it 'deletes build related records' do
+      it 'deletes build and pipeline related records' do
         expect { destroy_project(project, user, {}) }
           .to  change { Ci::Build.count }.by(-1)
           .and change { Ci::BuildTraceChunk.count }.by(-1)
           .and change { Ci::JobArtifact.count }.by(-2)
+          .and change { Ci::DeletedObject.count }.by(2)
+          .and change { Ci::PipelineArtifact.count }.by(-1)
           .and change { Ci::JobVariable.count }.by(-1)
           .and change { Ci::BuildPendingState.count }.by(-1)
           .and change { Ci::BuildReportResult.count }.by(-1)
@@ -52,15 +55,48 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
           .and change { Ci::Pipeline.count }.by(-1)
       end
 
-      it 'avoids N+1 queries', skip: 'skipped until fixed in https://gitlab.com/gitlab-org/gitlab/-/issues/24644' do
-        recorder = ActiveRecord::QueryRecorder.new { destroy_project(project, user, {}) }
+      context 'with abort_deleted_project_pipelines disabled' do
+        stub_feature_flags(abort_deleted_project_pipelines: false)
 
-        project = create(:project, :repository, namespace: user.namespace)
-        pipeline = create(:ci_pipeline, project: project)
-        builds = create_list(:ci_build, 3, :artifacts, pipeline: pipeline)
-        create_list(:ci_build_trace_chunk, 3, build: builds[0])
+        it 'avoids N+1 queries' do
+          recorder = ActiveRecord::QueryRecorder.new { destroy_project(project, user, {}) }
 
-        expect { destroy_project(project, project.owner, {}) }.not_to exceed_query_limit(recorder)
+          project = create(:project, :repository, namespace: user.namespace)
+          pipeline = create(:ci_pipeline, project: project)
+          builds = create_list(:ci_build, 3, :artifacts, pipeline: pipeline)
+          create(:ci_pipeline_artifact, pipeline: pipeline)
+          create_list(:ci_build_trace_chunk, 3, build: builds[0])
+
+          expect { destroy_project(project, project.owner, {}) }.not_to exceed_query_limit(recorder)
+        end
+      end
+
+      context 'with ci_optimize_project_records_destruction disabled' do
+        stub_feature_flags(ci_optimize_project_records_destruction: false)
+
+        it 'avoids N+1 queries' do
+          recorder = ActiveRecord::QueryRecorder.new { destroy_project(project, user, {}) }
+
+          project = create(:project, :repository, namespace: user.namespace)
+          pipeline = create(:ci_pipeline, project: project)
+          builds = create_list(:ci_build, 3, :artifacts, pipeline: pipeline)
+          create_list(:ci_build_trace_chunk, 3, build: builds[0])
+
+          expect { destroy_project(project, project.owner, {}) }.not_to exceed_query_limit(recorder)
+        end
+      end
+
+      context 'with ci_optimize_project_records_destruction and abort_deleted_project_pipelines enabled' do
+        it 'avoids N+1 queries' do
+          recorder = ActiveRecord::QueryRecorder.new { destroy_project(project, user, {}) }
+
+          project = create(:project, :repository, namespace: user.namespace)
+          pipeline = create(:ci_pipeline, project: project)
+          builds = create_list(:ci_build, 3, :artifacts, pipeline: pipeline)
+          create_list(:ci_build_trace_chunk, 3, build: builds[0])
+
+          expect { destroy_project(project, project.owner, {}) }.not_to exceed_query_limit(recorder)
+        end
       end
 
       it_behaves_like 'deleting the project'
@@ -97,24 +133,63 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
   end
 
   context 'with abort_deleted_project_pipelines feature disabled' do
-    it 'does not cancel project ci pipelines' do
+    before do
       stub_feature_flags(abort_deleted_project_pipelines: false)
+    end
 
+    it 'does not bulk-fail project ci pipelines' do
       expect(::Ci::AbortPipelinesService).not_to receive(:new)
 
       destroy_project(project, user, {})
     end
+
+    it 'does not destroy CI records via DestroyPipelineService' do
+      expect(::Ci::DestroyPipelineService).not_to receive(:new)
+
+      destroy_project(project, user, {})
+    end
   end
 
   context 'with abort_deleted_project_pipelines feature enabled' do
-    it 'performs cancel for project ci pipelines' do
-      stub_feature_flags(abort_deleted_project_pipelines: true)
-      pipelines = build_list(:ci_pipeline, 3, :running)
-      allow(project).to receive(:all_pipelines).and_return(pipelines)
+    let!(:pipelines)               { create_list(:ci_pipeline, 3, :running, project: project) }
+    let(:destroy_pipeline_service) { double('DestroyPipelineService', execute: nil) }
 
-      expect(::Ci::AbortPipelinesService).to receive_message_chain(:new, :execute).with(pipelines, :project_deleted)
+    context 'with ci_optimize_project_records_destruction disabled' do
+      before do
+        stub_feature_flags(ci_optimize_project_records_destruction: false)
+      end
 
-      destroy_project(project, user, {})
+      it 'bulk-fails project ci pipelines' do
+        expect(::Ci::AbortPipelinesService)
+          .to receive_message_chain(:new, :execute)
+          .with(project.all_pipelines, :project_deleted)
+
+        destroy_project(project, user, {})
+      end
+
+      it 'does not destroy CI records via DestroyPipelineService' do
+        expect(::Ci::DestroyPipelineService).not_to receive(:new)
+
+        destroy_project(project, user, {})
+      end
+    end
+
+    context 'with ci_optimize_project_records_destruction enabled' do
+      it 'executes DestroyPipelineService for project ci pipelines' do
+        allow(::Ci::DestroyPipelineService).to receive(:new).and_return(destroy_pipeline_service)
+
+        expect(::Ci::AbortPipelinesService)
+          .to receive_message_chain(:new, :execute)
+          .with(project.all_pipelines, :project_deleted)
+
+        pipelines.each do |pipeline|
+          expect(destroy_pipeline_service)
+            .to receive(:execute)
+            .with(pipeline)
+        end
+
+        destroy_project(project, user, {})
+      end
     end
   end
 

spec/support/database/cross-database-modification-allowlist.yml

@@ -1340,3 +1340,4 @@
 - "./spec/workers/stage_update_worker_spec.rb"
 - "./spec/workers/stuck_merge_jobs_worker_spec.rb"
 - "./ee/spec/requests/api/graphql/project/pipelines/dast_profile_spec.rb"
+- "./spec/services/projects/overwrite_project_service_spec.rb"