diff --git a/app/models/concerns/batch_destroy_dependent_associations.rb b/app/models/concerns/batch_destroy_dependent_associations.rb new file mode 100644 index 0000000000000000000000000000000000000000..353ee2e73d03789fb879574247e576a27ed0d27d --- /dev/null +++ b/app/models/concerns/batch_destroy_dependent_associations.rb @@ -0,0 +1,28 @@ +# Provides a way to work around Rails issue where dependent objects are all +# loaded into memory before destroyed: https://github.com/rails/rails/issues/22510. +# +# This concern allows an ActiveRecord module to destroy all its dependent +# associations in batches. The idea is borrowed from https://github.com/thisismydesign/batch_dependent_associations. +# +# The differences here with that gem: +# +# 1. We allow excluding certain associations. +# 2. We don't need to support delete_all since we can use the EachBatch concern. +module BatchDestroyDependentAssociations + extend ActiveSupport::Concern + + DEPENDENT_ASSOCIATIONS_BATCH_SIZE = 1000 + + def dependent_associations_to_destroy + self.class.reflect_on_all_associations(:has_many).select { |assoc| assoc.options[:dependent] == :destroy } + end + + def destroy_dependent_associations_in_batches(exclude: []) + dependent_associations_to_destroy.each do |association| + next if exclude.include?(association.name) + + # rubocop:disable GitlabSecurity/PublicSend + public_send(association.name).find_each(batch_size: DEPENDENT_ASSOCIATIONS_BATCH_SIZE, &:destroy) + end + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 0fe9f8880b41213ce3f135e5414178708ee29bb2..e275ac4dc6fff2f32cdc9085482f0a008161791c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -24,6 +24,7 @@ class Project < ActiveRecord::Base include ChronicDurationAttribute include FastDestroyAll::Helpers include WithUploads + include BatchDestroyDependentAssociations extend Gitlab::ConfigHelper diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index adbc498d0bf2eff3044b0f85f8cdcb65a0a8be6d..04a293a0fd63606035c134d63f366f5aceff73e5 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -124,7 +124,13 @@ module Projects trash_repositories! - project.team.truncate + # 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. + # + # Exclude container repositories because its before_destroy would be + # called multiple times, and it doesn't destroy any database records. + project.destroy_dependent_associations_in_batches(exclude: [:container_repositories]) project.destroy! end end diff --git a/changelogs/unreleased/sh-batch-dependent-destroys.yml b/changelogs/unreleased/sh-batch-dependent-destroys.yml new file mode 100644 index 0000000000000000000000000000000000000000..e297badc1fa9ee6416375d29f4a4fb1f2af29d45 --- /dev/null +++ b/changelogs/unreleased/sh-batch-dependent-destroys.yml @@ -0,0 +1,5 @@ +--- +title: Fix project destruction failing due to idle in transaction timeouts +merge_request: +author: +type: fixed diff --git a/spec/models/concerns/batch_destroy_dependent_associations_spec.rb b/spec/models/concerns/batch_destroy_dependent_associations_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c16b245bea8e73b9ae39835e9fd5e0c129641b70 --- /dev/null +++ b/spec/models/concerns/batch_destroy_dependent_associations_spec.rb @@ -0,0 +1,60 @@ +require 'spec_helper' + +describe BatchDestroyDependentAssociations do + class TestProject < ActiveRecord::Base + self.table_name = 'projects' + + has_many :builds, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :notification_settings, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + has_many :pages_domains + has_many :todos + + include BatchDestroyDependentAssociations + end + + describe '#dependent_associations_to_destroy' do + set(:project) { TestProject.new } + + it 'returns the right associations' do + expect(project.dependent_associations_to_destroy.map(&:name)).to match_array([:builds]) + end + end + + describe '#destroy_dependent_associations_in_batches' do + set(:project) { create(:project) } + set(:build) { create(:ci_build, project: project) } + set(:notification_setting) { create(:notification_setting, project: project) } + let!(:todos) { create(:todo, project: project) } + + it 'destroys multiple builds' do + create(:ci_build, project: project) + + expect(Ci::Build.count).to eq(2) + + project.destroy_dependent_associations_in_batches + + expect(Ci::Build.count).to eq(0) + end + + it 'destroys builds in batches' do + expect(project).to receive_message_chain(:builds, :find_each).and_yield(build) + expect(build).to receive(:destroy).and_call_original + + project.destroy_dependent_associations_in_batches + + expect(Ci::Build.count).to eq(0) + expect(Todo.count).to eq(1) + expect(User.count).to be > 0 + expect(NotificationSetting.count).to eq(User.count) + end + + it 'excludes associations' do + project.destroy_dependent_associations_in_batches(exclude: [:builds]) + + expect(Ci::Build.count).to eq(1) + expect(Todo.count).to eq(1) + expect(User.count).to be > 0 + expect(NotificationSetting.count).to eq(User.count) + end + end +end