Delete all issuable todos asynchronously when issuable is destroyed

The `Issuable::DestroyService` gets called for
`DELETE /api/projects/:id/merge_requests/:merge_request_iid`
endpoint.

When deleting an issuable (`Issue` or `MergeRequest`), the
associated Todo records need to be deleted as well. Before this
change, it was calling `Todo#destroy` on each associated todo that
results to N+1 issues.

Deletion of todos is now being done asynchronously since it's
possible that the deleted issuable has lots of associated todos
and doing it synchronously can take time. This also makes it
consistent with how we delete todos in other cases (other services
can be found under `app/services/todos/destroy/` directory).

Deletion is being done in batches to avoid statement timeouts in
case there are a lot of todos to be deleted.

app/models/concerns/issuable.rb

@@ -65,7 +65,7 @@ module Issuable
 
     has_many :label_links, as: :target, dependent: :destroy, inverse_of: :target # rubocop:disable Cop/ActiveRecordDependent
     has_many :labels, through: :label_links
-    has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :todos, as: :target
 
     has_one :metrics, inverse_of: model_name.singular.to_sym, autosave: true
 

app/models/user.rb

@@ -1659,8 +1659,7 @@ class User < ApplicationRecord
   def invalidate_cache_counts
     invalidate_issue_cache_counts
     invalidate_merge_request_cache_counts
-    invalidate_todos_done_count
-    invalidate_todos_pending_count
+    invalidate_todos_cache_counts
     invalidate_personal_projects_count
   end
 
@@ -1673,11 +1672,8 @@ class User < ApplicationRecord
     Rails.cache.delete(['users', id, 'review_requested_open_merge_requests_count'])
   end
 
-  def invalidate_todos_done_count
+  def invalidate_todos_cache_counts
     Rails.cache.delete(['users', id, 'todos_done_count'])
-  end
-
-  def invalidate_todos_pending_count
     Rails.cache.delete(['users', id, 'todos_pending_count'])
   end
 

app/services/issuable/destroy_service.rb

@@ -3,11 +3,25 @@
 module Issuable
   class DestroyService < IssuableBaseService
     def execute(issuable)
-      TodoService.new.destroy_target(issuable) do |issuable|
-        if issuable.destroy
-          issuable.update_project_counter_caches
-          issuable.assignees.each(&:invalidate_cache_counts)
-        end
+      if issuable.destroy
+        delete_todos(issuable)
+        issuable.update_project_counter_caches
+        issuable.assignees.each(&:invalidate_cache_counts)
+      end
+    end
+
+    private
+
+    def delete_todos(issuable)
+      actor = issuable.is_a?(Epic) ? issuable.resource_parent : issuable.resource_parent.group
+
+      if Feature.enabled?(:destroy_issuable_todos_async, actor, default_enabled: :yaml)
+        TodosDestroyer::DestroyedIssuableWorker
+          .perform_async(issuable.id, issuable.class.name)
+      else
+        TodosDestroyer::DestroyedIssuableWorker
+          .new
+          .perform(issuable.id, issuable.class.name)
       end
     end
   end

app/services/todos/destroy/destroyed_issuable_service.rb

+# frozen_string_literal: true
+
+module Todos
+  module Destroy
+    class DestroyedIssuableService
+      BATCH_SIZE = 100
+
+      def initialize(target_id, target_type)
+        @target_id = target_id
+        @target_type = target_type
+      end
+
+      def execute
+        inner_query = Todo.select(:id).for_target(target_id).for_type(target_type).limit(BATCH_SIZE)
+
+        delete_query = <<~SQL
+        DELETE FROM "#{Todo.table_name}"
+        WHERE id IN (#{inner_query.to_sql})
+        RETURNING user_id
+        SQL
+
+        loop do
+          result = ActiveRecord::Base.connection.execute(delete_query)
+
+          break if result.cmd_tuples == 0
+
+          user_ids = result.map { |row| row['user_id'] }.uniq
+
+          invalidate_todos_cache_counts(user_ids)
+        end
+      end
+
+      private
+
+      attr_reader :target_id, :target_type
+
+      def invalidate_todos_cache_counts(user_ids)
+        user_ids.each do |id|
+          # Only build a user instance since we only need its ID for
+          # `User#invalidate_todos_cache_counts` to work.
+          User.new(id: id).invalidate_todos_cache_counts
+        end
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -1403,6 +1403,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: todos_destroyer:todos_destroyer_destroyed_issuable
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: todos_destroyer:todos_destroyer_entity_leave
   :feature_category: :issue_tracking
   :has_external_dependencies:

app/workers/todos_destroyer/destroyed_issuable_worker.rb

+# frozen_string_literal: true
+
+module TodosDestroyer
+  class DestroyedIssuableWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    idempotent!
+
+    def perform(target_id, target_type)
+      ::Todos::Destroy::DestroyedIssuableService.new(target_id, target_type).execute
+    end
+  end
+end

changelogs/unreleased/325689-delete-issuable-todos-async.yml

+---
+title: Delete all issuable todos asynchronously when issuable is destroyed
+merge_request: 57830
+author:
+type: performance

config/feature_flags/development/destroy_issuable_todos_async.yml

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

spec/controllers/projects/issues_controller_spec.rb

@@ -1518,12 +1518,6 @@ RSpec.describe Projects::IssuesController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
         expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for issue' })
       end
-
-      it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once
-
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
-      end
     end
   end
 

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -722,12 +722,6 @@ RSpec.describe Projects::MergeRequestsController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
         expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for merge request' })
       end
-
-      it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once
-
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
-      end
     end
   end
 

spec/models/concerns/issuable_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Issuable do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:author) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
-    it { is_expected.to have_many(:todos).dependent(:destroy) }
+    it { is_expected.to have_many(:todos) }
     it { is_expected.to have_many(:labels) }
     it { is_expected.to have_many(:note_authors).through(:notes) }
 

spec/services/issuable/destroy_service_spec.rb

@@ -9,6 +9,32 @@ RSpec.describe Issuable::DestroyService do
   subject(:service) { described_class.new(project, user) }
 
   describe '#execute' do
+    shared_examples_for 'service deleting todos' do
+      it 'destroys associated todos asynchronously' do
+        expect(TodosDestroyer::DestroyedIssuableWorker)
+          .to receive(:perform_async)
+          .with(issuable.id, issuable.class.name)
+
+        subject.execute(issuable)
+      end
+
+      context 'when destroy_issuable_todos_async feature is disabled' do
+        before do
+          stub_feature_flags(destroy_issuable_todos_async: false)
+        end
+
+        it 'destroy associated todos synchronously' do
+          expect_next_instance_of(TodosDestroyer::DestroyedIssuableWorker) do |worker|
+            expect(worker)
+              .to receive(:perform)
+              .with(issuable.id, issuable.class.name)
+          end
+
+          subject.execute(issuable)
+        end
+      end
+    end
+
     context 'when issuable is an issue' do
       let!(:issue) { create(:issue, project: project, author: user, assignees: [user]) }
 
@@ -22,17 +48,14 @@ RSpec.describe Issuable::DestroyService do
         service.execute(issue)
       end
 
-      it 'updates the todo caches for users with todos on the issue' do
-        create(:todo, target: issue, user: user, author: user, project: project)
-
-        expect { service.execute(issue) }
-          .to change { user.todos_pending_count }.from(1).to(0)
-      end
-
       it 'invalidates the issues count cache for the assignees' do
         expect_any_instance_of(User).to receive(:invalidate_cache_counts).once
         service.execute(issue)
       end
+
+      it_behaves_like 'service deleting todos' do
+        let(:issuable) { issue }
+      end
     end
 
     context 'when issuable is a merge request' do
@@ -53,11 +76,8 @@ RSpec.describe Issuable::DestroyService do
         service.execute(merge_request)
       end
 
-      it 'updates the todo caches for users with todos on the merge request' do
-        create(:todo, target: merge_request, user: user, author: user, project: project)
-
-        expect { service.execute(merge_request) }
-          .to change { user.todos_pending_count }.from(1).to(0)
+      it_behaves_like 'service deleting todos' do
+        let(:issuable) { merge_request }
       end
     end
   end

spec/services/todos/destroy/destroyed_issuable_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::Destroy::DestroyedIssuableService do
+  describe '#execute' do
+    let_it_be(:target) { create(:merge_request) }
+    let_it_be(:pending_todo) { create(:todo, :pending, project: target.project, target: target, user: create(:user)) }
+    let_it_be(:done_todo) { create(:todo, :done, project: target.project, target: target, user: create(:user)) }
+
+    def execute
+      described_class.new(target.id, target.class.name).execute
+    end
+
+    it 'deletes todos for specified target ID and type' do
+      control_count = ActiveRecord::QueryRecorder.new { execute }.count
+
+      # Create more todos for the target
+      create(:todo, :pending, project: target.project, target: target, user: create(:user))
+      create(:todo, :pending, project: target.project, target: target, user: create(:user))
+      create(:todo, :done, project: target.project, target: target, user: create(:user))
+      create(:todo, :done, project: target.project, target: target, user: create(:user))
+
+      expect { execute }.not_to exceed_query_limit(control_count)
+      expect(target.reload.todos.count).to eq(0)
+    end
+
+    it 'invalidates todos cache counts of todo users', :use_clean_rails_redis_caching do
+      expect { execute }
+        .to change { pending_todo.user.todos_pending_count }.from(1).to(0)
+        .and change { done_todo.user.todos_done_count }.from(1).to(0)
+    end
+  end
+end

spec/workers/todos_destroyer/destroyed_issuable_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TodosDestroyer::DestroyedIssuableWorker do
+  let(:job_args) { [1, 'MergeRequest'] }
+
+  it 'calls the Todos::Destroy::DestroyedIssuableService' do
+    expect_next_instance_of(::Todos::Destroy::DestroyedIssuableService, *job_args) do |service|
+      expect(service).to receive(:execute)
+    end
+
+    described_class.new.perform(*job_args)
+  end
+end