Merge branch 'make_repository_moves_job_idempotent' into 'master'

Make ProjectUpdateRepositoryStorageWorker idempotent

See merge request gitlab-org/gitlab!35483

app/services/projects/update_repository_storage_service.rb

@@ -14,7 +14,11 @@ module Projects
     end
 
     def execute
-      repository_storage_move.start!
+      repository_storage_move.with_lock do
+        return ServiceResponse.success unless repository_storage_move.scheduled? # rubocop:disable Cop/AvoidReturnFromBlocks
+
+        repository_storage_move.start!
+      end
 
       raise SameFilesystemError if same_filesystem?(repository.storage, destination_storage_name)
 

app/workers/all_queues.yml

@@ -1530,7 +1530,7 @@
   :urgency: :low
   :resource_boundary: :unknown
   :weight: 1
-  :idempotent: 
+  :idempotent: true
   :tags: []
 - :name: prometheus_create_default_alerts
   :feature_category: :incident_management

app/workers/project_update_repository_storage_worker.rb

 # frozen_string_literal: true
 
-class ProjectUpdateRepositoryStorageWorker # rubocop:disable Scalability/IdempotentWorker
+class ProjectUpdateRepositoryStorageWorker
   include ApplicationWorker
 
+  idempotent!
   feature_category :gitaly
 
   def perform(project_id, new_repository_storage_key, repository_storage_move_id = nil)

changelogs/unreleased/make_repository_moves_job_idempotent.yml

+---
+title: Make ProjectUpdateRepositoryStorageWorker idempotent
+merge_request: 35483
+author:
+type: fixed

spec/factories/project_repository_storage_moves.rb

@@ -14,5 +14,13 @@ FactoryBot.define do
     trait :started do
       state { ProjectRepositoryStorageMove.state_machines[:state].states[:started].value }
     end
+
+    trait :finished do
+      state { ProjectRepositoryStorageMove.state_machines[:state].states[:finished].value }
+    end
+
+    trait :failed do
+      state { ProjectRepositoryStorageMove.state_machines[:state].states[:failed].value }
+    end
   end
 end

spec/services/projects/fork_service_spec.rb

@@ -327,7 +327,7 @@ RSpec.describe Projects::ForkService do
         destination_storage_name: 'test_second_storage'
       )
       Projects::UpdateRepositoryStorageService.new(storage_move).execute
-      fork_after_move = fork_project(project)
+      fork_after_move = fork_project(project.reload)
       pool_repository_before_move = PoolRepository.joins(:shard)
                                       .find_by(source_project: project, shards: { name: 'default' })
       pool_repository_after_move = PoolRepository.joins(:shard)

spec/services/projects/update_repository_storage_service_spec.rb

@@ -45,6 +45,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
             .and_call_original
 
           result = subject.execute
+          project.reload
 
           expect(result).to be_success
           expect(project).not_to be_repository_read_only
@@ -115,12 +116,37 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
             .and_return(checksum)
 
           result = subject.execute
+          project.reload
 
           expect(result).to be_success
           expect(project.repository_storage).to eq('test_second_storage')
           expect(project.reload_pool_repository).to be_nil
         end
       end
+
+      context 'when the repository move is finished' do
+        let(:repository_storage_move) { create(:project_repository_storage_move, :finished, project: project, destination_storage_name: destination) }
+
+        it 'is idempotent' do
+          expect do
+            result = subject.execute
+
+            expect(result).to be_success
+          end.not_to change(repository_storage_move, :state)
+        end
+      end
+
+      context 'when the repository move is failed' do
+        let(:repository_storage_move) { create(:project_repository_storage_move, :failed, project: project, destination_storage_name: destination) }
+
+        it 'is idempotent' do
+          expect do
+            result = subject.execute
+
+            expect(result).to be_success
+          end.not_to change(repository_storage_move, :state)
+        end
+      end
     end
 
     context 'with wiki repository' do

spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb

@@ -47,6 +47,7 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
       old_repository_path = repository.full_path
 
       result = subject.execute
+      project.reload
 
       expect(result).to be_success
       expect(project).not_to be_repository_read_only

spec/workers/project_update_repository_storage_worker_spec.rb

 # frozen_string_literal: true
 
 require 'spec_helper'
-require 'securerandom'
 
 RSpec.describe ProjectUpdateRepositoryStorageWorker do
   let(:project) { create(:project, :repository) }