Rollback transactions when the block returns earlier than expected

This behavior change is being announced via a deprecation message
since Rails 6.1 and now we are making it the default behavior.

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Rollback transactions when the block returns earlier than expected.
+
+    Before this change, when a transaction block returned early, the transaction would be committed.
+
+    The problem is that timeouts triggered inside the transaction block was also making the incomplete transaction
+    to be committed, so in order to avoid this mistake, the transaction block is rolled back.
+
+    *Rafael Mendonça França*
+
 *   Add middleware for automatic shard swapping.
 
     Provides a basic middleware to perform automatic shard swapping. Applications will provide a resolver which will determine for an individual request which shard should be used. Example:

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

@@ -333,26 +333,19 @@ def within_new_transaction(isolation: nil, joinable: true)
               # @connection still holds an open or invalid transaction, so we must not
               # put it back in the pool for reuse.
               @connection.throw_away! unless transaction.state.rolledback?
+            elsif Thread.current.status == "aborting" || (!completed && transaction.written)
+              # The transaction is still open but the block returned earlier.
+              #
+              # The block could return early because of a timeout or becase the thread is aborting,
+              # so we are rolling back to make sure the timeout didn't caused the transaction to be
+              # committed incompletely.
+              rollback_transaction
             else
-              if Thread.current.status == "aborting"
-                rollback_transaction
-              else
-                if !completed && transaction.written
-                  ActiveSupport::Deprecation.warn(<<~EOW)
-                    Using `return`, `break` or `throw` to exit a transaction block is
-                    deprecated without replacement. If the `throw` came from
-                    `Timeout.timeout(duration)`, pass an exception class as a second
-                    argument so it doesn't use `throw` to abort its block. This results
-                    in the transaction being committed, but in the next release of Rails
-                    it will rollback.
-                  EOW
-                end
-                begin
-                  commit_transaction
-                rescue Exception
-                  rollback_transaction(transaction) unless transaction.state.completed?
-                  raise
-                end
+              begin
+                commit_transaction
+              rescue Exception
+                rollback_transaction(transaction) unless transaction.state.completed?
+                raise
               end
             end
           end

activerecord/test/cases/transactions_test.rb

@@ -184,7 +184,7 @@ def test_add_to_null_transaction
     topic.send(:add_to_transaction)
   end
 
-  def test_successful_with_return
+  def test_rollback_with_return
     committed = false
 
     Topic.connection.class_eval do
@@ -195,33 +195,29 @@ def test_successful_with_return
       end
     end
 
-    assert_deprecated do
-      transaction_with_return
-    end
-    assert committed
+    transaction_with_return
+    assert_not committed
 
-    assert_predicate Topic.find(1), :approved?, "First should have been approved"
-    assert_not_predicate Topic.find(2), :approved?, "Second should have been unapproved"
+    assert_not_predicate Topic.find(1), :approved?
+    assert_predicate Topic.find(2), :approved?
   ensure
     Topic.connection.class_eval do
       remove_method :commit_db_transaction
       alias :commit_db_transaction :real_commit_db_transaction rescue nil
     end
   end
 
-  def test_deprecation_on_ruby_timeout
-    assert_deprecated do
-      catch do |timeout|
-        Topic.transaction do
-          @first.approved = true
-          @first.save!
+  def test_rollback_on_ruby_timeout
+    catch do |timeout|
+      Topic.transaction do
+        @first.approved = true
+        @first.save!
 
-          throw timeout
-        end
+        throw timeout
       end
     end
 
-    assert Topic.find(1).approved?, "First should have been approved"
+    assert_not_predicate Topic.find(1), :approved?
   end
 
   def test_early_return_from_transaction

guides/source/7_0_release_notes.md

@@ -128,6 +128,13 @@ Please refer to the [Changelog][active-record] for detailed changes.
 
 ### Notable changes
 
+*   Rollback transactions when the block returns earlier than expected.
+
+    Before this change, when a transaction block returned early, the transaction would be committed.
+
+    The problem is that timeouts triggered inside the transaction block was also making the incomplete transaction
+    to be committed, so in order to avoid this mistake, the transaction block is rolled back.
+
 Active Storage
 --------------