Delete all label links asynchronously when issuable gets destroyed

This is based on the solution we created in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57830 to
improve the performance of the `Issuable::DestroyService`.

Previously, the associated `label_links` to an Issuable (can be a
MergeRequest, Issue or Epic) gets destroyed when an issuable gets
destroyed. If there are 100 label links, that means 100 delete
SQL queries as well.

With this fix, the deletion will be done in batches. It's being
done in batches to avoid statement timeouts in case there are a
lot of label links to be deleted.

The worker will be performed asynchronously when the feature flag
is enabled.

Changelog: performance

app/models/concerns/issuable.rb

@@ -63,7 +63,7 @@ module Issuable
 
     has_many :note_authors, -> { distinct }, through: :notes, source: :author
 
-    has_many :label_links, as: :target, dependent: :destroy, inverse_of: :target # rubocop:disable Cop/ActiveRecordDependent
+    has_many :label_links, as: :target, inverse_of: :target
     has_many :labels, through: :label_links
     has_many :todos, as: :target
 

app/models/label_link.rb

@@ -9,4 +9,6 @@ class LabelLink < ApplicationRecord
 
   validates :target, presence: true, unless: :importing?
   validates :label, presence: true, unless: :importing?
+
+  scope :for_target, -> (target_id, target_type) { where(target_id: target_id, target_type: target_type) }
 end

app/services/issuable/destroy_label_links_service.rb

+# frozen_string_literal: true
+
+module Issuable
+  class DestroyLabelLinksService
+    BATCH_SIZE = 100
+
+    def initialize(target_id, target_type)
+      @target_id = target_id
+      @target_type = target_type
+    end
+
+    def execute
+      inner_query =
+        LabelLink
+          .select(:id)
+          .for_target(target_id, target_type)
+          .limit(BATCH_SIZE)
+
+      delete_query = <<~SQL
+      DELETE FROM "#{LabelLink.table_name}"
+      WHERE id IN (#{inner_query.to_sql})
+      SQL
+
+      loop do
+        result = ActiveRecord::Base.connection.execute(delete_query)
+
+        break if result.cmd_tuples == 0
+      end
+    end
+
+    private
+
+    attr_reader :target_id, :target_type
+  end
+end

app/services/issuable/destroy_service.rb

@@ -3,15 +3,13 @@
 module Issuable
   class DestroyService < IssuableBaseService
     def execute(issuable)
-      if issuable.destroy
-        after_destroy(issuable)
-      end
+      after_destroy(issuable) if issuable.destroy
     end
 
     private
 
     def after_destroy(issuable)
-      delete_todos(issuable)
+      delete_associated_records(issuable)
       issuable.update_project_counter_caches
       issuable.assignees.each(&:invalidate_cache_counts)
     end
@@ -20,9 +18,14 @@ module Issuable
       issuable.resource_parent.group
     end
 
-    def delete_todos(issuable)
+    def delete_associated_records(issuable)
       actor = group_for(issuable)
 
+      delete_todos(actor, issuable)
+      delete_label_links(actor, issuable)
+    end
+
+    def delete_todos(actor, issuable)
       if Feature.enabled?(:destroy_issuable_todos_async, actor, default_enabled: :yaml)
         TodosDestroyer::DestroyedIssuableWorker
           .perform_async(issuable.id, issuable.class.name)
@@ -32,6 +35,17 @@ module Issuable
           .perform(issuable.id, issuable.class.name)
       end
     end
+
+    def delete_label_links(actor, issuable)
+      if Feature.enabled?(:destroy_issuable_label_links_async, actor, default_enabled: :yaml)
+        Issuable::LabelLinksDestroyWorker
+          .perform_async(issuable.id, issuable.class.name)
+      else
+        Issuable::LabelLinksDestroyWorker
+          .new
+          .perform(issuable.id, issuable.class.name)
+      end
+    end
   end
 end
 

app/workers/all_queues.yml

@@ -2200,6 +2200,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: issuable_label_links_destroy
+  :worker_name: Issuable::LabelLinksDestroyWorker
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: issuables_clear_groups_issue_counter
   :worker_name: Issuables::ClearGroupsIssueCounterWorker
   :feature_category: :issue_tracking

app/workers/issuable/label_links_destroy_worker.rb

+# frozen_string_literal: true
+
+module Issuable
+  class LabelLinksDestroyWorker
+    include ApplicationWorker
+
+    idempotent!
+    feature_category :issue_tracking
+
+    def perform(target_id, target_type)
+      ::Issuable::DestroyLabelLinksService.new(target_id, target_type).execute
+    end
+  end
+end

changelogs/unreleased/325689-delete-all-label-links-async.yml

+---
+title: Delete all label links asynchronously when issuable gets destroyed
+merge_request: 60487
+author:
+type: performance

config/feature_flags/development/destroy_issuable_label_links_async.yml

+---
+name: destroy_issuable_label_links_async
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60487
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325689
+milestone: '13.12'
+type: development
+group: group::code review
+default_enabled: false

config/sidekiq_queues.yml

@@ -194,6 +194,8 @@
   - 1
 - - issuable_export_csv
   - 1
+- - issuable_label_links_destroy
+  - 1
 - - issuables_clear_groups_issue_counter
   - 1
 - - issue_placement

ee/spec/services/ee/issuable/destroy_service_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe Issuable::DestroyService do
       end
 
       it_behaves_like 'service deleting todos'
+      it_behaves_like 'service deleting label links'
     end
 
     context 'when destroying other issuable type' do

ee/spec/services/issuable/destroy_label_links_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuable::DestroyLabelLinksService do
+  describe '#execute' do
+    let_it_be(:target) { create(:epic) }
+
+    it_behaves_like 'service deleting label links of an issuable'
+  end
+end

spec/models/label_link_spec.rb

@@ -12,4 +12,15 @@ RSpec.describe LabelLink do
     let(:valid_items_for_bulk_insertion) { build_list(:label_link, 10) }
     let(:invalid_items_for_bulk_insertion) { [] } # class does not have any validations defined
   end
+
+  describe '.for_target' do
+    it 'returns the label links for a given target' do
+      label_link = create(:label_link, target: create(:merge_request))
+
+      create(:label_link, target: create(:issue))
+
+      expect(described_class.for_target(label_link.target_id, label_link.target_type))
+        .to contain_exactly(label_link)
+    end
+  end
 end

spec/services/issuable/destroy_label_links_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuable::DestroyLabelLinksService do
+  describe '#execute' do
+    context 'when target is an Issue' do
+      let_it_be(:target) { create(:issue) }
+
+      it_behaves_like 'service deleting label links of an issuable'
+    end
+
+    context 'when target is a MergeRequest' do
+      let_it_be(:target) { create(:merge_request) }
+
+      it_behaves_like 'service deleting label links of an issuable'
+    end
+  end
+end

spec/services/issuable/destroy_service_spec.rb

@@ -31,6 +31,10 @@ RSpec.describe Issuable::DestroyService do
       it_behaves_like 'service deleting todos' do
         let(:issuable) { issue }
       end
+
+      it_behaves_like 'service deleting label links' do
+        let(:issuable) { issue }
+      end
     end
 
     context 'when issuable is a merge request' do
@@ -54,6 +58,10 @@ RSpec.describe Issuable::DestroyService do
       it_behaves_like 'service deleting todos' do
         let(:issuable) { merge_request }
       end
+
+      it_behaves_like 'service deleting label links' do
+        let(:issuable) { merge_request }
+      end
     end
   end
 end

spec/support/shared_examples/services/destroy_label_links_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples_for 'service deleting label links of an issuable' do
+  let_it_be(:label_link) { create(:label_link, target: target) }
+
+  def execute
+    described_class.new(target.id, target.class.name).execute
+  end
+
+  it 'deletes label links for specified target ID and type' do
+    control_count = ActiveRecord::QueryRecorder.new { execute }.count
+
+    # Create more label links for the target
+    create(:label_link, target: target)
+    create(:label_link, target: target)
+
+    expect { execute }.not_to exceed_query_limit(control_count)
+    expect(target.reload.label_links.count).to eq(0)
+  end
+end

spec/support/shared_examples/services/issuable/destroy_service_shared_examples.rb

@@ -29,3 +29,33 @@ shared_examples_for 'service deleting todos' do
     end
   end
 end
+
+shared_examples_for 'service deleting label links' do
+  before do
+    stub_feature_flags(destroy_issuable_label_links_async: group)
+  end
+
+  it 'destroys associated label links asynchronously' do
+    expect(Issuable::LabelLinksDestroyWorker)
+      .to receive(:perform_async)
+      .with(issuable.id, issuable.class.name)
+
+    subject.execute(issuable)
+  end
+
+  context 'when destroy_issuable_label_links_async feature is disabled for group' do
+    before do
+      stub_feature_flags(destroy_issuable_label_links_async: false)
+    end
+
+    it 'destroy associated label links synchronously' do
+      expect_next_instance_of(Issuable::LabelLinksDestroyWorker) do |worker|
+        expect(worker)
+          .to receive(:perform)
+          .with(issuable.id, issuable.class.name)
+      end
+
+      subject.execute(issuable)
+    end
+  end
+end

spec/workers/issuable/label_links_destroy_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuable::LabelLinksDestroyWorker do
+  let(:job_args) { [1, 'MergeRequest'] }
+  let(:service) { double }
+
+  include_examples 'an idempotent worker' do
+    it 'calls the Issuable::DestroyLabelLinksService' do
+      expect(::Issuable::DestroyLabelLinksService).to receive(:new).twice.and_return(service)
+      expect(service).to receive(:execute).twice
+
+      subject
+    end
+  end
+end