diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index e2b2660ea71..f6e8b6655f2 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -15,40 +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 - Project.transaction do - project.team.truncate - project.destroy! + attempt_destroy_transaction(project) - 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 - end - - log_info("Project \"#{project.path_with_namespace}\" was removed") system_hook_service.execute_hooks_for(project, :destroy) + log_info("Project \"#{project.full_path}\" was removed") + true + 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 @@ -70,6 +78,26 @@ module Projects end end + 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 + + trash_repositories! + + project.team.truncate + project.destroy! + end + end + ## # This method makes sure that we correctly remove registry tags # for legacy image repository (when repository path equals project path). @@ -96,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 new file mode 100644 index 00000000000..4f3698f91e6 --- /dev/null +++ b/app/views/projects/_deletion_failed.html.haml @@ -0,0 +1,6 @@ +- project = local_assigns.fetch(:project) +- return unless 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 diff --git a/app/views/projects/_flash_messages.html.haml b/app/views/projects/_flash_messages.html.haml new file mode 100644 index 00000000000..f47d84ef755 --- /dev/null +++ b/app/views/projects/_flash_messages.html.haml @@ -0,0 +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 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 0f132a68ce1..d17709380d5 100644 --- a/app/views/projects/empty.html.haml +++ b/app/views/projects/empty.html.haml @@ -1,10 +1,6 @@ - @no_container = true -- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message -= content_for flash_message_container do - - if current_user && can?(current_user, :download_code, @project) - = render 'shared/no_ssh' - = render 'shared/no_password' += render partial: 'flash_messages', locals: { project: @project } = render "projects/head" = render "home_panel" diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 49d0a6828fe..a9b39cedb1d 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -1,15 +1,11 @@ - @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") -= content_for flash_message_container do - - if current_user && can?(current_user, :download_code, @project) - = render 'shared/no_ssh' - = render 'shared/no_password' += render partial: 'flash_messages', locals: { project: @project } = render "projects/head" = render "projects/last_push" diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb index b462327490e..a9188b78460 100644 --- a/app/workers/project_destroy_worker.rb +++ b/app/workers/project_destroy_worker.rb @@ -3,14 +3,11 @@ class ProjectDestroyWorker include DedicatedSidekiqQueue def perform(project_id, user_id, params) - begin - project = Project.unscoped.find(project_id) - rescue ActiveRecord::RecordNotFound - return - end - + project = Project.find(project_id) user = User.find(user_id) ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute + rescue ActiveRecord::RecordNotFound => error + 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/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..09f9d9b5b7a --- /dev/null +++ b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb @@ -0,0 +1,7 @@ +class AddColumnDeleteErrorToProjects < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :projects, :delete_error, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 7724af5b610..61bcd8c7e95 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1134,6 +1134,7 @@ ActiveRecord::Schema.define(version: 20170724214302) 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/features/projects/show_project_spec.rb b/spec/features/projects/show_project_spec.rb new file mode 100644 index 00000000000..1bc6fae9e7f --- /dev/null +++ b/spec/features/projects/show_project_spec.rb @@ -0,0 +1,20 @@ +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) } + + before do + sign_in(project.owner) + end + + it 'shows error message if deletion for project fails' do + project.update_attributes(delete_error: "Something went wrong", pending_delete: false) + + 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: #{project.delete_error}") + end + end +end 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..357e09bee95 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,51 @@ describe Projects::DestroyService, services: true do end it_behaves_like 'deleting the project with pipeline and build' - end - context 'with execute' do - 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 expected error' do + before do + 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 + + 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 + + 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 + + expect(project.reload.pending_delete).to be(false) + expect(project.delete_error).to include("Other error message") + end + end + end end describe 'container registry' do @@ -119,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 @@ -145,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 3d135f40c1f..f19c9dff941 100644 --- a/spec/workers/project_destroy_worker_spec.rb +++ b/spec/workers/project_destroy_worker_spec.rb @@ -1,24 +1,36 @@ require 'spec_helper' describe ProjectDestroyWorker do - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository, pending_delete: true) } let(:path) { project.repository.path_to_repo } subject { described_class.new } - describe "#perform" do - it "deletes the project" do + describe '#perform' do + it 'deletes the project' do subject.perform(project.id, project.owner.id, {}) expect(Project.all).not_to include(project) expect(Dir.exist?(path)).to be_falsey end - it "deletes the project but skips repo deletion" do + it 'deletes the project but skips repo deletion' do subject.perform(project.id, project.owner.id, { "skip_repo" => true }) expect(Project.all).not_to include(project) expect(Dir.exist?(path)).to be_truthy end + + 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 + + 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