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.

app/services/projects/destroy_service.rb

@@ -27,10 +27,6 @@ module Projects
       Projects::UnlinkForkService.new(project, current_user).execute
 
       Project.transaction do
-        project.team.truncate
-        project.destroy!
-        trash_repositories!
-
         unless remove_legacy_registry_tags
           raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
         end
@@ -42,11 +38,15 @@ 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!
+        trash_repositories!
       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
 

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

db/migrate/20170428064307_add_column_delete_error_to_projects.rb

+# 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

db/schema.rb

@@ -1388,6 +1388,7 @@ ActiveRecord::Schema.define(version: 20170719182937) do
     t.datetime "last_repository_updated_at"
     t.string "ci_config_path"
     t.boolean "disable_overriding_approvers_per_merge_request"
+    t.text "delete_error"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -411,7 +411,7 @@ Project:
 - sync_time
 - service_desk_enabled
 - last_repository_updated_at
-- ci_config_path
+- delete_error
 Author:
 - name
 ProjectFeature:

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
@@ -102,10 +123,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