Merge branch 'sh-improve-error-recovery' into 'master'

Geo: Rename the old directory first when swapping out the recovered directory

Closes #4083

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

app/services/geo/base_sync_service.rb

@@ -8,6 +8,7 @@ module Geo
   class BaseSyncService
     include ExclusiveLeaseGuard
     include ::Gitlab::Geo::ProjectLogHelpers
+    include ::Gitlab::ShellAdapter
     include Delay
 
     class << self
@@ -173,13 +174,17 @@ module Geo
       registry.public_send("last_#{type}_synced_at") # rubocop:disable GitlabSecurity/PublicSend
     end
 
+    def random_disk_path(prefix)
+      random_string = SecureRandom.hex(7)
+      "#{repository.disk_path}_#{prefix}#{random_string}"
+    end
+
     def disk_path_temp
-      unless @disk_path_temp
-        random_string = SecureRandom.hex(7)
-        @disk_path_temp = "#{repository.disk_path}_#{random_string}"
-      end
+      @disk_path_temp ||= random_disk_path('')
+    end
 
-      @disk_path_temp
+    def deleted_disk_path_temp
+      @deleted_path ||= "#{repository.disk_path}+failed-geo-sync"
     end
 
     def build_temporary_repository
@@ -199,16 +204,26 @@ module Geo
         "Setting newly downloaded repository as main",
         storage_path: project.repository_storage_path,
         temp_path: disk_path_temp,
+        deleted_disk_path_temp: deleted_disk_path_temp,
         disk_path: repository.disk_path
       )
 
-      unless gitlab_shell.remove_repository(project.repository_storage_path, repository.disk_path)
-        raise Gitlab::Shell::Error, 'Can not remove outdated main repository to replace it'
+      # Remove the deleted path in case it exists, but it may not be there
+      gitlab_shell.remove_repository(project.repository_storage_path, deleted_disk_path_temp)
+
+      # Move the original repository out of the way
+      unless gitlab_shell.mv_repository(project.repository_storage_path, repository.disk_path, deleted_disk_path_temp)
+        raise Gitlab::Shell::Error, 'Can not move original repository out of the way'
       end
 
       unless gitlab_shell.mv_repository(project.repository_storage_path, disk_path_temp, repository.disk_path)
         raise Gitlab::Shell::Error, 'Can not move temporary repository'
       end
+
+      # Purge the original repository
+      unless gitlab_shell.remove_repository(project.repository_storage_path, deleted_disk_path_temp)
+        raise Gitlab::Shell::Error, 'Can not remove outdated main repository'
+      end
     end
 
     # To prevent the retry time from storing invalid dates in the database,

app/services/geo/repository_sync_service.rb

 module Geo
   class RepositorySyncService < BaseSyncService
-    include Gitlab::ShellAdapter
-
     self.type = :repository
 
     private

lib/gitlab/shell_adapter.rb

@@ -5,7 +5,7 @@
 module Gitlab
   module ShellAdapter
     def gitlab_shell
-      Gitlab::Shell.new
+      @gitlab_shell ||= Gitlab::Shell.new
     end
   end
 end

spec/services/geo/repository_sync_service_spec.rb

@@ -172,9 +172,14 @@ describe Geo::RepositorySyncService do
       it 'tries to redownload repo' do
         create(:geo_project_registry, project: project, repository_retry_count: Geo::BaseSyncService::RETRY_BEFORE_REDOWNLOAD + 1)
 
-        expect_any_instance_of(described_class).to receive(:fetch_project_repository).with(true)
+        expect(subject).to receive(:fetch_project_repository).with(true).and_call_original
+        expect(subject.gitlab_shell).to receive(:mv_repository).exactly(2).times.and_call_original
+        expect(subject.gitlab_shell).to receive(:remove_repository).exactly(3).times.and_call_original
 
         subject.execute
+
+        expect(File.directory?("#{project.repository.path}+failed-geo-sync")).to be false
+        expect(File.directory?(project.repository.path)).to be true
       end
 
       it 'tries to redownload repo when force_redownload flag is set' do