Execute project hooks and services after commit when moving an issuedm-issue-move-transaction-error

5 files changed, 42 insertions, 9 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 3440c01b356..f9c640300ff 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -951,7 +951,9 @@ class Project < ActiveRecord::Base
   def send_move_instructions(old_path_with_namespace)
     # New project path needs to be committed to the DB or notification will
     # retrieve stale information
-    run_after_commit { NotificationService.new.project_was_moved(self, old_path_with_namespace) }
+    run_after_commit do
+      NotificationService.new.project_was_moved(self, old_path_with_namespace)
+    end
   end
 
   def owner
@@ -963,15 +965,19 @@ class Project < ActiveRecord::Base
   end
 
   def execute_hooks(data, hooks_scope = :push_hooks)
-    hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
-      hook.async_execute(data, hooks_scope.to_s)
+    run_after_commit_or_now do
+      hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
+        hook.async_execute(data, hooks_scope.to_s)
+      end
     end
   end
 
   def execute_services(data, hooks_scope = :push_hooks)
     # Call only service hooks that are active for this scope
-    services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend
-      service.async_execute(data)
+    run_after_commit_or_now do
+      services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend
+        service.async_execute(data)
+      end
     end
   end
 
diff --git a/changelogs/unreleased/dm-issue-move-transaction-error.yml b/changelogs/unreleased/dm-issue-move-transaction-error.yml
new file mode 100644
index 00000000000..0892169894a
--- /dev/null
+++ b/changelogs/unreleased/dm-issue-move-transaction-error.yml
@@ -0,0 +1,5 @@
+---
+title: Execute project hooks and services after commit when moving an issue
+merge_request:
+author:
+type: fixed
diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb
index bedd57ede04..cb611aa21df 100644
--- a/config/initializers/forbid_sidekiq_in_transactions.rb
+++ b/config/initializers/forbid_sidekiq_in_transactions.rb
@@ -1,5 +1,7 @@
 module Sidekiq
   module Worker
+    EnqueueFromTransactionError = Class.new(StandardError)
+
     mattr_accessor :skip_transaction_check
     self.skip_transaction_check = false
 
@@ -12,11 +14,11 @@ module Sidekiq
     end
 
     module ClassMethods
-      module NoSchedulingFromTransactions
+      module NoEnqueueingFromTransactions
         %i(perform_async perform_at perform_in).each do |name|
           define_method(name) do |*args|
             if !Sidekiq::Worker.skip_transaction_check && AfterCommitQueue.inside_transaction?
-              raise <<-MSG.strip_heredoc
+              raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG
                 `#{self}.#{name}` cannot be called inside a transaction as this can lead to
                 race conditions when the worker runs before the transaction is committed and
                 tries to access a model that has not been saved yet.
@@ -30,7 +32,7 @@ module Sidekiq
         end
       end
 
-      prepend NoSchedulingFromTransactions
+      prepend NoEnqueueingFromTransactions
     end
   end
 end
diff --git a/lib/after_commit_queue.rb b/lib/after_commit_queue.rb
index db63c5038ae..a4d8507960e 100644
--- a/lib/after_commit_queue.rb
+++ b/lib/after_commit_queue.rb
@@ -14,7 +14,15 @@ module AfterCommitQueue
 
   def run_after_commit_or_now(&block)
     if AfterCommitQueue.inside_transaction?
-      run_after_commit(&block)
+      if ActiveRecord::Base.connection.current_transaction.records.include?(self)
+        run_after_commit(&block)
+      else
+        # If the current transaction does not include this record, we can run
+        # the block now, even if it queues a Sidekiq job.
+        Sidekiq::Worker.skipping_transaction_check do
+          instance_eval(&block)
+        end
+      end
     else
       instance_eval(&block)
     end
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index f2b35a8fadf..2cad695d7c4 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/spec/services/issues/move_service_spec.rb
@@ -289,6 +289,18 @@ describe Issues::MoveService do
             .to raise_error(StandardError, /Cannot move issue/)
         end
       end
+
+      context 'project issue hooks' do
+        let(:hook) { create(:project_hook, project: old_project, issues_events: true) }
+
+        it 'executes project issue hooks' do
+          # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1,
+          # but since the entire spec run takes place in a transaction, we never
+          # actually get to the `after_commit` hook that queues these jobs.
+          expect { move_service.execute(old_issue, new_project) }
+            .not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError
+        end
+      end
     end
 
     describe 'move permissions' do