git: Stop calling Gitaly's Cleanup RPC

Historically, Gitaly easily ended up in states where the repository is
in a state such that many different Git commands failed. This is why
Gitaly provides a Cleanup RPC that cleans up any such data structures
which are known to cause problems, and we call it in many different
places.

There had been two main usecases for this:

    - Git is known to leave behind corrupted references in case of a
      hard shutdown because it doesn't fsync(3p) data to disk before
      renaming loose references into place. Such broken references may
      cause Git commands to fail which try to read them, and as such
      Gitaly used to perform housekeeping tasks where it walked through
      the repository to clean up any such corrupt references. Gitaly had
      to eventually remove this functionality from Cleanup though: in
      repositories hosted on NFS this was causing significant latency
      and thus caused timeouts. We thus don't do this anymore since
      7a1d224d0 (repository: Remove housekeeping from Cleanup RPC,
      2021-05-17).

    - Many of Gitaly's RPCs used to create worktrees to perform various
      operations. In some cases worktrees were pruned, but left behind
      broken references which as a result again caused corrupted
      references. The Cleanup RPC thus prunes any such per-worktree
      references and cleans up pointers to already-removed worktrees.
      Gitaly has since converted all RPCs except UserApplyPatch to not
      use worktrees anymore, and this remaining RPC is effectively
      unused: this week we had two calls of it across all of gitlab.com.

It is thus safe to say that the Cleanup RPC is effectively useless and
doesn't do anything. It doesn't clean up corrupted references, and
because we don't create worktrees we wouldn't ever need to clean them up
either. Furthermore, both of those actions are performed by other RPCs,
namely GarbageCollect and OptimizeRepository. The former one is
frequently executed by Rails, while the latter one is automatically
executed during our daily maintenance window on all repositories.

Let's thus stop calling Cleanup altogether such that its RPC can be
properly deprecated and removed in Gitaly.

Changelog: removed

ee/app/services/geo/design_repository_sync_service.rb

@@ -44,8 +44,6 @@ module Geo
       log_error(message, error)
 
       registry.fail_sync!(message, error, attrs)
-
-      repository.clean_stale_repository_files
     end
 
     def start_registry_sync!

ee/app/services/geo/framework_repository_sync_service.rb

@@ -168,8 +168,6 @@ module Geo
       registry = replicator.registry
       registry.force_to_redownload = force_to_redownload
       registry.failed!(message: message, error: error)
-
-      repository.clean_stale_repository_files
     end
 
     def download_time_in_seconds

ee/app/services/geo/repository_base_sync_service.rb

@@ -181,8 +181,6 @@ module Geo
       log_error(message, error)
 
       registry.fail_sync!(type, message, error, attrs)
-
-      repository.clean_stale_repository_files
     end
 
     def type

ee/spec/services/geo/framework_repository_sync_service_spec.rb

@@ -226,12 +226,6 @@ RSpec.describe Geo::FrameworkRepositorySyncService, :geo do
                                               last_sync_failure: 'Error syncing repository: shell error'
                                              )
         end
-
-        it 'calls repository cleanup' do
-          expect(repository).to receive(:clean_stale_repository_files)
-
-          subject.execute
-        end
       end
     end
 

ee/spec/services/geo/repository_sync_service_spec.rb

@@ -285,12 +285,6 @@ RSpec.describe Geo::RepositorySyncService, :geo do
                                               last_repository_sync_failure: 'Error syncing repository: shell error'
                                              )
         end
-
-        it 'calls repository cleanup' do
-          expect(repository).to receive(:clean_stale_repository_files)
-
-          subject.execute
-        end
       end
     end
 

ee/spec/services/geo/wiki_sync_service_spec.rb

@@ -224,12 +224,6 @@ RSpec.describe Geo::WikiSyncService, :geo do
                                               last_wiki_sync_failure: 'Error syncing wiki repository: shell error'
                                              )
         end
-
-        it 'calls repository cleanup' do
-          expect(repository).to receive(:clean_stale_repository_files)
-
-          subject.execute
-        end
       end
 
       context 'no Wiki repository' do

lib/gitlab/git/repository.rb

@@ -972,18 +972,6 @@ module Gitlab
         @praefect_info_client ||= Gitlab::GitalyClient::PraefectInfoService.new(self)
       end
 
-      def clean_stale_repository_files
-        wrapped_gitaly_errors do
-          gitaly_repository_client.cleanup if exists?
-        end
-      rescue Gitlab::Git::CommandError => e # Don't fail if we can't cleanup
-        Gitlab::AppLogger.error("Unable to clean repository on storage #{storage} with relative path #{relative_path}: #{e.message}")
-        Gitlab::Metrics.counter(
-          :failed_repository_cleanup_total,
-          'Number of failed repository cleanup events'
-        ).increment
-      end
-
       def branch_names_contains_sha(sha)
         gitaly_ref_client.branch_names_contains_sha(sha)
       end

lib/gitlab/git_access.rb

@@ -328,11 +328,6 @@ module Gitlab
           raise ForbiddenError, error_message(:push_code)
         end
       else
-        # If there are worktrees with a HEAD pointing to a non-existent object,
-        # calls to `git rev-list --all` will fail in git 2.15+. This should also
-        # clear stale lock files.
-        project.repository.clean_stale_repository_files if project.present?
-
         check_access!
       end
     end
@@ -467,11 +462,6 @@ module Gitlab
     def check_push_size!
       return unless check_size_limit?
 
-      # If there are worktrees with a HEAD pointing to a non-existent object,
-      # calls to `git rev-list --all` will fail in git 2.15+. This should also
-      # clear stale lock files.
-      repository.clean_stale_repository_files
-
       # Use #check_repository_disk_size to get correct push size whenever a lot of changes
       # gets pushed at the same time containing the same blobs. This is only
       # doable if GIT_OBJECT_DIRECTORY_RELATIVE env var is set and happens

lib/gitlab/gitaly_client/repository_service.rb

@@ -21,11 +21,6 @@ module Gitlab
         response.exists
       end
 
-      def cleanup
-        request = Gitaly::CleanupRequest.new(repository: @gitaly_repo)
-        GitalyClient.call(@storage, :repository_service, :cleanup, request, timeout: GitalyClient.fast_timeout)
-      end
-
       def garbage_collect(create_bitmap, prune:)
         request = Gitaly::GarbageCollectRequest.new(repository: @gitaly_repo, create_bitmap: create_bitmap, prune: prune)
         GitalyClient.call(@storage, :repository_service, :garbage_collect, request, timeout: GitalyClient.long_timeout)

spec/lib/gitlab/git/repository_spec.rb

@@ -2252,44 +2252,6 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
-  describe '#clean_stale_repository_files' do
-    let(:worktree_id) { 'rebase-1' }
-    let(:gitlab_worktree_path) { File.join(repository_path, 'gitlab-worktree', worktree_id) }
-    let(:admin_dir) { File.join(repository_path, 'worktrees') }
-
-    it 'cleans up the files' do
-      create_worktree = %W[git -C #{repository_path} worktree add --detach #{gitlab_worktree_path} master]
-      raise 'preparation failed' unless system(*create_worktree, err: '/dev/null')
-
-      FileUtils.touch(gitlab_worktree_path, mtime: Time.now - 8.hours)
-      # git rev-list --all will fail in git 2.16 if HEAD is pointing to a non-existent object,
-      # but the HEAD must be 40 characters long or git will ignore it.
-      File.write(File.join(admin_dir, worktree_id, 'HEAD'), Gitlab::Git::BLANK_SHA)
-
-      expect(rev_list_all).to be(false)
-      repository.clean_stale_repository_files
-
-      expect(rev_list_all).to be(true)
-      expect(File.exist?(gitlab_worktree_path)).to be_falsey
-    end
-
-    def rev_list_all
-      system(*%W[git -C #{repository_path} rev-list --all], out: '/dev/null', err: '/dev/null')
-    end
-
-    it 'increments a counter upon an error' do
-      expect(repository.gitaly_repository_client).to receive(:cleanup).and_raise(Gitlab::Git::CommandError)
-
-      counter = double(:counter)
-
-      expect(counter).to receive(:increment)
-      expect(Gitlab::Metrics).to receive(:counter).with(:failed_repository_cleanup_total,
-                                                        'Number of failed repository cleanup events').and_return(counter)
-
-      repository.clean_stale_repository_files
-    end
-  end
-
   describe '#squash' do
     let(:branch_name) { 'fix' }
     let(:start_sha) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }

spec/lib/gitlab/git_access_spec.rb

@@ -1008,11 +1008,6 @@ RSpec.describe Gitlab::GitAccess do
         end
       end
 
-      it 'cleans up the files' do
-        expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
-        expect { push_access_check }.not_to raise_error
-      end
-
       it 'avoids N+1 queries', :request_store do
         # Run this once to establish a baseline. Cached queries should get
         # cached, so that when we introduce another change we shouldn't see

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -21,16 +21,6 @@ RSpec.describe Gitlab::GitalyClient::RepositoryService do
     end
   end
 
-  describe '#cleanup' do
-    it 'sends a cleanup message' do
-      expect_any_instance_of(Gitaly::RepositoryService::Stub)
-        .to receive(:cleanup)
-        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
-
-      client.cleanup
-    end
-  end
-
   describe '#garbage_collect' do
     it 'sends a garbage_collect message' do
       expect_any_instance_of(Gitaly::RepositoryService::Stub)