Merge branch 'fix_rebalancing_issue_with_null_position' into 'master'

Skip rebalancing issues with null relative position

See merge request gitlab-org/gitlab!72206

app/models/issue.rb

@@ -166,6 +166,8 @@ class Issue < ApplicationRecord
   scope :by_project_id_and_iid, ->(composites) do
     where_composite(%i[project_id iid], composites)
   end
+  scope :with_null_relative_position, -> { where(relative_position: nil) }
+  scope :with_non_null_relative_position, -> { where.not(relative_position: nil) }
 
   after_commit :expire_etag_cache, unless: :importing?
   after_save :ensure_metrics, unless: :importing?

app/services/issues/relative_position_rebalancing_service.rb

@@ -82,7 +82,7 @@ module Issues
       collection.each do |project|
         caching.cache_current_project_id(project.id)
         index += 1
-        scope = Issue.in_projects(project).order_by_relative_position.select(:id, :relative_position)
+        scope = Issue.in_projects(project).order_by_relative_position.with_non_null_relative_position.select(:id, :relative_position)
 
         with_retry(PREFETCH_ISSUES_BATCH_SIZE, 100) do |batch_size|
           Gitlab::Pagination::Keyset::Iterator.new(scope: scope).each_batch(of: batch_size) do |batch|

app/workers/issue_placement_worker.rb

@@ -31,7 +31,7 @@ class IssuePlacementWorker
     # while preserving creation order.
     to_place = Issue
       .relative_positioning_query_base(issue)
-      .where(relative_position: nil)
+      .with_null_relative_position
       .order({ created_at: :asc }, { id: :asc })
       .limit(QUERY_LIMIT + 1)
       .to_a

spec/services/issues/relative_position_rebalancing_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_shared_state do
-  let_it_be(:project, reload: true) { create(:project) }
+  let_it_be(:project, reload: true) { create(:project, :repository_disabled, skip_disk_validation: true) }
   let_it_be(:user) { project.creator }
   let_it_be(:start) { RelativePositioning::START_POSITION }
   let_it_be(:max_pos) { RelativePositioning::MAX_POSITION }
@@ -28,12 +28,18 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s
     end
   end
 
+  let_it_be(:nil_clump, reload: true) do
+    (1..100).to_a.map do |i|
+      create(:issue, project: project, author: user, relative_position: nil)
+    end
+  end
+
   before do
     stub_feature_flags(issue_rebalancing_with_retry: false)
   end
 
   def issues_in_position_order
-    project.reload.issues.reorder(relative_position: :asc).to_a
+    project.reload.issues.order_by_relative_position.to_a
   end
 
   subject(:service) { described_class.new(Project.id_in(project)) }
@@ -44,16 +50,19 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s
 
       expect { service.execute }.not_to change { issues_in_position_order.map(&:id) }
 
+      caching = service.send(:caching)
       all_issues.each(&:reset)
 
       gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b|
         b.relative_position - a.relative_position
       end
 
+      expect(caching.issue_count).to eq(900)
       expect(gaps).to all(be > RelativePositioning::MIN_GAP)
       expect(all_issues.first.relative_position).to be > (RelativePositioning::MIN_POSITION * 0.9999)
       expect(all_issues.last.relative_position).to be < (RelativePositioning::MAX_POSITION * 0.9999)
       expect(project.root_namespace.issue_repositioning_disabled?).to be false
+      expect(project.issues.with_null_relative_position.count).to eq(100)
     end
 
     it 'is idempotent' do
@@ -111,7 +120,7 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s
       allow(caching).to receive(:concurrent_running_rebalances_count).and_return(10)
       allow(service).to receive(:caching).and_return(caching)
 
-      expect { service.execute }.not_to raise_error(Issues::RelativePositionRebalancingService::TooManyConcurrentRebalances)
+      expect { service.execute }.not_to raise_error
     end
 
     context 're-balancing is retried on statement timeout exceptions' do