Move after create tasks to async job

The MergeRequests::AfterCreateService was created in order to move
one problematic bit of code into an async job. It was noticed that these
items could probably be moved as well. That work was deferred to a
follow up issue to avoid slowing iteration. Since GitLab.com is
currently suffering from performance issues, this is now a priority.

app/services/merge_requests/after_create_service.rb

@@ -24,6 +24,16 @@ module MergeRequests
       merge_request.create_cross_references!(current_user)
 
       OnboardingProgressService.new(merge_request.target_project.namespace).execute(action: :merge_request_created)
+
+      todo_service.new_merge_request(merge_request, current_user)
+      merge_request.cache_merge_request_closes_issues!(current_user)
+
+      Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
+      link_lfs_objects(merge_request)
+    end
+
+    def link_lfs_objects(merge_request)
+      LinkLfsObjectsService.new(merge_request.target_project).execute(merge_request)
     end
   end
 end

app/services/merge_requests/create_service.rb

@@ -18,12 +18,6 @@ module MergeRequests
       # be performed in Sidekiq
       NewMergeRequestWorker.perform_async(issuable.id, current_user.id)
 
-      todo_service.new_merge_request(issuable, current_user)
-      issuable.cache_merge_request_closes_issues!(current_user)
-
-      Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
-      link_lfs_objects(issuable)
-
       super
     end
 
@@ -54,10 +48,6 @@ module MergeRequests
         raise Gitlab::Access::AccessDeniedError
       end
     end
-
-    def link_lfs_objects(issuable)
-      LinkLfsObjectsService.new(issuable.target_project).execute(issuable)
-    end
   end
 end
 

changelogs/unreleased/mk-speed-up-merge-request-creation-action.yml

+---
+title: Perform more merge request creation tasks asynchronously to improve response
+  times
+merge_request: 57453
+author:
+type: performance

spec/services/merge_requests/after_create_service_spec.rb

@@ -93,5 +93,93 @@ RSpec.describe MergeRequests::AfterCreateService do
         expect(merge_request.reload).to be_unchecked
       end
     end
+
+    it 'increments the usage data counter of create event' do
+      counter = Gitlab::UsageDataCounters::MergeRequestCounter
+
+      expect { execute_service }.to change { counter.read(:create) }.by(1)
+    end
+
+    context 'todos' do
+      it 'does not creates todos' do
+        attributes = {
+          project: merge_request.target_project,
+          target_id: merge_request.id,
+          target_type: merge_request.class.name
+        }
+
+        expect { execute_service }.not_to change { Todo.where(attributes).count }
+      end
+
+      context 'when merge request is assigned to someone' do
+        let_it_be(:assignee) { create(:user) }
+        let_it_be(:merge_request) { create(:merge_request, assignees: [assignee]) }
+
+        it 'creates a todo for new assignee' do
+          attributes = {
+            project: merge_request.target_project,
+            author: merge_request.author,
+            user: assignee,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::ASSIGNED,
+            state: :pending
+          }
+
+          expect { execute_service }.to change { Todo.where(attributes).count }.by(1)
+        end
+      end
+
+      context 'when reviewer is assigned' do
+        let_it_be(:reviewer) { create(:user) }
+        let_it_be(:merge_request) { create(:merge_request, reviewers: [reviewer]) }
+
+        it 'creates a todo for new reviewer' do
+          attributes = {
+            project: merge_request.target_project,
+            author: merge_request.author,
+            user: reviewer,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::REVIEW_REQUESTED,
+            state: :pending
+          }
+
+          expect { execute_service }.to change { Todo.where(attributes).count }.by(1)
+        end
+      end
+    end
+
+    context 'when saving references to issues that the created merge request closes' do
+      let_it_be(:first_issue) { create(:issue, project: merge_request.target_project) }
+      let_it_be(:second_issue) { create(:issue, project: merge_request.target_project) }
+
+      it 'creates a `MergeRequestsClosingIssues` record for each issue' do
+        merge_request.description = "Closes #{first_issue.to_reference} and #{second_issue.to_reference}"
+        merge_request.source_branch = "feature"
+        merge_request.target_branch = merge_request.target_project.default_branch
+        merge_request.save!
+
+        execute_service
+
+        issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id)
+        expect(issue_ids).to match_array([first_issue.id, second_issue.id])
+      end
+    end
+
+    it 'tracks merge request creation in usage data' do
+      expect(Gitlab::UsageDataCounters::MergeRequestCounter).to receive(:count).with(:create)
+
+      execute_service
+    end
+
+    it 'calls MergeRequests::LinkLfsObjectsService#execute' do
+      service = instance_spy(MergeRequests::LinkLfsObjectsService)
+      allow(MergeRequests::LinkLfsObjectsService).to receive(:new).with(merge_request.target_project).and_return(service)
+
+      execute_service
+
+      expect(service).to have_received(:execute).with(merge_request)
+    end
   end
 end

spec/services/merge_requests/create_service_spec.rb

@@ -47,16 +47,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           .to change { project.open_merge_requests_count }.from(0).to(1)
       end
 
-      it 'does not creates todos' do
-        attributes = {
-          project: project,
-          target_id: merge_request.id,
-          target_type: merge_request.class.name
-        }
-
-        expect(Todo.where(attributes).count).to be_zero
-      end
-
       it 'creates exactly 1 create MR event', :sidekiq_might_not_need_inline do
         attributes = {
           action: :created,
@@ -113,20 +103,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
 
         it { expect(merge_request.assignees).to eq([user2]) }
-
-        it 'creates a todo for new assignee' do
-          attributes = {
-            project: project,
-            author: user,
-            user: user2,
-            target_id: merge_request.id,
-            target_type: merge_request.class.name,
-            action: Todo::ASSIGNED,
-            state: :pending
-          }
-
-          expect(Todo.where(attributes).count).to eq 1
-        end
       end
 
       context 'when reviewer is assigned' do
@@ -142,20 +118,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
         it { expect(merge_request.reviewers).to eq([user2]) }
 
-        it 'creates a todo for new reviewer' do
-          attributes = {
-            project: project,
-            author: user,
-            user: user2,
-            target_id: merge_request.id,
-            target_type: merge_request.class.name,
-            action: Todo::REVIEW_REQUESTED,
-            state: :pending
-          }
-
-          expect(Todo.where(attributes).count).to eq 1
-        end
-
         it 'invalidates counter cache for reviewers', :use_clean_rails_memory_store_caching do
           expect { merge_request }
             .to change { user2.review_requested_open_merge_requests_count }
@@ -328,12 +290,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
       end
 
-      it 'increments the usage data counter of create event' do
-        counter = Gitlab::UsageDataCounters::MergeRequestCounter
-
-        expect { service.execute }.to change { counter.read(:create) }.by(1)
-      end
-
       context 'after_save callback to store_mentions' do
         let(:labels) { create_pair(:label, project: project) }
         let(:milestone) { create(:milestone, project: project) }
@@ -494,35 +450,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
       end
     end
 
-    context 'while saving references to issues that the created merge request closes' do
-      let(:first_issue) { create(:issue, project: project) }
-      let(:second_issue) { create(:issue, project: project) }
-
-      let(:opts) do
-        {
-          title: 'Awesome merge_request',
-          source_branch: 'feature',
-          target_branch: 'master',
-          force_remove_source_branch: '1'
-        }
-      end
-
-      before do
-        project.add_maintainer(user)
-        project.add_developer(user2)
-      end
-
-      it 'creates a `MergeRequestsClosingIssues` record for each issue' do
-        issue_closing_opts = opts.merge(description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}")
-        service = described_class.new(project, user, issue_closing_opts)
-        allow(service).to receive(:execute_hooks)
-        merge_request = service.execute
-
-        issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id)
-        expect(issue_ids).to match_array([first_issue.id, second_issue.id])
-      end
-    end
-
     context 'when source and target projects are different' do
       let(:target_project) { fork_project(project, nil, repository: true) }
 
@@ -571,14 +498,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           expect(merge_request).to be_persisted
         end
 
-        it 'calls MergeRequests::LinkLfsObjectsService#execute', :sidekiq_might_not_need_inline do
-          expect_next_instance_of(MergeRequests::LinkLfsObjectsService) do |service|
-            expect(service).to receive(:execute).with(instance_of(MergeRequest))
-          end
-
-          described_class.new(project, user, opts).execute
-        end
-
         it 'does not create the merge request when the target project is archived' do
           target_project.update!(archived: true)
 

spec/services/merge_requests/update_service_spec.rb

@@ -948,18 +948,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       it 'removes `MergeRequestsClosingIssues` records when issues are not closed anymore' do
-        opts = {
-          title: 'Awesome merge_request',
-          description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}",
-          source_branch: 'feature',
-          target_branch: 'master',
-          force_remove_source_branch: '1'
-        }
-
-        merge_request = MergeRequests::CreateService.new(project, user, opts).execute
-
-        issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id)
-        expect(issue_ids).to match_array([first_issue.id, second_issue.id])
+        create(:merge_requests_closing_issues, issue: first_issue, merge_request: merge_request)
+        create(:merge_requests_closing_issues, issue: second_issue, merge_request: merge_request)
 
         service = described_class.new(project, user, description: "not closing any issues")
         allow(service).to receive(:execute_hooks)