Double-check next value for internal ids.

This is useful for a transition period to migrate away from
`NoninternalAtomicId`. In a situation where both the old and new code to
generate a iid value is run at the same time (for example, during a
deploy different nodes may serve both versions), this will lead to
problems regarding the correct `last_value`. That is, what we track in
`InternalId` may get out of sync with the maximum iid present for
issues.

With this change, we double-check that and correct the `last_value` with
the maximum iid found in issues if necessary.

This is subject to be removed with the 10.8 release and tracked over
here: https://gitlab.com/gitlab-org/gitlab-ce/issues/45389

Closes #45269.

2 files changed, 55 insertions, 6 deletions

diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb
index cbec735c2dd..96a43006642 100644
--- a/app/models/internal_id.rb
+++ b/app/models/internal_id.rb
@@ -23,9 +23,12 @@ class InternalId < ActiveRecord::Base
   #
   # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL).
   # As such, the increment is atomic and safe to be called concurrently.
-  def increment_and_save!
+  #
+  # If a `maximum_iid` is passed in, this overrides the incremented value if it's
+  # greater than that. This can be used to correct the increment value if necessary.
+  def increment_and_save!(maximum_iid)
     lock!
-    self.last_value = (last_value || 0) + 1
+    self.last_value = [(last_value || 0) + 1, (maximum_iid || 0) + 1].max
     save!
     last_value
   end
@@ -89,7 +92,16 @@ class InternalId < ActiveRecord::Base
         # and increment its last value
         #
         # Note this will acquire a ROW SHARE lock on the InternalId record
-        (lookup || create_record).increment_and_save!
+
+        # Note we always calculate the maximum iid present here and
+        # pass it in to correct the InternalId entry if it's last_value is off.
+        #
+        # This can happen in a transition phase where both `AtomicInternalId` and
+        # `NonatomicInternalId` code runs (e.g. during a deploy).
+        #
+        # This is subject to be cleaned up with the 10.8 release:
+        # https://gitlab.com/gitlab-org/gitlab-ce/issues/45389.
+        (lookup || create_record).increment_and_save!(maximum_iid)
       end
     end
 
@@ -115,11 +127,15 @@ class InternalId < ActiveRecord::Base
         InternalId.create!(
           **scope,
           usage: usage_value,
-          last_value: init.call(subject) || 0
+          last_value: maximum_iid
         )
       end
     rescue ActiveRecord::RecordNotUnique
       lookup
     end
+
+    def maximum_iid
+      @maximum_iid ||= init.call(subject) || 0
+    end
   end
 end
diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb
index 581fd0293cc..8ef91e8fab5 100644
--- a/spec/models/internal_id_spec.rb
+++ b/spec/models/internal_id_spec.rb
@@ -5,7 +5,7 @@ describe InternalId do
   let(:usage) { :issues }
   let(:issue) { build(:issue, project: project) }
   let(:scope) { { project: project } }
-  let(:init) { ->(s) { s.project.issues.size } }
+  let(:init) { ->(s) { s.project.issues.maximum(:iid) } }
 
   context 'validations' do
     it { is_expected.to validate_presence_of(:usage) }
@@ -39,6 +39,29 @@ describe InternalId do
         end
       end
 
+      context 'with an InternalId record present and existing issues with a higher internal id' do
+        # This can happen if the old NonatomicInternalId is still in use
+        before do
+          issues = Array.new(rand(1..10)).map { create(:issue, project: project) }
+
+          issue = issues.last
+          issue.iid = issues.map { |i| i.iid }.max + 1
+          issue.save
+        end
+
+        let(:maximum_iid) { project.issues.map { |i| i.iid }.max }
+
+        it 'updates last_value to the maximum internal id present' do
+          subject
+
+          expect(described_class.find_by(project: project, usage: described_class.usages[usage.to_s]).last_value).to eq(maximum_iid + 1)
+        end
+
+        it 'returns next internal id correctly' do
+          expect(subject).to eq(maximum_iid + 1)
+        end
+      end
+
       context 'with concurrent inserts on table' do
         it 'looks up the record if it was created concurrently' do
           args = { **scope, usage: described_class.usages[usage.to_s] }
@@ -81,7 +104,8 @@ describe InternalId do
 
   describe '#increment_and_save!' do
     let(:id) { create(:internal_id) }
-    subject { id.increment_and_save! }
+    let(:maximum_iid) { nil }
+    subject { id.increment_and_save!(maximum_iid) }
 
     it 'returns incremented iid' do
       value = id.last_value
@@ -102,5 +126,14 @@ describe InternalId do
         expect(subject).to eq(1)
       end
     end
+
+    context 'with maximum_iid given' do
+      let(:id) { create(:internal_id, last_value: 1) }
+      let(:maximum_iid) { id.last_value + 10 }
+
+      it 'returns maximum_iid instead' do
+        expect(subject).to eq(12)
+      end
+    end
   end
 end