diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 336d029d330cc170a1a5cd03e84296e109dd8ef5..b14b31302f50f0a5f527409db6c9e6ff9bfe837a 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -7,9 +7,16 @@ class DestroyService < BaseService DestroyError = Class.new(StandardError) DELETED_FLAG = '+deleted'.freeze + REPO_REMOVAL_DELAY = 5.minutes.to_i def async_execute project.update_attribute(:pending_delete, true) + + # Ensure no repository +deleted paths are kept, + # regardless of any issue with the ProjectDestroyWorker + # job process. + schedule_stale_repos_removal + job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params) Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}") end @@ -92,14 +99,23 @@ def remove_repository(path) log_info(%Q{Repository "#{path}" moved to "#{new_path}" for project "#{project.full_path}"}) project.run_after_commit do - # self is now project - GitlabShellWorker.perform_in(5.minutes, :remove_repository, self.repository_storage, new_path) + GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY, :remove_repository, self.repository_storage, new_path) end else false end end + def schedule_stale_repos_removal + repo_paths = [removal_path(repo_path), removal_path(wiki_path)] + + # Ideally it should wait until the regular removal phase finishes, + # so let's delay it a bit further. + repo_paths.each do |path| + GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY * 2, :remove_repository, project.repository_storage, path) + end + end + def rollback_repository(old_path, new_path) # There is a possibility project does not have repository or wiki return true unless repo_exists?(old_path) @@ -113,13 +129,11 @@ def repo_exists?(path) end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def mv_repository(from_path, to_path) - return true unless gitlab_shell.exists?(project.repository_storage, from_path + '.git') + return true unless repo_exists?(from_path) gitlab_shell.mv_repository(project.repository_storage, from_path, to_path) end - # rubocop: enable CodeReuse/ActiveRecord def attempt_rollback(project, message) return unless project diff --git a/changelogs/unreleased/osw-enforces-project-removal-with-past-failed-attempts.yml b/changelogs/unreleased/osw-enforces-project-removal-with-past-failed-attempts.yml new file mode 100644 index 0000000000000000000000000000000000000000..6a2a67e7aa85cb04777e6ba701b2ac936c897d4b --- /dev/null +++ b/changelogs/unreleased/osw-enforces-project-removal-with-past-failed-attempts.yml @@ -0,0 +1,5 @@ +--- +title: Cleanup stale +deleted repo paths on project removal (adjusts project removal bug) +merge_request: 24269 +author: +type: fixed diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 12ddf8447bdd338ee65df2fde215df1aa644d356..dfbdfa2ab69fbddff30f0f49339d3504904abc1f 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -281,6 +281,40 @@ end end + context 'repository +deleted path removal' do + def removal_path(path) + "#{path}+#{project.id}#{described_class::DELETED_FLAG}" + end + + context 'regular phase' do + it 'schedules +deleted removal of existing repos' do + service = described_class.new(project, user, {}) + allow(service).to receive(:schedule_stale_repos_removal) + + expect(GitlabShellWorker).to receive(:perform_in) + .with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path)) + + service.execute + end + end + + context 'stale cleanup' do + let!(:async) { true } + + it 'schedules +deleted wiki and repo removal' do + allow(ProjectDestroyWorker).to receive(:perform_async) + + expect(GitlabShellWorker).to receive(:perform_in) + .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path)) + + expect(GitlabShellWorker).to receive(:perform_in) + .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path)) + + destroy_project(project, user, {}) + end + end + end + context '#attempt_restore_repositories' do let(:path) { project.disk_path + '.git' }