Move exception handling to execute

app/services/ee/projects/destroy_service.rb

@@ -4,12 +4,16 @@ module EE
       def execute
         raise NotImplementedError unless defined?(super)
 
-        super
+        succeeded = super
 
+        if succeeded
           mirror_cleanup(project)
           log_geo_event(project)
         end
 
+        succeeded
+      end
+
       def mirror_cleanup(project)
         return unless project.mirror?
 
@@ -29,7 +33,7 @@ module EE
         # 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, wiki_path)
+        flush_caches(project)
 
         trash_repositories!
         remove_tracking_entries!

app/services/projects/destroy_service.rb

@@ -16,25 +16,26 @@ module Projects
     def execute
       return false unless can?(current_user, :remove_project, project)
 
-      repo_path = project.path_with_namespace
-      wiki_path = repo_path + '.wiki'
-
       # 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, wiki_path)
+      flush_caches(project)
 
       Projects::UnlinkForkService.new(project, current_user).execute
 
-      attempt_destroy_transaction(project, repo_path, wiki_path)
+      attempt_destroy_transaction(project)
 
       system_hook_service.execute_hooks_for(project, :destroy)
-
       log_info("Project \"#{project.full_path}\" was removed")
+
       true
-    rescue Projects::DestroyService::DestroyError => error
-      Rails.logger.error("Deletion failed on #{project.full_path} with the following message: #{error.message}")
+    rescue => error
+      attempt_rollback(project, error.message)
       false
+    rescue Exception => error # rubocop:disable Lint/RescueException
+      # Project.transaction can raise Exception
+      attempt_rollback(project, error.message)
+      raise
     end
 
     private
@@ -78,7 +79,14 @@ module Projects
       end
     end
 
-    def attempt_destroy_transaction(project, repo_path, wiki_path)
+    def attempt_rollback(project, message)
+      return unless project
+
+      project.update_attributes(delete_error: message, pending_delete: false)
+      log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
+    end
+
+    def attempt_destroy_transaction(project)
       Project.transaction do
         unless remove_legacy_registry_tags
           raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
@@ -89,9 +97,6 @@ module Projects
         project.team.truncate
         project.destroy!
       end
-    rescue Exception => error # rubocop:disable Lint/RescueException
-      project.update_attributes(delete_error: error.message, pending_delete: false)
-      raise
     end
 
     ##
@@ -120,7 +125,7 @@ module Projects
       "#{path}+#{project.id}#{DELETED_FLAG}"
     end
 
-    def flush_caches(project, wiki_path)
+    def flush_caches(project)
       project.repository.before_delete
 
       Repository.new(wiki_path, project).before_delete

app/views/projects/_deletion_failed.html.haml

-- if @project.delete_error.present?
-  .project-deletion-failed-message.alert.alert-warning
-    This project was scheduled for deletion, but failed with the following message:
-    = @project.delete_error
+- project = local_assigns.fetch(:project)
+- return unless project.delete_error.present?
 
-    .alert-link-group
-      = link_to "Don't show again", profile_path(user: { hide_no_ssh_key: true }), method: :put, class: 'alert-link'
-      |
-      = link_to 'Remind later', '#', class: 'hide-no-ssh-message alert-link'
+.project-deletion-failed-message.alert.alert-warning
+  This project was scheduled for deletion, but failed with the following message:
+  = project.delete_error

app/views/projects/_flash_messages.html.haml

+- project = local_assigns.fetch(:project)
 - flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
 
 = content_for flash_message_container do
-  = render 'deletion_failed'
+  = render partial: 'deletion_failed', locals: { project: project }
   - if current_user && can?(current_user, :download_code, project)
     = render 'shared/no_ssh'
     = render 'shared/no_password'

app/views/projects/empty.html.haml

 - @no_container = true
-- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
 
 = render partial: 'flash_messages', locals: { project: @project }
+
 = render "projects/head"
 = render "home_panel"
 

app/views/projects/show.html.haml

 - @no_container = true
 - breadcrumb_title "Project"
 - @content_class = "limit-container-width" unless fluid_layout
-- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
 
 = content_for :meta_tags do
   = auto_discovery_link_tag(:atom, project_path(@project, rss_url_options), title: "#{@project.name} activity")
 
 = render partial: 'flash_messages', locals: { project: @project }
+
 = render "projects/head"
 = render "projects/last_push"
 = render "home_panel"

app/workers/project_destroy_worker.rb

@@ -8,6 +8,6 @@ class ProjectDestroyWorker
 
     ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
   rescue ActiveRecord::RecordNotFound => error
-    logger.error("Failed to delete project #{project.path_with_namespace} (#{project.id}): #{error.message}")
+    logger.error("Failed to delete project (#{project_id}): #{error.message}")
   end
 end

changelogs/unreleased/29289-project-destroy-clean-up-after-failure.yml

+---
+title: Handle errors while a project is being deleted asynchronously.
+merge_request: 11088
+author:

spec/features/projects/show_project_spec.rb

@@ -3,28 +3,18 @@ require 'spec_helper'
 describe 'Project show page', feature: true do
   context 'when project pending delete' do
     let(:project) { create(:project, :empty_repo, pending_delete: true) }
-    let(:worker) { ProjectDestroyWorker.new }
 
     before do
       sign_in(project.owner)
     end
 
     it 'shows flash error if deletion for project fails' do
-      error_message = "some error message"
-      project.update_attributes(delete_error: error_message, pending_delete: false)
+      project.update_attributes(delete_error: "Something went wrong", pending_delete: false)
 
-      visit namespace_project_path(project.namespace, project)
+      visit project_path(project)
 
       expect(page).to have_selector('.project-deletion-failed-message')
-      expect(page).to have_content("This project was scheduled for deletion, but failed with the following message: #{error_message}")
-    end
-
-    it 'renders 404 if project was successfully deleted' do
-      worker.perform(project.id, project.owner.id, {})
-
-      visit namespace_project_path(project.namespace, project)
-
-      expect(page).to have_http_status(404)
+      expect(page).to have_content("This project was scheduled for deletion, but failed with the following message: #{project.delete_error}")
     end
   end
 end

spec/lib/gitlab/import_export/safe_model_attributes.yml

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

spec/services/ee/projects/destroy_service_spec.rb

@@ -40,5 +40,14 @@ describe Projects::DestroyService, services: true do
         expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1)
       end
     end
+
+    it 'does not log event to the Geo log if project deletion fails' do
+      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)
+      end
+    end
   end
 end

spec/services/projects/destroy_service_spec.rb

@@ -145,28 +145,27 @@ describe Projects::DestroyService, services: true do
 
       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"))
+          expect_any_instance_of(Project)
+            .to receive(:destroy!).and_raise(StandardError.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
+      context 'when `execute` raises unexpected error' do
         before do
-        expect_any_instance_of(Projects::DestroyService)
-          .to receive(:execute).and_raise(ArgumentError)
+          expect_any_instance_of(Project)
+            .to receive(:destroy!).and_raise(Exception.new("Other error message"))
         end
 
-      it 'allows the error to bubble up' do
+        it 'allows error to bubble up and rolls back project deletion' do
           expect do
-          Sidekiq::Testing.inline! { Projects::DestroyService.new(project, user, {}).execute }
-        end.to raise_error(ArgumentError)
+            Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+          end.to raise_error
+
+          expect(project.reload.pending_delete).to be(false)
+          expect(project.delete_error).to include("Other error message")
+        end
       end
     end
   end
@@ -195,8 +194,7 @@ describe Projects::DestroyService, services: true do
           expect_any_instance_of(ContainerRepository)
             .to receive(:delete_tags!).and_return(false)
 
-          expect{ destroy_project(project, user) }
-            .to raise_error(ActiveRecord::RecordNotDestroyed)
+          expect(destroy_project(project, user)).to be false
         end
       end
     end
@@ -221,8 +219,7 @@ describe Projects::DestroyService, services: true do
           expect_any_instance_of(ContainerRepository)
             .to receive(:delete_tags!).and_return(false)
 
-          expect { destroy_project(project, user) }
-            .to raise_error(Projects::DestroyService::DestroyError)
+          expect(destroy_project(project, user)).to be false
         end
       end
     end

spec/workers/project_destroy_worker_spec.rb

@@ -21,17 +21,16 @@ describe ProjectDestroyWorker do
       expect(Dir.exist?(path)).to be_truthy
     end
 
-    describe 'when StandardError is raised' do
-      it 'reverts pending_delete attribute with a error message' do
-        allow_any_instance_of(::Projects::DestroyService).to receive(:execute).and_raise(StandardError, "some error message")
-
+    it 'does not raise error when project could not be found' do
       expect do
-          subject.perform(project.id, project.owner.id, {})
-        end.to change { project.reload.pending_delete }.from(true).to(false)
-
-        expect(Project.all).to include(project)
-        expect(project.delete_error).to eq("some error message")
+        subject.perform(-1, project.owner.id, {})
+      end.not_to raise_error
     end
+
+    it 'does not raise error when user could not be found' do
+      expect do
+        subject.perform(project.id, -1, {})
+      end.not_to raise_error
     end
   end
 end