:email_changed notification fired before commit

[ClearlyClaire]

Environment

• Ruby 2.7
• Rails 5.2.4.2
• Devise 4.7.1

Current behavior

When changing an user's email address with reconfirmable set to true, two calls to send_devise_notification are made, one for :email_changed and one for :confirmation_instructions.

The first of those two is called within an after_update hook, meaning it is called after save has been called, but before being properly committed to the database.

When the notification is handled in a worker, the worker may load the model before the changes have been committed to the database, thus operating on old data.

The comments in

devise/lib/devise/models/authenticatable.rb

Lines 133 to 194 in 406915c

	# This is an internal method called every time Devise needs
	# to send a notification/mail. This can be overridden if you
	# need to customize the e-mail delivery logic. For instance,
	# if you are using a queue to deliver e-mails (active job, delayed
	# job, sidekiq, resque, etc), you must add the delivery to the queue
	# just after the transaction was committed. To achieve this,
	# you can override send_devise_notification to store the
	# deliveries until the after_commit callback is triggered.
	#
	# The following example uses Active Job's `deliver_later` :
	#
	# class User
	# devise :database_authenticatable, :confirmable
	#
	# after_commit :send_pending_devise_notifications
	#
	# protected
	#
	# def send_devise_notification(notification, *args)
	# # If the record is new or changed then delay the
	# # delivery until the after_commit callback otherwise
	# # send now because after_commit will not be called.
	# # For Rails < 6 is `changed?` instead of `saved_changes?`.
	# if new_record? || saved_changes?
	# pending_devise_notifications << [notification, args]
	# else
	# render_and_send_devise_message(notification, *args)
	# end
	# end
	#
	# private
	#
	# def send_pending_devise_notifications
	# pending_devise_notifications.each do |notification, args|
	# render_and_send_devise_message(notification, *args)
	# end
	#
	# # Empty the pending notifications array because the
	# # after_commit hook can be called multiple times which
	# # could cause multiple emails to be sent.
	# pending_devise_notifications.clear
	# end
	#
	# def pending_devise_notifications
	# @pending_devise_notifications ||= []
	# end
	#
	# def render_and_send_devise_message(notification, *args)
	# message = devise_mailer.send(notification, self, *args)
	#
	# # Deliver later with Active Job's `deliver_later`
	# if message.respond_to?(:deliver_later)
	# message.deliver_later
	# # Remove once we move to Rails 4.2+ only, as `deliver` is deprecated.
	# elsif message.respond_to?(:deliver_now)
	# message.deliver_now
	# else
	# message.deliver
	# end
	# end
	#
	# end

hint at a way to do that, but neither changed? nor saved_changes? actually differentiate between the after_update and the after_commit situation. Using one variant will make both mailers execute immediately, not solving the problem, and using the other variant will make both notifications be enqueued, but process the queue before the second notification gets queued.

Expected behavior

send_devise_notification should only be called in after_commit hooks, or, alternatively, the documentation should describe a way to properly handle the case where some notifications are sent from after_update hooks and others from after_commit hooks.

[Bandes]

I am late to the party, but I am running up against this problem as well and have yet to find a way to mitigate it.

[ClearlyClaire]

In Mastodon, we ended up checking whether we are in a transaction, if so queue the notifications, otherwise send them right away. The relevant pieces of code look like this:

class User < ApplicationRecord
  after_commit :send_pending_devise_notifications

  def send_devise_notification(notification, *args, **kwargs)
    # This method can be called in `after_update` and `after_commit` hooks,
    # but we must make sure the mailer is actually called *after* commit,
    # otherwise it may work on stale data. To do this, figure out if we are
    # within a transaction.

    # It seems like devise sends keyword arguments as a hash in the last
    # positional argument
    kwargs = args.pop if args.last.is_a?(Hash) && kwargs.empty?

    if ActiveRecord::Base.connection.current_transaction.try(:records)&.include?(self)
      pending_devise_notifications << [notification, args, kwargs]
    else
      render_and_send_devise_message(notification, *args, **kwargs)
    end
  end

  def send_pending_devise_notifications
    pending_devise_notifications.each do |notification, args, kwargs|
      render_and_send_devise_message(notification, *args, **kwargs)
    end

    # Empty the pending notifications array because the
    # after_commit hook can be called multiple times which
    # could cause multiple emails to be sent.
    pending_devise_notifications.clear
  end

  def pending_devise_notifications
    @pending_devise_notifications ||= []
  end
end