Change mechanism for classes to handle failure

Since classes that include RelativePositioning no longer can override
low-level implementation methods to hook into internal details, this
means that catching query timeouts and other failures systematically is
no longer straightforward.

This change allows classes to implement a new callback hook:
`#could_not_move`, which is called with an exception if any of the
movement methods times out.

app/models/concerns/relative_positioning.rb

@@ -488,7 +488,7 @@ module RelativePositioning
         max_relative_position = terminus.relative_position
         [[(MAX_POSITION - max_relative_position) / gaps, IDEAL_DISTANCE].min, max_relative_position]
       else
-        terminus = min_sibling
+        terminus = context.min_sibling
         terminus.shift_right
         min_relative_position = terminus.relative_position
         [[(min_relative_position - MIN_POSITION) / gaps, IDEAL_DISTANCE].min, min_relative_position]
@@ -574,26 +574,43 @@ module RelativePositioning
     before, after = [before, after].sort_by(&:relative_position) if before && after
 
     RelativePositioning.mover.move(self, before, after)
+  rescue ActiveRecord::QueryCanceled, NoSpaceLeft => e
+    could_not_move(e)
+    raise e
   end
 
   def move_after(before = self)
     RelativePositioning.mover.move(self, before, nil)
+  rescue ActiveRecord::QueryCanceled, NoSpaceLeft => e
+    could_not_move(e)
+    raise e
   end
 
   def move_before(after = self)
     RelativePositioning.mover.move(self, nil, after)
+  rescue ActiveRecord::QueryCanceled, NoSpaceLeft => e
+    could_not_move(e)
+    raise e
   end
 
   def move_to_end
     RelativePositioning.mover.move_to_end(self)
-  rescue NoSpaceLeft
+  rescue NoSpaceLeft => e
+    could_not_move(e)
     self.relative_position = MAX_POSITION
+  rescue ActiveRecord::QueryCanceled => e
+    could_not_move(e)
+    raise e
   end
 
   def move_to_start
     RelativePositioning.mover.move_to_start(self)
-  rescue NoSpaceLeft
+  rescue NoSpaceLeft => e
+    could_not_move(e)
     self.relative_position = MIN_POSITION
+  rescue ActiveRecord::QueryCanceled => e
+    could_not_move(e)
+    raise e
   end
 
   # This method is used during rebalancing - override it to customise the update
@@ -609,4 +626,8 @@ module RelativePositioning
   def exclude_self(relation, excluded: self)
     relation.id_not_in(excluded.id)
   end
+
+  # Override if you want to be notified of failures to move
+  def could_not_move(exception)
+  end
 end

app/models/issue.rb

@@ -444,20 +444,9 @@ class Issue < ApplicationRecord
     Gitlab::EtagCaching::Store.new.touch(key)
   end
 
-  def find_next_gap_before
-    super
-  rescue ActiveRecord::QueryCanceled => e
-    # Symptom of running out of space - schedule rebalancing
-    IssueRebalancingWorker.perform_async(nil, project_id)
-    raise e
-  end
-
-  def find_next_gap_after
-    super
-  rescue ActiveRecord::QueryCanceled => e
+  def could_not_move(exception)
     # Symptom of running out of space - schedule rebalancing
     IssueRebalancingWorker.perform_async(nil, project_id)
-    raise e
   end
 end
 

spec/models/issue_spec.rb

@@ -1187,14 +1187,15 @@ RSpec.describe Issue do
 
   describe 'scheduling rebalancing' do
     before do
-      allow(issue).to receive(:find_next_gap) { raise ActiveRecord::QueryCanceled }
+      allow_next_instance_of(RelativePositioning::Mover) do |mover|
+        allow(mover).to receive(:move) { raise ActiveRecord::QueryCanceled }
+      end
     end
 
-    let(:project) { build(:project_empty_repo) }
+    let(:project) { build_stubbed(:project_empty_repo) }
     let(:issue) { build_stubbed(:issue, relative_position: 100, project: project) }
 
-    describe '#find_next_gap_before' do
-      it 'schedules rebalancing if we time-out when finding a gap' do
+    it 'schedules rebalancing if we time-out when moving' do
       lhs = build_stubbed(:issue, relative_position: 99, project: project)
       to_move = build(:issue, project: project)
       expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
@@ -1203,16 +1204,6 @@ RSpec.describe Issue do
     end
   end
 
-    describe '#find_next_gap_after' do
-      it 'schedules rebalancing if we time-out when finding a gap' do
-        allow(issue).to receive(:find_next_gap) { raise ActiveRecord::QueryCanceled }
-        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
-
-        expect { issue.move_sequence_after }.to raise_error(ActiveRecord::QueryCanceled)
-      end
-    end
-  end
-
   describe '#allows_reviewers?' do
     it 'returns false as issues do not support reviewers feature' do
       stub_feature_flags(merge_request_reviewers: true)

spec/support/shared_examples/models/relative_positioning_shared_examples.rb

@@ -5,6 +5,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do
   let(:item2) { create_item }
   let(:new_item) { create_item }
 
+  let(:set_size) { RelativePositioning.mover.context(item1).scoped_items.count }
+
   def create_item(params = {})
     create(factory, params.merge(default_params))
   end
@@ -383,7 +385,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     end
 
     it 'places items at most IDEAL_DISTANCE from the start when the range is open' do
-      n = item1.send(:scoped_items).count
+      n = set_size
 
       expect([item1, item2].map(&:relative_position)).to all(be >= (RelativePositioning::START_POSITION - (n * RelativePositioning::IDEAL_DISTANCE)))
     end
@@ -434,7 +436,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     end
 
     it 'places items at most IDEAL_DISTANCE from the start when the range is open' do
-      n = item1.send(:scoped_items).count
+      n = set_size
 
       expect([item1, item2].map(&:relative_position)).to all(be <= (RelativePositioning::START_POSITION + (n * RelativePositioning::IDEAL_DISTANCE)))
     end