Merge branch 'sh-fix-project-destroy-in-namespace' into 'master'mk-fix-orphaned-projects-on-9-2

Defer project destroys within a namespace in Groups::DestroyService#async_execute

See merge request !12435

5 files changed, 54 insertions, 22 deletions

diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index d6b0ab0e52c..7fb1ae187da 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -224,6 +224,12 @@ class Namespace < ActiveRecord::Base
     parent.present?
   end
 
+  def soft_delete_without_removing_associations
+    # We can't use paranoia's `#destroy` since this will hard-delete projects.
+    # Project uses `pending_delete` instead of the acts_as_paranoia gem.
+    self.deleted_at = Time.now
+  end
+
   private
 
   def repository_storage_paths
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index 497fdb09cdc..074be104700 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -1,8 +1,7 @@
 module Groups
   class DestroyService < Groups::BaseService
     def async_execute
-      # Soft delete via paranoia gem
-      group.destroy
+      group.soft_delete_without_removing_associations
       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
diff --git a/changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml b/changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml
new file mode 100644
index 00000000000..9309f961345
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml
@@ -0,0 +1,4 @@
+---
+title: Defer project destroys within a namespace in Groups::DestroyService#async_execute
+merge_request:
+author:
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 60ba8858df2..978fcb8f3a9 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -323,6 +323,17 @@ describe Namespace, models: true do
     end
   end
 
+  describe '#soft_delete_without_removing_associations' do
+    let(:project1) { create(:project_empty_repo, namespace: namespace) }
+
+    it 'updates the deleted_at timestamp but preserves projects' do
+      namespace.soft_delete_without_removing_associations
+
+      expect(Project.all).to include(project1)
+      expect(namespace.deleted_at).not_to be_nil
+    end
+  end
+
   describe '#user_ids_for_project_authorizations' do
     it 'returns the user IDs for which to refresh authorizations' do
       expect(namespace.user_ids_for_project_authorizations).
diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
index a37257d1bf4..d59b37bee36 100644
--- a/spec/services/groups/destroy_service_spec.rb
+++ b/spec/services/groups/destroy_service_spec.rb
@@ -15,6 +15,14 @@ describe Groups::DestroyService, services: true do
     group.add_user(user, Gitlab::Access::OWNER)
   end
 
+  def destroy_group(group, user, async)
+    if async
+      Groups::DestroyService.new(group, user).async_execute
+    else
+      Groups::DestroyService.new(group, user).execute
+    end
+  end
+
   shared_examples 'group destruction' do |async|
     context 'database records' do
       before do
@@ -30,30 +38,14 @@ describe Groups::DestroyService, services: true do
     context 'file system' do
       context 'Sidekiq inline' do
         before do
-          # Run sidekiq immediatly to check that renamed dir will be removed
+          # Run sidekiq immediately to check that renamed dir will be removed
           Sidekiq::Testing.inline! { destroy_group(group, user, async) }
         end
 
-        it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
-        it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
-      end
-
-      context 'Sidekiq fake' do
-        before do
-          # Don't run sidekiq to check if renamed repository exists
-          Sidekiq::Testing.fake! { destroy_group(group, user, async) }
+        it 'verifies that paths have been deleted' do
+          expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey
+          expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey
         end
-
-        it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
-        it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
-      end
-    end
-
-    def destroy_group(group, user, async)
-      if async
-        Groups::DestroyService.new(group, user).async_execute
-      else
-        Groups::DestroyService.new(group, user).execute
       end
     end
   end
@@ -61,6 +53,26 @@ describe Groups::DestroyService, services: true do
   describe 'asynchronous delete' do
     it_behaves_like 'group destruction', true
 
+    context 'Sidekiq fake' do
+      before do
+        # Don't run Sidekiq to verify that group and projects are not actually destroyed
+        Sidekiq::Testing.fake! { destroy_group(group, user, true) }
+      end
+
+      after do
+        # Clean up stale directories
+        gitlab_shell.rm_namespace(project.repository_storage_path, group.path)
+        gitlab_shell.rm_namespace(project.repository_storage_path, remove_path)
+      end
+
+      it 'verifies original paths and projects still exist' do
+        expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_truthy
+        expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey
+        expect(Project.unscoped.count).to eq(1)
+        expect(Group.unscoped.count).to eq(2)
+      end
+    end
+
     context 'potential race conditions' do
       context "when the `GroupDestroyWorker` task runs immediately" do
         it "deletes the group" do