Commit 9fdf5b42 authored by Stan Hu's avatar Stan Hu
Browse files

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 5e436696b06f
# 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 @@ def attempt_destroy_transaction(project)
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