Merge branch '220104-fix-race-in-read-only-repository-logic' into 'master'

Fix a race condition checking whether a project is read-only

See merge request gitlab-org/gitlab!45160

app/models/project.rb

@@ -2090,21 +2090,36 @@ class Project < ApplicationRecord
     (auto_devops || build_auto_devops)&.predefined_variables
   end
 
-  # Tries to set repository as read_only, checking for existing Git transfers in progress beforehand
+  RepositoryReadOnlyError = Class.new(StandardError)
+
+  # Tries to set repository as read_only, checking for existing Git transfers in
+  # progress beforehand. Setting a repository read-only will fail if it is
+  # already in that state.
   #
-  # @return [Boolean] true when set to read_only or false when an existing git transfer is in progress
+  # @return nil. Failures will raise an exception
   def set_repository_read_only!
     with_lock do
-      break false if git_transfer_in_progress?
+      raise RepositoryReadOnlyError, _('Git transfer in progress') if
+        git_transfer_in_progress?
+
+      raise RepositoryReadOnlyError, _('Repository already read-only') if
+        self.class.where(id: id).pick(:repository_read_only)
 
+      raise ActiveRecord::RecordNotSaved, _('Database update failed') unless
         update_column(:repository_read_only, true)
+
+      nil
     end
   end
 
-  # Set repository as writable again
+  # Set repository as writable again. Unlike setting it read-only, this will
+  # succeed if the repository is already writable.
   def set_repository_writable!
     with_lock do
+      raise ActiveRecord::RecordNotSaved, _('Database update failed') unless
         update_column(:repository_read_only, false)
+
+      nil
     end
   end
 

app/models/project_repository_storage_move.rb

@@ -46,8 +46,15 @@ class ProjectRepositoryStorageMove < ApplicationRecord
       transition replicated: :cleanup_failed
     end
 
-    after_transition initial: :scheduled do |storage_move|
-      storage_move.project.update_column(:repository_read_only, true)
+    around_transition initial: :scheduled do |storage_move, block|
+      block.call
+
+      begin
+        storage_move.project.set_repository_read_only!
+      rescue => err
+        errors.add(:project, err.message)
+        next false
+      end
 
       storage_move.run_after_commit do
         ProjectUpdateRepositoryStorageWorker.perform_async(
@@ -56,17 +63,18 @@ class ProjectRepositoryStorageMove < ApplicationRecord
           storage_move.id
         )
       end
+
+      true
     end
 
-    after_transition started: :replicated do |storage_move|
-      storage_move.project.update_columns(
-        repository_read_only: false,
-        repository_storage: storage_move.destination_storage_name
-      )
+    before_transition started: :replicated do |storage_move|
+      storage_move.project.set_repository_writable!
+
+      storage_move.project.update_column(:repository_storage, storage_move.destination_storage_name)
     end
 
-    after_transition started: :failed do |storage_move|
-      storage_move.project.update_column(:repository_read_only, false)
+    before_transition started: :failed do |storage_move|
+      storage_move.project.set_repository_writable!
     end
 
     state :initial, value: 1

app/services/projects/hashed_storage/base_repository_service.rb

@@ -79,14 +79,13 @@ module Projects
       end
 
       def try_to_set_repository_read_only!
-        # Mitigate any push operation to start during migration
-        unless project.set_repository_read_only!
-          migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress"
+        project.set_repository_read_only!
+      rescue Project::RepositoryReadOnlyError => err
+        migration_error = "Target repository '#{old_disk_path}' cannot be made read-only: #{err.message}"
         logger.error migration_error
 
         raise RepositoryInUseError, migration_error
       end
-      end
 
       def wiki_path_suffix
         @wiki_path_suffix ||= Gitlab::GlRepository::WIKI.path_suffix

app/services/projects/hashed_storage/migrate_repository_service.rb

@@ -21,8 +21,10 @@ module Projects
           project.storage_version = nil
         end
 
-        project.repository_read_only = false
+        project.transaction do
           project.save!(validate: false)
+          project.set_repository_writable!
+        end
 
         if result && block_given?
           yield

changelogs/unreleased/220104-fix-race-in-read-only-repository-logic.yml

+---
+title: Fix a race condition checking whether a project is read-only
+merge_request: 45160
+author:
+type: fixed

locale/gitlab.pot

@@ -8411,6 +8411,9 @@ msgstr ""
 msgid "Data is still calculating..."
 msgstr ""
 
+msgid "Database update failed"
+msgstr ""
+
 msgid "Datasource name not found"
 msgstr ""
 
@@ -12249,6 +12252,9 @@ msgstr ""
 msgid "Git strategy for pipelines"
 msgstr ""
 
+msgid "Git transfer in progress"
+msgstr ""
+
 msgid "Git version"
 msgstr ""
 
@@ -22297,6 +22303,9 @@ msgstr ""
 msgid "Repository Settings"
 msgstr ""
 
+msgid "Repository already read-only"
+msgstr ""
+
 msgid "Repository check"
 msgstr ""
 

spec/models/project_spec.rb

@@ -3003,14 +3003,23 @@ RSpec.describe Project do
   describe '#set_repository_read_only!' do
     let(:project) { create(:project) }
 
-    it 'returns true when there is no existing git transfer in progress' do
-      expect(project.set_repository_read_only!).to be_truthy
+    it 'makes the repository read-only' do
+      expect { project.set_repository_read_only! }
+        .to change(project, :repository_read_only?)
+        .from(false)
+        .to(true)
     end
 
-    it 'returns false when there is an existing git transfer in progress' do
+    it 'raises an error if the project is already read-only' do
+      project.set_repository_read_only!
+
+      expect { project.set_repository_read_only! }.to raise_error(described_class::RepositoryReadOnlyError, /already read-only/)
+    end
+
+    it 'raises an error when there is an existing git transfer in progress' do
       allow(project).to receive(:git_transfer_in_progress?) { true }
 
-      expect(project.set_repository_read_only!).to be_falsey
+      expect { project.set_repository_read_only! }.to raise_error(described_class::RepositoryReadOnlyError, /in progress/)
     end
   end