Active Record commit transaction on return, break and throw

Fix: https://github.com/rails/rails/issues/45017
Ref: https://github.com/rails/rails/pull/29333
Ref: https://github.com/ruby/timeout/pull/30

Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library
started using `throw` to interupt execution which had the adverse effect of committing open transactions.

To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer
than to potentially commit an incomplete transaction.

Using `return`, `break` or `throw` inside a `transaction` block was essentially deprecated from Rails 6.1 onwards.

However with the release of `timeout 0.4.0`, `Timeout.timeout` now raises an error again, and Active Record is able
to return to its original, less surprising, behavior.

Gemfile.lock

@@ -74,6 +74,7 @@ PATH
     activerecord (7.1.0.alpha)
       activemodel (= 7.1.0.alpha)
       activesupport (= 7.1.0.alpha)
+      timeout (>= 0.4.0)
     activestorage (7.1.0.alpha)
       actionpack (= 7.1.0.alpha)
       activejob (= 7.1.0.alpha)
@@ -334,7 +335,7 @@ GEM
     mysql2 (0.5.4)
     net-http-persistent (4.0.1)
       connection_pool (~> 2.2)
-    net-imap (0.3.4)
+    net-imap (0.3.6)
       date
       net-protocol
     net-pop (0.1.2)
@@ -511,7 +512,7 @@ GEM
     terser (1.1.13)
       execjs (>= 0.3.0, < 3)
     thor (1.2.2)
-    timeout (0.3.2)
+    timeout (0.4.0)
     tomlrb (2.0.3)
     trailblazer-option (0.1.2)
     turbo-rails (1.3.2)

activerecord/CHANGELOG.md

@@ -1,3 +1,34 @@
+*   Bring back the historical behavior of committing transaction on non-local return.
+
+    ```ruby
+    Model.transaction do
+      model.save
+      return
+      other_model.save # not executed
+    end
+    ```
+
+    Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library
+    started using `throw` to interrupt execution which had the adverse effect of committing open transactions.
+
+    To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer
+    than to potentially commit an incomplete transaction.
+
+    Using `return`, `break` or `throw` inside a `transaction` block was essentially deprecated from Rails 6.1 onwards.
+
+    However with the release of `timeout 0.4.0`, `Timeout.timeout` now raises an error again, and Active Record is able
+    to return to its original, less surprising, behavior.
+
+    This historical behavior can now be opt-ed in via:
+
+    ```
+    Rails.application.config.active_record.commit_transaction_on_non_local_return = true
+    ```
+
+    And is the default for new applications created in Rails 7.1.
+
+    *Jean Boussier*
+
 *   Deprecate `name` argument on `#remove_connection`.
 
     The `name` argument is deprecated on `#remove_connection` without replacement. `#remove_connection` should be called directly on the class that established the connection.

activerecord/activerecord.gemspec

@@ -37,4 +37,5 @@
 
   s.add_dependency "activesupport", version
   s.add_dependency "activemodel",   version
+  s.add_dependency "timeout", ">= 0.4.0"
 end

activerecord/lib/active_record.rb

@@ -320,6 +320,9 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :run_after_transaction_callbacks_in_order_defined
   self.run_after_transaction_callbacks_in_order_defined = false
 
+  singleton_class.attr_accessor :commit_transaction_on_non_local_return
+  self.commit_transaction_on_non_local_return = false
+
   ##
   # :singleton-method:
   # Specify a threshold for the size of query result sets. If the number of

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

@@ -90,7 +90,7 @@ def invalidate!; end
 
     class Transaction # :nodoc:
       attr_reader :connection, :state, :savepoint_name, :isolation_level
-      attr_accessor :written, :written_indirectly
+      attr_accessor :written
 
       delegate :invalidate!, :invalidated?, to: :@state
 
@@ -463,10 +463,6 @@ def commit_transaction
 
           dirty_current_transaction if transaction.dirty?
 
-          if current_transaction.open?
-            current_transaction.written_indirectly ||= transaction.written || transaction.written_indirectly
-          end
-
           transaction.commit
           transaction.commit_records
         end
@@ -498,30 +494,25 @@ def within_new_transaction(isolation: nil, joinable: true)
             raise
           ensure
             unless error
+              # In 7.1 we enforce timeout >= 0.4.0 which no longer use throw, so we can
+              # go back to the original behavior of committing on non-local return.
+              # If users are using throw, we assume it's not an error case.
+              completed = true if ActiveRecord.commit_transaction_on_non_local_return
+
               if Thread.current.status == "aborting"
                 rollback_transaction
               elsif !completed && transaction.written
-                # This was deprecated in 6.1, and has now changed to a rollback
-                rollback_transaction
-              elsif !completed && !transaction.written_indirectly
-                # This was a silent commit in 6.1, but now becomes a rollback; we skipped
-                # the warning because (having not been written) the change generally won't
-                # have any effect
+                ActiveRecord.deprecator.warn(<<~EOW)
+                  A transaction is being rolled back because the transaction block was
+                  exited using `return`, `break` or `throw`.
+                  In Rails 7.2 this transaction will be committed instead.
+                  To opt-in to the new behavior now and suppress this warning
+                  you can set:
+
+                    Rails.application.config.active_record.commit_transaction_on_non_local_return = true
+                EOW
                 rollback_transaction
               else
-                if !completed && transaction.written_indirectly
-                  # This is the case that was missed in the 6.1 deprecation, so we have to
-                  # do it now
-                  ActiveRecord.deprecator.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 ActiveRecord::ConnectionFailed

activerecord/test/cases/transactions_test.rb

@@ -17,6 +17,11 @@ class TransactionTest < ActiveRecord::TestCase
 
   def setup
     @first, @second = Topic.find(1, 2).sort_by(&:id)
+    @commit_transaction_on_non_local_return_was = ActiveRecord.commit_transaction_on_non_local_return
+  end
+
+  def teardown
+    ActiveRecord.commit_transaction_on_non_local_return = @commit_transaction_on_non_local_return_was
   end
 
   def test_rollback_dirty_changes
@@ -270,7 +275,7 @@ def test_successful_with_return_outside_inner_transaction
       end
     end
 
-    assert_deprecated(ActiveRecord.deprecator) do
+    assert_not_deprecated(ActiveRecord.deprecator) do
       transaction_with_shallow_return
     end
     assert committed
@@ -285,7 +290,7 @@ def test_successful_with_return_outside_inner_transaction
   end
 
   def test_deprecation_on_ruby_timeout_outside_inner_transaction
-    assert_deprecated(ActiveRecord.deprecator) do
+    assert_not_deprecated(ActiveRecord.deprecator) do
       catch do |timeout|
         Topic.transaction do
           Topic.transaction(requires_new: true) do
@@ -312,7 +317,9 @@ def test_rollback_with_return
       end
     end
 
-    transaction_with_return
+    assert_deprecated(ActiveRecord.deprecator) do
+      transaction_with_return
+    end
     assert_not committed
 
     assert_not_predicate Topic.find(1), :approved?
@@ -325,24 +332,76 @@ def test_rollback_with_return
   end
 
   def test_rollback_on_ruby_timeout
-    catch do |timeout|
-      Topic.transaction do
-        @first.approved = true
-        @first.save!
+    assert_deprecated(ActiveRecord.deprecator) do
+      catch do |timeout|
+        Topic.transaction do
+          @first.approved = true
+          @first.save!
 
-        throw timeout
+          throw timeout
+        end
       end
     end
 
     assert_not_predicate Topic.find(1), :approved?
   end
 
-  def test_early_return_from_transaction
-    assert_not_deprecated(ActiveRecord.deprecator) do
-      @first.with_lock do
-        break
+  def test_break_from_transaction_7_1_behavior
+    ActiveRecord.commit_transaction_on_non_local_return = true
+
+    @first.transaction do
+      assert_not_predicate @first, :approved?
+      @first.update!(approved: true)
+
+      break if true
+
+      # dead code
+      assert_predicate @first, :approved?
+      @first.update!(approved: false)
+    end
+
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    assert_predicate Topic.find(2), :approved?, "Second should have been approved"
+  end
+
+  def test_thow_from_transaction_7_1_behavior
+    ActiveRecord.commit_transaction_on_non_local_return = true
+
+    catch(:not_an_error) do
+      @first.transaction do
+        assert_not_predicate @first, :approved?
+        @first.update!(approved: true)
+
+        throw :not_an_error
+
+        # dead code
+        assert_predicate @first, :approved?
+        @first.update!(approved: false)
       end
     end
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    assert_predicate Topic.find(2), :approved?, "Second should have been approved"
+  end
+
+  def _test_return_from_transaction_7_1_behavior
+    @first.transaction do
+      assert_not_predicate @first, :approved?
+      @first.update!(approved: true)
+
+      return if true
+
+      # dead code
+      assert_predicate @first, :approved?
+      @first.update!(approved: false)
+    end
+  end
+
+  def test_return_from_transaction_7_1_behavior
+    ActiveRecord.commit_transaction_on_non_local_return = true
+
+    _test_return_from_transaction_7_1_behavior
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    assert_predicate Topic.find(2), :approved?, "Second should have been approved"
   end
 
   def test_number_of_transactions_in_commit

guides/source/configuring.md

@@ -69,6 +69,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.active_record.allow_deprecated_singular_associations_name`](#config-active-record-allow-deprecated-singular-associations-name): `false`
 - [`config.active_record.before_committed_on_all_records`](#config-active-record-before-committed-on-all-records): `true`
 - [`config.active_record.belongs_to_required_validates_foreign_key`](#config-active-record-belongs-to-required-validates-foreign-key): `false`
+- [`config.active_record.commit_transaction_on_non_local_return`](#config-active-record-commit-transaction-on-non-local-return): `true`
 - [`config.active_record.default_column_serializer`](#config-active-record-default-column-serializer): `nil`
 - [`config.active_record.encryption.hash_digest_class`](#config-active-record-encryption-hash-digest-class): `OpenSSL::Digest::SHA256`
 - [`config.active_record.encryption.support_sha1_for_non_deterministic_encryption`](#config-active-record-encryption-support-sha1-for-non-deterministic-encryption): `false`
@@ -1274,6 +1275,39 @@ The default value depends on the `config.load_defaults` target version:
 | (original)            | `false`              |
 | 7.0                   | `true`               |
 
+#### `config.active_record.commit_transaction_on_non_local_return`
+
+Defines whether `return`, `break` and `throw` inside a `transaction` block cause the transaction to be
+committed or rolled back. e.g.:
+
+```ruby
+Model.transaction do
+  model.save
+  return
+  other_model.save # not executed
+end
+```
+
+If set to `false`, it will be rolled back.
+
+If set to `true`, the above transaction will be committed.
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `false`              |
+| 7.1                   | `true`               |
+
+Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library
+started using `throw` to interrupt execution which had the adverse effect of committing open transactions.
+
+To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer
+than to potentially commit an incomplete transaction.
+
+Using `return`, `break` or `throw` inside a `transaction` block was essentially deprecated from Rails 6.1 onwards.
+
+However with the release of `timeout 0.4.0`, `Timeout.timeout` now raises an error again, and Active Record is able
+to return to its original, less surprising, behavior.
+
 #### `config.active_record.raise_on_assign_to_attr_readonly`
 
 Enable raising on assignment to attr_readonly attributes. The previous

railties/lib/rails/application/configuration.rb

@@ -276,6 +276,7 @@ def load_defaults(target_version)
 
           if respond_to?(:active_record)
             active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false
+            active_record.commit_transaction_on_non_local_return = true
             active_record.allow_deprecated_singular_associations_name = false
             active_record.sqlite3_adapter_strict_strings_by_default = true
             active_record.query_log_tags_format = :sqlcommenter

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -170,6 +170,10 @@
 # In previous versions of Rails, they ran in the inverse order.
 # Rails.application.config.active_record.run_after_transaction_callbacks_in_order_defined = true
 
+# Whether a `transaction` block is committed or rolled back when exited via `return`, `break` or `throw`.
+#
+# Rails.application.config.active_record.commit_transaction_on_non_local_return = true
+
 # ** Please read carefully, this must be configured in config/application.rb **
 # Change the format of the cache entry.
 # Changing this default means that all new cache entries added to the cache