From 6606a45030ecd4035b095d33d32f1372c3562b02 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 15 Feb 2017 23:50:05 -0800
Subject: [PATCH] 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.
---
 app/models/namespace.rb                      | 12 ++++++++++--
 app/models/project.rb                        |  2 ++
 app/services/groups/destroy_service.rb       |  4 +++-
 spec/services/groups/destroy_service_spec.rb | 19 ++++++++++++++++---
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 6de4d08fc28..bd0336c984a 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -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"
diff --git a/app/models/project.rb b/app/models/project.rb
index ed43fc2e575..fc5b1a66910 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
index 7f2d28086f5..2e2d7f884ac 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -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
diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
index f86189b68e9..32c2ed8cae7 100644
--- a/spec/services/groups/destroy_service_spec.rb
+++ b/spec/services/groups/destroy_service_spec.rb
@@ -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