Re-deprecate return/throw exits from transactions

We previously weren't warning about changes written in a nested
transaction.

Author: matthewd

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

@@ -87,7 +87,7 @@ def dirty!; end
 
     class Transaction # :nodoc:
       attr_reader :connection, :state, :savepoint_name, :isolation_level
-      attr_accessor :written
+      attr_accessor :written, :written_indirectly
 
       def initialize(connection, isolation: nil, joinable: true, run_commit_callbacks: false)
         @connection = connection
@@ -384,6 +384,10 @@ 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
@@ -420,19 +424,37 @@ 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 because 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
-              begin
-                commit_transaction
-              rescue Exception
-                rollback_transaction(transaction) unless transaction.state.completed?
-                raise
+              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
+                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
+                  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
               end
             end
           end

activerecord/test/cases/transactions_test.rb

@@ -183,11 +183,65 @@ def transaction_with_return
     end
   end
 
+  def transaction_with_shallow_return
+    Topic.transaction do
+      Topic.transaction(requires_new: true) do
+        @first.approved  = true
+        @second.approved = false
+        @first.save
+        @second.save
+      end
+      return
+    end
+  end
+
   def test_add_to_null_transaction
     topic = Topic.new
     topic.send(:add_to_transaction)
   end
 
+  def test_successful_with_return_outside_inner_transaction
+    committed = false
+
+    Topic.connection.class_eval do
+      alias :real_commit_db_transaction :commit_db_transaction
+      define_method(:commit_db_transaction) do
+        committed = true
+        real_commit_db_transaction
+      end
+    end
+
+    assert_deprecated do
+      transaction_with_shallow_return
+    end
+    assert committed
+
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    assert_not_predicate Topic.find(2), :approved?, "Second should have been unapproved"
+  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_outside_inner_transaction
+    assert_deprecated do
+      catch do |timeout|
+        Topic.transaction do
+          Topic.transaction(requires_new: true) do
+            @first.approved = true
+            @first.save!
+          end
+
+          throw timeout
+        end
+      end
+    end
+
+    assert Topic.find(1).approved?, "First should have been approved"
+  end
+
   def test_rollback_with_return
     committed = false