summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/services/projects/destroy_service.rb57
-rw-r--r--app/views/projects/_deletion_failed.html.haml13
-rw-r--r--app/views/projects/_flash_messages.html.haml5
-rw-r--r--app/views/projects/empty.html.haml1
-rw-r--r--app/views/projects/show.html.haml1
-rw-r--r--app/workers/project_destroy_worker.rb2
-rw-r--r--changelogs/unreleased/29289-project-destroy-clean-up-after-failure.yml4
-rw-r--r--spec/features/projects/show_project_spec.rb18
-rw-r--r--spec/services/projects/destroy_service_spec.rb37
-rw-r--r--spec/workers/project_destroy_worker_spec.rb19
10 files changed, 81 insertions, 76 deletions
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 7e38aacc91a..f6e8b6655f2 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -15,29 +15,48 @@ 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
+ def repo_path
+ project.path_with_namespace
+ end
+
+ def wiki_path
+ repo_path + '.wiki'
+ end
+
+ def trash_repositories!
+ 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
+
def remove_repository(path)
# Skip repository removal. We use this flag when remove user or group
return true if params[:skip_repo] == true
@@ -59,26 +78,24 @@ 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.')
end
- 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
+ trash_repositories!
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
##
@@ -107,7 +124,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
diff --git a/app/views/projects/_deletion_failed.html.haml b/app/views/projects/_deletion_failed.html.haml
index 028510b5671..4f3698f91e6 100644
--- a/app/views/projects/_deletion_failed.html.haml
+++ b/app/views/projects/_deletion_failed.html.haml
@@ -1,9 +1,6 @@
-- 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
diff --git a/app/views/projects/_flash_messages.html.haml b/app/views/projects/_flash_messages.html.haml
index 6c9d466c761..f47d84ef755 100644
--- a/app/views/projects/_flash_messages.html.haml
+++ b/app/views/projects/_flash_messages.html.haml
@@ -1,5 +1,8 @@
+- 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'
diff --git a/app/views/projects/empty.html.haml b/app/views/projects/empty.html.haml
index 3d7c72ae61a..d17709380d5 100644
--- a/app/views/projects/empty.html.haml
+++ b/app/views/projects/empty.html.haml
@@ -1,5 +1,4 @@
- @no_container = true
-- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
= render partial: 'flash_messages', locals: { project: @project }
diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml
index 3926149e790..a9b39cedb1d 100644
--- a/app/views/projects/show.html.haml
+++ b/app/views/projects/show.html.haml
@@ -1,7 +1,6 @@
- @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")
diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb
index e695ec060f0..a9188b78460 100644
--- a/app/workers/project_destroy_worker.rb
+++ b/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
diff --git a/changelogs/unreleased/29289-project-destroy-clean-up-after-failure.yml b/changelogs/unreleased/29289-project-destroy-clean-up-after-failure.yml
new file mode 100644
index 00000000000..488b37ac37f
--- /dev/null
+++ b/changelogs/unreleased/29289-project-destroy-clean-up-after-failure.yml
@@ -0,0 +1,4 @@
+---
+title: Handle errors while a project is being deleted asynchronously.
+merge_request: 11088
+author:
diff --git a/spec/features/projects/show_project_spec.rb b/spec/features/projects/show_project_spec.rb
index 5aa0d8f0026..1bc6fae9e7f 100644
--- a/spec/features/projects/show_project_spec.rb
+++ b/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)
+ it 'shows error message if deletion for project fails' do
+ 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
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index a629afe723d..357e09bee95 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -130,30 +130,29 @@ describe Projects::DestroyService, services: true do
it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository"
end
- context 'when `execute` raises any other error' do
+ context 'when `execute` raises expected 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 unexpected error' do
+ before do
+ expect_any_instance_of(Project)
+ .to receive(:destroy!).and_raise(Exception.new("Other error message"))
+ end
- context 'when `execute` raises an error' do
- before do
- expect_any_instance_of(Projects::DestroyService)
- .to receive(:execute).and_raise(ArgumentError)
- end
+ it 'allows error to bubble up and rolls back project deletion' do
+ expect do
+ Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+ end.to raise_error
- it 'allows the error to bubble up' do
- expect do
- Sidekiq::Testing.inline! { Projects::DestroyService.new(project, user, {}).execute }
- end.to raise_error(ArgumentError)
+ expect(project.reload.pending_delete).to be(false)
+ expect(project.delete_error).to include("Other error message")
+ end
end
end
end
@@ -182,8 +181,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
@@ -208,8 +206,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
diff --git a/spec/workers/project_destroy_worker_spec.rb b/spec/workers/project_destroy_worker_spec.rb
index 29f0295de42..f19c9dff941 100644
--- a/spec/workers/project_destroy_worker_spec.rb
+++ b/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")
-
- expect do
- subject.perform(project.id, project.owner.id, {})
- end.to change { project.reload.pending_delete }.from(true).to(false)
+ it 'does not raise error when project could not be found' do
+ expect do
+ subject.perform(-1, project.owner.id, {})
+ end.not_to raise_error
+ end
- expect(Project.all).to include(project)
- expect(project.delete_error).to eq("some error message")
- 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
View Lorry Controller Status