Commit 72a85ae9 authored by Timothy Andrew's avatar Timothy Andrew Committed by Tiago Botelho

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.
parent 445cd22c
......@@ -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
......
......@@ -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
# 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
......@@ -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
......
......@@ -396,6 +396,7 @@ Project:
- build_allow_git_fetch
- last_repository_updated_at
- ci_config_path
- delete_error
Author:
- name
ProjectFeature:
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment