Merge branch 'sh-add-cleanup-rpc-gitaly' into 'master'

Automatically cleanup stale worktrees and lock files upon a push

Closes #44115

See merge request gitlab-org/gitlab-ce!18095

Gemfile

@@ -421,7 +421,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.91.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.94.0', require: 'gitaly'
 gem 'grpc', '~> 1.10.0'
 
 # Locked until https://github.com/google/protobuf/issues/4210 is closed

Gemfile.lock

@@ -290,7 +290,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.91.0)
+    gitaly-proto (0.94.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (5.3.3)
@@ -1061,7 +1061,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.91.0)
+  gitaly-proto (~> 0.94.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)

changelogs/unreleased/sh-add-cleanup-rpc-gitaly.yml

+---
+title: Automatically cleanup stale worktrees and lock files upon a push
+merge_request:
+author:
+type: fixed

lib/gitlab/git/repository.rb

@@ -1369,6 +1369,18 @@ module Gitlab
         raise CommandError.new(e)
       end
 
+      def clean_stale_repository_files
+        gitaly_migrate(:repository_cleanup, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
+          gitaly_repository_client.cleanup if is_enabled && exists?
+        end
+      rescue Gitlab::Git::CommandError => e # Don't fail if we can't cleanup
+        Rails.logger.error("Unable to clean repository on storage #{storage} with path #{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_migrate(:branch_names_contains_sha) do |is_enabled|
           if is_enabled
@@ -1463,6 +1475,33 @@ module Gitlab
         run_git!(['rev-list', '--max-count=1', oldrev, "^#{newrev}"])
       end
 
+      def with_worktree(worktree_path, branch, sparse_checkout_files: nil, env:)
+        base_args = %w(worktree add --detach)
+
+        # Note that we _don't_ want to test for `.present?` here: If the caller
+        # passes an non nil empty value it means it still wants sparse checkout
+        # but just isn't interested in any file, perhaps because it wants to
+        # checkout files in by a changeset but that changeset only adds files.
+        if sparse_checkout_files
+          # Create worktree without checking out
+          run_git!(base_args + ['--no-checkout', worktree_path], env: env)
+          worktree_git_path = run_git!(%w(rev-parse --git-dir), chdir: worktree_path).chomp
+
+          configure_sparse_checkout(worktree_git_path, sparse_checkout_files)
+
+          # After sparse checkout configuration, checkout `branch` in worktree
+          run_git!(%W(checkout --detach #{branch}), chdir: worktree_path, env: env)
+        else
+          # Create worktree and checkout `branch` in it
+          run_git!(base_args + [worktree_path, branch], env: env)
+        end
+
+        yield
+      ensure
+        FileUtils.rm_rf(worktree_path) if File.exist?(worktree_path)
+        FileUtils.rm_rf(worktree_git_path) if worktree_git_path && File.exist?(worktree_git_path)
+      end
+
       private
 
       def local_write_ref(ref_path, ref, old_ref: nil, shell: true)
@@ -1549,33 +1588,6 @@ module Gitlab
         File.exist?(path) && !clean_stuck_worktree(path)
       end
 
-      def with_worktree(worktree_path, branch, sparse_checkout_files: nil, env:)
-        base_args = %w(worktree add --detach)
-
-        # Note that we _don't_ want to test for `.present?` here: If the caller
-        # passes an non nil empty value it means it still wants sparse checkout
-        # but just isn't interested in any file, perhaps because it wants to
-        # checkout files in by a changeset but that changeset only adds files.
-        if sparse_checkout_files
-          # Create worktree without checking out
-          run_git!(base_args + ['--no-checkout', worktree_path], env: env)
-          worktree_git_path = run_git!(%w(rev-parse --git-dir), chdir: worktree_path).chomp
-
-          configure_sparse_checkout(worktree_git_path, sparse_checkout_files)
-
-          # After sparse checkout configuration, checkout `branch` in worktree
-          run_git!(%W(checkout --detach #{branch}), chdir: worktree_path, env: env)
-        else
-          # Create worktree and checkout `branch` in it
-          run_git!(base_args + [worktree_path, branch], env: env)
-        end
-
-        yield
-      ensure
-        FileUtils.rm_rf(worktree_path) if File.exist?(worktree_path)
-        FileUtils.rm_rf(worktree_git_path) if worktree_git_path && File.exist?(worktree_git_path)
-      end
-
       def clean_stuck_worktree(path)
         return false unless File.mtime(path) < 15.minutes.ago
 

lib/gitlab/git_access.rb

@@ -238,6 +238,11 @@ module Gitlab
     end
 
     def check_change_access!(changes)
+      # 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
+
       changes_list = Gitlab::ChangesList.new(changes)
 
       # Iterate over all changes to find if user allowed all of them to be applied

lib/gitlab/gitaly_client/repository_service.rb

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

spec/lib/gitlab/git/repository_spec.rb

@@ -2257,6 +2257,39 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
     end
 
+    describe '#clean_stale_repository_files' do
+      let(:worktree_path) { File.join(repository.path, 'worktrees', 'delete-me') }
+
+      it 'cleans up the files' do
+        repository.with_worktree(worktree_path, 'master', env: ENV) do
+          FileUtils.touch(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(worktree_path, 'HEAD'), Gitlab::Git::BLANK_SHA)
+
+          # git 2.16 fails with "fatal: bad object HEAD"
+          expect { repository.rev_list(including: :all) }.to raise_error(Gitlab::Git::Repository::GitError)
+
+          repository.clean_stale_repository_files
+
+          expect { repository.rev_list(including: :all) }.not_to raise_error
+          expect(File.exist?(worktree_path)).to be_falsey
+        end
+      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 '#delete_remote_branches' do
       subject do
         repository.delete_remote_branches('downstream-remote', ['master'])

spec/lib/gitlab/git_access_spec.rb

@@ -855,6 +855,20 @@ describe Gitlab::GitAccess do
                                                             admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
       end
     end
+
+    context 'when pushing to a project' do
+      let(:project) { create(:project, :public, :repository) }
+      let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow" }
+
+      before do
+        project.add_developer(user)
+      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
+    end
   end
 
   describe 'build authentication abilities' do

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -17,6 +17,16 @@ 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)