diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 08524f739d2cc7fca0fef88b50756ec12595b0b1..478c7cd156ff5b5e3029c5fcfb47f22e10b67c95 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -65,7 +65,7 @@ def award_emojis_loaded?
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
diff --git a/app/models/user.rb b/app/models/user.rb
index a0b23429f9fb0cc69ccba6f226736d95c65888c6..a4b0f96e97559d686b19db1c276ca5ce86fc962e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1668,8 +1668,7 @@ def update_todos_count_cache
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
@@ -1682,11 +1681,8 @@ def invalidate_merge_request_cache_counts
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
diff --git a/app/services/issuable/destroy_service.rb b/app/services/issuable/destroy_service.rb
index 4c64655a622f2ef404d000604b08538981c13979..b2458aec0b1df2ea3103b7256dbcd538fa09f048 100644
--- a/app/services/issuable/destroy_service.rb
+++ b/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
diff --git a/app/services/todos/destroy/destroyed_issuable_service.rb b/app/services/todos/destroy/destroyed_issuable_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..db12965224b39cba4da1dd4bc77bd5782efb9867
--- /dev/null
+++ b/app/services/todos/destroy/destroyed_issuable_service.rb
@@ -0,0 +1,46 @@
+# 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
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 24c96095ccc16a7fa8e6dbe09f9f0b0e87d080bd..532e49410727403de0e44b33824143046999f14a 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -1411,6 +1411,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:
diff --git a/app/workers/todos_destroyer/destroyed_issuable_worker.rb b/app/workers/todos_destroyer/destroyed_issuable_worker.rb
new file mode 100644
index 0000000000000000000000000000000000000000..6ca1959ff3452467365b4e2cbaef9bd842f2211d
--- /dev/null
+++ b/app/workers/todos_destroyer/destroyed_issuable_worker.rb
@@ -0,0 +1,14 @@
+# 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
diff --git a/changelogs/unreleased/325689-delete-issuable-todos-async.yml b/changelogs/unreleased/325689-delete-issuable-todos-async.yml
new file mode 100644
index 0000000000000000000000000000000000000000..b8335448bafce4180e1a2ed12bdf1779da092235
--- /dev/null
+++ b/changelogs/unreleased/325689-delete-issuable-todos-async.yml
@@ -0,0 +1,5 @@
+---
+title: Delete all issuable todos asynchronously when issuable is destroyed
+merge_request: 57830
+author:
+type: performance
diff --git a/config/feature_flags/development/destroy_issuable_todos_async.yml b/config/feature_flags/development/destroy_issuable_todos_async.yml
new file mode 100644
index 0000000000000000000000000000000000000000..c39e551bdd9909dc681e1cc6559a02173177db78
--- /dev/null
+++ b/config/feature_flags/development/destroy_issuable_todos_async.yml
@@ -0,0 +1,8 @@
+---
+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
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 5be98dbb096289e318ac0408d41b216671e1ab2f..530db0537eb58f2ee000a14aef56fa553e004132 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -1518,12 +1518,6 @@ def post_spam
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
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 82b969cad40f3ee3c9362212030c9939a50365fa..337a4a19b2ec5ff444121a8196d177ea2903738b 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -728,12 +728,6 @@ def merge_when_pipeline_succeeds
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
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 07f3efd70f6c4344975f4e8c48b313923c5617e7..1df70f387076948e022323de0aced28179a69608 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -16,7 +16,7 @@
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) }
diff --git a/spec/services/issuable/destroy_service_spec.rb b/spec/services/issuable/destroy_service_spec.rb
index 8d62932f9867abb14bc764335f9d579f5789754e..fadab77a04328a30996d38c9b35aeac47d462ce8 100644
--- a/spec/services/issuable/destroy_service_spec.rb
+++ b/spec/services/issuable/destroy_service_spec.rb
@@ -9,6 +9,32 @@
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 @@
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 @@
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
diff --git a/spec/services/todos/destroy/destroyed_issuable_service_spec.rb b/spec/services/todos/destroy/destroyed_issuable_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..24f74bae7c8afbed267d6c54134ffe0c02c3b387
--- /dev/null
+++ b/spec/services/todos/destroy/destroyed_issuable_service_spec.rb
@@ -0,0 +1,34 @@
+# 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
diff --git a/spec/workers/todos_destroyer/destroyed_issuable_worker_spec.rb b/spec/workers/todos_destroyer/destroyed_issuable_worker_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..6ccad25ad76a5f59cc4159c97ba1693918051367
--- /dev/null
+++ b/spec/workers/todos_destroyer/destroyed_issuable_worker_spec.rb
@@ -0,0 +1,15 @@
+# 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