diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8974277..48a04612 100644 --- a/.github/workflows/ci.yml +++ b/.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=3' + run: bundle exec rake test + env: + TESTOPTS: "--profile=5" + RAILS_MINITEST_PLUGIN: "1" # Make sure that we use the minitest plugin for profiling. diff --git a/CHANGELOG.md b/CHANGELOG.md index da362d9e..240832f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Ongoing +- Fix transaction state on rollback ([#364](https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/364)) + ## 7.1.1 - 2024-05-01 - Enable precision support ([#318](https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/318)) diff --git a/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb b/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb index 17f2da7d..5993f8bd 100644 --- a/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb +++ b/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb @@ -31,6 +31,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 diff --git a/test/cases/transactions_test.rb b/test/cases/transactions_test.rb new file mode 100644 index 00000000..6f3a39d6 --- /dev/null +++ b/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