Commit 6606a450 authored by Stan Hu's avatar Stan Hu
Browse files

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

There are two problems in the current implementation:

1. If a project is marked for deletion via the `pending_delete` flag
and then the namespace was quickly deleted, it's possible that the
namespace skips over that project and leaves that project in
an orphaned state.

2. Before namespace deletion, the namespace attempts to clean up
all the relevant storage paths. However, if all projects have been
removed synchronously, then the namespace will not be able to clean anything.
To prevent this, we should load the paths to be deleted before
actually destroying projects.

The specs were missing this second case due to a permission issue
that caused project removal never to happen.
parent dd1732431497
......@@ -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 @@ def parent_changed?
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 @@ def repository_storage_paths
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 @@ def update_forks_visibility_level
# 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 @@ def async_execute
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 @@
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 @@
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 @@ def destroy_group(group, user, async)
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