Move repository when project is removed

Ths commit does next:

* When we remove project we move repository to path+deleted.git
* Then we schedule removal of path+deleted with sidekiq
* If repository move failed we abort project removal

This should help us with NFS issue when project get removed but
repository stayed. The full explanation of problem is below:

* rm -rf project.git
* rm -rf removes project.git/objects/foo
* NFS server renames foo to foo.nfsXXXX because some NFS client (think
* Unicorn) still has the file open
* rm -rf exits, but project.git/objects/foo.nfsXXX still exists
* Unicorn closes the file, the NFS client closes the file (foo), and the
* NFS server removes foo.nfsXXX
* the directory project.git/objects/ still exists => problem

So now we move repository and even if repository removal failed

Repository directory is moved so no bugs with project removed but
repository directory taken. User still able to create new project with
same name. From administrator perspective you can easily find stalled
repositories by searching `*+deleted.git`

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

4 files changed, 70 insertions, 27 deletions

diff --git a/CHANGELOG b/CHANGELOG
index dbc54021d40..381369f0dea 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -40,6 +40,7 @@ v 7.12.0 (unreleased)
   - Add an option to automatically sign-in with an Omniauth provider
   - Better performance for web editor (switched from satellites to rugged)
   - GitLab CI service sends .gitlab-ci.yaml in each push call
+  - When remove project - move repository and schedule it removal
 
 v 7.11.4
   - Fix missing bullets when creating lists
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index dc430351551..4ca5fc65459 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -97,18 +97,15 @@ class ProjectsController < ApplicationController
     return access_denied! unless can?(current_user, :remove_project, @project)
 
     ::Projects::DestroyService.new(@project, current_user, {}).execute
+    flash[:alert] = 'Project deleted.'
 
-    respond_to do |format|
-      format.html do
-        flash[:alert] = 'Project deleted.'
-
-        if request.referer.include?('/admin')
-          redirect_to admin_namespaces_projects_path
-        else
-          redirect_to dashboard_path
-        end
-      end
+    if request.referer.include?('/admin')
+      redirect_to admin_namespaces_projects_path
+    else
+      redirect_to dashboard_path
     end
+  rescue Projects::DestroyService::DestroyError => ex
+    redirect_to edit_project_path(@project), alert: ex.message
   end
 
   def autocomplete_sources
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 7e1d753b021..53bf36b1010 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -1,28 +1,67 @@
 module Projects
   class DestroyService < BaseService
+    include Gitlab::ShellAdapter
+
+    class DestroyError < StandardError; end
+
+    DELETED_FLAG = '+deleted'
+
     def execute
       return false unless can?(current_user, :remove_project, project)
 
       project.team.truncate
       project.repository.expire_cache unless project.empty_repo?
 
-      if project.destroy
-        GitlabShellWorker.perform_async(
-          :remove_repository,
-          project.path_with_namespace
-        )
+      repo_path = project.path_with_namespace
+      wiki_path = repo_path + '.wiki'
+
+      Project.transaction do
+        project.destroy!
 
-        GitlabShellWorker.perform_async(
-          :remove_repository,
-          project.path_with_namespace + ".wiki"
-        )
+        unless remove_repository(repo_path)
+          raise_error('Failed to remove project repository. Please try again or contact administrator')
+        end
+
+        unless remove_repository(wiki_path)
+          raise_error('Failed to remove wiki repository. Please try again or contact administrator')
+        end
+      end
+
+      project.satellite.destroy
+      log_info("Project \"#{project.name}\" was removed")
+      system_hook_service.execute_hooks_for(project, :destroy)
+      true
+    end
 
-        project.satellite.destroy
+    private
 
-        log_info("Project \"#{project.name}\" was removed")
-        system_hook_service.execute_hooks_for(project, :destroy)
-        true
+    def remove_repository(path)
+      unless gitlab_shell.exists?(path + '.git')
+        return true
       end
+
+      new_path = removal_path(path)
+
+      if gitlab_shell.mv_repository(path, new_path)
+        log_info("Repository \"#{path}\" moved to \"#{new_path}\"")
+        GitlabShellWorker.perform_in(30.seconds, :remove_repository, new_path)
+      else
+        false
+      end
+    end
+
+    def raise_error(message)
+      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
   end
 end
diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb
index 530f9d93de4..172d4902add 100644
--- a/lib/gitlab/backend/shell.rb
+++ b/lib/gitlab/backend/shell.rb
@@ -244,6 +244,16 @@ module Gitlab
       end
     end
 
+    # Check if such directory exists in repositories.
+    #
+    # Usage:
+    #   exists?('gitlab')
+    #   exists?('gitlab/cookies.git')
+    #
+    def exists?(dir_name)
+      File.exists?(full_path(dir_name))
+    end
+
     protected
 
     def gitlab_shell_path
@@ -264,10 +274,6 @@ module Gitlab
       File.join(repos_path, dir_name)
     end
 
-    def exists?(dir_name)
-      File.exists?(full_path(dir_name))
-    end
-
     def gitlab_shell_projects_path
       File.join(gitlab_shell_path, 'bin', 'gitlab-projects')
     end