Merge branch '5750-geo-gitlab-git-commanderror-exit-status-128' into 'master'

Resolve "Geo: Gitlab::Git::CommandError (13:exit status 128) when calculating checksums"

Closes #5750

See merge request gitlab-org/gitlab-ee!5486

GITALY_SERVER_VERSION

-0.98.0
+0.99.0

ee/app/services/geo/repository_verify_secondary_service.rb

@@ -53,7 +53,7 @@ module Geo
 
     def calculate_checksum
       repository.checksum
-    rescue Gitlab::Git::Repository::NoRepository
+    rescue Gitlab::Git::Repository::NoRepository, Gitlab::Git::Repository::InvalidRepository
       Gitlab::Git::Repository::EMPTY_REPOSITORY_CHECKSUM
     end
 

ee/app/workers/geo/repository_verification/primary/single_worker.rb

@@ -35,7 +35,7 @@ module Geo
 
         def calculate_checksum(type, repository)
           update_repository_state!(type, checksum: repository.checksum)
-        rescue Gitlab::Git::Repository::NoRepository
+        rescue Gitlab::Git::Repository::NoRepository, Gitlab::Git::Repository::InvalidRepository
           update_repository_state!(type, checksum: Gitlab::Git::Repository::EMPTY_REPOSITORY_CHECKSUM)
         rescue => e
           log_error('Error calculating the repository checksum', e, type: type)

ee/changelogs/unreleased/5750-geo-gitlab-git-commanderror-exit-status-128.yml

+---
+title: Geo - Returns a dummy checksum when there is no valid repository on disk
+merge_request: 5486
+author:
+type: fixed

ee/spec/workers/geo/repository_verification/primary/single_worker_spec.rb

@@ -3,10 +3,11 @@ require 'spec_helper'
 describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean_gitlab_redis_cache do
   include ::EE::GeoHelpers
 
-  set(:project_with_repositories) { create(:project, :repository, :wiki_repo) }
-  set(:project_without_repositories) { create(:project) }
+  set(:project) { create(:project, :repository, :wiki_repo) }
+  set(:project_empty_repo) { create(:project) }
 
   let!(:primary) { create(:geo_node, :primary) }
+  let(:repository) { double }
 
   before do
     stub_current_geo_node(primary)
@@ -20,17 +21,17 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'does not calculate the checksum when not running on a primary' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect(project_without_repositories.repository).not_to receive(:checksum)
+      expect(project_empty_repo.repository).not_to receive(:checksum)
 
-      subject.perform(project_without_repositories.id)
+      subject.perform(project_empty_repo.id)
     end
 
     it 'does not calculate the checksum when project is pending deletion' do
-      project_with_repositories.update!(pending_delete: true)
+      project.update!(pending_delete: true)
 
-      expect(project_with_repositories.repository).not_to receive(:checksum)
+      expect(project.repository).not_to receive(:checksum)
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
     end
 
     it 'does not raise an error when project could not be found' do
@@ -38,9 +39,9 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     end
 
     it 'calculates the checksum for unverified projects' do
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
-      expect(project_with_repositories.repository_state).to have_attributes(
+      expect(project.repository_state).to have_attributes(
         repository_verification_checksum: instance_of(String),
         last_repository_verification_failure: nil,
         wiki_verification_checksum: instance_of(String),
@@ -51,11 +52,11 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'calculates the checksum for outdated projects' do
       repository_state =
         create(:repository_state,
-          project: project_with_repositories,
+          project: project,
           repository_verification_checksum: nil,
           wiki_verification_checksum: nil)
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
       repository_state.reload
 
@@ -66,11 +67,11 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'calculates the checksum for outdated repositories' do
       repository_state =
         create(:repository_state,
-          project: project_with_repositories,
+          project: project,
           repository_verification_checksum: nil,
           wiki_verification_checksum: 'e123')
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
       repository_state.reload
 
@@ -81,11 +82,11 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'calculates the checksum for outdated wikis' do
       repository_state =
         create(:repository_state,
-          project: project_with_repositories,
+          project: project,
           repository_verification_checksum: 'f123',
           wiki_verification_checksum: nil)
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
       repository_state.reload
 
@@ -96,11 +97,11 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'does not recalculate the checksum for projects up to date' do
       repository_state =
         create(:repository_state,
-          project: project_with_repositories,
+          project: project,
           repository_verification_checksum: 'f123',
           wiki_verification_checksum: 'e123')
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
       expect(repository_state.reload).to have_attributes(
         repository_verification_checksum: 'f123',
@@ -109,11 +110,11 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     end
 
     it 'does not calculate the wiki checksum when wiki is not enabled for project' do
-      project_with_repositories.update!(wiki_enabled: false)
+      project.update!(wiki_enabled: false)
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
-      expect(project_with_repositories.repository_state).to have_attributes(
+      expect(project.repository_state).to have_attributes(
         repository_verification_checksum: instance_of(String),
         last_repository_verification_failure: nil,
         wiki_verification_checksum: nil,
@@ -122,9 +123,9 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     end
 
     it 'does not mark the calculating as failed when there is no repo' do
-      subject.perform(project_without_repositories.id)
+      subject.perform(project_empty_repo.id)
 
-      expect(project_without_repositories.repository_state).to have_attributes(
+      expect(project_empty_repo.repository_state).to have_attributes(
         repository_verification_checksum: '0000000000000000000000000000000000000000',
         last_repository_verification_failure: nil,
         wiki_verification_checksum: '0000000000000000000000000000000000000000',
@@ -132,27 +133,38 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
       )
     end
 
-    it 'keeps track of failures when calculating the repository checksum' do
-      repository = double
+    it 'does not mark the calculating as failed for non-valid repo' do
+      project_broken_repo = create(:project, :broken_repo)
+
+      subject.perform(project_broken_repo.id)
+
+      expect(project_broken_repo.repository_state).to have_attributes(
+        repository_verification_checksum: '0000000000000000000000000000000000000000',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: '0000000000000000000000000000000000000000',
+        last_wiki_verification_failure: nil
+      )
+    end
 
+    it 'keeps track of failures when calculating the repository checksum' do
       allow(Repository).to receive(:new).with(
-        project_with_repositories.full_path,
-        project_with_repositories,
-        disk_path: project_with_repositories.disk_path
+        project.full_path,
+        project,
+        disk_path: project.disk_path
       ).and_return(repository)
 
       allow(Repository).to receive(:new).with(
-        project_with_repositories.wiki.full_path,
-        project_with_repositories,
-        disk_path: project_with_repositories.wiki.disk_path,
+        project.wiki.full_path,
+        project,
+        disk_path: project.wiki.disk_path,
         is_wiki: true
       ).and_return(repository)
 
       allow(repository).to receive(:checksum).twice.and_raise('Something went wrong')
 
-      subject.perform(project_with_repositories.id)
+      subject.perform(project.id)
 
-      expect(project_with_repositories.repository_state).to have_attributes(
+      expect(project.repository_state).to have_attributes(
         repository_verification_checksum: nil,
         last_repository_verification_failure: 'Something went wrong',
         wiki_verification_checksum: nil,

lib/gitlab/git/repository.rb

@@ -30,6 +30,7 @@ module Gitlab
       EMPTY_REPOSITORY_CHECKSUM = '0000000000000000000000000000000000000000'.freeze
 
       NoRepository = Class.new(StandardError)
+      InvalidRepository = Class.new(StandardError)
       InvalidBlobName = Class.new(StandardError)
       InvalidRef = Class.new(StandardError)
       GitError = Class.new(StandardError)
@@ -1447,6 +1448,8 @@ module Gitlab
         raise NoRepository.new(e)
       rescue GRPC::InvalidArgument => e
         raise ArgumentError.new(e)
+      rescue GRPC::DataLoss => e
+        raise InvalidRepository.new(e)
       rescue GRPC::BadStatus => e
         raise CommandError.new(e)
       end
@@ -2555,10 +2558,12 @@ module Gitlab
         output, status = run_git(args)
 
         if status.nil? || !status.zero?
-          # Empty repositories return with a non-zero status and an empty output.
-          return EMPTY_REPOSITORY_CHECKSUM if output&.empty?
+          # Non-valid git repositories return 128 as the status code and an error output
+          raise InvalidRepository if status == 128 && output.to_s.downcase =~ /not a git repository/
+          # Empty repositories returns with a non-zero status and an empty output.
+          raise ChecksumError, output unless output.blank?
 
-          raise ChecksumError, output
+          return EMPTY_REPOSITORY_CHECKSUM
         end
 
         refs = output.split("\n")

spec/lib/gitlab/git/repository_spec.rb

@@ -2275,7 +2275,22 @@ describe Gitlab::Git::Repository, seed_helper: true do
         expect(empty_repo.checksum).to eq '0000000000000000000000000000000000000000'
       end
 
-      it 'raises a no repository exception when there is no repo' do
+      it 'raises Gitlab::Git::Repository::InvalidRepository error for non-valid git repo' do
+        FileUtils.rm_rf(File.join(storage_path, 'non-valid.git'))
+
+        system(git_env, *%W(#{Gitlab.config.git.bin_path} clone --bare #{TEST_REPO_PATH} non-valid.git),
+               chdir: SEED_STORAGE_PATH,
+               out: '/dev/null',
+               err: '/dev/null')
+
+        File.truncate(File.join(storage_path, 'non-valid.git/HEAD'), 0)
+
+        non_valid = described_class.new('default', 'non-valid.git', '')
+
+        expect { non_valid.checksum }.to raise_error(Gitlab::Git::Repository::InvalidRepository)
+      end
+
+      it 'raises Gitlab::Git::Repository::NoRepository error when there is no repo' do
         broken_repo = described_class.new('default', 'a/path.git', '')
 
         expect { broken_repo.checksum }.to raise_error(Gitlab::Git::Repository::NoRepository)