Merge branch 'ajk-relative-positioning-async-move-to-end' into 'master'

Move issues to the end on creation in the new-issue-worker

See merge request gitlab-org/gitlab!41313

app/services/issues/base_service.rb

@@ -17,8 +17,6 @@ module Issues
       Issues::CloseService
     end
 
-    private
-
     NO_REBALANCING_NEEDED = ((RelativePositioning::MIN_POSITION * 0.9999)..(RelativePositioning::MAX_POSITION * 0.9999)).freeze
 
     def rebalance_if_needed(issue)
@@ -32,6 +30,8 @@ module Issues
       IssueRebalancingWorker.perform_async(nil, issue.project_id)
     end
 
+    private
+
     def create_assignee_note(issue, old_assignees)
       SystemNoteService.change_issuable_assignees(
         issue, issue.project, current_user, old_assignees)

app/services/issues/create_service.rb

@@ -16,12 +16,12 @@ module Issues
 
     def before_create(issue)
       spam_check(issue, current_user, action: :create)
-      issue.move_to_end
 
       # current_user (defined in BaseService) is not available within run_after_commit block
       user = current_user
       issue.run_after_commit do
         NewIssueWorker.perform_async(issue.id, user.id)
+        IssuePlacementWorker.perform_async(issue.id)
       end
     end
 
@@ -30,7 +30,6 @@ module Issues
       user_agent_detail_service.create
       resolve_discussions_with_issue(issuable)
       delete_milestone_total_issue_counter_cache(issuable.milestone)
-      rebalance_if_needed(issuable)
 
       super
     end

app/workers/all_queues.yml

@@ -1452,6 +1452,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: issue_placement
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :cpu
+  :weight: 2
+  :idempotent: true
+  :tags: []
 - :name: issue_rebalancing
   :feature_category: :issue_tracking
   :has_external_dependencies:

app/workers/issue_placement_worker.rb

+# frozen_string_literal: true
+
+class IssuePlacementWorker
+  include ApplicationWorker
+
+  idempotent!
+  feature_category :issue_tracking
+  urgency :high
+  worker_resource_boundary :cpu
+  weight 2
+
+  # Move at most the most recent 100 issues
+  QUERY_LIMIT = 100
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def perform(issue_id)
+    issue = Issue.id_in(issue_id).first
+    return unless issue
+
+    # Move the most recent 100 unpositioned items to the end.
+    # This is to deal with out-of-order execution of the worker,
+    # while preserving creation order.
+    to_place = Issue
+      .relative_positioning_query_base(issue)
+      .where(relative_position: nil)
+      .order({ created_at: :desc }, { id: :desc })
+      .limit(QUERY_LIMIT)
+
+    Issue.move_nulls_to_end(to_place.to_a.reverse)
+    Issues::BaseService.new(nil).rebalance_if_needed(to_place.max_by(&:relative_position))
+  rescue RelativePositioning::NoSpaceLeft => e
+    Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id)
+    IssueRebalancingWorker.perform_async(nil, issue.project_id)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

app/workers/new_issue_worker.rb

@@ -12,8 +12,8 @@ class NewIssueWorker # rubocop:disable Scalability/IdempotentWorker
   def perform(issue_id, user_id)
     return unless objects_found?(issue_id, user_id)
 
-    EventCreateService.new.open_issue(issuable, user)
-    NotificationService.new.new_issue(issuable, user)
+    ::EventCreateService.new.open_issue(issuable, user)
+    ::NotificationService.new.new_issue(issuable, user)
     issuable.create_cross_references!(user)
   end
 

changelogs/unreleased/ajk-relative-positioning-async-move-to-end.yml

+---
+title: Ensure issue creation is not blocked by positioning
+merge_request: 41313
+author:
+type: fixed

config/sidekiq_queues.yml

@@ -138,6 +138,8 @@
   - 2
 - - irker
   - 1
+- - issue_placement
+  - 2
 - - issue_rebalancing
   - 1
 - - jira_connect

spec/fixtures/api/schemas/issue.json

@@ -12,7 +12,7 @@
     "title": { "type": "string" },
     "confidential": { "type": "boolean" },
     "due_date": { "type": ["date", "null"] },
-    "relative_position": { "type": "integer" },
+    "relative_position": { "type": ["integer", "null"] },
     "time_estimate": { "type": "integer" },
     "issue_sidebar_endpoint": { "type": "string" },
     "toggle_subscription_endpoint": { "type": "string" },

spec/services/issues/create_service_spec.rb

@@ -75,35 +75,10 @@ RSpec.describe Issues::CreateService do
         expect(Todo.where(attributes).count).to eq 1
       end
 
-      it 'rebalances if needed' do
-        create(:issue, project: project, relative_position: RelativePositioning::MAX_POSITION)
-        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
+      it 'moves the issue to the end, in an asynchronous worker' do
+        expect(IssuePlacementWorker).to receive(:perform_async).with(Integer)
 
-        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
-      end
-
-      it 'does not rebalance if the flag is disabled' do
-        stub_feature_flags(rebalance_issues: false)
-
-        create(:issue, project: project, relative_position: RelativePositioning::MAX_POSITION)
-        expect(IssueRebalancingWorker).not_to receive(:perform_async)
-
-        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
-      end
-
-      it 'does rebalance if the flag is enabled for the project' do
-        stub_feature_flags(rebalance_issues: project)
-
-        create(:issue, project: project, relative_position: RelativePositioning::MAX_POSITION)
-        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
-
-        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
-      end
-
-      it 'does not rebalance unless needed' do
-        expect(IssueRebalancingWorker).not_to receive(:perform_async)
-
-        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
+        described_class.new(project, user, opts).execute
       end
 
       context 'when label belongs to project group' do

spec/services/issues/reorder_service_spec.rb

@@ -3,9 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Issues::ReorderService do
-  let_it_be(:user)    { create(:user) }
-  let_it_be(:project) { create(:project) }
+  let_it_be(:user)    { create_default(:user) }
   let_it_be(:group)   { create(:group) }
+  let_it_be(:project, reload: true) { create(:project, namespace: group) }
 
   shared_examples 'issues reorder service' do
     context 'when reordering issues' do
@@ -14,7 +14,7 @@ RSpec.describe Issues::ReorderService do
       end
 
       it 'returns false with both invalid params' do
-        params = { move_after_id: nil, move_before_id: 1 }
+        params = { move_after_id: nil, move_before_id: non_existing_record_id }
 
         expect(service(params).execute(issue1)).to be_falsey
       end
@@ -27,27 +27,39 @@ RSpec.describe Issues::ReorderService do
         expect(issue1.relative_position)
           .to be_between(issue2.relative_position, issue3.relative_position)
       end
+
+      it 'sorts issues if only given one neighbour, on the left' do
+        params = { move_before_id: issue3.id }
+
+        service(params).execute(issue1)
+
+        expect(issue1.relative_position).to be > issue3.relative_position
+      end
+
+      it 'sorts issues if only given one neighbour, on the right' do
+        params = { move_after_id: issue1.id }
+
+        service(params).execute(issue3)
+
+        expect(issue3.relative_position).to be < issue1.relative_position
+      end
     end
   end
 
   describe '#execute' do
-    let(:issue1) { create(:issue, project: project, relative_position: 10) }
-    let(:issue2) { create(:issue, project: project, relative_position: 20) }
-    let(:issue3) { create(:issue, project: project, relative_position: 30) }
+    let_it_be(:issue1, reload: true) { create(:issue, project: project, relative_position: 10) }
+    let_it_be(:issue2) { create(:issue, project: project, relative_position: 20) }
+    let_it_be(:issue3, reload: true) { create(:issue, project: project, relative_position: 30) }
 
     context 'when ordering issues in a project' do
-      let(:parent) { project }
-
       before do
-        parent.add_developer(user)
+        project.add_developer(user)
       end
 
       it_behaves_like 'issues reorder service'
     end
 
     context 'when ordering issues in a group' do
-      let(:project) { create(:project, namespace: group) }
-
       before do
         group.add_developer(user)
       end

spec/workers/issue_placement_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IssuePlacementWorker do
+  describe '#perform' do
+    let_it_be(:time) { Time.now.utc }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:author) { create(:user) }
+    let_it_be(:common_attrs) { { author: author, project: project } }
+    let_it_be(:unplaced) { common_attrs.merge(relative_position: nil) }
+    let_it_be(:issue) { create(:issue, **unplaced, created_at: time) }
+    let_it_be(:issue_a) { create(:issue, **unplaced, created_at: time - 1.minute) }
+    let_it_be(:issue_b) { create(:issue, **unplaced, created_at: time - 2.minutes) }
+    let_it_be(:issue_c) { create(:issue, **unplaced, created_at: time + 1.minute) }
+    let_it_be(:issue_d) { create(:issue, **unplaced, created_at: time + 2.minutes) }
+    let_it_be(:issue_e) { create(:issue, **common_attrs, relative_position: 10, created_at: time + 1.minute) }
+    let_it_be(:issue_f) { create(:issue, **unplaced, created_at: time + 1.minute) }
+
+    let_it_be(:irrelevant) { create(:issue, relative_position: nil, created_at: time) }
+
+    it 'places all issues created at most 5 minutes before this one at the end, most recent last' do
+      expect do
+        described_class.new.perform(issue.id)
+      end.not_to change { irrelevant.reset.relative_position }
+
+      expect(project.issues.order_relative_position_asc)
+        .to eq([issue_e, issue_b, issue_a, issue, issue_c, issue_f, issue_d])
+      expect(project.issues.where(relative_position: nil)).not_to exist
+    end
+
+    it 'schedules rebalancing if needed' do
+      issue_a.update!(relative_position: RelativePositioning::MAX_POSITION)
+
+      expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
+
+      described_class.new.perform(issue.id)
+    end
+
+    it 'limits the sweep to QUERY_LIMIT records' do
+      # Ensure there are more than N issues in this set
+      n = described_class::QUERY_LIMIT
+      create_list(:issue, n - 5, **unplaced)
+
+      expect(Issue).to receive(:move_nulls_to_end).with(have_attributes(count: n)).and_call_original
+
+      described_class.new.perform(issue.id)
+
+      expect(project.issues.where(relative_position: nil)).to exist
+    end
+
+    it 'anticipates the failure to find the issue' do
+      id = non_existing_record_id
+
+      expect { described_class.new.perform(id) }.not_to raise_error
+    end
+
+    it 'anticipates the failure to place the issues, and schedules rebalancing' do
+      allow(Issue).to receive(:move_nulls_to_end) { raise RelativePositioning::NoSpaceLeft }
+
+      expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
+      expect(Gitlab::ErrorTracking)
+        .to receive(:log_exception)
+        .with(RelativePositioning::NoSpaceLeft, issue_id: issue.id)
+
+      described_class.new.perform(issue.id)
+    end
+  end
+end

spec/workers/new_issue_worker_spec.rb

@@ -39,10 +39,10 @@ RSpec.describe NewIssueWorker do
     end
 
     context 'when everything is ok' do
-      let(:project) { create(:project, :public) }
-      let(:mentioned) { create(:user) }
-      let(:user) { create(:user) }
-      let(:issue) { create(:issue, project: project, description: "issue for #{mentioned.to_reference}") }
+      let_it_be(:user) { create_default(:user) }
+      let_it_be(:project) { create(:project, :public) }
+      let_it_be(:mentioned) { create(:user) }
+      let_it_be(:issue) { create(:issue, project: project, description: "issue for #{mentioned.to_reference}") }
 
       it 'creates a new event record' do
         expect { worker.perform(issue.id, user.id) }.to change { Event.count }.from(0).to(1)