Do not attempt to fetch following a successful snapshot

ee/app/services/geo/base_sync_service.rb

@@ -55,26 +55,11 @@ module Geo
     def fetch_repository(redownload)
       log_info("Trying to fetch #{type}")
       clean_up_temporary_repository
+
       update_registry!(started_at: DateTime.now)
 
       if redownload
-        log_info("Redownloading #{type}")
-        temp_repo = build_temporary_repository
-
-        if Feature.enabled?(:geo_redownload_with_snapshot)
-          begin
-            fetch_snapshot(temp_repo)
-          rescue => err
-            log_error('Snapshot attempt failed', err)
-          end
-        end
-
-        # A git fetch should be attempted, regardless of whether a snapshot was
-        # performed. A git fetch *may* succeed where a snapshot has failed. If
-        # the snapshot succeeded, it may not be in a consistent state - the git
-        # git fetch may repair it.
-        fetch_geo_mirror(temp_repo)
-
+        redownload_repository
         set_temp_repository_as_main
       else
         ensure_repository
@@ -82,6 +67,21 @@ module Geo
       end
     end
 
+    def redownload_repository
+      log_info("Redownloading #{type}")
+
+      return if fetch_snapshot
+
+      log_info("Attempting to fetch repository via git")
+
+      # `git fetch` needs an empty bare repository to fetch into
+      unless gitlab_shell.create_repository(project.repository_storage, disk_path_temp)
+        raise Gitlab::Shell::Error, 'Can not create a temporary repository'
+      end
+
+      fetch_geo_mirror(temp_repo)
+    end
+
     def retry_count
       registry.public_send("#{type}_retry_count") || -1 # rubocop:disable GitlabSecurity/PublicSend
     end
@@ -114,11 +114,24 @@ module Geo
       end
     end
 
-    def fetch_snapshot(repository)
-      repository.create_from_snapshot(
-        ::Gitlab::Geo.primary_node.snapshot_url(repository),
+    # Use snapshotting for redownloads *only* when enabled.
+    #
+    # If writes happen to the repository while snapshotting, it may be
+    # returned in an inconsistent state. However, a subsequent git fetch
+    # will be enqueued by the log cursor, which should resolve any problems
+    # it is possible to fix.
+    def fetch_snapshot
+      return unless Feature.enabled?(:geo_redownload_with_snapshot)
+
+      log_info("Attempting to fetch repository via snapshot")
+
+      temp_repo.create_from_snapshot(
+        ::Gitlab::Geo.primary_node.snapshot_url(temp_repo),
         ::Gitlab::Geo::RepoSyncRequest.new.authorization
       )
+    rescue => err
+      log_error('Snapshot attempt failed', err)
+      false
     end
 
     def registry
@@ -193,21 +206,15 @@ module Geo
       @deleted_path ||= "@failed-geo-sync/#{repository.disk_path}"
     end
 
-    def build_temporary_repository
-      unless gitlab_shell.create_repository(project.repository_storage, disk_path_temp)
-        raise Gitlab::Shell::Error, 'Can not create a temporary repository'
-      end
-
-      log_info("Created temporary repository")
-
-      ::Repository.new(repository.full_path, repository.project, disk_path: disk_path_temp, is_wiki: repository.is_wiki)
+    def temp_repo
+      @temp_repo ||= ::Repository.new(repository.full_path, repository.project, disk_path: disk_path_temp, is_wiki: repository.is_wiki)
     end
 
     def clean_up_temporary_repository
       exists = gitlab_shell.exists?(project.repository_storage_path, disk_path_temp)
 
       if exists && !gitlab_shell.remove_repository(project.repository_storage_path, disk_path_temp)
-        raise Gitlab::Shell::Error, "Temporary #{type} can not been removed"
+        raise Gitlab::Shell::Error, "Temporary #{type} can not be removed"
       end
     end
 
@@ -240,7 +247,7 @@ module Geo
       ensure_repository_namespace(repository.disk_path)
 
       unless gitlab_shell.mv_repository(project.repository_storage_path, disk_path_temp, repository.disk_path)
-        raise Gitlab::Shell::Error, 'Can not move temporary repository'
+        raise Gitlab::Shell::Error, 'Can not move temporary repository to canonical location'
       end
 
       # Purge the original repository

ee/spec/support/services/base_sync_service_shared_examples.rb

@@ -53,11 +53,11 @@ shared_examples 'sync retries use the snapshot RPC' do
 
   context 'snapshot synchronization method' do
     before do
-      allow(subject).to receive(:build_temporary_repository) { repository }
+      allow(subject).to receive(:temp_repo) { repository }
     end
 
     def receive_create_from_snapshot
-      receive(:create_from_snapshot).with(primary.snapshot_url(repository), match(/^GL-Geo/))
+      receive(:create_from_snapshot).with(primary.snapshot_url(repository), match(/^GL-Geo/)) { Gitaly::CreateRepositoryFromSnapshotResponse.new }
     end
 
     it 'does not attempt to snapshot for initial sync' do
@@ -79,9 +79,9 @@ shared_examples 'sync retries use the snapshot RPC' do
     context 'registry is ready to be snapshotted' do
       let!(:registry) { create(:geo_project_registry, project: project, repository_retry_count: retry_count + 1, wiki_retry_count: retry_count + 1) }
 
-      it 'attempts to snapshot + fetch' do
+      it 'attempts to snapshot' do
         expect(repository).to receive_create_from_snapshot
-        expect(subject).to receive(:fetch_geo_mirror).with(repository)
+        expect(subject).not_to receive(:fetch_geo_mirror).with(repository)
 
         subject.execute
       end