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

4 files changed, 59 insertions, 5 deletions

diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index c5713fb7818..2afa2ad8d20 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -35,7 +35,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') }
@@ -217,6 +217,18 @@ class Namespace < ActiveRecord::Base
     [owner_id]
   end
 
+  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
@@ -230,7 +242,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"
diff --git a/app/models/project.rb b/app/models/project.rb
index b45f22d94d9..5fdc4377309 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -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') }
 
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
new file mode 100644
index 00000000000..2e2d7f884ac
--- /dev/null
+++ b/app/services/groups/destroy_service.rb
@@ -0,0 +1,27 @@
+module Groups
+  class DestroyService < Groups::BaseService
+    def async_execute
+      # Soft delete via paranoia gem
+      group.destroy
+      job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
+      Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
+    end
+
+    def execute
+      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
+        ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
+      end
+
+      group.children.each do |group|
+        DestroyService.new(group, current_user).async_execute
+      end
+
+      group.really_destroy!
+    end
+  end
+end
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb
index 538e85cdc89..3d785cc39a7 100644
--- a/spec/services/destroy_group_service_spec.rb
+++ b/spec/services/destroy_group_service_spec.rb
@@ -9,14 +9,18 @@ describe DestroyGroupService, 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 DestroyGroupService, 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 DestroyGroupService, 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