Handle errors while a project is being deleted asynchronously.

1. Rescue all errors that `Projects::DestroyService` might throw, to prevent the
   worker from leaving things in an inconsistent state

2. Unmark the project as `pending_delete`

3. Add a `delete_error` text column to `projects`, and save the error message in
   there, to be shown to the project masters/owners.

6 files changed, 104 insertions, 4 deletions

diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index e2b2660ea71..682407ac896 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -26,9 +26,6 @@ module Projects
       Projects::UnlinkForkService.new(project, current_user).execute
 
       Project.transaction do
-        project.team.truncate
-        project.destroy!
-
         unless remove_legacy_registry_tags
           raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
         end
@@ -40,10 +37,14 @@ module Projects
         unless remove_repository(wiki_path)
           raise_error('Failed to remove wiki repository. Please try again or contact administrator.')
         end
+
+        project.team.truncate
+        project.destroy!
       end
 
-      log_info("Project \"#{project.path_with_namespace}\" was removed")
       system_hook_service.execute_hooks_for(project, :destroy)
+
+      log_info("Project \"#{project.path_with_namespace}\" was removed")
       true
     end
 
diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb
index b462327490e..482e1e38cd1 100644
--- a/app/workers/project_destroy_worker.rb
+++ b/app/workers/project_destroy_worker.rb
@@ -12,5 +12,8 @@ class ProjectDestroyWorker
     user = User.find(user_id)
 
     ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
+  rescue StandardError => error
+    project.assign_attributes(delete_error: error.message, pending_delete: false)
+    project.save!(validate: false)
   end
 end
diff --git a/db/migrate/20170428064307_add_column_delete_error_to_projects.rb b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb
new file mode 100644
index 00000000000..ef5fc2cdea5
--- /dev/null
+++ b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb
@@ -0,0 +1,31 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddColumnDeleteErrorToProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :projects, :delete_error, :text
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 284b2068166..0ba2bd31517 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1134,6 +1134,7 @@ ActiveRecord::Schema.define(version: 20170717150329) do
     t.integer "cached_markdown_version"
     t.datetime "last_repository_updated_at"
     t.string "ci_config_path"
+    t.text "delete_error"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 4ef3db3721f..0f2db3380a7 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -396,6 +396,7 @@ Project:
 - build_allow_git_fetch
 - last_repository_updated_at
 - ci_config_path
+- delete_error
 Author:
 - name
 ProjectFeature:
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index b399d3402fd..a629afe723d 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -36,6 +36,27 @@ describe Projects::DestroyService, services: true do
     end
   end
 
+  shared_examples 'handles errors thrown during async destroy' do |error_message|
+    it 'does not allow the error to bubble up' do
+      expect do
+        Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+      end.not_to raise_error
+    end
+
+    it 'unmarks the project as "pending deletion"' do
+      Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+
+      expect(project.reload.pending_delete).to be(false)
+    end
+
+    it 'stores an error message in `projects.delete_error`' do
+      Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+
+      expect(project.reload.delete_error).to be_present
+      expect(project.delete_error).to include(error_message)
+    end
+  end
+
   context 'Sidekiq inline' do
     before do
       # Run sidekiq immediatly to check that renamed repository will be removed
@@ -89,10 +110,52 @@ describe Projects::DestroyService, services: true do
     end
 
     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(Projects::DestroyService)
+            .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
+
+      context 'when `remove_repository` fails' do
+        before do
+          expect_any_instance_of(Projects::DestroyService)
+            .to receive(:remove_repository).and_return(false)
+        end
+
+        it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository"
+      end
+
+      context 'when `execute` raises any other error' do
+        before do
+          expect_any_instance_of(Projects::DestroyService)
+            .to receive(:execute).and_raise(ArgumentError.new("Other error message"))
+        end
+
+        it_behaves_like 'handles errors thrown during async destroy', "Other error message"
+      end
+    end
   end
 
   context 'with execute' do
     it_behaves_like 'deleting the project with pipeline and build'
+
+    context 'when `execute` raises an error' do
+      before do
+        expect_any_instance_of(Projects::DestroyService)
+          .to receive(:execute).and_raise(ArgumentError)
+      end
+
+      it 'allows the error to bubble up' do
+        expect do
+          Sidekiq::Testing.inline! { Projects::DestroyService.new(project, user, {}).execute }
+        end.to raise_error(ArgumentError)
+      end
+    end
   end
 
   describe 'container registry' do