Simplify the implementation by only calling the checks in the right places

Gemfile.lock

@@ -524,6 +524,7 @@ GEM
     zeitwerk (2.4.2)
 
 PLATFORMS
+  ruby
   x86_64-darwin-19
   x86_64-darwin-20
   x86_64-linux

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Do not try to rollback transactions that failed due to a `ActiveRecord::TransactionRollbackError`.
+
+    *Jamie McCarthy*
+
 *   Active Record Encryption will now encode values as UTF-8 when using deterministic
     encryption. The encoding is part of the encrypted payload, so different encodings for
     different values result in different ciphertexts. This can break unique constraints and

activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

@@ -37,10 +37,6 @@ module ActiveRecord
         @state == :invalidated
       end
 
-      def uncommittable?
-        invalidated? || rolledback?
-      end
-
       def fully_completed?
         completed?
       end
@@ -59,6 +55,11 @@ module ActiveRecord
         @state = :fully_rolledback
       end
 
+      def invalidate!
+        @children&.each { |c| c.invalidate! }
+        @state = :invalidated
+      end
+
       def commit!
         @state = :committed
       end
@@ -67,11 +68,6 @@ module ActiveRecord
         @state = :fully_committed
       end
 
-      def invalidate!
-        @children.each { |c| c.invalidate! }
-        @state = :invalidated
-      end
-
       def nullify!
         @state = nil
       end
@@ -166,10 +162,6 @@ module ActiveRecord
         ite&.each { |i| i.committed!(should_run_callbacks: false) }
       end
 
-      def invalidate
-        @state.invalidate!
-      end
-
       def full_rollback?; true; end
       def joinable?; @joinable; end
       def closed?; false; end
@@ -195,14 +187,11 @@ module ActiveRecord
       end
 
       def rollback
-        connection.rollback_to_savepoint(savepoint_name) if materialized? && !state.invalidated?
+        connection.rollback_to_savepoint(savepoint_name) if materialized?
         @state.rollback!
       end
 
       def commit
-        if state.invalidated?
-          raise ActiveRecord::StatementInvalid, "cannot commit after transaction invalidated"
-        end
         connection.release_savepoint(savepoint_name) if materialized?
         @state.commit!
       end
@@ -222,14 +211,11 @@ module ActiveRecord
       end
 
       def rollback
-        connection.rollback_db_transaction if materialized? && !state.invalidated?
+        connection.rollback_db_transaction if materialized?
         @state.full_rollback!
       end
 
       def commit
-        if state.invalidated?
-          raise ActiveRecord::StatementInvalid, "cannot commit after transaction invalidated"
-        end
         connection.commit_db_transaction if materialized?
         @state.full_commit!
       end
@@ -307,9 +293,6 @@ module ActiveRecord
       def commit_transaction
         @connection.lock.synchronize do
           transaction = @stack.last
-          if transaction.state.invalidated?
-            raise ActiveRecord::StatementInvalid, "cannot commit after transaction invalidated"
-          end
 
           begin
             transaction.before_commit_records
@@ -342,12 +325,13 @@ module ActiveRecord
             rollback_transaction
             after_failure_actions(transaction, error)
           end
+
           raise
         ensure
           if transaction
             if error
-              # @connection still holds an open transaction, so we must not
+              # @connection still holds an open or invalid transaction, so we must not
-              # put it back in the pool for reuse
+              # put it back in the pool for reuse.
               @connection.throw_away! unless transaction.state.rolledback?
             else
               if Thread.current.status == "aborting"

activerecord/test/cases/adapters/mysql2/nested_deadlock_test.rb

@@ -13,12 +13,12 @@ module ActiveRecord
 
     setup do
       @abort, Thread.abort_on_exception = Thread.abort_on_exception, false
+      Thread.report_on_exception, @original_report_on_exception = false, Thread.report_on_exception
 
       @connection = ActiveRecord::Base.connection
       @connection.clear_cache!
 
-      @connection.drop_table "samples", if_exists: true
+      @connection.create_table("samples", force: true) do |t|
-      @connection.create_table("samples") do |t|
         t.integer "value"
       end
 
@@ -30,6 +30,7 @@ module ActiveRecord
       @connection.drop_table "samples", if_exists: true
 
       Thread.abort_on_exception = @abort
+      Thread.report_on_exception = @original_report_on_exception
     end
 
     test "deadlock correctly raises Deadlocked inside nested SavepointTransaction" do
@@ -45,7 +46,7 @@ module ActiveRecord
               Sample.transaction(requires_new: true) do
                 s1.lock!
                 barrier.wait
-                s2.update_attributes value: 1
+                s2.update value: 1
               end
             end
           end
@@ -55,16 +56,15 @@ module ActiveRecord
               Sample.transaction(requires_new: true) do
                 s2.lock!
                 barrier.wait
-                s1.update_attributes value: 2
+                s1.update value: 2
               end
             end
           ensure
             thread.join
           end
-
         rescue ActiveRecord::StatementInvalid => e
-          if /SAVEPOINT active_record_. does not exist: ROLLBACK TO SAVEPOINT/ =~ e.to_s
+          if /SAVEPOINT active_record_. does not exist/ =~ e.to_s
-            assert nil, "ROLLBACK TO SAVEPOINT query issued for savepoint that no longer exists due to deadlock: #{e}"
+            flunk "ROLLBACK TO SAVEPOINT query issued for savepoint that no longer exists due to deadlock: #{e}"
           else
             raise e
           end

activerecord/test/cases/adapters/postgresql/transaction_nested_test.rb

@@ -14,12 +14,15 @@ module ActiveRecord
 
     setup do
       @abort, Thread.abort_on_exception = Thread.abort_on_exception, false
+      Thread.report_on_exception, @original_report_on_exception = false, Thread.report_on_exception
 
       @connection = ActiveRecord::Base.connection
 
-      @connection.drop_table "samples", if_exists: true
+      @connection.transaction do
-      @connection.create_table("samples") do |t|
+        @connection.drop_table "samples", if_exists: true
-        t.integer "value"
+        @connection.create_table("samples") do |t|
+          t.integer "value"
+        end
       end
 
       Sample.reset_column_information
@@ -29,6 +32,7 @@ module ActiveRecord
       @connection.drop_table "samples", if_exists: true
 
       Thread.abort_on_exception = @abort
+      Thread.report_on_exception = @original_report_on_exception
     end
 
     test "unserializable transaction raises SerializationFailure inside nested SavepointTransaction" do
@@ -36,32 +40,30 @@ module ActiveRecord
         before = Concurrent::CyclicBarrier.new(2)
         after = Concurrent::CyclicBarrier.new(2)
 
-        begin
+        thread = Thread.new do
-          thread = Thread.new do
+          with_warning_suppression do
-            with_warning_suppression do
+            Sample.transaction(isolation: :serializable, requires_new: false) do
-              Sample.transaction(isolation: :serializable, requires_new: false) do
+              Sample.transaction(requires_new: true) do
-                Sample.transaction(requires_new: true) do
+                before.wait
-                  before.wait
+                Sample.create value: Sample.sum(:value)
-                  Sample.create value: Sample.sum(:value)
+                after.wait
-                  after.wait
-                end
               end
             end
           end
+        end
 
-          begin
+        begin
-            with_warning_suppression do
+          with_warning_suppression do
-              Sample.transaction(isolation: :serializable, requires_new: false) do
+            Sample.transaction(isolation: :serializable, requires_new: false) do
-                Sample.transaction(requires_new: true) do
+              Sample.transaction(requires_new: true) do
-                  before.wait
+                before.wait
-                  Sample.create value: Sample.sum(:value)
+                Sample.create value: Sample.sum(:value)
-                  after.wait
+                after.wait
-                end
               end
             end
-          ensure
-            thread.join
           end
+        ensure
+          thread.join
         end
       end
     end
@@ -74,35 +76,32 @@ module ActiveRecord
           s1 = Sample.create value: 1
           s2 = Sample.create value: 2
 
-          begin
+          thread = Thread.new do
-            thread = Thread.new do
+            Sample.transaction(requires_new: false) do
-              Sample.transaction(requires_new: false) do
+              Sample.transaction(requires_new: true) do
-                Sample.transaction(requires_new: true) do
+                s1.lock!
-                  s1.lock!
+                barrier.wait
-                  barrier.wait
+                s2.update value: 1
-                  s2.update_attributes value: 1
-                end
               end
             end
+          end
 
-            begin
+          begin
-              Sample.transaction(requires_new: false) do
+            Sample.transaction(requires_new: false) do
-                Sample.transaction(requires_new: true) do
+              Sample.transaction(requires_new: true) do
-                  s2.lock!
+                s2.lock!
-                  barrier.wait
+                barrier.wait
-                  s1.update_attributes value: 2
+                s1.update value: 2
-                end
               end
-            ensure
-              thread.join
             end
+          ensure
+            thread.join
           end
         end
       end
     end
 
     private
-
       def with_warning_suppression
         log_level = ActiveRecord::Base.connection.client_min_messages
         ActiveRecord::Base.connection.client_min_messages = "error"