Rails 8.0+ transaction materialization breaks other tests #1212

Activity

1. headius commented on May 28, 2026

   

   headius

   on May 28, 2026

   MemberAuthor

   More actions

   My way of disabling the test:

   diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb
   index 5bd1a06804..e9283264b6 100644
   --- a/activerecord/test/cases/adapter_test.rb
   +++ b/activerecord/test/cases/adapter_test.rb
   @@ -510,7 +510,7 @@ def reset_fixtures(*fixture_names)
          end
      end
    
   -  class AdapterConnectionTest < ActiveRecord::TestCase
   +  false && class AdapterConnectionTest < ActiveRecord::TestCase
        unless in_memory_db?
          self.use_transactional_tests = false
    
   
2. headius commented on May 28, 2026

   

   headius

   on May 28, 2026

   MemberAuthor

   More actions

   The MySQL tests run even better than the SQLite tests with this block disabled:

   8831 runs, 29971 assertions, 37 failures, 79 errors, 44 skips
   
3. ryudoawaru commented on Jul 27, 2026

   

   ryudoawaru

   on Jul 27, 2026

   More actions

   I think I've found the root cause. It's not the materialize_transactions logic itself — it's Rails' raw_transaction_open? test helper colliding with a legacy method alias on the JDBC raw connection.

   The collision

   The helper probes for an open transaction on SQLite like this (adapter_test.rb):

   when "SQLite"
     begin
       connection.instance_variable_get(:@raw_connection).transaction { nil }
       false
     rescue
       true
     end

   On MRI, SQLite3::Database#transaction with a block is BEGIN → yield → COMMIT — a net-zero probe that raises cannot start a transaction within a transaction when one is already open. Neat.

   Under AR-JDBC, @raw_connection is a SQLite3JdbcConnection, and its transaction method is something else entirely: a legacy alias of begin dating back to AR 4.0 support (RubyJdbcConnection.java#L335-L344). Since the Java method doesn't take a block, the block is silently ignored. So the probe:

   • with no transaction open: calls setAutoCommit(false) — opening a real transaction that nothing ever commits — and returns the "right" answer while leaving a zombie transaction behind;
   • with a transaction open: hits if (getAutoCommit()) → no-op, no exception, so the helper reports "no transaction" (this is why the assertions in that block also fail directly).

   Reproduction (72-stable, AR 7.2.3.1, jruby-9.4.14.0 — the alias and the helper are identical on master / Rails 8):

   raw = ActiveRecord::Base.connection.instance_variable_get(:@raw_connection)
   java = raw.jdbc_connection(true)
   
   java.auto_commit          # => true
   raw.transaction { :ran }  # => nil   (block ignored; MRI returns the block value)
   java.auto_commit          # => false (zombie transaction is now open)
   raw.transaction { nil }   # => nil   (no exception; MRI raises "cannot start a transaction within a transaction")
   
   # everything that follows silently lands inside the zombie tx — DDL included:
   conn.execute("CREATE TABLE poison_check (id integer)")
   conn.execute("INSERT INTO poison_check (id) VALUES (1)")
   raw.rollback
   conn.select_value("SELECT COUNT(*) FROM poison_check")
   # => SQLiteException: no such table: poison_check  (the rollback erased the CREATE TABLE)

   That last part is the cascade mechanism: after the probe runs, every subsequent statement on that pooled connection — fixture loads, schema changes, the transactional-test wrapper — runs inside a transaction Rails doesn't know about, and the next stray rollback/commit lands in the wrong place. Hence order-dependent failures in the thousands, and why disabling the whole AdapterConnectionTest block stabilizes the suite.

   Proposed fix

   Give the SQLite JDBC connection a real transaction method with SQLite3::Database semantics (block: begin → yield → commit, rollback + re-raise on error; raise when already in a transaction), shadowing the legacy alias — same shim approach as #1220 took for the PG branch of this helper (transaction_status / PG::PQTRANS_*). Removing the "transaction" alias from the base class outright would also work but risks breaking external callers, so I'd keep that separate.

   Happy to prepare a PR if this direction sounds right.
4. headius commented on Aug 6, 2026

   

   headius

   on Aug 6, 2026

   MemberAuthor

   More actions

   @ryudoawaru Note there were some related fixes for transaction management in #1218 from @skunkworker.

   I see what you see and yes, the transaction method should be split off and handle a passed block appropriately. Go ahead and make a PR for that.