Refactor project destroy services

In preparation for
https://gitlab.com/gitlab-org/gitlab/merge_requests/22672,
we need to refactor Projects::DestroyService. At the
moment, this service has hardcoded the repositories it
has to delete.

Nevertheless, we have now more requirements that
we had before. We need to remove Snippet repositories
and also DesignManagement ones.

In this commit, we refactor the existing
Projects::DestroyService creating two services just for
dealing with repositories and extracting this
functionality from this service.

We also split some of the existing functionality. The
current destroy service delete things but also is able
to rollback stuff. We can move this functionality into
a different service.

app/services/projects/destroy_rollback_service.rb

+# frozen_string_literal: true
+
+module Projects
+  class DestroyRollbackService < BaseService
+    include Gitlab::ShellAdapter
+
+    def execute
+      return unless project
+
+      Projects::ForksCountService.new(project).delete_cache
+
+      unless rollback_repository(project.repository)
+        raise_error(s_('DeleteProject|Failed to restore project repository. Please contact the administrator.'))
+      end
+
+      unless rollback_repository(project.wiki.repository)
+        raise_error(s_('DeleteProject|Failed to restore wiki repository. Please contact the administrator.'))
+      end
+    end
+
+    private
+
+    def rollback_repository(repository)
+      return true unless repository
+
+      result = Repositories::DestroyRollbackService.new(repository).execute
+
+      result[:status] == :success
+    end
+  end
+end

app/services/projects/destroy_service.rb

@@ -6,9 +6,6 @@ module Projects
 
     DestroyError = Class.new(StandardError)
 
-    DELETED_FLAG = '+deleted'
-    REPO_REMOVAL_DELAY = 5.minutes.to_i
-
     def async_execute
       project.update_attribute(:pending_delete, true)
 
@@ -18,7 +15,7 @@ module Projects
       schedule_stale_repos_removal
 
       job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
-      Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}") # rubocop:disable Gitlab/RailsLogger
+      log_info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}")
     end
 
     def execute
@@ -48,82 +45,34 @@ module Projects
       raise
     end
 
-    def attempt_repositories_rollback
-      return unless @project
-
-      flush_caches(@project)
-
-      unless rollback_repository(removal_path(repo_path), repo_path)
-        raise_error(s_('DeleteProject|Failed to restore project repository. Please contact the administrator.'))
-      end
-
-      unless rollback_repository(removal_path(wiki_path), wiki_path)
-        raise_error(s_('DeleteProject|Failed to restore wiki repository. Please contact the administrator.'))
-      end
-    end
-
     private
 
-    def repo_path
-      project.disk_path
-    end
-
-    def wiki_path
-      project.wiki.disk_path
-    end
-
     def trash_repositories!
-      unless remove_repository(repo_path)
+      unless remove_repository(project.repository)
         raise_error(s_('DeleteProject|Failed to remove project repository. Please try again or contact administrator.'))
       end
 
-      unless remove_repository(wiki_path)
+      unless remove_repository(project.wiki.repository)
         raise_error(s_('DeleteProject|Failed to remove wiki repository. Please try again or contact administrator.'))
       end
     end
 
-    def remove_repository(path)
-      # There is a possibility project does not have repository or wiki
-      return true unless repo_exists?(path)
+    def remove_repository(repository)
+      return true unless repository
 
-      new_path = removal_path(path)
+      result = Repositories::DestroyService.new(repository).execute
 
-      if mv_repository(path, new_path)
-        log_info(%Q{Repository "#{path}" moved to "#{new_path}" for project "#{project.full_path}"})
-
-        project.run_after_commit do
-          GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY, :remove_repository, self.repository_storage, new_path)
-        end
-      else
-        false
-      end
+      result[:status] == :success
     end
 
     def schedule_stale_repos_removal
-      repo_paths = [removal_path(repo_path), removal_path(wiki_path)]
+      repos = [project.repository, project.wiki.repository]
 
-      # Ideally it should wait until the regular removal phase finishes,
-      # so let's delay it a bit further.
-      repo_paths.each do |path|
-        GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY * 2, :remove_repository, project.repository_storage, path)
-      end
-    end
+      repos.each do |repository|
+        next unless repository
 
-    def rollback_repository(old_path, new_path)
-      # There is a possibility project does not have repository or wiki
-      return true unless repo_exists?(old_path)
-
-      mv_repository(old_path, new_path)
-    end
-
-    def repo_exists?(path)
-      gitlab_shell.repository_exists?(project.repository_storage, path + '.git')
-    end
-
-    def mv_repository(from_path, to_path)
-      return true unless repo_exists?(from_path)
-
-      gitlab_shell.mv_repository(project.repository_storage, from_path, to_path)
+        Repositories::ShellDestroyService.new(repository).execute(Repositories::ShellDestroyService::STALE_REMOVAL_DELAY)
+      end
     end
 
     def attempt_rollback(project, message)
@@ -191,32 +140,9 @@ module Projects
       raise DestroyError.new(message)
     end
 
-    # Build a path for removing repositories
-    # We use `+` because its not allowed by GitLab so user can not create
-    # project with name cookies+119+deleted and capture someone stalled repository
-    #
-    # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
-    #
-    def removal_path(path)
-      "#{path}+#{project.id}#{DELETED_FLAG}"
-    end
-
     def flush_caches(project)
-      ignore_git_errors(repo_path) { project.repository.before_delete }
-
-      ignore_git_errors(wiki_path) { Repository.new(wiki_path, project, disk_path: repo_path).before_delete }
-
       Projects::ForksCountService.new(project).delete_cache
     end
-
-    # If we get a Gitaly error, the repository may be corrupted. We can
-    # ignore these errors since we're going to trash the repositories
-    # anyway.
-    def ignore_git_errors(disk_path, &block)
-      yield
-    rescue Gitlab::Git::CommandError => e
-      Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
-    end
   end
 end
 

app/services/projects/overwrite_project_service.rb

@@ -55,7 +55,7 @@ module Projects
     end
 
     def attempt_restore_repositories(project)
-      ::Projects::DestroyService.new(project, @current_user).attempt_repositories_rollback
+      ::Projects::DestroyRollbackService.new(project, @current_user).execute
     end
 
     def add_source_project_to_fork_network(source_project)

app/services/repositories/base_service.rb

+# frozen_string_literal: true
+
+class Repositories::BaseService < BaseService
+  include Gitlab::ShellAdapter
+
+  DELETED_FLAG = '+deleted'
+
+  attr_reader :repository
+
+  delegate :project, :disk_path, :full_path, to: :repository
+  delegate :repository_storage, to: :project
+
+  def initialize(repository)
+    @repository = repository
+  end
+
+  def repo_exists?(path)
+    gitlab_shell.repository_exists?(repository_storage, path + '.git')
+  end
+
+  def mv_repository(from_path, to_path)
+    return true unless repo_exists?(from_path)
+
+    gitlab_shell.mv_repository(repository_storage, from_path, to_path)
+  end
+
+  # Build a path for removing repositories
+  # We use `+` because its not allowed by GitLab so user can not create
+  # project with name cookies+119+deleted and capture someone stalled repository
+  #
+  # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
+  #
+  def removal_path
+    "#{disk_path}+#{project.id}#{DELETED_FLAG}"
+  end
+
+  # If we get a Gitaly error, the repository may be corrupted. We can
+  # ignore these errors since we're going to trash the repositories
+  # anyway.
+  def ignore_git_errors(&block)
+    yield
+  rescue Gitlab::Git::CommandError => e
+    Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
+  end
+
+  def move_error(path)
+    error = %Q{Repository "#{path}" could not be moved}
+
+    log_error(error)
+    error(error)
+  end
+end

app/services/repositories/destroy_rollback_service.rb

+# frozen_string_literal: true
+
+class Repositories::DestroyRollbackService < Repositories::BaseService
+  def execute
+    # There is a possibility project does not have repository or wiki
+    return success unless repo_exists?(removal_path)
+
+    # Flush the cache for both repositories.
+    ignore_git_errors { repository.before_delete }
+
+    if mv_repository(removal_path, disk_path)
+      log_info(%Q{Repository "#{removal_path}" moved to "#{disk_path}" for repository "#{full_path}"})
+
+      success
+    else
+      move_error(removal_path)
+    end
+  end
+end

app/services/repositories/destroy_service.rb

+# frozen_string_literal: true
+
+class Repositories::DestroyService < Repositories::BaseService
+  def execute
+    return success unless repository
+    return success unless repo_exists?(disk_path)
+
+    # 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).
+    ignore_git_errors { repository.before_delete }
+
+    if mv_repository(disk_path, removal_path)
+      log_info(%Q{Repository "#{disk_path}" moved to "#{removal_path}" for repository "#{full_path}"})
+
+      current_repository = repository
+      project.run_after_commit do
+        Repositories::ShellDestroyService.new(current_repository).execute
+      end
+
+      log_info("Project \"#{project.full_path}\" was removed")
+
+      success
+    else
+      move_error(disk_path)
+    end
+  end
+end

app/services/repositories/shell_destroy_service.rb

+# frozen_string_literal: true
+
+class Repositories::ShellDestroyService < Repositories::BaseService
+  REPO_REMOVAL_DELAY = 5.minutes.to_i
+  STALE_REMOVAL_DELAY = REPO_REMOVAL_DELAY * 2
+
+  def execute(delay = REPO_REMOVAL_DELAY)
+    return success unless repository
+
+    GitlabShellWorker.perform_in(delay,
+                                 :remove_repository,
+                                 repository_storage,
+                                 removal_path)
+  end
+end

ee/app/services/ee/projects/destroy_service.rb

@@ -34,8 +34,8 @@ module EE
       def log_geo_event(project)
         ::Geo::RepositoryDeletedEventStore.new(
           project,
-          repo_path: repo_path,
-          wiki_path: wiki_path
+          repo_path: project.disk_path,
+          wiki_path: project.wiki.disk_path
         ).create!
       end
 

spec/services/projects/destroy_rollback_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::DestroyRollbackService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:repository_storage) { project.repository_storage }
+
+  subject { described_class.new(project, user, {}).execute }
+
+  describe '#execute' do
+    let(:path) { repository.disk_path + '.git' }
+    let(:removal_path) { "#{repository.disk_path}+#{project.id}#{Repositories::DestroyService::DELETED_FLAG}.git" }
+
+    before do
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_truthy
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_falsey
+      end
+
+      # Don't run sidekiq to check if renamed repository exists
+      Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
+
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_falsey
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_truthy
+      end
+    end
+
+    it 'restores the repositories' do
+      Sidekiq::Testing.fake! { subject }
+
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_truthy
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_falsey
+      end
+    end
+  end
+
+  def destroy_project(project, user, params = {})
+    Projects::DestroyService.new(project, user, params).execute
+  end
+end

spec/services/repositories/destroy_rollback_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::DestroyRollbackService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:path) { repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  subject { described_class.new(repository).execute }
+
+  before do
+    # Dont run sidekiq to check if renamed repository exists
+    Sidekiq::Testing.fake! { destroy_project(project, user) }
+  end
+
+  it 'moves the repository from the +deleted folder' do
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+
+    subject
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+  end
+
+  it 'logs the successful action' do
+    expect(Gitlab::AppLogger).to receive(:info)
+
+    subject
+  end
+
+  it 'flushes the repository cache' do
+    expect(repository).to receive(:before_delete)
+
+    subject
+  end
+
+  it 'returns success and does not perform any action if repository path does not exist' do
+    expect(repository).to receive(:disk_path).and_return('foo')
+    expect(repository).not_to receive(:before_delete)
+
+    result = subject
+
+    expect(result[:status]).to eq :success
+  end
+
+  context 'when move operation cannot be performed' do
+    let(:service) { described_class.new(repository) }
+
+    before do
+      allow(service).to receive(:mv_repository).and_return(false)
+    end
+
+    it 'returns error' do
+      result = service.execute
+
+      expect(result[:status]).to eq :error
+    end
+
+    it 'logs the error' do
+      expect(Gitlab::AppLogger).to receive(:error)
+
+      service.execute
+    end
+  end
+
+  def destroy_project(project, user)
+    Projects::DestroyService.new(project, user, {}).execute
+  end
+end

spec/services/repositories/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::DestroyService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:path) { repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  subject { described_class.new(project.repository).execute }
+
+  it 'moves the repository to a +deleted folder' do
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+
+    subject
+
+    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_truthy
+  end
+
+  it 'schedules the repository deletion' do
+    subject
+
+    expect(Repositories::ShellDestroyService).to receive(:new).with(repository).and_call_original
+
+    expect(GitlabShellWorker).to receive(:perform_in)
+      .with(Repositories::ShellDestroyService::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+    # Because GitlabShellWorker is inside a run_after_commit callback we need to
+    # trigger the callback
+    project.touch
+  end
+
+  it 'removes the repository', :sidekiq_inline do
+    subject
+
+    project.touch
+
+    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
+  end
+
+  it 'flushes the repository cache' do
+    expect(repository).to receive(:before_delete)
+
+    subject
+  end
+
+  it 'does not perform any action if repository path does not exist and returns success' do
+    expect(repository).to receive(:disk_path).and_return('foo')
+    expect(repository).not_to receive(:before_delete)
+
+    result = subject
+
+    expect(result[:status]).to eq :success
+  end
+
+  context 'when move operation cannot be performed' do
+    let(:service) { described_class.new(repository) }
+
+    before do
+      allow(service).to receive(:mv_repository).and_return(false)
+    end
+
+    it 'returns error' do
+      result = service.execute
+
+      expect(result[:status]).to eq :error
+    end
+
+    it 'logs the error' do
+      expect(Gitlab::AppLogger).to receive(:error)
+
+      service.execute
+    end
+  end
+end

spec/services/repositories/shell_destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::ShellDestroyService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:path) { project.repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  it 'returns success if the repository is nil' do
+    expect(GitlabShellWorker).not_to receive(:perform_in)
+
+    result = described_class.new(nil).execute
+
+    expect(result[:status]).to eq :success
+  end
+
+  it 'schedules the repository deletion' do
+    expect(GitlabShellWorker).to receive(:perform_in)
+      .with(described_class::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+    described_class.new(project.repository).execute
+  end
+end