From 83e49040f326b41f751d0dc0a1bc22d7b81d64eb Mon Sep 17 00:00:00 2001
From: "allison.browne" <abrowne@gitlab.com>
Date: Wed, 12 Aug 2020 14:07:35 -0400
Subject: [PATCH 1/4] Remove project destory transaction behind flag

Remove project destroy transaction so that the job can continue
deleting the objects it did not get to
---
 app/services/projects/destroy_service.rb      | 36 +++++++++++--------
 .../services/projects/destroy_service_spec.rb | 31 ++++++++++++++++
 2 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 37487261f2c822..e9f70f4c712eeb 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -28,7 +28,7 @@ def execute
 
       Projects::UnlinkForkService.new(project, current_user).execute
 
-      attempt_destroy_transaction(project)
+      attempt_destroy(project)
 
       system_hook_service.execute_hooks_for(project, :destroy)
       log_info("Project \"#{project.full_path}\" was deleted")
@@ -98,29 +98,35 @@ def attempt_rollback(project, message)
       log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
     end
 
-    def attempt_destroy_transaction(project)
+    def attempt_destroy(project)
       unless remove_registry_tags
         raise_error(s_('DeleteProject|Failed to remove some tags in project container registry. Please try again or contact administrator.'))
       end
 
       project.leave_pool_repository
 
-      Project.transaction do
-        log_destroy_event
-        trash_relation_repositories!
-        trash_project_repositories!
-
-        # Rails attempts to load all related records into memory before
-        # destroying: https://github.com/rails/rails/issues/22510
-        # This ensures we delete records in batches.
-        #
-        # Exclude container repositories because its before_destroy would be
-        # called multiple times, and it doesn't destroy any database records.
-        project.destroy_dependent_associations_in_batches(exclude: [:container_repositories, :snippets])
-        project.destroy!
+      if Feature.enabled?(:project_transactionless_destroy, project)
+        destroy_project_related_records(project)
+      else
+        Project.transaction { destroy_project_related_records(project) }
       end
     end
 
+    def destroy_project_related_records(project)
+      log_destroy_event
+      trash_relation_repositories!
+      trash_project_repositories!
+
+      # Rails attempts to load all related records into memory before
+      # destroying: https://github.com/rails/rails/issues/22510
+      # This ensures we delete records in batches.
+      #
+      # Exclude container repositories because its before_destroy would be
+      # called multiple times, and it doesn't destroy any database records.
+      project.destroy_dependent_associations_in_batches(exclude: [:container_repositories, :snippets])
+      project.destroy!
+    end
+
     def log_destroy_event
       log_info("Attempting to destroy #{project.full_path} (#{project.id})")
     end
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 56b19c33ece290..79b44a7861068d 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -14,6 +14,7 @@
   before do
     stub_container_registry_config(enabled: true)
     stub_container_registry_tags(repository: :any, tags: [])
+    stub_feature_flags(project_transactionless_destroy: false)
   end
 
   shared_examples 'deleting the project' do
@@ -68,6 +69,36 @@
     destroy_project(project, user, {})
   end
 
+  context 'when project_transactionless_destroy enabled', :sidekiq_inline do
+    let!(:pipeline) { create(:ci_pipeline, project: project) }
+    let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
+
+    before do
+      stub_feature_flags(project_transactionless_destroy: true)
+    end
+
+    it_behaves_like 'deleting the project'
+
+    context 'error while destroying' do
+      before do
+        # We can expect this to timeout for very large projects
+        # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440
+        allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom')
+        destroy_project(project, user, {})
+        allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original
+        destroy_project(project, user, {})
+      end
+
+      it 'destroys project related records on retry' do
+        expect(Project.unscoped.all).not_to include(project)
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+        expect(project.all_pipelines).to be_empty
+        expect(project.builds).to be_empty
+      end
+    end
+  end
+
   context 'when project has remote mirrors' do
     let!(:project) do
       create(:project, :repository, namespace: user.namespace).tap do |project|
-- 
GitLab


From 7e535053f314acef56b25c9bf38ceda0b207371b Mon Sep 17 00:00:00 2001
From: "allison.browne" <abrowne@gitlab.com>
Date: Wed, 12 Aug 2020 15:42:30 -0400
Subject: [PATCH 2/4] Test all behavior with and without feature flag

The behavior should largely stay the same minus the one new test
extract to project destroy shared example
---
 .../services/projects/destroy_service_spec.rb | 477 +++++++++---------
 1 file changed, 240 insertions(+), 237 deletions(-)

diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 79b44a7861068d..1b294511bf3c29 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -14,7 +14,6 @@
   before do
     stub_container_registry_config(enabled: true)
     stub_container_registry_tags(repository: :any, tags: [])
-    stub_feature_flags(project_transactionless_destroy: false)
   end
 
   shared_examples 'deleting the project' do
@@ -61,345 +60,349 @@
     end
   end
 
-  it_behaves_like 'deleting the project'
-
-  it 'invalidates personal_project_count cache' do
-    expect(user).to receive(:invalidate_personal_projects_count)
-
-    destroy_project(project, user, {})
-  end
-
-  context 'when project_transactionless_destroy enabled', :sidekiq_inline do
-    let!(:pipeline) { create(:ci_pipeline, project: project) }
-    let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
-
-    before do
-      stub_feature_flags(project_transactionless_destroy: true)
-    end
-
+  shared_examples 'project destroy' do
     it_behaves_like 'deleting the project'
 
-    context 'error while destroying' do
-      before do
-        # We can expect this to timeout for very large projects
-        # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440
-        allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom')
-        destroy_project(project, user, {})
-        allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original
-        destroy_project(project, user, {})
-      end
+    it 'invalidates personal_project_count cache' do
+      expect(user).to receive(:invalidate_personal_projects_count)
 
-      it 'destroys project related records on retry' do
-        expect(Project.unscoped.all).not_to include(project)
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
-        expect(project.all_pipelines).to be_empty
-        expect(project.builds).to be_empty
-      end
+      destroy_project(project, user, {})
     end
-  end
 
-  context 'when project has remote mirrors' do
-    let!(:project) do
-      create(:project, :repository, namespace: user.namespace).tap do |project|
-        project.remote_mirrors.create(url: 'http://test.com')
+    context 'when project has remote mirrors' do
+      let!(:project) do
+        create(:project, :repository, namespace: user.namespace).tap do |project|
+          project.remote_mirrors.create(url: 'http://test.com')
+        end
       end
-    end
 
-    it 'destroys them' do
-      expect(RemoteMirror.count).to eq(1)
+      it 'destroys them' do
+        expect(RemoteMirror.count).to eq(1)
 
-      destroy_project(project, user, {})
-
-      expect(RemoteMirror.count).to eq(0)
-    end
-  end
+        destroy_project(project, user, {})
 
-  context 'when project has exports' do
-    let!(:project_with_export) do
-      create(:project, :repository, namespace: user.namespace).tap do |project|
-        create(:import_export_upload,
-               project: project,
-               export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz'))
+        expect(RemoteMirror.count).to eq(0)
       end
     end
 
-    it 'destroys project and export' do
-      expect do
-        destroy_project(project_with_export, user, {})
-      end.to change(ImportExportUpload, :count).by(-1)
-
-      expect(Project.all).not_to include(project_with_export)
-    end
-  end
-
-  context 'Sidekiq fake' do
-    before do
-      # Dont run sidekiq to check if renamed repository exists
-      Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
-    end
+    context 'when project has exports' do
+      let!(:project_with_export) do
+        create(:project, :repository, namespace: user.namespace).tap do |project|
+          create(:import_export_upload,
+                 project: project,
+                 export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz'))
+        end
+      end
 
-    it { expect(Project.all).not_to include(project) }
+      it 'destroys project and export' do
+        expect do
+          destroy_project(project_with_export, user, {})
+        end.to change(ImportExportUpload, :count).by(-1)
 
-    it do
-      expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+        expect(Project.all).not_to include(project_with_export)
+      end
     end
 
-    it do
-      expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
-    end
-  end
+    context 'Sidekiq fake' do
+      before do
+        # Dont run sidekiq to check if renamed repository exists
+        Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
+      end
 
-  context 'when flushing caches fail due to Git errors' do
-    before do
-      allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError)
-      allow(Gitlab::GitLogger).to receive(:warn).with(
-        class: Repositories::DestroyService.name,
-        container_id: project.id,
-        disk_path: project.disk_path,
-        message: 'Gitlab::Git::CommandError').and_call_original
-    end
+      it { expect(Project.all).not_to include(project) }
 
-    it_behaves_like 'deleting the project'
-  end
+      it do
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+      end
 
-  context 'when flushing caches fail due to Redis' do
-    before do
-      new_user = create(:user)
-      project.team.add_user(new_user, Gitlab::Access::DEVELOPER)
-      allow_any_instance_of(described_class).to receive(:flush_caches).and_raise(::Redis::CannotConnectError)
+      it do
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+      end
     end
 
-    it 'keeps project team intact upon an error' do
-      perform_enqueued_jobs do
-        destroy_project(project, user, {})
-      rescue ::Redis::CannotConnectError
+    context 'when flushing caches fail due to Git errors' do
+      before do
+        allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError)
+        allow(Gitlab::GitLogger).to receive(:warn).with(
+          class: Repositories::DestroyService.name,
+          container_id: project.id,
+          disk_path: project.disk_path,
+          message: 'Gitlab::Git::CommandError').and_call_original
       end
 
-      expect(project.team.members.count).to eq 2
+      it_behaves_like 'deleting the project'
     end
-  end
 
-  context 'with async_execute', :sidekiq_inline do
-    let(:async) { true }
-
-    context 'async delete of project with private issue visibility' do
+    context 'when flushing caches fail due to Redis' do
       before do
-        project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE)
+        new_user = create(:user)
+        project.team.add_user(new_user, Gitlab::Access::DEVELOPER)
+        allow_any_instance_of(described_class).to receive(:flush_caches).and_raise(::Redis::CannotConnectError)
       end
 
-      it_behaves_like 'deleting the project'
+      it 'keeps project team intact upon an error' do
+        perform_enqueued_jobs do
+          destroy_project(project, user, {})
+        rescue ::Redis::CannotConnectError
+        end
+
+        expect(project.team.members.count).to eq 2
+      end
     end
 
-    it_behaves_like 'deleting the project with pipeline and build'
+    context 'with async_execute', :sidekiq_inline do
+      let(:async) { true }
 
-    context 'errors' do
-      context 'when `remove_legacy_registry_tags` fails' do
+      context 'async delete of project with private issue visibility' do
         before do
-          expect_any_instance_of(described_class)
-            .to receive(:remove_legacy_registry_tags).and_return(false)
+          project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE)
         end
 
-        it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags"
+        it_behaves_like 'deleting the project'
       end
 
-      context 'when `remove_repository` fails' do
-        before do
-          expect_any_instance_of(described_class)
-            .to receive(:remove_repository).and_return(false)
+      it_behaves_like 'deleting the project with pipeline and build'
+
+      context 'errors' do
+        context 'when `remove_legacy_registry_tags` fails' do
+          before do
+            expect_any_instance_of(described_class)
+              .to receive(:remove_legacy_registry_tags).and_return(false)
+          end
+
+          it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags"
         end
 
-        it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository"
-      end
+        context 'when `remove_repository` fails' do
+          before do
+            expect_any_instance_of(described_class)
+              .to receive(:remove_repository).and_return(false)
+          end
 
-      context 'when `execute` raises expected error' do
-        before do
-          expect_any_instance_of(Project)
-            .to receive(:destroy!).and_raise(StandardError.new("Other error message"))
+          it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository"
         end
 
-        it_behaves_like 'handles errors thrown during async destroy', "Other error message"
-      end
+        context 'when `execute` raises expected error' do
+          before do
+            expect_any_instance_of(Project)
+              .to receive(:destroy!).and_raise(StandardError.new("Other error message"))
+          end
 
-      context 'when `execute` raises unexpected error' do
-        before do
-          expect_any_instance_of(Project)
-            .to receive(:destroy!).and_raise(Exception.new('Other error message'))
+          it_behaves_like 'handles errors thrown during async destroy', "Other error message"
         end
 
-        it 'allows error to bubble up and rolls back project deletion' do
-          expect do
-            destroy_project(project, user, {})
-          end.to raise_error(Exception, 'Other error message')
+        context 'when `execute` raises unexpected error' do
+          before do
+            expect_any_instance_of(Project)
+              .to receive(:destroy!).and_raise(Exception.new('Other error message'))
+          end
 
-          expect(project.reload.pending_delete).to be(false)
-          expect(project.delete_error).to include("Other error message")
+          it 'allows error to bubble up and rolls back project deletion' do
+            expect do
+              destroy_project(project, user, {})
+            end.to raise_error(Exception, 'Other error message')
+
+            expect(project.reload.pending_delete).to be(false)
+            expect(project.delete_error).to include("Other error message")
+          end
         end
       end
     end
-  end
 
-  describe 'container registry' do
-    context 'when there are regular container repositories' do
-      let(:container_repository) { create(:container_repository) }
+    describe 'container registry' do
+      context 'when there are regular container repositories' do
+        let(:container_repository) { create(:container_repository) }
 
-      before do
-        stub_container_registry_tags(repository: project.full_path + '/image',
-                                     tags: ['tag'])
-        project.container_repositories << container_repository
-      end
+        before do
+          stub_container_registry_tags(repository: project.full_path + '/image',
+                                       tags: ['tag'])
+          project.container_repositories << container_repository
+        end
 
-      context 'when image repository deletion succeeds' do
-        it 'removes tags' do
-          expect_any_instance_of(ContainerRepository)
-            .to receive(:delete_tags!).and_return(true)
+        context 'when image repository deletion succeeds' do
+          it 'removes tags' do
+            expect_any_instance_of(ContainerRepository)
+              .to receive(:delete_tags!).and_return(true)
 
-          destroy_project(project, user)
+            destroy_project(project, user)
+          end
         end
-      end
 
-      context 'when image repository deletion fails' do
-        it 'raises an exception' do
-          expect_any_instance_of(ContainerRepository)
-            .to receive(:delete_tags!).and_raise(RuntimeError)
+        context 'when image repository deletion fails' do
+          it 'raises an exception' do
+            expect_any_instance_of(ContainerRepository)
+              .to receive(:delete_tags!).and_raise(RuntimeError)
+
+            expect(destroy_project(project, user)).to be false
+          end
+        end
+
+        context 'when registry is disabled' do
+          before do
+            stub_container_registry_config(enabled: false)
+          end
+
+          it 'does not attempting to remove any tags' do
+            expect(Projects::ContainerRepository::DestroyService).not_to receive(:new)
 
-          expect(destroy_project(project, user)).to be false
+            destroy_project(project, user)
+          end
         end
       end
 
-      context 'when registry is disabled' do
+      context 'when there are tags for legacy root repository' do
         before do
-          stub_container_registry_config(enabled: false)
+          stub_container_registry_tags(repository: project.full_path,
+                                       tags: ['tag'])
         end
 
-        it 'does not attempting to remove any tags' do
-          expect(Projects::ContainerRepository::DestroyService).not_to receive(:new)
+        context 'when image repository tags deletion succeeds' do
+          it 'removes tags' do
+            expect_any_instance_of(ContainerRepository)
+              .to receive(:delete_tags!).and_return(true)
 
-          destroy_project(project, user)
+            destroy_project(project, user)
+          end
+        end
+
+        context 'when image repository tags deletion fails' do
+          it 'raises an exception' do
+            expect_any_instance_of(ContainerRepository)
+              .to receive(:delete_tags!).and_return(false)
+
+            expect(destroy_project(project, user)).to be false
+          end
         end
       end
     end
 
-    context 'when there are tags for legacy root repository' do
+    context 'for a forked project with LFS objects' do
+      let(:forked_project) { fork_project(project, user) }
+
       before do
-        stub_container_registry_tags(repository: project.full_path,
-                                     tags: ['tag'])
+        project.lfs_objects << create(:lfs_object)
+        forked_project.reload
       end
 
-      context 'when image repository tags deletion succeeds' do
-        it 'removes tags' do
-          expect_any_instance_of(ContainerRepository)
-            .to receive(:delete_tags!).and_return(true)
-
-          destroy_project(project, user)
-        end
+      it 'destroys the fork' do
+        expect { destroy_project(forked_project, user) }
+          .not_to raise_error
       end
+    end
 
-      context 'when image repository tags deletion fails' do
-        it 'raises an exception' do
-          expect_any_instance_of(ContainerRepository)
-            .to receive(:delete_tags!).and_return(false)
+    context 'as the root of a fork network' do
+      let!(:fork_1) { fork_project(project, user) }
+      let!(:fork_2) { fork_project(project, user) }
 
-          expect(destroy_project(project, user)).to be false
-        end
+      it 'updates the fork network with the project name' do
+        fork_network = project.fork_network
+
+        destroy_project(project, user)
+
+        fork_network.reload
+
+        expect(fork_network.deleted_root_project_name).to eq(project.full_name)
+        expect(fork_network.root_project).to be_nil
       end
     end
-  end
 
-  context 'for a forked project with LFS objects' do
-    let(:forked_project) { fork_project(project, user) }
+    context 'repository +deleted path removal' do
+      context 'regular phase' do
+        it 'schedules +deleted removal of existing repos' do
+          service = described_class.new(project, user, {})
+          allow(service).to receive(:schedule_stale_repos_removal)
 
-    before do
-      project.lfs_objects << create(:lfs_object)
-      forked_project.reload
-    end
+          expect(Repositories::ShellDestroyService).to receive(:new).and_call_original
+          expect(GitlabShellWorker).to receive(:perform_in)
+            .with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
 
-    it 'destroys the fork' do
-      expect { destroy_project(forked_project, user) }
-        .not_to raise_error
-    end
-  end
+          service.execute
+        end
+      end
 
-  context 'as the root of a fork network' do
-    let!(:fork_1) { fork_project(project, user) }
-    let!(:fork_2) { fork_project(project, user) }
+      context 'stale cleanup' do
+        let(:async) { true }
 
-    it 'updates the fork network with the project name' do
-      fork_network = project.fork_network
+        it 'schedules +deleted wiki and repo removal' do
+          allow(ProjectDestroyWorker).to receive(:perform_async)
 
-      destroy_project(project, user)
+          expect(Repositories::ShellDestroyService).to receive(:new).with(project.repository).and_call_original
+          expect(GitlabShellWorker).to receive(:perform_in)
+            .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
 
-      fork_network.reload
+          expect(Repositories::ShellDestroyService).to receive(:new).with(project.wiki.repository).and_call_original
+          expect(GitlabShellWorker).to receive(:perform_in)
+            .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path))
 
-      expect(fork_network.deleted_root_project_name).to eq(project.full_name)
-      expect(fork_network.root_project).to be_nil
+          destroy_project(project, user, {})
+        end
+      end
     end
-  end
 
-  context 'repository +deleted path removal' do
-    context 'regular phase' do
-      it 'schedules +deleted removal of existing repos' do
-        service = described_class.new(project, user, {})
-        allow(service).to receive(:schedule_stale_repos_removal)
+    context 'snippets' do
+      let!(:snippet1) { create(:project_snippet, project: project, author: user) }
+      let!(:snippet2) { create(:project_snippet, project: project, author: user) }
 
-        expect(Repositories::ShellDestroyService).to receive(:new).and_call_original
-        expect(GitlabShellWorker).to receive(:perform_in)
-          .with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
+      it 'does not include snippets when deleting in batches' do
+        expect(project).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:container_repositories, :snippets] })
 
-        service.execute
+        destroy_project(project, user)
       end
-    end
 
-    context 'stale cleanup' do
-      let(:async) { true }
+      it 'calls the bulk snippet destroy service' do
+        expect(project.snippets.count).to eq 2
 
-      it 'schedules +deleted wiki and repo removal' do
-        allow(ProjectDestroyWorker).to receive(:perform_async)
+        expect(Snippets::BulkDestroyService).to receive(:new)
+          .with(user, project.snippets).and_call_original
 
-        expect(Repositories::ShellDestroyService).to receive(:new).with(project.repository).and_call_original
-        expect(GitlabShellWorker).to receive(:perform_in)
-          .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
+        expect do
+          destroy_project(project, user)
+        end.to change(Snippet, :count).by(-2)
+      end
 
-        expect(Repositories::ShellDestroyService).to receive(:new).with(project.wiki.repository).and_call_original
-        expect(GitlabShellWorker).to receive(:perform_in)
-          .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path))
+      context 'when an error is raised deleting snippets' do
+        it 'does not delete project' do
+          allow_next_instance_of(Snippets::BulkDestroyService) do |instance|
+            allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'foo'))
+          end
 
-        destroy_project(project, user, {})
+          expect(destroy_project(project, user)).to be_falsey
+          expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+        end
       end
     end
   end
 
-  context 'snippets' do
-    let!(:snippet1) { create(:project_snippet, project: project, author: user) }
-    let!(:snippet2) { create(:project_snippet, project: project, author: user) }
+  it_behaves_like 'project destroy'
 
-    it 'does not include snippets when deleting in batches' do
-      expect(project).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:container_repositories, :snippets] })
+  context 'error while destroying', :sidekiq_inline do
+    let!(:pipeline) { create(:ci_pipeline, project: project) }
+    let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
 
-      destroy_project(project, user)
+    before do
+      # We can expect this to timeout for very large projects
+      # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440
+      allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom')
+      destroy_project(project, user, {})
+      allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original
+      destroy_project(project, user, {})
     end
 
-    it 'calls the bulk snippet destroy service' do
-      expect(project.snippets.count).to eq 2
-
-      expect(Snippets::BulkDestroyService).to receive(:new)
-        .with(user, project.snippets).and_call_original
-
-      expect do
-        destroy_project(project, user)
-      end.to change(Snippet, :count).by(-2)
+    it 'destroys project related records on retry' do
+      expect(Project.unscoped.all).not_to include(project)
+      expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+      expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+      expect(project.all_pipelines).to be_empty
+      expect(project.builds).to be_empty
     end
+  end
 
-    context 'when an error is raised deleting snippets' do
-      it 'does not delete project' do
-        allow_next_instance_of(Snippets::BulkDestroyService) do |instance|
-          allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'foo'))
-        end
-
-        expect(destroy_project(project, user)).to be_falsey
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
-      end
+  context 'when project_transactionless_destroy disabled', :sidekiq_inline do
+    before do
+      stub_feature_flags(project_transactionless_destroy: false)
     end
+
+    it_behaves_like 'project destroy'
   end
 
   def destroy_project(project, user, params = {})
-- 
GitLab


From 1a8dff54650be7cce9d54dccbe1815c263380e95 Mon Sep 17 00:00:00 2001
From: "allison.browne" <abrowne@gitlab.com>
Date: Mon, 17 Aug 2020 12:08:36 -0400
Subject: [PATCH 3/4] Add new test for retry of destroy service

Also move Geo and audit logging to after project delete
---
 app/services/projects/destroy_service.rb      |   2 +-
 .../services/ee/projects/destroy_service.rb   |  44 ++++---
 .../services/projects/destroy_service_spec.rb | 120 ++++++++++--------
 lib/gitlab/ci/features.rb                     |   4 +
 .../services/projects/destroy_service_spec.rb |  42 +++---
 5 files changed, 117 insertions(+), 95 deletions(-)

diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index e9f70f4c712eeb..bfbc83f209c2f6 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -105,7 +105,7 @@ def attempt_destroy(project)
 
       project.leave_pool_repository
 
-      if Feature.enabled?(:project_transactionless_destroy, project)
+      if Gitlab::Ci::Features.project_transactionless_destroy(project)
         destroy_project_related_records(project)
       else
         Project.transaction { destroy_project_related_records(project) }
diff --git a/ee/app/services/ee/projects/destroy_service.rb b/ee/app/services/ee/projects/destroy_service.rb
index 18c14224aaa5c8..2fc1c60a69bc46 100644
--- a/ee/app/services/ee/projects/destroy_service.rb
+++ b/ee/app/services/ee/projects/destroy_service.rb
@@ -17,10 +17,30 @@ def execute
         end
       end
 
-      override :log_destroy_event
-      def log_destroy_event
-        super
+      # Removes physical repository in a Geo replicated secondary node
+      # There is no need to do any database operation as it will be
+      # replicated by itself.
+      def geo_replicate
+        return unless ::Gitlab::Geo.secondary?
+
+        # Flush the cache for both repositories. This has to be done _before_
+        # removing the physical repositories as some expiration code depends on
+        # Git data (e.g. a list of branch names).
+        flush_caches(project)
+
+        trash_project_repositories!
+
+        log_info("Project \"#{project.name}\" was removed")
+      end
+
+      private
+
+      override :destroy_project_related_records
+      def destroy_project_related_records(project)
+        super && log_destroy_success
+      end
 
+      def log_destroy_success
         log_geo_event(project)
         log_audit_event(project)
       end
@@ -39,24 +59,6 @@ def log_geo_event(project)
         ).create!
       end
 
-      # Removes physical repository in a Geo replicated secondary node
-      # There is no need to do any database operation as it will be
-      # replicated by itself.
-      def geo_replicate
-        return unless ::Gitlab::Geo.secondary?
-
-        # Flush the cache for both repositories. This has to be done _before_
-        # removing the physical repositories as some expiration code depends on
-        # Git data (e.g. a list of branch names).
-        flush_caches(project)
-
-        trash_project_repositories!
-
-        log_info("Project \"#{project.name}\" was removed")
-      end
-
-      private
-
       def log_audit_event(project)
         ::AuditEventService.new(
           current_user,
diff --git a/ee/spec/services/projects/destroy_service_spec.rb b/ee/spec/services/projects/destroy_service_spec.rb
index 54d311c49aa63a..bf34451bc3096f 100644
--- a/ee/spec/services/projects/destroy_service_spec.rb
+++ b/ee/spec/services/projects/destroy_service_spec.rb
@@ -20,79 +20,93 @@
     stub_container_registry_tags(repository: :any, tags: [])
   end
 
-  context 'when project is a mirror' do
-    it 'decrements capacity if mirror was scheduled' do
-      max_capacity = Gitlab::CurrentSettings.mirror_max_capacity
-      project_mirror = create(:project, :mirror, :repository, :import_scheduled)
-
-      Gitlab::Mirror.increment_capacity(project_mirror.id)
+  shared_examples 'project destroy ee' do
+    context 'when project is a mirror' do
+      let(:max_capacity) { Gitlab::CurrentSettings.mirror_max_capacity }
+      let(:project_mirror) { create(:project, :mirror, :repository, :import_scheduled) }
+      let(:result) { described_class.new(project_mirror, project_mirror.owner, {}).execute }
+
+      before do
+        Gitlab::Mirror.increment_capacity(project_mirror.id)
+      end
 
-      expect do
-        described_class.new(project_mirror, project_mirror.owner, {}).execute
-      end.to change { Gitlab::Mirror.available_capacity }.from(max_capacity - 1).to(max_capacity)
+      it 'decrements capacity if mirror was scheduled' do
+        expect {result}.to change { Gitlab::Mirror.available_capacity }.from(max_capacity - 1).to(max_capacity)
+      end
     end
-  end
 
-  context 'when running on a primary node' do
-    let_it_be(:primary) { create(:geo_node, :primary) }
-    let_it_be(:secondary) { create(:geo_node) }
+    context 'when running on a primary node' do
+      let_it_be(:primary) { create(:geo_node, :primary) }
+      let_it_be(:secondary) { create(:geo_node) }
 
-    before do
-      stub_current_geo_node(primary)
-    end
+      before do
+        stub_current_geo_node(primary)
+      end
+
+      it 'logs an event to the Geo event log' do
+        # Run Sidekiq immediately to check that renamed repository will be removed
+        Sidekiq::Testing.inline! do
+          expect(subject).to receive(:log_destroy_success).and_call_original
+          expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1)
+        end
+      end
 
-    it 'logs an event to the Geo event log' do
-      # Run Sidekiq immediately to check that renamed repository will be removed
-      Sidekiq::Testing.inline! do
+      it 'does not log event to the Geo log if project deletion fails' do
         expect(subject).to receive(:log_destroy_event).and_call_original
-        expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1)
+        expect(project).to receive(:destroy!).and_raise(StandardError.new('Other error message'))
+
+        Sidekiq::Testing.inline! do
+          expect { subject.execute }.not_to change(Geo::RepositoryDeletedEvent, :count)
+        end
       end
     end
 
-    it 'does not log event to the Geo log if project deletion fails' do
-      expect(subject).to receive(:log_destroy_event).and_call_original
-      expect_any_instance_of(Project)
-        .to receive(:destroy!).and_raise(StandardError.new('Other error message'))
-
-      Sidekiq::Testing.inline! do
-        expect { subject.execute }.not_to change(Geo::RepositoryDeletedEvent, :count)
+    context 'audit events' do
+      include_examples 'audit event logging' do
+        let(:operation) { subject.execute }
+
+        let(:fail_condition!) do
+          expect(project).to receive(:destroy!).and_raise(StandardError.new('Other error message'))
+        end
+
+        let(:attributes) do
+          {
+            author_id: user.id,
+            entity_id: project.id,
+            entity_type: 'Project',
+            details: {
+              remove: 'project',
+              author_name: user.name,
+              target_id: project.full_path,
+              target_type: 'Project',
+              target_details: project.full_path
+            }
+          }
+        end
       end
     end
-  end
 
-  context 'audit events' do
-    include_examples 'audit event logging' do
-      let(:operation) { subject.execute }
-      let(:fail_condition!) do
-        expect_any_instance_of(Project)
-          .to receive(:destroy!).and_raise(StandardError.new('Other error message'))
+    context 'system hooks exception' do
+      before do
+        allow_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).and_raise('something went wrong')
       end
 
-      let(:attributes) do
-        {
-           author_id: user.id,
-           entity_id: project.id,
-           entity_type: 'Project',
-           details: {
-             remove: 'project',
-             author_name: user.name,
-             target_id: project.full_path,
-             target_type: 'Project',
-             target_details: project.full_path
-           }
-         }
+      it 'logs an audit event' do
+        expect(subject).to receive(:log_destroy_event).and_call_original
+        expect { subject.execute }.to change(AuditEvent, :count)
       end
     end
   end
 
-  context 'system hooks exception' do
+  context 'when project_transactionless_destroy enabled' do
+    it_behaves_like 'project destroy ee'
+  end
+
+  context 'when project_transactionless_destroy disabled', :sidekiq_inline do
     before do
-      allow_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).and_raise('something went wrong')
+      stub_feature_flags(project_transactionless_destroy: false)
     end
 
-    it 'logs an audit event' do
-      expect(subject).to receive(:log_destroy_event).and_call_original
-      expect { subject.execute }.to change(AuditEvent, :count)
-    end
+    it_behaves_like 'project destroy ee'
   end
 end
diff --git a/lib/gitlab/ci/features.rb b/lib/gitlab/ci/features.rb
index 2f6667d3600204..45a71346397070 100644
--- a/lib/gitlab/ci/features.rb
+++ b/lib/gitlab/ci/features.rb
@@ -79,6 +79,10 @@ def self.reset_ci_minutes_for_all_namespaces?
       def self.expand_names_for_cross_pipeline_artifacts?(project)
         ::Feature.enabled?(:ci_expand_names_for_cross_pipeline_artifacts, project)
       end
+
+      def self.project_transactionless_destroy(project)
+        Feature.enabled?(:project_transactionless_destroy, project)
+      end
     end
   end
 end
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 1b294511bf3c29..a3711c9e17fd19 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Projects::DestroyService do
+RSpec.describe Projects::DestroyService, :aggregate_failures do
   include ProjectForksHelper
 
   let_it_be(:user) { create(:user) }
@@ -371,32 +371,34 @@
         end
       end
     end
-  end
 
-  it_behaves_like 'project destroy'
+    context 'error while destroying', :sidekiq_inline do
+      let!(:pipeline) { create(:ci_pipeline, project: project) }
+      let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
+      let!(:build_trace) { create(:ci_build_trace_chunk, build: builds[0]) }
 
-  context 'error while destroying', :sidekiq_inline do
-    let!(:pipeline) { create(:ci_pipeline, project: project) }
-    let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
+      it 'deletes on retry' do
+        # We can expect this to timeout for very large projects
+        # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440
+        allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom')
+        destroy_project(project, user, {})
 
-    before do
-      # We can expect this to timeout for very large projects
-      # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440
-      allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom')
-      destroy_project(project, user, {})
-      allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original
-      destroy_project(project, user, {})
-    end
+        allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original
+        destroy_project(project, user, {})
 
-    it 'destroys project related records on retry' do
-      expect(Project.unscoped.all).not_to include(project)
-      expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
-      expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
-      expect(project.all_pipelines).to be_empty
-      expect(project.builds).to be_empty
+        expect(Project.unscoped.all).not_to include(project)
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+        expect(project.all_pipelines).to be_empty
+        expect(project.builds).to be_empty
+      end
     end
   end
 
+  context 'when project_transactionless_destroy enabled' do
+    it_behaves_like 'project destroy'
+  end
+
   context 'when project_transactionless_destroy disabled', :sidekiq_inline do
     before do
       stub_feature_flags(project_transactionless_destroy: false)
-- 
GitLab


From 06f2f581bcd3818d4840bda6c3b8ea12e57e7c73 Mon Sep 17 00:00:00 2001
From: "allison.browne" <abrowne@gitlab.com>
Date: Thu, 20 Aug 2020 14:04:48 -0400
Subject: [PATCH 4/4] Add explict default_enabled

---
 app/services/projects/destroy_service.rb          | 2 +-
 ee/app/services/ee/projects/destroy_service.rb    | 4 ++--
 ee/spec/services/projects/destroy_service_spec.rb | 4 ++--
 lib/gitlab/ci/features.rb                         | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index bfbc83f209c2f6..bec75657530bdd 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -105,7 +105,7 @@ def attempt_destroy(project)
 
       project.leave_pool_repository
 
-      if Gitlab::Ci::Features.project_transactionless_destroy(project)
+      if Gitlab::Ci::Features.project_transactionless_destroy?(project)
         destroy_project_related_records(project)
       else
         Project.transaction { destroy_project_related_records(project) }
diff --git a/ee/app/services/ee/projects/destroy_service.rb b/ee/app/services/ee/projects/destroy_service.rb
index 2fc1c60a69bc46..6237e690085755 100644
--- a/ee/app/services/ee/projects/destroy_service.rb
+++ b/ee/app/services/ee/projects/destroy_service.rb
@@ -37,10 +37,10 @@ def geo_replicate
 
       override :destroy_project_related_records
       def destroy_project_related_records(project)
-        super && log_destroy_success
+        super && log_destroy_events
       end
 
-      def log_destroy_success
+      def log_destroy_events
         log_geo_event(project)
         log_audit_event(project)
       end
diff --git a/ee/spec/services/projects/destroy_service_spec.rb b/ee/spec/services/projects/destroy_service_spec.rb
index bf34451bc3096f..8a21fe6c3d10c5 100644
--- a/ee/spec/services/projects/destroy_service_spec.rb
+++ b/ee/spec/services/projects/destroy_service_spec.rb
@@ -23,7 +23,7 @@
   shared_examples 'project destroy ee' do
     context 'when project is a mirror' do
       let(:max_capacity) { Gitlab::CurrentSettings.mirror_max_capacity }
-      let(:project_mirror) { create(:project, :mirror, :repository, :import_scheduled) }
+      let_it_be(:project_mirror) { create(:project, :mirror, :repository, :import_scheduled) }
       let(:result) { described_class.new(project_mirror, project_mirror.owner, {}).execute }
 
       before do
@@ -46,7 +46,7 @@
       it 'logs an event to the Geo event log' do
         # Run Sidekiq immediately to check that renamed repository will be removed
         Sidekiq::Testing.inline! do
-          expect(subject).to receive(:log_destroy_success).and_call_original
+          expect(subject).to receive(:log_destroy_events).and_call_original
           expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1)
         end
       end
diff --git a/lib/gitlab/ci/features.rb b/lib/gitlab/ci/features.rb
index 45a71346397070..934d1a4c9f10bd 100644
--- a/lib/gitlab/ci/features.rb
+++ b/lib/gitlab/ci/features.rb
@@ -80,8 +80,8 @@ def self.expand_names_for_cross_pipeline_artifacts?(project)
         ::Feature.enabled?(:ci_expand_names_for_cross_pipeline_artifacts, project)
       end
 
-      def self.project_transactionless_destroy(project)
-        Feature.enabled?(:project_transactionless_destroy, project)
+      def self.project_transactionless_destroy?(project)
+        Feature.enabled?(:project_transactionless_destroy, project, default_enabled: false)
       end
     end
   end
-- 
GitLab