Merge pull request #37251 from kamipo/fix_after_commit_callback

Rollback in `after_commit` should not rollback state that already been succeeded

activerecord/lib/active_record/transactions.rb

@@ -340,6 +340,7 @@ def before_committed! # :nodoc:
     # 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:
+      force_clear_transaction_record_state
       if should_run_callbacks && (destroyed? || persisted?)
         @_committed_already_called = true
         _run_commit_without_transaction_enrollment_callbacks
@@ -347,7 +348,6 @@ def committed!(should_run_callbacks: true) #:nodoc:
       end
     ensure
       @_committed_already_called = false
-      force_clear_transaction_record_state
     end
 
     # Call the #after_rollback callbacks. The +force_restore_state+ argument indicates if the record

activerecord/test/cases/transaction_callbacks_test.rb

@@ -350,6 +350,24 @@ def test_after_rollback_callback_should_not_swallow_errors_when_set_to_raise
     end
   end
 
+  def test_after_commit_callback_should_not_rollback_state_that_already_been_succeeded
+    klass = Class.new(TopicWithCallbacks) do
+      self.inheritance_column = nil
+      validates :title, presence: true
+      def self.name; "TopicWithCallbacks"; end
+    end
+
+    first = klass.new(title: "foo")
+    first.after_commit_block { |r| r.update(title: nil) if r.persisted? }
+    first.save!
+
+    assert_predicate first, :persisted?
+    assert_not_nil first.id
+  ensure
+    first.destroy!
+  end
+  uses_transaction :test_after_commit_callback_should_not_rollback_state_that_already_been_succeeded
+
   def test_after_rollback_callback_when_raise_should_restore_state
     error_class = Class.new(StandardError)