Commit 3050082e authored by Douwe Maan's avatar Douwe Maan

Merge branch 'sh-namespace-cleanup-deleted-projects' into 'master'

Fix a number of race conditions that can occur during namespace deletion

See merge request !9294
parents bf07af9b 6606a450
......@@ -42,7 +42,7 @@ class Namespace < ActiveRecord::Base
after_commit :refresh_access_of_projects_invited_groups, on: :update, if: -> { previous_changes.key?('share_with_group_lock') }
# Save the storage paths before the projects are destroyed to use them on after destroy
before_destroy(prepend: true) { @old_repository_storage_paths = repository_storage_paths }
before_destroy(prepend: true) { prepare_for_destroy }
after_destroy :rm_dir
scope :root, -> { where('type IS NULL') }
......@@ -211,6 +211,14 @@ class Namespace < ActiveRecord::Base
parent_id_changed?
end
def prepare_for_destroy
old_repository_storage_paths
end
def old_repository_storage_paths
@old_repository_storage_paths ||= repository_storage_paths
end
private
def repository_storage_paths
......@@ -224,7 +232,7 @@ class Namespace < ActiveRecord::Base
def rm_dir
# Remove the namespace directory in all storages paths used by member projects
@old_repository_storage_paths.each do |repository_storage_path|
old_repository_storage_paths.each do |repository_storage_path|
# Move namespace directory into trash.
# We will remove it later async
new_path = "#{path}+#{id}+deleted"
......
......@@ -214,6 +214,8 @@ class Project < ActiveRecord::Base
# Scopes
default_scope { where(pending_delete: false) }
scope :with_deleted, -> { unscope(where: :pending_delete) }
scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) }
scope :sorted_by_stars, -> { reorder('projects.star_count DESC') }
......
......@@ -8,7 +8,9 @@ module Groups
end
def execute
group.projects.each do |project|
group.prepare_for_destroy
group.projects.with_deleted.each do |project|
# Execute the destruction of the models immediately to ensure atomic cleanup.
# Skip repository removal because we remove directory with namespace
# that contain all these repositories
......
......@@ -9,14 +9,18 @@ describe Groups::DestroyService, services: true do
let!(:gitlab_shell) { Gitlab::Shell.new }
let!(:remove_path) { group.path + "+#{group.id}+deleted" }
before do
group.add_user(user, Gitlab::Access::OWNER)
end
shared_examples 'group destruction' do |async|
context 'database records' do
before do
destroy_group(group, user, async)
end
it { expect(Group.all).not_to include(group) }
it { expect(Project.all).not_to include(project) }
it { expect(Group.unscoped.all).not_to include(group) }
it { expect(Project.unscoped.all).not_to include(project) }
end
context 'file system' do
......@@ -32,7 +36,7 @@ describe Groups::DestroyService, services: true do
context 'Sidekiq fake' do
before do
# Dont run sidekiq to check if renamed repository exists
# Don't run sidekiq to check if renamed repository exists
Sidekiq::Testing.fake! { destroy_group(group, user, async) }
end
......@@ -95,4 +99,13 @@ describe Groups::DestroyService, services: true do
describe 'synchronous delete' do
it_behaves_like 'group destruction', false
end
context 'projects in pending_delete' do
before do
project.pending_delete = true
project.save
end
it_behaves_like 'group destruction', false
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