diff --git a/app/models/concerns/async_devise_email.rb b/app/models/concerns/async_devise_email.rb
index 38c99dc7e713dbaacc282a7d7cc43d4da23b0364..7cdbed2eef6261cdd7388e6fd6b9879de1627c68 100644
--- a/app/models/concerns/async_devise_email.rb
+++ b/app/models/concerns/async_devise_email.rb
@@ -2,6 +2,7 @@
module AsyncDeviseEmail
extend ActiveSupport::Concern
+ include AfterCommitQueue
private
@@ -9,6 +10,8 @@ module AsyncDeviseEmail
def send_devise_notification(notification, *args)
return true unless can?(:receive_notifications)
- devise_mailer.__send__(notification, self, *args).deliver_later # rubocop:disable GitlabSecurity/PublicSend
+ run_after_commit_or_now do
+ devise_mailer.__send__(notification, self, *args).deliver_later # rubocop:disable GitlabSecurity/PublicSend
+ end
end
end
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 8618cdffbe459cce5bcd2cd4863d9a6fc5869887..112fcc389e3375a46c1f4fc37a6cc80a223d86c2 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -112,7 +112,10 @@ def post_update_hook
end
def after_accept_invite
- notification_service.accept_group_invite(self)
+ run_after_commit_or_now do
+ notification_service.accept_group_invite(self)
+ end
+
update_two_factor_requirement
super
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 995c26d7221c0fd24a32e6f57048c4107ef4f5d5..fe69d9f93825d3b89e3275608532e8c2eb4df39f 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -158,7 +158,9 @@ def post_destroy_hook
end
def after_accept_invite
- notification_service.accept_project_invite(self)
+ run_after_commit_or_now do
+ notification_service.accept_project_invite(self)
+ end
super
end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 57eaf27069dd5927c95675b714877fa3cc858d9f..9f08ff9d66d07653f1bbaaa82b91cf5a32b81ab0 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -231,7 +231,10 @@ class << self
# rubocop: disable CodeReuse/ServiceClass
after_transition [:unchecked, :checking] => :cannot_be_merged do |merge_request, transition|
if merge_request.notify_conflict?
- NotificationService.new.merge_request_unmergeable(merge_request)
+ merge_request.run_after_commit do
+ NotificationService.new.merge_request_unmergeable(merge_request)
+ end
+
TodoService.new.merge_request_became_unmergeable(merge_request)
end
end
diff --git a/app/models/user.rb b/app/models/user.rb
index ba187ddfc239347854d199511a142eb0cc3b3156..fcf1f8bfc5829f2d70c4426fb4532e3cbdb744e6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -414,7 +414,9 @@ def blocked?
after_transition any => :deactivated do |user|
next unless Gitlab::CurrentSettings.user_deactivation_emails_enabled
- NotificationService.new.user_deactivated(user.name, user.notification_email_or_default)
+ user.run_after_commit do
+ NotificationService.new.user_deactivated(user.name, user.notification_email_or_default)
+ end
end
# rubocop: enable CodeReuse/ServiceClass
diff --git a/app/services/projects/import_export/export_service.rb b/app/services/projects/import_export/export_service.rb
index 72492b6f5a5cd6a072a3af9c98d03bc5fca0e56b..d8d3542259016507c260c7b1f9b5788056c252f5 100644
--- a/app/services/projects/import_export/export_service.rb
+++ b/app/services/projects/import_export/export_service.rb
@@ -143,7 +143,12 @@ def notify_error
project_id: project.id
)
- notification_service.project_not_exported(project, current_user, shared.errors)
+ user = current_user
+ errors = shared.errors
+
+ project.run_after_commit_or_now do |project|
+ NotificationService.new.project_not_exported(project, user, errors)
+ end
end
end
end
diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb
index 2ea6c9a7343b80d1c3c2a0767e50176da29e0e48..c96524b824d60ed47fa43bc4c86cffe9e69b7214 100644
--- a/config/initializers/forbid_sidekiq_in_transactions.rb
+++ b/config/initializers/forbid_sidekiq_in_transactions.rb
@@ -16,19 +16,31 @@ def self.skip_transaction_check
Thread.current[:sidekiq_worker_skip_transaction_check]
end
+ def self.inside_transaction?
+ ::ApplicationRecord.inside_transaction? || ::Ci::ApplicationRecord.inside_transaction?
+ end
+
+ def self.raise_exception_for_being_inside_a_transaction?
+ !skip_transaction_check && inside_transaction?
+ end
+
+ def self.raise_inside_transaction_exception(cause:)
+ raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG
+ #{cause} cannot be enqueued 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.
+
+ Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead.
+ MSG
+ end
+
module ClassMethods
module NoEnqueueingFromTransactions
%i(perform_async perform_at perform_in).each do |name|
define_method(name) do |*args|
- if !Sidekiq::Worker.skip_transaction_check && inside_transaction?
+ if Sidekiq::Worker.raise_exception_for_being_inside_a_transaction?
begin
- 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.
-
- Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead.
- MSG
+ Sidekiq::Worker.raise_inside_transaction_exception(cause: "#{self}.#{name}")
rescue Sidekiq::Worker::EnqueueFromTransactionError => e
Gitlab::AppLogger.error(e.message) if ::Rails.env.production?
Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
@@ -38,14 +50,50 @@ module NoEnqueueingFromTransactions
super(*args)
end
end
+ end
+
+ prepend NoEnqueueingFromTransactions
+ end
+ end
+end
- private
+# We deliver emails using the `deliver_later` method and it uses ActiveJob
+# under the hood, which later processes the email via the defined ActiveJob adapter's `enqueue` method.
+# For GitLab, the ActiveJob adapter is Sidekiq (in development and production environments).
+# We need to set the following up to override the ActiveJob adapater
+# so as to ensure that no mailer jobs are enqueued from within a transaction.
+module ActiveJob
+ module QueueAdapters
+ module NoEnqueueingFromTransactions
+ %i(enqueue enqueue_at).each do |name|
+ define_method(name) do |*args|
+ if Sidekiq::Worker.raise_exception_for_being_inside_a_transaction?
+ begin
+ job = args.first
+ Sidekiq::Worker.raise_inside_transaction_exception(
+ cause: "The #{job.class} job, enqueued into the queue: #{job.queue_name}"
+ )
+ rescue Sidekiq::Worker::EnqueueFromTransactionError => e
+ Gitlab::AppLogger.error(e.message) if ::Rails.env.production?
+ Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
+ end
+ end
- def inside_transaction?
- ::ApplicationRecord.inside_transaction? || ::Ci::ApplicationRecord.inside_transaction?
+ super(*args)
end
end
+ end
+
+ # This adapter is used in development & production environments.
+ class SidekiqAdapter
+ prepend NoEnqueueingFromTransactions
+ end
+ # This adapter is used in test environment.
+ # If we don't override the test environment adapter,
+ # we won't be seeing any failing jobs during the CI run,
+ # even if we enqueue mailers from within a transaction.
+ class TestAdapter
prepend NoEnqueueingFromTransactions
end
end
diff --git a/ee/app/models/ee/project_import_state.rb b/ee/app/models/ee/project_import_state.rb
index 2f649984cf698991d2b5bb8c7efc3f64390fd5c2..25b988105b3b5dd809bddbb2f387e7bb357e017c 100644
--- a/ee/app/models/ee/project_import_state.rb
+++ b/ee/app/models/ee/project_import_state.rb
@@ -32,7 +32,9 @@ module ProjectImportState
after_transition started: :failed do |state, _|
if state.mirror? && state.retry_limit_exceeded?
- ::NotificationService.new.mirror_was_hard_failed(state.project)
+ state.run_after_commit do
+ ::NotificationService.new.mirror_was_hard_failed(state.project)
+ end
end
end
diff --git a/spec/initializers/forbid_sidekiq_in_transactions_spec.rb b/spec/initializers/forbid_sidekiq_in_transactions_spec.rb
index 6cd15d37ad4606439e24147219c3c64c7607089b..a89ac73f6fadce9350ba839ec2a5585f19951a6c 100644
--- a/spec/initializers/forbid_sidekiq_in_transactions_spec.rb
+++ b/spec/initializers/forbid_sidekiq_in_transactions_spec.rb
@@ -3,36 +3,57 @@
require 'spec_helper'
RSpec.describe 'Sidekiq::Worker' do
- let(:worker_class) do
- Class.new do
- include Sidekiq::Worker
+ shared_examples_for 'a forbiddable operation within a transaction' do
+ it 'allows the operation outside of a transaction' do
+ expect { operation }.not_to raise_error
+ end
- def perform
+ it 'forbids the operation within a transaction' do
+ ApplicationRecord.transaction do
+ expect { operation }.to raise_error(Sidekiq::Worker::EnqueueFromTransactionError)
end
end
- end
- it 'allows sidekiq worker outside of a transaction' do
- expect { worker_class.perform_async }.not_to raise_error
- end
+ it 'allows the oepration within a transaction if skipped' do
+ Sidekiq::Worker.skipping_transaction_check do
+ ApplicationRecord.transaction do
+ expect { operation }.not_to raise_error
+ end
+ end
+ end
- it 'forbids queue sidekiq worker in a transaction' do
- Project.transaction do
- expect { worker_class.perform_async }.to raise_error(Sidekiq::Worker::EnqueueFromTransactionError)
+ it 'forbids the operation if it is within a Ci::ApplicationRecord transaction' do
+ Ci::Pipeline.transaction do
+ expect { operation }.to raise_error(Sidekiq::Worker::EnqueueFromTransactionError)
+ end
end
end
- it 'allows sidekiq worker in a transaction if skipped' do
- Sidekiq::Worker.skipping_transaction_check do
- Project.transaction do
- expect { worker_class.perform_async }.not_to raise_error
+ context 'for sidekiq workers' do
+ let(:worker_class) do
+ Class.new do
+ include Sidekiq::Worker
+
+ def perform
+ end
end
end
+
+ let(:operation) { worker_class.perform_async }
+
+ it_behaves_like 'a forbiddable operation within a transaction'
end
- it 'forbids queue sidekiq worker in a Ci::ApplicationRecord transaction' do
- Ci::Pipeline.transaction do
- expect { worker_class.perform_async }.to raise_error(Sidekiq::Worker::EnqueueFromTransactionError)
+ context 'for mailers' do
+ let(:mailer_class) do
+ Class.new(ApplicationMailer) do
+ def test_mail
+ end
+ end
end
+
+ let(:operation) { mailer_class.test_mail.deliver_later }
+
+ it_behaves_like 'a forbiddable operation within a transaction'
end
end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index b6ad66f41b50bdcbe13ba59e20e44e6d506eee19..a9796c28870b64f57ff9df206574e3252d1c0124 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -1858,7 +1858,7 @@ def invite_to_group(group, inviter:, user: nil, tasks_to_be_done: [])
end
end
- subject { ActionMailer::Base.deliveries.last }
+ subject { ActionMailer::Base.deliveries.first }
it_behaves_like 'an email sent from GitLab'
it_behaves_like "a user cannot unsubscribe through footer link"