Should attempt `committed!`/`rolledback!` to all enrolled records in the transaction

Currently, `committed!`/`rolledback!` will only be attempted for the
first enrolled record in the transaction, that will cause some
problematic behaviors.

The first one problem, `clear_transaction_record_state` won't be called
even if the transaction is finalized except the first enrolled record.
This means that de-duplicated records in the transaction won't refer
latest state (e.g. won't happen rolling back record state).

The second one problem, the enrolled order is not always the same as the
order in which the actions actually happened, the first enrolled record
may succeed no actions (e.g. `destroy` has already succeeded on another
record during `before_destroy`), it will lose to fire any transactional
callbacks.

To avoid both problems, we should attempt `committed!`/`rolledback!` to
all enrolled records in the transaction.

4 files changed, 71 insertions, 18 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index c9e84e48cc..7b6321d83b 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -98,9 +98,13 @@ module ActiveRecord
       end
 
       def rollback_records
-        ite = records.uniq
+        ite = records.uniq(&:object_id)
+        already_run_callbacks = {}
         while record = ite.shift
-          record.rolledback!(force_restore_state: full_rollback?)
+          trigger_callbacks = record.trigger_transactional_callbacks?
+          should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
+          already_run_callbacks[record] ||= trigger_callbacks
+          record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks)
         end
       ensure
         ite.each do |i|
@@ -113,10 +117,14 @@ module ActiveRecord
       end
 
       def commit_records
-        ite = records.uniq
+        ite = records.uniq(&:object_id)
+        already_run_callbacks = {}
         while record = ite.shift
           if @run_commit_callbacks
-            record.committed!
+            trigger_callbacks = record.trigger_transactional_callbacks?
+            should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
+            already_run_callbacks[record] ||= trigger_callbacks
+            record.committed!(should_run_callbacks: should_run_callbacks)
           else
             # if not running callbacks, only adds the record to the parent transaction
             connection.add_transaction_record(record)
diff --git a/activerecord/lib/active_record/touch_later.rb b/activerecord/lib/active_record/touch_later.rb
index b60cc96165..6872b7844e 100644
--- a/activerecord/lib/active_record/touch_later.rb
+++ b/activerecord/lib/active_record/touch_later.rb
@@ -18,6 +18,7 @@ module ActiveRecord
 
       surreptitiously_touch @_defer_touch_attrs
       add_to_transaction
+      @_new_record_before_last_commit ||= false
 
       # touch the parents as we are not calling the after_save callbacks
       self.class.reflect_on_all_associations(:belongs_to).each do |r|
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 9e03deede7..148bc0550c 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -333,7 +333,7 @@ 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!(should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && trigger_transactional_callbacks?
+      if should_run_callbacks
         @_committed_already_called = true
         _run_commit_without_transaction_enrollment_callbacks
         _run_commit_callbacks
@@ -346,7 +346,7 @@ module ActiveRecord
     # 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, should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && trigger_transactional_callbacks?
+      if should_run_callbacks
         _run_rollback_callbacks
         _run_rollback_without_transaction_enrollment_callbacks
       end
@@ -378,6 +378,11 @@ module ActiveRecord
       status
     end
 
+    def trigger_transactional_callbacks? # :nodoc:
+      (@_new_record_before_last_commit || _trigger_update_callback) && persisted? ||
+        _trigger_destroy_callback && destroyed?
+    end
+
     private
       attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback
 
@@ -392,10 +397,7 @@ module ActiveRecord
           level: 0
         }
         @_start_transaction_state[:level] += 1
-        remember_new_record_before_last_commit
-      end
 
-      def remember_new_record_before_last_commit
         if _committed_already_called
           @_new_record_before_last_commit = false
         else
@@ -457,10 +459,6 @@ module ActiveRecord
         self.class.connection.add_transaction_record(self)
       end
 
-      def trigger_transactional_callbacks?
-        @_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback
-      end
-
       def has_transactional_callbacks?
         !_rollback_callbacks.empty? || !_commit_callbacks.empty? || !_before_commit_callbacks.empty?
       end
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index 135e2cb382..100bd6a925 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -551,6 +551,8 @@ class CallbacksOnMultipleActionsTest < ActiveRecord::TestCase
 end
 
 class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
+  self.use_transactional_tests = false
+
   class TopicWithHistory < ActiveRecord::Base
     self.table_name = :topics
 
@@ -564,11 +566,22 @@ class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
   end
 
   class TopicWithCallbacksOnDestroy < TopicWithHistory
-    after_commit(on: :destroy) { |record| record.class.history << :destroy }
+    after_commit(on: :destroy) { |record| record.class.history << :commit_on_destroy }
+    after_rollback(on: :destroy) { |record| record.class.history << :rollback_on_destroy }
+
+    before_destroy :before_destroy_for_transaction
+
+    private
+      def before_destroy_for_transaction; end
   end
 
   class TopicWithCallbacksOnUpdate < TopicWithHistory
-    after_commit(on: :update) { |record| record.class.history << :update }
+    after_commit(on: :update) { |record| record.class.history << :commit_on_update }
+
+    before_save :before_save_for_transaction
+
+    private
+      def before_save_for_transaction; end
   end
 
   def test_trigger_once_on_multiple_deletions
@@ -576,10 +589,39 @@ class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
     topic = TopicWithCallbacksOnDestroy.new
     topic.save
     topic_clone = TopicWithCallbacksOnDestroy.find(topic.id)
+
+    topic.define_singleton_method(:before_destroy_for_transaction) do
+      topic_clone.destroy
+    end
+
     topic.destroy
-    topic_clone.destroy
 
-    assert_equal [:destroy], TopicWithCallbacksOnDestroy.history
+    assert_equal [:commit_on_destroy], TopicWithCallbacksOnDestroy.history
+  end
+
+  def test_rollback_on_multiple_deletions
+    TopicWithCallbacksOnDestroy.clear_history
+    topic = TopicWithCallbacksOnDestroy.new
+    topic.save
+    topic_clone = TopicWithCallbacksOnDestroy.find(topic.id)
+
+    topic.define_singleton_method(:before_destroy_for_transaction) do
+      topic_clone.update!(author_name: "Test Author Clone")
+      topic_clone.destroy
+    end
+
+    TopicWithCallbacksOnDestroy.transaction do
+      topic.update!(author_name: "Test Author")
+      topic.destroy
+      raise ActiveRecord::Rollback
+    end
+
+    assert_not_predicate topic, :destroyed?
+    assert_not_predicate topic_clone, :destroyed?
+    assert_equal [nil, "Test Author"], topic.author_name_change_to_be_saved
+    assert_equal [nil, "Test Author Clone"], topic_clone.author_name_change_to_be_saved
+
+    assert_equal [:rollback_on_destroy], TopicWithCallbacksOnDestroy.history
   end
 
   def test_trigger_on_update_where_row_was_deleted
@@ -587,7 +629,11 @@ class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
     topic = TopicWithCallbacksOnUpdate.new
     topic.save
     topic_clone = TopicWithCallbacksOnUpdate.find(topic.id)
-    topic.destroy
+
+    topic_clone.define_singleton_method(:before_save_for_transaction) do
+      topic.destroy
+    end
+
     topic_clone.author_name = "Test Author"
     topic_clone.save