Merge branch 'bvl-fix-race-condition-creating-signature' into 'master'

Avoid race conditions when creating GpgSignature

Closes #57304

See merge request gitlab-org/gitlab-ce!24939

app/models/application_record.rb

@@ -7,6 +7,12 @@ class ApplicationRecord < ActiveRecord::Base
     where(id: ids)
   end
 
+  def self.safe_find_or_create_by!(*args)
+    safe_find_or_create_by(*args).tap do |record|
+      record.validate! unless record.persisted?
+    end
+  end
+
   def self.safe_find_or_create_by(*args)
     transaction(requires_new: true) do
       find_or_create_by(*args)

app/models/gpg_signature.rb

 # frozen_string_literal: true
 
-class GpgSignature < ActiveRecord::Base
+class GpgSignature < ApplicationRecord
   include ShaAttribute
 
   sha_attribute :commit_sha
@@ -33,6 +33,11 @@ class GpgSignature < ActiveRecord::Base
     )
   end
 
+  def self.safe_create!(attributes)
+    create_with(attributes)
+      .safe_find_or_create_by!(commit_sha: attributes[:commit_sha])
+  end
+
   def gpg_key=(model)
     case model
     when GpgKey

changelogs/unreleased/bvl-fix-race-condition-creating-signature.yml

+---
+title: Avoid race conditions when creating GpgSignature
+merge_request: 24939
+author:
+type: fixed

lib/gitlab/gpg/commit.rb

@@ -88,9 +88,10 @@ module Gitlab
 
       def create_cached_signature!
         using_keychain do |gpg_key|
-          signature = GpgSignature.new(attributes(gpg_key))
-          signature.save! unless Gitlab::Database.read_only?
-          signature
+          attributes = attributes(gpg_key)
+          break GpgSignature.new(attributes) if Gitlab::Database.read_only?
+
+          GpgSignature.safe_create!(attributes)
         end
       end
 

spec/models/application_record_spec.rb

@@ -11,7 +11,7 @@ describe ApplicationRecord do
     end
   end
 
-  describe '#safe_find_or_create_by' do
+  describe '.safe_find_or_create_by' do
     it 'creates the user avoiding race conditions' do
       expect(Suggestion).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique)
       allow(Suggestion).to receive(:find_or_create_by).and_call_original
@@ -20,4 +20,17 @@ describe ApplicationRecord do
         .to change { Suggestion.count }.by(1)
     end
   end
+
+  describe '.safe_find_or_create_by!' do
+    it 'creates a record using safe_find_or_create_by' do
+      expect(Suggestion).to receive(:find_or_create_by).and_call_original
+
+      expect(Suggestion.safe_find_or_create_by!(build(:suggestion).attributes))
+        .to be_a(Suggestion)
+    end
+
+    it 'raises a validation error if the record was not persisted' do
+      expect { Suggestion.find_or_create_by!(note: nil) }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+  end
 end

spec/models/gpg_signature_spec.rb

@@ -23,6 +23,41 @@ RSpec.describe GpgSignature do
     it { is_expected.to validate_presence_of(:gpg_key_primary_keyid) }
   end
 
+  describe '.safe_create!' do
+    let(:attributes) do
+      {
+        commit_sha: commit_sha,
+        project: project,
+        gpg_key_primary_keyid: gpg_key.keyid
+      }
+    end
+
+    it 'finds a signature by commit sha if it existed' do
+      gpg_signature
+
+      expect(described_class.safe_create!(commit_sha: commit_sha)).to eq(gpg_signature)
+    end
+
+    it 'creates a new signature if it was not found' do
+      expect { described_class.safe_create!(attributes) }.to change { described_class.count }.by(1)
+    end
+
+    it 'assigns the correct attributes when creating' do
+      signature = described_class.safe_create!(attributes)
+
+      expect(signature.project).to eq(project)
+      expect(signature.commit_sha).to eq(commit_sha)
+      expect(signature.gpg_key_primary_keyid).to eq(gpg_key.keyid)
+    end
+
+    it 'does not raise an error in case of a race condition' do
+      expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique)
+      allow(described_class).to receive(:find_or_create_by).and_call_original
+
+      described_class.safe_create!(attributes)
+    end
+  end
+
   describe '#commit' do
     it 'fetches the commit through the project' do
       expect_any_instance_of(Project).to receive(:commit).with(commit_sha).and_return(commit)