Merge branch 'destroy-project-related-ci-records' into 'master'

Preemptively destroy project-related CI records before the Project#destroy callbacks

See merge request gitlab-org/gitlab!71342

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"