Commit 760fdd1d authored by Stan Hu's avatar Stan Hu

Fix project destruction failing due to idle in transaction timeouts

When deleting associated records, Rails loads all associations into memory
(https://github.com/rails/rails/issues/22510) before destroying them. This
can cause a surge in memory and cause destruction of objects to fail
due to idle in transaction database timeouts. This fix is inspired from
https://github.com/thisismydesign to destroy `has_many` relationships
in batches.

Closes #44610
parent ba58a66a
# 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
......@@ -24,6 +24,7 @@ class Project < ActiveRecord::Base
include ChronicDurationAttribute
include FastDestroyAll::Helpers
include WithUploads
include BatchDestroyDependentAssociations
extend Gitlab::ConfigHelper
......
......@@ -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
......
---
title: Fix project destruction failing due to idle in transaction timeouts
merge_request:
author:
type: fixed
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment