fix: transaction state on rollback (#364)

Fixes #258

Author: BuonOmo

.github/workflows/ci.yml

@@ -88,4 +88,7 @@ jobs:
           done
           cat ${{ github.workspace }}/setup.sql | cockroach sql --insecure
       - name: Test
-        run: bundle exec rake test TESTOPTS='--profile=5'
+        run: bundle exec rake test
+        env:
+          TESTOPTS: "--profile=5"
+          RAILS_MINITEST_PLUGIN: "1" # Make sure that we use the minitest plugin for profiling.

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Ongoing
 
+- Fix transaction state on rollback ([#364](https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/364))
+
 ## 7.2.0 - 2024-09-24
 
 - Add support for Rails 7.2 ([#337](https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/337))

lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb

@@ -45,6 +45,27 @@ def within_new_transaction(isolation: nil, joinable: true, attempts: 0)
           within_new_transaction(isolation: isolation, joinable: joinable, attempts: attempts + 1) { yield }
         end
 
+        # OVERRIDE: the `rescue ActiveRecord::StatementInvalid` block is new, see comment.
+        def rollback_transaction(transaction = nil)
+          @connection.lock.synchronize do
+            transaction ||= @stack.last
+            begin
+              transaction.rollback
+            rescue ActiveRecord::StatementInvalid => err
+              # This is important to make Active Record aware the record was not inserted/saved
+              # Otherwise Active Record will assume save was successful and it doesn't retry the transaction
+              # See this thread for more details:
+              # https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/258#issuecomment-2256633329
+              transaction.rollback_records if err.cause.is_a?(PG::NoActiveSqlTransaction)
+
+              raise
+            ensure
+              @stack.pop if @stack.last == transaction
+            end
+            transaction.rollback_records
+          end
+        end
+
         def retryable?(error)
           return true if serialization_error?(error)
           return true if error.is_a? ActiveRecord::SerializationFailure

test/cases/transactions_test.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require "cases/helper_cockroachdb"
+
+module CockroachDB
+  class TransactionsTest < ActiveRecord::TestCase
+    self.use_transactional_tests = false
+
+    class Avenger < ActiveRecord::Base
+      singleton_class.attr_accessor :cyclic_barrier
+
+      validate :validate_unique_username
+
+      def validate_unique_username
+        self.class.cyclic_barrier.wait
+        duplicate = self.class.where(name: name).any?
+        errors.add("Duplicate username!") if duplicate
+      end
+    end
+
+    def test_concurrent_insert_with_processes
+      conn = ActiveRecord::Base.lease_connection
+      conn.create_table :avengers, force: true do |t|
+        t.string :name
+      end
+      ActiveRecord::Base.reset_column_information
+
+      avengers = %w[Hulk Thor Loki]
+      Avenger.cyclic_barrier = Concurrent::CyclicBarrier.new(avengers.size - 1)
+      Thread.current[:name] = "Main" # For debug logs.
+
+      assert_queries_match(/ROLLBACK/) do # Ensure we are properly testing the retry mechanism.
+        avengers.map do |name|
+          Thread.fork do
+            Thread.current[:name] = name # For debug logs.
+            Avenger.create!(name: name)
+          end
+        end.each(&:join)
+      end
+
+      assert_equal avengers.size, Avenger.count
+    ensure
+      Thread.current[:name] = nil
+      conn = ActiveRecord::Base.lease_connection
+      conn.drop_table :avengers, if_exists: true
+    end
+  end
+end