Merge branch '223272-geo-project-removal-leaves-orphaned-data-on-disk' into 'master'

Geo - Schedule the repositories deletion after project removal

Closes #223272

See merge request gitlab-org/gitlab!34963

app/services/repositories/destroy_service.rb

@@ -14,8 +14,17 @@ class Repositories::DestroyService < Repositories::BaseService
       log_info(%Q{Repository "#{disk_path}" moved to "#{removal_path}" for repository "#{full_path}"})
 
       current_repository = repository
-      container.run_after_commit do
+
+      # Because GitlabShellWorker is inside a run_after_commit callback it will
+      # never be triggered on a read-only instance.
+      #
+      # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/223272
+      if Gitlab::Database.read_only?
         Repositories::ShellDestroyService.new(current_repository).execute
+      else
+        container.run_after_commit do
+          Repositories::ShellDestroyService.new(current_repository).execute
+        end
       end
 
       log_info("Repository \"#{full_path}\" was removed")

ee/changelogs/unreleased/223272-geo-project-removal-leaves-orphaned-data-on-disk.yml

+---
+title: Geo - Fix repositories deletion after project removal
+merge_request: 34963
+author:
+type: fixed

ee/spec/services/geo/repository_destroy_service_spec.rb

@@ -24,34 +24,41 @@ RSpec.describe Geo::RepositoryDestroyService, :geo do
 
   describe '#execute' do
     context 'with a project on a legacy storage' do
-      let(:project) { create(:project_empty_repo, :legacy_storage) }
+      let(:project) { create(:project_empty_repo, :legacy_storage, :wiki_repo) }
+      let(:repository_disk_path) { "#{project.disk_path}.git" }
+      let(:repository_deleted_disk_path) { "#{project.disk_path}+#{project.id}#{Repositories::ShellDestroyService::DELETED_FLAG}.git" }
+      let(:wiki_disk_path) { "#{project.disk_path}.wiki.git" }
+      let(:wiki_deleted_disk_path) { "#{project.disk_path}.wiki+#{project.id}#{Repositories::ShellDestroyService::DELETED_FLAG}.git" }
 
       subject(:service) { described_class.new(project.id, project.name, project.disk_path, project.repository_storage) }
 
-      it 'delegates project removal to Projects::DestroyService' do
-        expect_any_instance_of(EE::Projects::DestroyService).to receive(:geo_replicate)
+      it 'delegates project removal to Projects::DestroyService#geo_replicate' do
+        expect_next_instance_of(Projects::DestroyService) do |destroy_service|
+          expect(destroy_service).to receive(:geo_replicate).once
+        end
 
         service.execute
       end
 
-      it 'removes the repository from disk' do
-        project.delete
-
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_truthy
+      it 'moves the repository/wiki to a +deleted folder' do
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_disk_path)).to be_truthy
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_deleted_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_disk_path)).to be_truthy
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_deleted_disk_path)).to be_falsey
 
         service.execute
 
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_deleted_disk_path)).to be_truthy
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_deleted_disk_path)).to be_truthy
       end
 
-      it 'cleans up deleted repositories' do
-        project.delete
+      it 'cleans up the repository/wiki +deleted folders', :sidekiq_inline do
+        subject.execute
 
-        expect(::GitlabShellWorker).to receive(:perform_in)
-          .with(5.minutes, :remove_repository, project.repository_storage, "#{project.disk_path}+#{project.id}+deleted")
-          .and_return(true)
-
-        service.execute
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_deleted_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_deleted_disk_path)).to be_falsey
       end
 
       it 'removes the tracking entries' do
@@ -66,34 +73,41 @@ RSpec.describe Geo::RepositoryDestroyService, :geo do
     end
 
     context 'with a project on a hashed storage' do
-      let(:project) { create(:project_empty_repo) }
+      let(:project) { create(:project_empty_repo, :wiki_repo) }
+      let(:repository_disk_path) { "#{project.disk_path}.git" }
+      let(:repository_deleted_disk_path) { "#{project.disk_path}+#{project.id}#{Repositories::ShellDestroyService::DELETED_FLAG}.git" }
+      let(:wiki_disk_path) { "#{project.disk_path}.wiki.git" }
+      let(:wiki_deleted_disk_path) { "#{project.disk_path}.wiki+#{project.id}#{Repositories::ShellDestroyService::DELETED_FLAG}.git" }
 
       subject(:service) { described_class.new(project.id, project.name, project.disk_path, project.repository_storage) }
 
       it 'delegates project removal to Projects::DestroyService' do
-        expect_any_instance_of(EE::Projects::DestroyService).to receive(:geo_replicate)
+        expect_next_instance_of(Projects::DestroyService) do |destroy_service|
+          expect(destroy_service).to receive(:geo_replicate).once
+        end
 
         service.execute
       end
 
-      it 'removes the repository from disk' do
-        project.delete
-
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_truthy
+      it 'moves the repository/wiki to a +deleted folder' do
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_disk_path)).to be_truthy
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_deleted_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_disk_path)).to be_truthy
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_deleted_disk_path)).to be_falsey
 
         service.execute
 
-        expect(project.gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_deleted_disk_path)).to be_truthy
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_deleted_disk_path)).to be_truthy
       end
 
-      it 'cleans up deleted repositories' do
-        project.delete
-
-        expect(::GitlabShellWorker).to receive(:perform_in)
-          .with(5.minutes, :remove_repository, project.repository_storage, "#{project.disk_path}+#{project.id}+deleted")
-          .and_return(true)
+      it 'cleans up the repository/wiki +deleted folders', :sidekiq_inline do
+        subject.execute
 
-        service.execute
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, repository_deleted_disk_path)).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, wiki_deleted_disk_path)).to be_falsey
       end
 
       it 'removes the tracking entries' do
@@ -113,7 +127,9 @@ RSpec.describe Geo::RepositoryDestroyService, :geo do
       subject(:service) { described_class.new(project.id, project.name, project.disk_path, project.repository_storage) }
 
       it 'delegates project removal to Projects::DestroyService' do
-        expect_any_instance_of(EE::Projects::DestroyService).to receive(:geo_replicate)
+        expect_next_instance_of(Projects::DestroyService) do |destroy_service|
+          expect(destroy_service).to receive(:geo_replicate).once
+        end
 
         service.execute
       end
@@ -159,7 +175,9 @@ RSpec.describe Geo::RepositoryDestroyService, :geo do
         subject(:service) { described_class.new(project.id) }
 
         it 'delegates project removal to Projects::DestroyService' do
-          expect_any_instance_of(EE::Projects::DestroyService).to receive(:geo_replicate)
+          expect_next_instance_of(Projects::DestroyService) do |destroy_service|
+            expect(destroy_service).to receive(:geo_replicate).once
+          end
 
           service.execute
         end

spec/services/repositories/destroy_service_spec.rb

@@ -34,6 +34,21 @@ RSpec.describe Repositories::DestroyService do
     project.touch
   end
 
+  context 'on a read-only instance' do
+    before do
+      allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+    end
+
+    it 'schedules the repository deletion' do
+      expect(Repositories::ShellDestroyService).to receive(:new).with(repository).and_call_original
+
+      expect(GitlabShellWorker).to receive(:perform_in)
+        .with(Repositories::ShellDestroyService::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+      subject
+    end
+  end
+
   it 'removes the repository', :sidekiq_inline do
     subject