Add option to stop swallowing errors on callbacks.

Currently, Active Record will rescue any errors raised within
after_rollback/after_create callbacks and print them to the
logs. Next versions of rails will not rescue those errors anymore,
and just bubble them up, as the other callbacks.

This adds a opt-in flag to enable that behaviour, of not rescuing
the errors.
    Example:
      # For not swallow errors in after_commit/after_rollback
      config.active_record.errors_in_transactional_callbacks = true

[fixes #13460]

2 files changed, 45 insertions, 11 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index 8f06cf3a1f..90be835d8a 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -22,6 +22,10 @@ module ActiveRecord
         @state == :rolledback
       end
 
+      def completed?
+        committed? || rolledback?
+      end
+
       def set_state(state)
         if !VALID_STATES.include?(state)
           raise ArgumentError, "Invalid transaction state: #{state}"
@@ -63,13 +67,19 @@ module ActiveRecord
       end
 
       def rollback_records
-        records.uniq.each do |record|
+        ite = records.uniq
+        while record = ite.shift
           begin
             record.rolledback! full_rollback?
           rescue => e
+            raise if ActiveRecord::Base.raise_in_transactional_callbacks
             record.logger.error(e) if record.respond_to?(:logger) && record.logger
           end
         end
+      ensure
+        ite.each do |i|
+          i.rolledback!(full_rollback?, false)
+        end
       end
 
       def commit
@@ -77,13 +87,19 @@ module ActiveRecord
       end
 
       def commit_records
-        records.uniq.each do |record|
+        ite = records.uniq
+        while record = ite.shift
           begin
             record.committed!
           rescue => e
+            raise if ActiveRecord::Base.raise_in_transactional_callbacks
             record.logger.error(e) if record.respond_to?(:logger) && record.logger
           end
         end
+      ensure
+        ite.each do |i|
+          i.committed!(false)
+        end
       end
 
       def full_rollback?; true; end
@@ -103,14 +119,14 @@ module ActiveRecord
       end
 
       def rollback
-        super
         connection.rollback_to_savepoint(savepoint_name)
+        super
         rollback_records
       end
 
       def commit
-        super
         connection.release_savepoint(savepoint_name)
+        super
         parent = connection.transaction_manager.current_transaction
         records.each { |r| parent.add_record(r) }
       end
@@ -130,14 +146,14 @@ module ActiveRecord
       end
 
       def rollback
-        super
         connection.rollback_db_transaction
+        super
         rollback_records
       end
 
       def commit
-        super
         connection.commit_db_transaction
+        super
         commit_records
       end
     end
@@ -177,7 +193,7 @@ module ActiveRecord
         begin
           commit_transaction unless error
         rescue Exception
-          transaction.rollback
+          transaction.rollback unless transaction.state.completed?
           raise
         end
       end
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 7e4dc4c895..9a56291b35 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -3,11 +3,23 @@ module ActiveRecord
   module Transactions
     extend ActiveSupport::Concern
     ACTIONS = [:create, :destroy, :update]
+    CALLBACK_WARN_MESSAGE = <<-EOF
+Currently, Active Record will rescue any errors raised within
+after_rollback/after_create callbacks and print them to the logs. In the next
+version, these errors will no longer be rescued. Instead, they will simply
+bubble just like other Active Record callbacks.
+
+You can opt into the new behavior and remove this warning by setting
+config.active_record.raise_in_transactional_callbacks to true.
+EOF
 
     included do
       define_callbacks :commit, :rollback,
                        terminator: ->(_, result) { result == false },
                        scope: [:kind, :name]
+
+      mattr_accessor :raise_in_transactional_callbacks, instance_writer: false
+      self.raise_in_transactional_callbacks = false
     end
 
     # = Active Record Transactions
@@ -223,6 +235,9 @@ module ActiveRecord
       def after_commit(*args, &block)
         set_options_for_callbacks!(args)
         set_callback(:commit, :after, *args, &block)
+        unless ActiveRecord::Base.raise_in_transactional_callbacks
+          ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE)
+        end
       end
 
       # This callback is called after a create, update, or destroy are rolled back.
@@ -231,6 +246,9 @@ module ActiveRecord
       def after_rollback(*args, &block)
         set_options_for_callbacks!(args)
         set_callback(:rollback, :after, *args, &block)
+        unless ActiveRecord::Base.raise_in_transactional_callbacks
+          ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE)
+        end
       end
 
       private
@@ -290,16 +308,16 @@ module ActiveRecord
     #
     # Ensure that it is not called if the object was never persisted (failed create),
     # but call it after the commit of a destroyed object.
-    def committed! #:nodoc:
-      run_callbacks :commit if destroyed? || persisted?
+    def committed!(should_run_callbacks = true) #:nodoc:
+      run_callbacks :commit if should_run_callbacks && destroyed? || persisted?
     ensure
       force_clear_transaction_record_state
     end
 
     # Call the +after_rollback+ callbacks. The +force_restore_state+ argument indicates if the record
     # state should be rolled back to the beginning or just to the last savepoint.
-    def rolledback!(force_restore_state = false) #:nodoc:
-      run_callbacks :rollback
+    def rolledback!(force_restore_state = false, should_run_callbacks = true) #:nodoc:
+      run_callbacks :rollback if should_run_callbacks
     ensure
       restore_transaction_record_state(force_restore_state)
       clear_transaction_record_state