Add VerificationState to PackageFileRegistry

Via a new VerifiableRegistry concern, to manage verification fields of
registry tables.

ee/app/models/concerns/geo/replicable_registry.rb

@@ -26,14 +26,6 @@ module Geo::ReplicableRegistry
     def registry_consistency_worker_enabled?
       replicator_class.enabled?
     end
-
-    def verification_pending_batch(batch_size:)
-      [] # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/13981
-    end
-
-    def verification_failed_batch(batch_size:)
-      [] # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/13981
-    end
   end
 
   def replicator_class
@@ -105,5 +97,9 @@ module Geo::ReplicableRegistry
 
       super()
     end
+
+    def replicator
+      self.class.replicator_class.new(model_record_id: model_record_id)
+    end
   end
 end

ee/app/models/concerns/geo/verifiable_registry.rb

+# frozen_string_literal: true
+
+module Geo::VerifiableRegistry
+  extend ActiveSupport::Concern
+
+  class_methods do
+    extend ::Gitlab::Utils::Override
+
+    # Overrides a method in `Gitlab::Geo::VerificationState`. This method is
+    # used by `Gitlab::Geo::VerificationState.start_verification_batch` to
+    # produce a query which must return values of the primary key of the
+    # *model*, not of the *registry*. We need this so we can instantiate
+    # Replicators.
+    override :verification_state_model_key
+    def verification_state_model_key
+      self::MODEL_FOREIGN_KEY
+    end
+  end
+
+  included do
+    extend ::Gitlab::Utils::Override
+
+    sha_attribute :verification_checksum_mismatched
+
+    override :clear_verification_failure_fields!
+    def clear_verification_failure_fields!
+      super
+
+      # Note: If the return value of a `before_transition` block is `false`,
+      # then the transition is halted. Anything else, including `nil`, does not
+      # halt the transition.
+      self.checksum_mismatch = false
+      self.verification_checksum_mismatched = nil
+    end
+
+    # Records a checksum mismatch
+    #
+    # @param [String] checksum value which does not match the primary checksum
+    def verification_failed_due_to_mismatch!(checksum, primary_checksum)
+      message = 'Checksum does not match the primary checksum'
+      details = { checksum: checksum, primary_checksum: primary_checksum }
+
+      log_info(message, details)
+
+      self.verification_failure = "#{message} #{details}".truncate(255)
+      self.verification_checksum = checksum
+      self.verification_checksum_mismatched = checksum
+      self.checksum_mismatch = true
+
+      self.verification_failed!
+    end
+
+    private
+
+    override :track_checksum_result!
+    def track_checksum_result!(checksum, calculation_started_at)
+      unless replicator.matches_checksum?(checksum)
+        return verification_failed_due_to_mismatch!(checksum, replicator.primary_checksum)
+      end
+
+      verification_succeeded_with_checksum!(checksum, calculation_started_at)
+    end
+  end
+end

ee/app/models/concerns/geo/verifiable_replicator.rb

@@ -120,7 +120,7 @@ module Geo
       # Also, if another verification job is running, this will make that job
       # set state to pending after it finishes, since the calculated checksum
       # is already invalidated.
-      model_record.verification_started!
+      verification_state_tracker.verification_started!
 
       Geo::VerificationWorker.perform_async(replicable_name, model_record.id)
     end
@@ -128,7 +128,7 @@ module Geo
     # Calculates checksum and asks the model/registry to manage verification
     # state.
     def verify
-      model_record.track_checksum_attempt! do
+      verification_state_tracker.track_checksum_attempt! do
         model_record.calculate_checksum
       end
     end
@@ -158,5 +158,9 @@ module Geo
     def secondary_checksum
       registry.verification_checksum
     end
+
+    def verification_state_tracker
+      Gitlab::Geo.secondary? ? registry : model_record
+    end
   end
 end

ee/app/models/geo/package_file_registry.rb

@@ -2,13 +2,11 @@
 
 class Geo::PackageFileRegistry < Geo::BaseRegistry
   include ::Geo::ReplicableRegistry
-  include ShaAttribute
+  include ::Gitlab::Geo::VerificationState
+  include ::Geo::VerifiableRegistry
 
   MODEL_CLASS = ::Packages::PackageFile
   MODEL_FOREIGN_KEY = :package_file_id
 
   belongs_to :package_file, class_name: 'Packages::PackageFile'
-
-  sha_attribute :verification_checksum
-  sha_attribute :verification_checksum_mismatched
 end

ee/lib/gitlab/geo/verification_state.rb

@@ -59,7 +59,6 @@ module Gitlab
           end
 
           before_transition any => :verification_failed do |instance, _|
-            instance.verification_checksum = nil
             instance.verification_retry_count ||= 0
             instance.verification_retry_count += 1
             instance.verification_retry_at = instance.next_retry_time(instance.verification_retry_count)
@@ -67,10 +66,8 @@ module Gitlab
           end
 
           before_transition any => :verification_succeeded do |instance, _|
-            instance.verification_retry_count = 0
-            instance.verification_retry_at = nil
-            instance.verification_failure = nil
             instance.verified_at = Time.current
+            instance.clear_verification_failure_fields!
           end
 
           event :verification_started do
@@ -138,7 +135,9 @@ module Gitlab
           # This query performs a write, so we need to wrap it in a transaction
           # to stick to the primary database.
           self.transaction do
-            self.connection.execute(query).to_a.map { |row| row[self.primary_key] }
+            self.connection.execute(query).to_a.map do |row|
+              row[self.verification_state_model_key.to_s]
+            end
           end
         end
 
@@ -155,11 +154,16 @@ module Gitlab
             UPDATE #{table_name}
             SET "verification_state" = #{started_enum_value},
               "verification_started_at" = NOW()
-            WHERE #{self.primary_key} IN (#{relation.select(self.primary_key).to_sql})
-            RETURNING #{self.primary_key}
+            WHERE #{self.verification_state_model_key} IN (#{relation.select(self.verification_state_model_key).to_sql})
+            RETURNING #{self.verification_state_model_key}
           SQL
         end
 
+        # Overridden in ReplicableRegistry
+        def verification_state_model_key
+          self.primary_key
+        end
+
         # Fail verification for records which started verification a long time ago
         def fail_verification_timeouts
           attrs = {
@@ -177,6 +181,13 @@ module Gitlab
         end
       end
 
+      # Overridden by ReplicableRegistry
+      def clear_verification_failure_fields!
+        self.verification_retry_count = 0
+        self.verification_retry_at = nil
+        self.verification_failure = nil
+      end
+
       # Provides a safe and easy way to manage the verification state for a
       # synchronous checksum calculation.
       #
@@ -222,6 +233,8 @@ module Gitlab
 
         self.verification_failure = message
         self.verification_failure += ": #{error.message}" if error.respond_to?(:message)
+        self.verification_failure.truncate(255)
+        self.verification_checksum = nil
 
         self.verification_failed!
       end

ee/spec/lib/gitlab/geo/verification_state_spec.rb

@@ -244,6 +244,7 @@ RSpec.describe Gitlab::Geo::VerificationState do
       expect(subject.reload.verification_failed?).to be_truthy
       expect(subject.reload.verification_failure).to eq 'Failure to calculate checksum: An error message'
       expect(subject.verification_retry_count).to be 1
+      expect(subject.verification_checksum).to be_nil
     end
   end
 end

ee/spec/models/geo/package_file_registry_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe Geo::PackageFileRegistry, :geo, type: :model do
   end
 
   include_examples 'a Geo framework registry'
+  include_examples 'a Geo verifiable registry'
 
   describe '.find_registry_differences' do
     let(:synced_group) { create(:group) }

ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb

@@ -327,15 +327,41 @@ RSpec.shared_examples 'a verifiable replicator' do
 
   describe '#verify' do
     it 'wraps the checksum calculation in track_checksum_attempt!' do
-      tracker = double('tracker', calculate_checksum: 'abc123')
-      allow(replicator).to receive(:model_record).and_return(tracker)
+      tracker = double('tracker')
+      allow(replicator).to receive(:verification_state_tracker).and_return(tracker)
 
+      expect(model_record).to receive(:calculate_checksum)
       expect(tracker).to receive(:track_checksum_attempt!).and_yield
 
       replicator.verify
     end
   end
 
+  describe '#verification_state_tracker' do
+    context 'on a Geo primary' do
+      before do
+        stub_primary_node
+      end
+
+      it 'returns model_record' do
+        expect(replicator.verification_state_tracker).to eq(model_record)
+      end
+    end
+
+    context 'on a Geo secondary' do
+      before do
+        stub_secondary_node
+      end
+
+      it 'returns registry' do
+        registry = double('registry')
+        allow(replicator).to receive(:registry).and_return(registry)
+
+        expect(replicator.verification_state_tracker).to eq(registry)
+      end
+    end
+  end
+
   context 'integration tests' do
     before do
       model_record.save!
@@ -362,5 +388,34 @@ RSpec.shared_examples 'a verifiable replicator' do
         end
       end
     end
+
+    context 'on a secondary' do
+      before do
+        stub_secondary_node
+      end
+
+      describe 'background backfill' do
+        it 'verifies registries' do
+          registry = replicator.registry
+          registry.start
+          registry.synced!
+
+          expect do
+            Geo::VerificationBatchWorker.new.perform(replicator.replicable_name)
+          end.to change { registry.reload.verification_succeeded? }.from(false).to(true)
+        end
+      end
+
+      describe 'triggered by events' do
+        it 'verifies registries' do
+          registry = replicator.registry
+          registry.save!
+
+          expect do
+            Geo::VerificationWorker.new.perform(replicator.replicable_name, replicator.model_record_id)
+          end.to change { registry.reload.verification_succeeded? }.from(false).to(true)
+        end
+      end
+    end
   end
 end

ee/spec/support/shared_examples/models/geo_verifiable_registry_shared_examples.rb

+# frozen_string_literal: true
+
+# TODO: Include these examples in 'a Geo framework registry' when *all*
+# registries are verifiable https://gitlab.com/gitlab-org/gitlab/-/issues/280768
+RSpec.shared_examples 'a Geo verifiable registry' do
+  let(:registry_class_factory) { described_class.underscore.tr('/', '_').to_sym }
+
+  subject(:registry_record) { create(registry_class_factory, :synced) }
+
+  describe '#verification_succeeded!', :aggregate_failures do
+    before do
+      subject.verification_started!
+    end
+
+    it 'clears checksum mismatch fields' do
+      subject.update!(checksum_mismatch: true, verification_checksum_mismatched: 'abc123')
+      subject.verification_checksum = 'abc123'
+
+      expect do
+        subject.verification_succeeded!
+      end.to change { subject.verification_succeeded? }.from(false).to(true)
+
+      expect(subject.checksum_mismatch).to eq(false)
+      expect(subject.verification_checksum_mismatched).to eq(nil)
+    end
+  end
+
+  describe '#track_checksum_attempt!', :aggregate_failures do
+    context 'when verification was not yet started' do
+      it 'starts verification' do
+        expect do
+          subject.track_checksum_attempt! do
+            'a_checksum_value'
+          end
+        end.to change { subject.verification_started_at }.from(nil)
+      end
+
+      context 'comparison with primary checksum' do
+        let(:replicator) { double('replicator') }
+        let(:calculated_checksum) { 'abc123' }
+
+        before do
+          allow(subject).to receive(:replicator).and_return(replicator)
+          allow(replicator).to receive(:matches_checksum?).with(calculated_checksum).and_return(matches_checksum)
+        end
+
+        context 'when the calculated checksum matches the primary checksum' do
+          let(:matches_checksum) { true }
+
+          it 'transitions to verification_succeeded and updates the checksum' do
+            expect do
+              subject.track_checksum_attempt! do
+                calculated_checksum
+              end
+            end.to change { subject.verification_succeeded? }.from(false).to(true)
+
+            expect(subject.verification_checksum).to eq(calculated_checksum)
+          end
+        end
+
+        context 'when the calculated checksum does not match the primary checksum' do
+          let(:matches_checksum) { false }
+
+          it 'transitions to verification_failed and updates mismatch fields' do
+            allow(replicator).to receive(:primary_checksum).and_return(calculated_checksum)
+
+            expect do
+              subject.track_checksum_attempt! do
+                calculated_checksum
+              end
+            end.to change { subject.verification_failed? }.from(false).to(true)
+
+            expect(subject.verification_checksum).to eq(calculated_checksum)
+            expect(subject.verification_checksum_mismatched).to eq(calculated_checksum)
+            expect(subject.checksum_mismatch).to eq(true)
+            expect(subject.verification_failure).to match('Checksum does not match the primary checksum')
+          end
+        end
+      end
+    end
+
+    context 'when verification was started' do
+      it 'does not update verification_started_at' do
+        subject.verification_started!
+        expected = subject.verification_started_at
+
+        subject.track_checksum_attempt! do
+          'a_checksum_value'
+        end
+
+        expect(subject.verification_started_at).to be_within(1.second).of(expected)
+      end
+    end
+
+    it 'yields to the checksum calculation' do
+      expect do |probe|
+        subject.track_checksum_attempt!(&probe)
+      end.to yield_with_no_args
+    end
+
+    context 'when an error occurs while yielding' do
+      it 'sets verification_failed' do
+        subject.track_checksum_attempt! do
+          raise 'an error'
+        end
+
+        expect(subject.reload.verification_failed?).to be_truthy
+      end
+    end
+  end
+end