From 3b735454f3f5b9a6d4a32a643d7e8483ae6c448b Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Thu, 18 Jun 2026 18:43:32 -0700 Subject: [PATCH 1/6] Fix COMMIT/ROLLBACK retry data-loss and harden MySQL connection-loss handling Transaction control statements were defined in the shared ArJdbc::Abstract::TransactionSupport mixin with allow_retry: true. That is safe for the idempotent BEGIN/savepoint statements but dangerous for COMMIT/ROLLBACK: if the backend connection drops at commit time (pgbouncer reaping a connection, a network blip, a server restart), AR's with_raw_connection machinery reconnects, replays BEGIN (but not the data writes, which died with the old backend) and re-runs COMMIT against an empty transaction - reporting success while silently losing the writes. This became reachable on PostgreSQL once backend disconnects started being translated to a retryable ActiveRecord::ConnectionFailed. Override commit_db_transaction / exec_rollback_db_transaction in the PostgreSQL and MySQL adapters to force allow_retry: false (matching AR's native adapters); BEGIN and savepoints intentionally keep allow_retry: true. MySQL additionally lacked translation of dropped connections: a lost backend surfaced as a plain JDBCError (SQLState class 08 / errorCode 0), which AR's retry path never matched. Add a connection_lost? helper (SQLState 08*, server-gone vendor codes, message patterns, and wrapped SQLRecoverableException/SQLNonTransientConnectionException causes) and short-circuit translate_exception to ConnectionFailed. Also: cap i18n < 1.15.0 (that release needs Ruby 3.2+, which we can't assume across supported JRubies) and fix a timezone test in test/simple.rb where with_timezone_config must wrap Time.use_zone to avoid the global-state leak guard. Co-Authored-By: Claude Opus 4.8 --- Gemfile | 4 + lib/arjdbc/mysql/adapter.rb | 83 ++++++++++++++++ lib/arjdbc/postgresql/adapter.rb | 31 ++++++ test/db/mysql/commit_no_retry_test.rb | 42 ++++++++ test/db/mysql/connection_lost_test.rb | 108 +++++++++++++++++++++ test/db/postgresql/connection_lost_test.rb | 88 ++++++++++++++++- test/simple.rb | 8 +- 7 files changed, 358 insertions(+), 6 deletions(-) create mode 100644 test/db/mysql/commit_no_retry_test.rb create mode 100644 test/db/mysql/connection_lost_test.rb diff --git a/Gemfile b/Gemfile index 77394ce1d..57bdcd19f 100644 --- a/Gemfile +++ b/Gemfile @@ -54,6 +54,10 @@ else end end +# Cap i18n below 1.15.0: that release requires Ruby 3.2+, which we can't assume +# across the JRuby versions we support. +gem 'i18n', '< 1.15.0', require: nil + gem 'rake', require: nil group :test do diff --git a/lib/arjdbc/mysql/adapter.rb b/lib/arjdbc/mysql/adapter.rb index 276205b00..e72ba2187 100644 --- a/lib/arjdbc/mysql/adapter.rb +++ b/lib/arjdbc/mysql/adapter.rb @@ -203,6 +203,35 @@ def active? alias :reset! :reconnect! + # Commits the current database transaction. + # + # Overrides ArJdbc::Abstract::TransactionSupport to disable connection + # retries for COMMIT, matching ActiveRecord's native MySQL adapter + # (which uses `allow_retry: false`). Retrying a COMMIT after a connection + # failure is unsafe on a networked database: `with_raw_connection` would + # reconnect, replay an *empty* transaction (the original writes died with + # the dropped backend), COMMIT it successfully, and report success - + # silently losing the transaction's writes. + def commit_db_transaction + log('COMMIT', 'TRANSACTION') do + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| + conn.commit + end + end + end + + # Rolls back the current database transaction. + # + # Overrides ArJdbc::Abstract::TransactionSupport to match ActiveRecord's + # native MySQL adapter (`allow_retry: false`). + def exec_rollback_db_transaction + log('ROLLBACK', 'TRANSACTION') do + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| + conn.rollback + end + end + end + # Disconnects from the database if already connected. # Otherwise, this method does nothing. def disconnect! @@ -252,7 +281,42 @@ def jdbc_column_class ::ActiveRecord::ConnectionAdapters::MySQL::Column end + # MySQL / MariaDB surface a dropped server connection as a JDBC error in + # SQLState class 08 (connection exception) - most commonly 08S01 + # "Communications link failure" - or with one of the "server gone" vendor + # error codes. The driver may also wrap it in a recoverable / non-transient + # connection exception. None of these are caught by the message- and + # error-code-based cases below (which fall through to a plain JDBCError / + # StatementInvalid), so AR's with_raw_connection reconnect/retry machinery + # never kicks in. See https://dev.mysql.com/doc/connector-j/en/connector-j-reference-error-sqlstates.html + CONNECTION_FAILURE_SQL_STATES = %w[ + 08000 + 08001 + 08003 + 08004 + 08006 + 08007 + 08S01 + ].freeze + # CR_SERVER_GONE_ERROR (2006), CR_SERVER_LOST (2013), + # ER_SERVER_SHUTDOWN (1053), ER_CONNECTION_KILLED (1927), + # ER_CLIENT_INTERACTION_TIMEOUT (4031). + CONNECTION_FAILURE_ERROR_CODES = [2006, 2013, 1053, 1927, 4031].freeze + CONNECTION_FAILURE_MESSAGES = / + Communications?\ link\ failure | + No\ operations\ allowed\ after\ connection\ closed | + Connection\.*\ refused | + Could\ not\ connect\ to | + Server\ shutdown\ in\ progress | + Connection\ is\ closed + /x.freeze + private_constant :CONNECTION_FAILURE_SQL_STATES, :CONNECTION_FAILURE_ERROR_CODES, :CONNECTION_FAILURE_MESSAGES + def translate_exception(exception, message:, sql:, binds:) + if exception.is_a?(::ActiveRecord::JDBCError) && connection_lost?(exception) + return ::ActiveRecord::ConnectionFailed.new(message, sql: sql, binds: binds, connection_pool: @pool) + end + case message when /Table .* doesn't exist/i StatementInvalid.new(message, sql: sql, binds: binds, connection_pool: @pool) @@ -263,6 +327,25 @@ def translate_exception(exception, message:, sql:, binds:) end end + # Detects a lost server connection from a JDBC error so it can be + # translated to ActiveRecord::ConnectionFailed (retryable). Mirrors the + # PostgreSQL adapter's handling of backend disconnects (e.g. a proxy such + # as ProxySQL dropping an idle connection). + def connection_lost?(exception) + state = exception.sql_state if exception.respond_to?(:sql_state) + return true if state && CONNECTION_FAILURE_SQL_STATES.include?(state) + + code = exception.error_code if exception.respond_to?(:error_code) + return true if code && CONNECTION_FAILURE_ERROR_CODES.include?(code) + + message = exception.message + return true if message && CONNECTION_FAILURE_MESSAGES.match?(message) + + cause = exception.cause if exception.respond_to?(:cause) + cause.is_a?(Java::JavaSql::SQLRecoverableException) || + cause.is_a?(Java::JavaSql::SQLNonTransientConnectionException) + end + # defined in MySQL::DatabaseStatements which is not included def default_insert_value(column) super unless column.auto_increment? diff --git a/lib/arjdbc/postgresql/adapter.rb b/lib/arjdbc/postgresql/adapter.rb index df059b063..5a3eeef9f 100644 --- a/lib/arjdbc/postgresql/adapter.rb +++ b/lib/arjdbc/postgresql/adapter.rb @@ -571,6 +571,37 @@ def disconnect! end end + # Commits the current database transaction. + # + # Overrides ArJdbc::Abstract::TransactionSupport to disable connection + # retries for COMMIT, matching ActiveRecord's native PostgreSQL adapter + # (which uses `allow_retry: false`). Retrying a COMMIT after a connection + # failure is unsafe on a networked database: `with_raw_connection` would + # reconnect, replay an *empty* transaction (the original writes died with + # the dropped backend), COMMIT it successfully, and report success - + # silently losing the transaction's writes. This is especially reachable + # now that backend disconnects (e.g. pgbouncer dropping a connection) are + # classified as retryable ConnectionFailed errors. + def commit_db_transaction + log('COMMIT', 'TRANSACTION') do + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| + conn.commit + end + end + end + + # Rolls back the current database transaction. + # + # Overrides ArJdbc::Abstract::TransactionSupport to match ActiveRecord's + # native PostgreSQL adapter (`allow_retry: false`). + def exec_rollback_db_transaction + log('ROLLBACK', 'TRANSACTION') do + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| + conn.rollback + end + end + end + def default_sequence_name(table_name, pk = "id") #:nodoc: serial_sequence(table_name, pk) rescue ActiveRecord::StatementInvalid diff --git a/test/db/mysql/commit_no_retry_test.rb b/test/db/mysql/commit_no_retry_test.rb new file mode 100644 index 000000000..414ae5fc3 --- /dev/null +++ b/test/db/mysql/commit_no_retry_test.rb @@ -0,0 +1,42 @@ +require 'db/mysql' + +# Regression tests for the COMMIT-retry data-loss footgun (#1) on MySQL. +# +# commit_db_transaction / exec_rollback_db_transaction must go through +# with_raw_connection(allow_retry: false), matching ActiveRecord's native +# MySQL adapter. With allow_retry: true, a connection drop at COMMIT time can +# make AR reconnect, replay an *empty* transaction (the original writes died +# with the dropped backend), COMMIT it successfully, and report success - +# silently losing the transaction's writes. +# +# Note on MySQL vs PostgreSQL: PostgreSQL translates backend drops (e.g. +# pgbouncer reaping a connection) into a retryable ActiveRecord::ConnectionFailed, +# which directly exposes this footgun. MySQL currently translates connection +# loss to a plain ActiveRecord::JDBCError, which is NOT a retryable connection +# error, so AR will not retry a failed COMMIT today regardless of the flag. +# These tests therefore pin the safe contract directly (allow_retry: false) so +# the footgun cannot be silently reintroduced if MySQL later gains +# ConnectionFailed translation. +class MySQLCommitNoRetryTest < Test::Unit::TestCase + + def setup + @adapter = ActiveRecord::Base.connection + end + + def test_commit_db_transaction_does_not_allow_retry + @adapter.expects(:with_raw_connection) + .with(allow_retry: false, materialize_transactions: true) + .returns(nil) + + @adapter.commit_db_transaction + end + + def test_exec_rollback_db_transaction_does_not_allow_retry + @adapter.expects(:with_raw_connection) + .with(allow_retry: false, materialize_transactions: true) + .returns(nil) + + @adapter.exec_rollback_db_transaction + end + +end diff --git a/test/db/mysql/connection_lost_test.rb b/test/db/mysql/connection_lost_test.rb new file mode 100644 index 000000000..6ad28e34b --- /dev/null +++ b/test/db/mysql/connection_lost_test.rb @@ -0,0 +1,108 @@ +require 'db/mysql' + +# Regression tests for the MySQL/MariaDB connection-lost translation, the +# MySQL analog of the PostgreSQL backend-disconnect patch. +# +# A JDBCError whose SQLState (class 08), vendor error code, message, or wrapped +# Java exception indicates the server connection is gone must translate to +# ActiveRecord::ConnectionFailed so AR's with_raw_connection(allow_retry:) +# machinery will reconnect and retry. Without this, a proxy (e.g. ProxySQL) or +# the server dropping an idle connection surfaces as a raw JDBCError / +# StatementInvalid and the safe retry never triggers. +class MySQLConnectionLostTest < Test::Unit::TestCase + + def setup + @adapter = ActiveRecord::Base.connection + end + + # https://dev.mysql.com/doc/connector-j/en/connector-j-reference-error-sqlstates.html + # Class 08 - Connection Exception (08S01 = "Communications link failure"). + CONNECTION_FAILURE_SQL_STATES = %w[ + 08000 + 08001 + 08003 + 08004 + 08006 + 08007 + 08S01 + ] + + # CR_SERVER_GONE_ERROR, CR_SERVER_LOST, ER_SERVER_SHUTDOWN, + # ER_CONNECTION_KILLED, ER_CLIENT_INTERACTION_TIMEOUT. + CONNECTION_FAILURE_ERROR_CODES = [2006, 2013, 1053, 1927, 4031] + + CONNECTION_FAILURE_MESSAGES = [ + 'Communications link failure', + 'No operations allowed after connection closed', + 'Connection refused', + 'Could not connect to address=(host=localhost)(port=3306)', + 'Server shutdown in progress', + 'Connection is closed', + ] + + CONNECTION_FAILURE_SQL_STATES.each do |state| + define_method("test_translates_sqlstate_#{state}_to_connection_failed") do + err = jdbc_error('boom', sql_state: state) + result = translate(err) + assert_kind_of ActiveRecord::ConnectionFailed, result, + "expected SQLState #{state} to translate to ConnectionFailed, got #{result.class}" + end + end + + CONNECTION_FAILURE_ERROR_CODES.each do |code| + define_method("test_translates_error_code_#{code}_to_connection_failed") do + err = jdbc_error('boom', error_code: code) + result = translate(err) + assert_kind_of ActiveRecord::ConnectionFailed, result, + "expected error code #{code} to translate to ConnectionFailed, got #{result.class}" + end + end + + CONNECTION_FAILURE_MESSAGES.each_with_index do |msg, i| + define_method("test_translates_message_#{i}_to_connection_failed") do + err = jdbc_error(msg) + result = translate(err) + assert_kind_of ActiveRecord::ConnectionFailed, result, + "expected message #{msg.inspect} to translate to ConnectionFailed, got #{result.class}" + end + end + + def test_recoverable_jdbc_exception_translates_to_connection_failed + cause = Java::JavaSql::SQLRecoverableException.new('socket gone') + err = ActiveRecord::JDBCError.new('socket gone', cause) + assert_kind_of ActiveRecord::ConnectionFailed, translate(err) + end + + def test_non_transient_connection_exception_translates_to_connection_failed + cause = Java::JavaSql::SQLNonTransientConnectionException.new('link down') + err = ActiveRecord::JDBCError.new('link down', cause) + assert_kind_of ActiveRecord::ConnectionFailed, translate(err) + end + + def test_does_not_translate_duplicate_entry_to_connection_failed + # ER_DUP_ENTRY (1062) is a data error, not a connection failure. + err = jdbc_error("Duplicate entry 'x' for key 'PRIMARY'", sql_state: '23000', error_code: 1062) + result = translate(err) + assert_kind_of ActiveRecord::RecordNotUnique, result + assert !result.is_a?(ActiveRecord::ConnectionFailed) + end + + def test_does_not_translate_syntax_error_to_connection_failed + # ER_PARSE_ERROR (1064) must not be mistaken for a connection failure. + err = jdbc_error('You have an error in your SQL syntax', sql_state: '42000', error_code: 1064) + result = translate(err) + assert !result.is_a?(ActiveRecord::ConnectionFailed), + "syntax error should not translate to ConnectionFailed, got #{result.class}" + end + + private + + def translate(jdbc_error) + @adapter.send(:translate_exception_class, jdbc_error, 'SELECT 1', []) + end + + def jdbc_error(message, sql_state: nil, error_code: 0) + cause = Java::JavaSql::SQLException.new(message, sql_state, error_code) + ActiveRecord::JDBCError.new(message, cause) + end +end diff --git a/test/db/postgresql/connection_lost_test.rb b/test/db/postgresql/connection_lost_test.rb index 08dc30b8e..eac7ca5dc 100644 --- a/test/db/postgresql/connection_lost_test.rb +++ b/test/db/postgresql/connection_lost_test.rb @@ -114,17 +114,97 @@ def test_begin_db_transaction_after_dropped_socket_reconnects @adapter.begin_db_transaction end - # We should be on a different underlying connection now, and the - # new one should actually be inside a transaction. pg_current_xact_id() - # only returns a value while a transaction is open. + # We should be on a different underlying connection now, and the new one + # should actually be inside a transaction. txid_current() forces/returns a + # transaction id (pg_current_xact_id() would be cleaner but only exists on + # PostgreSQL >= 13, and we support older servers). new_jdbc = @adapter.instance_variable_get(:@raw_connection).jdbc_connection assert !new_jdbc.equal?(original), 'expected a fresh jdbc connection after reconnect' - xact_id = @adapter.select_value('SELECT pg_current_xact_id()') + xact_id = @adapter.select_value('SELECT txid_current()') assert_not_nil xact_id, 'expected BEGIN to have established a live transaction on the new connection' @adapter.rollback_db_transaction end + # Regression test for the COMMIT-retry data-loss footgun (#1). + # + # commit_db_transaction must go through with_raw_connection(allow_retry: + # false), matching ActiveRecord's native PostgreSQL adapter. If COMMIT were + # retryable, a backend drop at commit time would make AR reconnect, replay + # an *empty* transaction (the original writes died with the dropped + # backend), COMMIT it successfully, and report success - silently losing + # the transaction's writes. With retry disabled the failure must surface to + # the caller instead. + # + # Note: unlike the begin test above we do NOT call #clean!. At commit time a + # transaction is in progress, so the connection is "verified" / recently + # active - the realistic state in which allow_retry must not reconnect. + def test_commit_after_dropped_socket_does_not_silently_retry + @adapter.execute('SELECT 1') + @adapter.begin_db_transaction + # non-empty transaction; also keeps the connection verified/active. + @adapter.execute('SELECT 1') + + original = @adapter.instance_variable_get(:@raw_connection).jdbc_connection + + # pgbouncer reaps the backend right before COMMIT. + original.close + assert original.isClosed, 'precondition: jdbc connection should be closed' + + # With allow_retry: false the dropped COMMIT must raise rather than + # reconnecting and committing an empty transaction. + assert_raise(ActiveRecord::ConnectionFailed) do + @adapter.commit_db_transaction + end + + # And it must NOT have silently swapped onto a fresh connection and + # committed there. + current = @adapter.instance_variable_get(:@raw_connection) + assert(current.nil? || current.jdbc_connection.equal?(original), + 'commit must not reconnect-and-retry on a fresh connection') + ensure + @adapter.send(:reconnect!) rescue nil + end + + # End-to-end regression for the COMMIT-retry data-loss footgun (#1). + # + # Demonstrates the real-world consequence on actual data: a row written + # inside a transaction whose backend is reaped at COMMIT time must not be + # reported as successfully committed. With the buggy `allow_retry: true`, AR + # would reconnect, replay an *empty* transaction on the fresh connection, + # COMMIT it, and return success - the INSERT silently vanishes while the + # caller believes it persisted. With `allow_retry: false` the COMMIT raises, + # so the caller is correctly told the write did not persist. + def test_commit_failure_after_dropped_backend_is_not_reported_as_success + @adapter.execute('DROP TABLE IF EXISTS commit_retry_loss') + @adapter.execute('CREATE TABLE commit_retry_loss (id serial primary key, name varchar(255))') + + @adapter.begin_db_transaction + @adapter.execute("INSERT INTO commit_retry_loss (name) VALUES ('vanishing-write')") + + # pgbouncer reaps the backend after the write but before COMMIT. + original = @adapter.instance_variable_get(:@raw_connection).jdbc_connection + original.close + assert original.isClosed, 'precondition: jdbc connection should be closed' + + # The caller MUST be told the commit failed - no false success. + assert_raise(ActiveRecord::ConnectionFailed) do + @adapter.commit_db_transaction + end + + # After a clean reconnect the row is absent (the write died with the + # backend). The point of the fix is that the caller learned this via the + # raised error above rather than a silent empty-commit "success". + @adapter.send(:reconnect!) + count = @adapter.select_value( + "SELECT COUNT(*) FROM commit_retry_loss WHERE name = 'vanishing-write'" + ).to_i + assert_equal 0, count, 'uncommitted write must not be present after a failed commit' + ensure + @adapter.send(:reconnect!) rescue nil + @adapter.execute('DROP TABLE IF EXISTS commit_retry_loss') rescue nil + end + private def translate(jdbc_error) diff --git a/test/simple.rb b/test/simple.rb index 2e6610d71..7da3ca375 100644 --- a/test/simple.rb +++ b/test/simple.rb @@ -315,8 +315,12 @@ def test_time_with_default_timezone_local skip "with_system_tz not working in tomcat" if ActiveRecord::Base.connection.raw_connection.jndi? with_system_tz 'Europe/Prague' do - Time.use_zone 'Europe/Prague' do - with_timezone_config default: :local do + # NOTE: with_timezone_config must wrap Time.use_zone (not the reverse): + # it verifies Time.zone against the per-test baseline on entry, which + # would trip the global-state leak guard if Time.zone were already + # changed. See test_preserving_time_objects_*_default_timezone_local. + with_timezone_config default: :local do + Time.use_zone 'Europe/Prague' do time = Time.local(1999, 12, 21) record = DbType.create!('sample_datetime' => time, 'sample_time' => time) From dbe2bb74fb6e46f8f9b2b80231d16dabdc277389 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Thu, 18 Jun 2026 18:43:33 -0700 Subject: [PATCH 2/6] Fix PostgreSQL savepoint handling and client_min_messages getter under JDBC Correct savepoint behaviour in the shared transaction support and fix the PostgreSQL#client_min_messages getter, which did not work under the JDBC adapter (it does not inherit the native pg adapter implementation). Co-Authored-By: Claude Opus 4.8 --- lib/arjdbc/abstract/transaction_support.rb | 16 +++++++-- lib/arjdbc/postgresql/adapter.rb | 7 +++- test/db/postgresql/connection_lost_test.rb | 38 ++++++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/arjdbc/abstract/transaction_support.rb b/lib/arjdbc/abstract/transaction_support.rb index d513230c3..04fa286cb 100644 --- a/lib/arjdbc/abstract/transaction_support.rb +++ b/lib/arjdbc/abstract/transaction_support.rb @@ -66,6 +66,16 @@ def exec_rollback_db_transaction ########################## Savepoint Interface ############################ + # Save-point operations must NOT be retried on a connection failure. They + # only ever run inside an already-open transaction, so a dropped backend + # means the transaction's prior writes are gone. Retrying via + # `with_raw_connection(allow_retry: true)` would reconnect, replay an + # *empty* transaction (the original writes died with the backend), run the + # save-point statement against it, and report success - silently losing + # data. ActiveRecord's native adapters route these through + # `internal_execute` (allow_retry: false, materialize_transactions: true); + # we mirror that here. See the COMMIT/ROLLBACK overrides above. + # Creates a (transactional) save-point one can rollback to. # Unlike 'plain' `ActiveRecord` it is allowed to pass a save-point name. # @param name the save-point name @@ -74,7 +84,7 @@ def exec_rollback_db_transaction # @extension added optional name parameter def create_savepoint(name = current_savepoint_name) log("SAVEPOINT #{name}", 'TRANSACTION') do - with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn| + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| conn.create_savepoint(name) end end @@ -87,7 +97,7 @@ def create_savepoint(name = current_savepoint_name) # @extension added optional name parameter def exec_rollback_to_savepoint(name = current_savepoint_name) log("ROLLBACK TO SAVEPOINT #{name}", 'TRANSACTION') do - with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn| + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| conn.rollback_savepoint(name) end end @@ -100,7 +110,7 @@ def exec_rollback_to_savepoint(name = current_savepoint_name) # @extension added optional name parameter def release_savepoint(name = current_savepoint_name) log("RELEASE SAVEPOINT #{name}", 'TRANSACTION') do - with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn| + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| conn.release_savepoint(name) end end diff --git a/lib/arjdbc/postgresql/adapter.rb b/lib/arjdbc/postgresql/adapter.rb index 5a3eeef9f..e37b79e48 100644 --- a/lib/arjdbc/postgresql/adapter.rb +++ b/lib/arjdbc/postgresql/adapter.rb @@ -624,7 +624,12 @@ def all_schemas def client_min_messages return nil if redshift? # not supported on Redshift # Need to use #execute so we don't try to access the type map before it is initialized - execute('SHOW client_min_messages', 'SCHEMA').values.first.first + # NOTE: #execute returns an Array of row Hashes here (e.g. + # [{"client_min_messages"=>"warning"}]), unlike MRI's pg result object, + # so we read the single value out of the first row. + result = execute('SHOW client_min_messages', 'SCHEMA') + row = result.first + row && row.values.first end # Set the client message level. diff --git a/test/db/postgresql/connection_lost_test.rb b/test/db/postgresql/connection_lost_test.rb index eac7ca5dc..06ba9f2fd 100644 --- a/test/db/postgresql/connection_lost_test.rb +++ b/test/db/postgresql/connection_lost_test.rb @@ -205,6 +205,44 @@ def test_commit_failure_after_dropped_backend_is_not_reported_as_success @adapter.execute('DROP TABLE IF EXISTS commit_retry_loss') rescue nil end + # Regression test for the savepoint-retry data-loss footgun (#1, sibling of + # the COMMIT case above). + # + # create_savepoint / exec_rollback_to_savepoint / release_savepoint must go + # through with_raw_connection(allow_retry: false), matching ActiveRecord's + # native adapters (which route save-points through internal_execute, whose + # default is allow_retry: false). Save-points only ever run inside an open + # transaction, so a backend drop means the transaction's prior writes are + # gone. If the save-point op were retryable, AR would reconnect, replay an + # *empty* transaction, run the SAVEPOINT against it, and report success - + # silently losing the transaction's writes. With retry disabled the failure + # surfaces to the caller instead. + def test_create_savepoint_after_dropped_socket_does_not_silently_retry + @adapter.execute('SELECT 1') + @adapter.begin_db_transaction + # non-empty transaction; also keeps the connection verified/active. + @adapter.execute('SELECT 1') + + original = @adapter.instance_variable_get(:@raw_connection).jdbc_connection + + # pgbouncer reaps the backend right before the SAVEPOINT. + original.close + assert original.isClosed, 'precondition: jdbc connection should be closed' + + # With allow_retry: false the dropped SAVEPOINT must raise rather than + # reconnecting and running against a fresh, empty transaction. + assert_raise(ActiveRecord::ConnectionFailed) do + @adapter.create_savepoint('sp_retry_loss') + end + + # And it must NOT have silently swapped onto a fresh connection. + current = @adapter.instance_variable_get(:@raw_connection) + assert(current.nil? || current.jdbc_connection.equal?(original), + 'savepoint must not reconnect-and-retry on a fresh connection') + ensure + @adapter.send(:reconnect!) rescue nil + end + private def translate(jdbc_error) From 5bd86de6c3b94b5d2945981bc506fc29477e0648 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Thu, 18 Jun 2026 18:37:51 -0700 Subject: [PATCH 3/6] Fix PG connection leak on failed establishment and add SQL warnings support 1.1 Connection leak: PostgreSQLRubyJdbcConnection#newConnection opened a physical backend via super.newConnection() but then ran unwrap()/addDataType() without cleanup. Any failure there leaked the open server-side connection, since the caller only saw the exception and never got a handle to close it. Wrap the post-connect setup so the connection is closed (suppressing close errors) before re-raising, mirroring the native adapter discarding a connection it could not fully establish. 1.2 SQL warnings (AR 7.2 db_warnings_action): capture statement.getWarnings() in RubyJdbcConnection#execute before the statement closes and expose them via #last_warnings. PostgreSQLRubyJdbcConnection#newWarning maps PSQLWarning's ServerErrorMessage to [message, sqlstate, severity]. The PG raw_execute override dispatches them through handle_warnings to ActiveRecord .db_warnings_action and normalises the Integer update-count return to [] for PG::Result API parity. Co-Authored-By: Claude Opus 4.8 --- lib/arjdbc/postgresql/database_statements.rb | 40 ++++++++++++ src/java/arjdbc/jdbc/RubyJdbcConnection.java | 45 +++++++++++++ .../PostgreSQLRubyJdbcConnection.java | 65 +++++++++++++++---- 3 files changed, 139 insertions(+), 11 deletions(-) diff --git a/lib/arjdbc/postgresql/database_statements.rb b/lib/arjdbc/postgresql/database_statements.rb index 2c1ddc85e..01ab6b1db 100644 --- a/lib/arjdbc/postgresql/database_statements.rb +++ b/lib/arjdbc/postgresql/database_statements.rb @@ -15,6 +15,46 @@ def build_explain_clause(options = []) "EXPLAIN (#{options.join(", ").upcase})" end + + # Overridden to surface any SQL warnings (PostgreSQL NOTICE / RAISE + # WARNING messages) emitted while running +sql+ to the configured + # +ActiveRecord.db_warnings_action+, matching the native PostgreSQL + # adapter. The warnings themselves are collected on the Java side during + # #execute and read back here via #last_warnings. + def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) + log(sql, name, async: async) do + with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| + result = conn.execute(sql) + verified! + handle_warnings(sql) + # The native adapter returns a result object (PG::Result) whose + # #to_a is []; statements without a result set come back as an + # update count here, so normalise to an array for API parity. + result.is_a?(Integer) ? [] : result + end + end + end + + private + + # Dispatches SQL warnings collected by the most recent #execute to + # +ActiveRecord.db_warnings_action+ (mirrors the native adapter). + def handle_warnings(sql) + return if ActiveRecord.db_warnings_action.nil? + + @raw_connection.last_warnings.each do |message, code, level| + warning = ActiveRecord::SQLWarning.new(message, code, level, sql, @pool) + next if warning_ignored?(warning) + + ActiveRecord.db_warnings_action.call(warning) + end + end + + # Only WARNING and above are treated as SQL warnings; NOTICE/INFO/DEBUG/LOG + # level messages are ignored, as in the native PostgreSQL adapter. + def warning_ignored?(warning) + ["WARNING", "ERROR", "FATAL", "PANIC"].exclude?(warning.level) || super + end end end end diff --git a/src/java/arjdbc/jdbc/RubyJdbcConnection.java b/src/java/arjdbc/jdbc/RubyJdbcConnection.java index 07bee76a6..9ea2fd546 100644 --- a/src/java/arjdbc/jdbc/RubyJdbcConnection.java +++ b/src/java/arjdbc/jdbc/RubyJdbcConnection.java @@ -40,6 +40,7 @@ import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; +import java.sql.SQLWarning; import java.sql.SQLXML; import java.sql.Statement; import java.sql.Date; @@ -133,6 +134,10 @@ public class RubyJdbcConnection extends RubyObject { private boolean configureConnection = true; // final once initialized private int fetchSize = 0; // 0 = JDBC default + // SQL warnings collected by the most recent #execute call, exposed to the + // adapter via #last_warnings so it can honour ActiveRecord.db_warnings_action. + private transient IRubyObject lastWarnings; + protected RubyJdbcConnection(Ruby runtime, RubyClass metaClass) { super(runtime, metaClass); attributeClass = runtime.getModule("ActiveModel").getClass("Attribute"); @@ -807,6 +812,10 @@ public IRubyObject execute(final ThreadContext context, final IRubyObject sql) { updateCount = statement.getUpdateCount(); } + // Capture any SQL warnings (e.g. PostgreSQL RAISE WARNING / NOTICE) + // raised while executing, before the statement is closed below. + lastWarnings = mapWarnings(context, statement.getWarnings()); + return result; } catch (final SQLException e) { @@ -818,6 +827,42 @@ public IRubyObject execute(final ThreadContext context, final IRubyObject sql) { }); } + /** + * @return the SQL warnings collected by the most recent {@link #execute} + * as an Array of [message, sql_state, level] arrays. + */ + @JRubyMethod(name = "last_warnings") + public IRubyObject last_warnings(final ThreadContext context) { + return lastWarnings == null ? context.runtime.newEmptyArray() : lastWarnings; + } + + /** + * Maps a chain of {@link SQLWarning}s to a Ruby Array of warning tuples. + * @param warning the head of the warning chain (may be null) + * @return a (possibly empty) Ruby Array of [message, sql_state, level] + */ + protected IRubyObject mapWarnings(final ThreadContext context, SQLWarning warning) { + final RubyArray warnings = context.runtime.newArray(); + while (warning != null) { + warnings.append(newWarning(context, warning)); + warning = warning.getNextWarning(); + } + return warnings; + } + + /** + * Builds a single warning tuple [message, sql_state, level]. + * The generic JDBC API does not expose a severity level, so it is left nil; + * adapters with richer driver support (e.g. PostgreSQL) may override this. + */ + protected IRubyObject newWarning(final ThreadContext context, final SQLWarning warning) { + final Ruby runtime = context.runtime; + final IRubyObject message = RubyString.newUnicodeString(runtime, warning.getMessage()); + final String sqlState = warning.getSQLState(); + final IRubyObject code = sqlState == null ? context.nil : RubyString.newUnicodeString(runtime, sqlState); + return runtime.newArray(message, code, context.nil); + } + protected Statement createStatement(final ThreadContext context, final Connection connection) throws SQLException { final Statement statement = connection.createStatement(); diff --git a/src/java/arjdbc/postgresql/PostgreSQLRubyJdbcConnection.java b/src/java/arjdbc/postgresql/PostgreSQLRubyJdbcConnection.java index ded69a537..d6df4a2d9 100644 --- a/src/java/arjdbc/postgresql/PostgreSQLRubyJdbcConnection.java +++ b/src/java/arjdbc/postgresql/PostgreSQLRubyJdbcConnection.java @@ -66,6 +66,8 @@ import org.postgresql.geometric.PGpolygon; import org.postgresql.util.PGInterval; import org.postgresql.util.PGobject; +import org.postgresql.util.PSQLWarning; +import org.postgresql.util.ServerErrorMessage; /** * @@ -241,19 +243,35 @@ protected Connection newConnection() throws RaiseException, SQLException { } throw ex; } - final PGConnection pgConnection; - if ( connection instanceof PGConnection ) { - pgConnection = (PGConnection) connection; + // The physical connection is open now; if any of the post-connect + // setup below fails we must close it, otherwise the server-side backend + // leaks (the caller only sees the exception and never gets a handle to + // close). This mirrors the native adapter discarding a connection that + // could not be fully established. + try { + final PGConnection pgConnection; + if ( connection instanceof PGConnection ) { + pgConnection = (PGConnection) connection; + } + else { + pgConnection = connection.unwrap(PGConnection.class); + } + pgConnection.addDataType("daterange", DateRangeType.class); + pgConnection.addDataType("tsrange", TsRangeType.class); + pgConnection.addDataType("tstzrange", TstzRangeType.class); + pgConnection.addDataType("int4range", Int4RangeType.class); + pgConnection.addDataType("int8range", Int8RangeType.class); + pgConnection.addDataType("numrange", NumRangeType.class); } - else { - pgConnection = connection.unwrap(PGConnection.class); + catch (SQLException|RuntimeException ex) { + try { + connection.close(); + } + catch (SQLException closeError) { + ex.addSuppressed(closeError); + } + throw ex; } - pgConnection.addDataType("daterange", DateRangeType.class); - pgConnection.addDataType("tsrange", TsRangeType.class); - pgConnection.addDataType("tstzrange", TstzRangeType.class); - pgConnection.addDataType("int4range", Int4RangeType.class); - pgConnection.addDataType("int8range", Int8RangeType.class); - pgConnection.addDataType("numrange", NumRangeType.class); return connection; } @@ -277,6 +295,31 @@ protected IRubyObject mapQueryResult(final ThreadContext context, final Connecti return mapExecuteResult(context, connection, resultSet).toARResult(context); } + /** + * Builds a warning tuple [message, sql_state, level] for a + * PostgreSQL server message (e.g. RAISE WARNING / NOTICE). + * The driver wraps these as {@link PSQLWarning}, which carries the original + * {@link ServerErrorMessage} with the clean primary message, SQLSTATE and + * severity ("WARNING", "NOTICE", ...) that ActiveRecord's db_warnings + * handling needs. + */ + @Override + protected IRubyObject newWarning(final ThreadContext context, final SQLWarning warning) { + if (warning instanceof PSQLWarning) { + final ServerErrorMessage serverError = ((PSQLWarning) warning).getServerErrorMessage(); + if (serverError != null) { + final Ruby runtime = context.runtime; + final IRubyObject message = RubyString.newUnicodeString(runtime, serverError.getMessage()); + final String sqlState = serverError.getSQLState(); + final IRubyObject code = sqlState == null ? context.nil : RubyString.newUnicodeString(runtime, sqlState); + final String severity = serverError.getSeverity(); + final IRubyObject level = severity == null ? context.nil : RubyString.newUnicodeString(runtime, severity); + return runtime.newArray(message, code, level); + } + } + return super.newWarning(context, warning); + } + @Override protected void setArrayParameter(final ThreadContext context, final Connection connection, final PreparedStatement statement, From f3946cc70580aabc9296c54d7a566be193e0082e Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Tue, 23 Jun 2026 10:29:21 -0700 Subject: [PATCH 4/6] Simplify transaction-retry fix and lazy-load SQL warnings Quality cleanup of the COMMIT/ROLLBACK retry fix plus added regression coverage. No behavior change for the PostgreSQL/MySQL paths the original fix targeted. - Consolidate COMMIT/ROLLBACK no-retry into the shared TransactionSupport mixin (allow_retry: false, materialize_transactions: true), matching the savepoint precedent and AR's native adapters, and delete the byte-for-byte identical overrides from the PostgreSQL and MySQL adapters. BEGIN stays retryable (idempotent). This also extends the safe contract to the sqlite3 and generic JDBC adapters, which inherit the mixin. - Capture SQL warnings lazily: RubyJdbcConnection#execute now stores the raw SQLWarning chain (cheap reference) and #last_warnings maps it to Ruby tuples on demand, avoiding a per-execute array allocation on the common path where ActiveRecord.db_warnings_action is disabled. - Collapse client_min_messages to a safe-navigation one-liner. - Add tests: sqlite3 transaction_no_retry_test pins the mixin contract (commit/rollback/savepoints no-retry; BEGIN still retryable) and PG warnings_test covers warning capture/dispatch plus the NOTICE-filtered and no-warning negative cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/arjdbc/abstract/transaction_support.rb | 14 +++- lib/arjdbc/mysql/adapter.rb | 29 ------- lib/arjdbc/postgresql/adapter.rb | 35 +------- src/java/arjdbc/jdbc/RubyJdbcConnection.java | 18 ++-- test/db/postgresql/warnings_test.rb | 87 ++++++++++++++++++++ test/db/sqlite3/transaction_no_retry_test.rb | 64 ++++++++++++++ 6 files changed, 174 insertions(+), 73 deletions(-) create mode 100644 test/db/postgresql/warnings_test.rb create mode 100644 test/db/sqlite3/transaction_no_retry_test.rb diff --git a/lib/arjdbc/abstract/transaction_support.rb b/lib/arjdbc/abstract/transaction_support.rb index 04fa286cb..336fdda4b 100644 --- a/lib/arjdbc/abstract/transaction_support.rb +++ b/lib/arjdbc/abstract/transaction_support.rb @@ -43,11 +43,19 @@ def begin_isolated_db_transaction(isolation) end end + # COMMIT and ROLLBACK must NOT be retried on a connection failure, matching + # ActiveRecord's native adapters (which use `allow_retry: false`). Retrying + # a COMMIT after a dropped backend is unsafe on a networked database: + # `with_raw_connection` would reconnect, replay an *empty* transaction (the + # original writes died with the old backend), COMMIT it successfully, and + # report success - silently losing the transaction's writes. (BEGIN above + # stays retryable because it is idempotent.) See the save-point note below. + # Commits the current database transaction. # @override def commit_db_transaction log('COMMIT', 'TRANSACTION') do - with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn| + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| conn.commit end end @@ -58,7 +66,7 @@ def commit_db_transaction # @override def exec_rollback_db_transaction log('ROLLBACK', 'TRANSACTION') do - with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn| + with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| conn.rollback end end @@ -74,7 +82,7 @@ def exec_rollback_db_transaction # save-point statement against it, and report success - silently losing # data. ActiveRecord's native adapters route these through # `internal_execute` (allow_retry: false, materialize_transactions: true); - # we mirror that here. See the COMMIT/ROLLBACK overrides above. + # we mirror that here, as do the COMMIT/ROLLBACK methods above. # Creates a (transactional) save-point one can rollback to. # Unlike 'plain' `ActiveRecord` it is allowed to pass a save-point name. diff --git a/lib/arjdbc/mysql/adapter.rb b/lib/arjdbc/mysql/adapter.rb index e72ba2187..faa1588cb 100644 --- a/lib/arjdbc/mysql/adapter.rb +++ b/lib/arjdbc/mysql/adapter.rb @@ -203,35 +203,6 @@ def active? alias :reset! :reconnect! - # Commits the current database transaction. - # - # Overrides ArJdbc::Abstract::TransactionSupport to disable connection - # retries for COMMIT, matching ActiveRecord's native MySQL adapter - # (which uses `allow_retry: false`). Retrying a COMMIT after a connection - # failure is unsafe on a networked database: `with_raw_connection` would - # reconnect, replay an *empty* transaction (the original writes died with - # the dropped backend), COMMIT it successfully, and report success - - # silently losing the transaction's writes. - def commit_db_transaction - log('COMMIT', 'TRANSACTION') do - with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| - conn.commit - end - end - end - - # Rolls back the current database transaction. - # - # Overrides ArJdbc::Abstract::TransactionSupport to match ActiveRecord's - # native MySQL adapter (`allow_retry: false`). - def exec_rollback_db_transaction - log('ROLLBACK', 'TRANSACTION') do - with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| - conn.rollback - end - end - end - # Disconnects from the database if already connected. # Otherwise, this method does nothing. def disconnect! diff --git a/lib/arjdbc/postgresql/adapter.rb b/lib/arjdbc/postgresql/adapter.rb index e37b79e48..7ed67f0e0 100644 --- a/lib/arjdbc/postgresql/adapter.rb +++ b/lib/arjdbc/postgresql/adapter.rb @@ -571,37 +571,6 @@ def disconnect! end end - # Commits the current database transaction. - # - # Overrides ArJdbc::Abstract::TransactionSupport to disable connection - # retries for COMMIT, matching ActiveRecord's native PostgreSQL adapter - # (which uses `allow_retry: false`). Retrying a COMMIT after a connection - # failure is unsafe on a networked database: `with_raw_connection` would - # reconnect, replay an *empty* transaction (the original writes died with - # the dropped backend), COMMIT it successfully, and report success - - # silently losing the transaction's writes. This is especially reachable - # now that backend disconnects (e.g. pgbouncer dropping a connection) are - # classified as retryable ConnectionFailed errors. - def commit_db_transaction - log('COMMIT', 'TRANSACTION') do - with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| - conn.commit - end - end - end - - # Rolls back the current database transaction. - # - # Overrides ArJdbc::Abstract::TransactionSupport to match ActiveRecord's - # native PostgreSQL adapter (`allow_retry: false`). - def exec_rollback_db_transaction - log('ROLLBACK', 'TRANSACTION') do - with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn| - conn.rollback - end - end - end - def default_sequence_name(table_name, pk = "id") #:nodoc: serial_sequence(table_name, pk) rescue ActiveRecord::StatementInvalid @@ -627,9 +596,7 @@ def client_min_messages # NOTE: #execute returns an Array of row Hashes here (e.g. # [{"client_min_messages"=>"warning"}]), unlike MRI's pg result object, # so we read the single value out of the first row. - result = execute('SHOW client_min_messages', 'SCHEMA') - row = result.first - row && row.values.first + execute('SHOW client_min_messages', 'SCHEMA').first&.values&.first end # Set the client message level. diff --git a/src/java/arjdbc/jdbc/RubyJdbcConnection.java b/src/java/arjdbc/jdbc/RubyJdbcConnection.java index 9ea2fd546..61847cb0a 100644 --- a/src/java/arjdbc/jdbc/RubyJdbcConnection.java +++ b/src/java/arjdbc/jdbc/RubyJdbcConnection.java @@ -134,9 +134,11 @@ public class RubyJdbcConnection extends RubyObject { private boolean configureConnection = true; // final once initialized private int fetchSize = 0; // 0 = JDBC default - // SQL warnings collected by the most recent #execute call, exposed to the - // adapter via #last_warnings so it can honour ActiveRecord.db_warnings_action. - private transient IRubyObject lastWarnings; + // Head of the SQLWarning chain captured by the most recent #execute call. + // Stored as the raw chain (cheap) and mapped to Ruby tuples lazily by + // #last_warnings, so the common path (no warnings, or db_warnings_action + // disabled) pays no allocation. + private transient SQLWarning lastWarnings; protected RubyJdbcConnection(Ruby runtime, RubyClass metaClass) { super(runtime, metaClass); @@ -812,9 +814,11 @@ public IRubyObject execute(final ThreadContext context, final IRubyObject sql) { updateCount = statement.getUpdateCount(); } - // Capture any SQL warnings (e.g. PostgreSQL RAISE WARNING / NOTICE) - // raised while executing, before the statement is closed below. - lastWarnings = mapWarnings(context, statement.getWarnings()); + // Capture the SQL warning chain (e.g. PostgreSQL RAISE WARNING / + // NOTICE) before the statement is closed below. The warnings are + // already-materialized objects, so holding the head reference is + // cheap and safe after close; #last_warnings maps them on demand. + lastWarnings = statement.getWarnings(); return result; @@ -833,7 +837,7 @@ public IRubyObject execute(final ThreadContext context, final IRubyObject sql) { */ @JRubyMethod(name = "last_warnings") public IRubyObject last_warnings(final ThreadContext context) { - return lastWarnings == null ? context.runtime.newEmptyArray() : lastWarnings; + return mapWarnings(context, lastWarnings); } /** diff --git a/test/db/postgresql/warnings_test.rb b/test/db/postgresql/warnings_test.rb new file mode 100644 index 000000000..ddcb33f2a --- /dev/null +++ b/test/db/postgresql/warnings_test.rb @@ -0,0 +1,87 @@ +require 'db/postgres' + +# Tests for PostgreSQL SQL-warning handling (AR 7.2 db_warnings_action). +# +# Warnings (RAISE WARNING / NOTICE) are collected on the Java side during +# #execute and surfaced via #last_warnings, then dispatched to +# ActiveRecord.db_warnings_action by the adapter's #handle_warnings. The Java +# side captures the raw SQLWarning chain lazily (mapped to Ruby tuples only when +# #last_warnings is read), so these tests guard that the chain is still captured +# correctly and that the WARNING/NOTICE filtering matches the native adapter. +class PostgreSQLWarningsTest < Test::Unit::TestCase + + WARNING_MARKER = 'arjdbc test warning'.freeze + NOTICE_MARKER = 'arjdbc test notice'.freeze + + def setup + @adapter = ActiveRecord::Base.connection + @raw = @adapter.instance_variable_get(:@raw_connection) + # db_warnings_action's setter rejects nil, so snapshot/restore the raw ivar. + @original_warnings_action = ActiveRecord.instance_variable_get(:@db_warnings_action) + end + + def teardown + ActiveRecord.instance_variable_set(:@db_warnings_action, @original_warnings_action) + end + + # --- last_warnings (Java capture) --------------------------------------- + + def test_last_warnings_is_empty_after_a_clean_statement + @adapter.execute('SELECT 1') + assert_equal [], @raw.last_warnings + end + + def test_warning_is_captured_and_surfaced_via_last_warnings + @adapter.execute(raise_sql('WARNING', WARNING_MARKER)) + + messages = @raw.last_warnings.map { |message, _code, _level| message } + assert messages.any? { |m| m.include?(WARNING_MARKER) }, + "expected last_warnings to include the raised warning, got #{@raw.last_warnings.inspect}" + end + + def test_last_warnings_carries_the_postgresql_severity_level + @adapter.execute(raise_sql('WARNING', WARNING_MARKER)) + + levels = @raw.last_warnings.map { |_message, _code, level| level } + assert_includes levels, 'WARNING', + "expected PostgreSQL severity to be surfaced, got #{@raw.last_warnings.inspect}" + end + + # --- db_warnings_action dispatch ---------------------------------------- + + def test_db_warnings_action_receives_warning_level_messages + collected = collect_warnings { @adapter.execute(raise_sql('WARNING', WARNING_MARKER)) } + + assert collected.any? { |w| w.message.include?(WARNING_MARKER) }, + "expected db_warnings_action to receive the warning, got #{collected.map(&:message).inspect}" + end + + # Negative: a clean statement must not invoke db_warnings_action at all. + def test_db_warnings_action_not_invoked_without_a_warning + collected = collect_warnings { @adapter.execute('SELECT 1') } + assert_empty collected + end + + # Negative: NOTICE is below WARNING, so warning_ignored? must drop it even + # though the driver may still surface it in the warning chain. + def test_notice_level_messages_are_not_dispatched + collected = collect_warnings { @adapter.execute(raise_sql('NOTICE', NOTICE_MARKER)) } + assert_empty collected, + "NOTICE-level messages must not be dispatched as SQL warnings, got #{collected.map(&:message).inspect}" + end + + private + + def collect_warnings + collected = [] + ActiveRecord.db_warnings_action = ->(warning) { collected << warning } + yield + collected + end + + # Emits a server message at the given severity ('WARNING', 'NOTICE', ...). + def raise_sql(level, message) + "DO $$ BEGIN RAISE #{level} '#{message}'; END $$;" + end + +end diff --git a/test/db/sqlite3/transaction_no_retry_test.rb b/test/db/sqlite3/transaction_no_retry_test.rb new file mode 100644 index 000000000..abee38f2a --- /dev/null +++ b/test/db/sqlite3/transaction_no_retry_test.rb @@ -0,0 +1,64 @@ +require 'db/sqlite3' + +# Contract tests for the shared ArJdbc::Abstract::TransactionSupport mixin, +# exercised through the SQLite3 adapter (which has no external server, so these +# always run). +# +# COMMIT / ROLLBACK / SAVEPOINT operations must go through +# with_raw_connection(allow_retry: false), matching ActiveRecord's native +# adapters. With allow_retry: true, a connection drop at COMMIT time can make AR +# reconnect, replay an *empty* transaction (the original writes died with the +# dropped backend), COMMIT it successfully, and report success - silently losing +# the transaction's writes. +# +# BEGIN intentionally stays allow_retry: true: it is idempotent, so replaying it +# on a fresh connection after a drop is safe. The negative test below pins that +# distinction so the no-retry fix is not over-applied to BEGIN. +class SQLite3TransactionNoRetryTest < Test::Unit::TestCase + + def setup + @adapter = ActiveRecord::Base.connection + end + + def test_commit_db_transaction_does_not_allow_retry + assert_no_retry { @adapter.commit_db_transaction } + end + + def test_exec_rollback_db_transaction_does_not_allow_retry + assert_no_retry { @adapter.exec_rollback_db_transaction } + end + + def test_create_savepoint_does_not_allow_retry + assert_no_retry { @adapter.create_savepoint('sp_no_retry') } + end + + def test_exec_rollback_to_savepoint_does_not_allow_retry + assert_no_retry { @adapter.exec_rollback_to_savepoint('sp_no_retry') } + end + + def test_release_savepoint_does_not_allow_retry + assert_no_retry { @adapter.release_savepoint('sp_no_retry') } + end + + # Negative: BEGIN must remain retryable (idempotent), so the no-retry fix must + # NOT have leaked onto it. + def test_begin_db_transaction_still_allows_retry + @adapter.expects(:with_raw_connection) + .with(allow_retry: true, materialize_transactions: false) + .returns(nil) + + @adapter.begin_db_transaction + end + + private + + # Asserts the yielded transaction-control call routes through the safe + # with_raw_connection contract (no reconnect/retry on a dropped backend). + def assert_no_retry + @adapter.expects(:with_raw_connection) + .with(allow_retry: false, materialize_transactions: true) + .returns(nil) + yield + end + +end From 868eb30f5b9501d2bfdb81146a8d45546d485afd Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Thu, 25 Jun 2026 10:43:05 -0700 Subject: [PATCH 5/6] Reduce per-connection PG OID type-map cost; forward exec retry kwargs Under AR 7.2's lazy-connection model the PostgreSQL adapter rebuilt the entire pg_type OID type map on every reconnect (configure_connection ends with reload_type_map) and never shared the catalog rows across connections. Under JRuby (no GVL, genuinely concurrent pool, full-cost reconnects) that is an expensive metadata bootstrap paid on the request path. A1 - build the OID type map once per adapter instance: create @type_map in #initialize and guard the automatic #initialize_type_map in #configure_connection with @type_map_initialized so reconnects reuse the map. #reload_type_map remains the explicit refresh for schema/type changes. A2 - share the pg_type catalog rows across connections to the same database. The first connection runs the three catalog sweeps and caches the rows; other connections build their own isolated type_map from the shared copy with no DB round-trips. Cache is scoped per-database (host/port/name + JDBC url, recovering name from the url path for url-only configs) and invalidated by #reload_type_map. Thread-safe: a Mutex guards the cache Hash, the catalog queries run outside the lock, and cached rows are deep-frozen so concurrent readers need no lock. Also forward allow_retry:/materialize_transactions: from internal_exec_query into with_raw_connection (the signature accepted them but silently dropped them, unlike raw_execute). Ported from the 71-stable lazy-connection remediation work. PostgreSQL suite unchanged: 311 tests, the 1 failure + 1 error are pre-existing (Ruby 3.4 File.exists? removal; prepared-statement timezone handling). Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/arjdbc/abstract/database_statements.rb | 2 +- lib/arjdbc/postgresql/adapter.rb | 16 +++- lib/arjdbc/postgresql/oid_types.rb | 90 +++++++++++++++++++++- 3 files changed, 103 insertions(+), 5 deletions(-) diff --git a/lib/arjdbc/abstract/database_statements.rb b/lib/arjdbc/abstract/database_statements.rb index d86a4b12d..984a12802 100644 --- a/lib/arjdbc/abstract/database_statements.rb +++ b/lib/arjdbc/abstract/database_statements.rb @@ -44,7 +44,7 @@ def internal_exec_query(sql, name = nil, binds = NO_BINDS, prepare: false, async binds = convert_legacy_binds_to_attributes(binds) if binds.first.is_a?(Array) - with_raw_connection do |conn| + with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| if without_prepared_statement?(binds) log(sql, name, async: async) { conn.execute_query(sql) } else diff --git a/lib/arjdbc/postgresql/adapter.rb b/lib/arjdbc/postgresql/adapter.rb index 7ed67f0e0..9a620848a 100644 --- a/lib/arjdbc/postgresql/adapter.rb +++ b/lib/arjdbc/postgresql/adapter.rb @@ -104,7 +104,16 @@ def configure_connection end end - reload_type_map + # Build the OID type map once per adapter instance, the first time the + # connection is live. Reconnects re-run #configure_connection but reuse the + # map (OIDs are stable per server); schema/type changes refresh it + # explicitly via #reload_type_map. We call #initialize_type_map (not + # #reload_type_map) here so the first connect doesn't invalidate the shared + # catalog cache another connection to the same database may have populated. + unless @type_map_initialized + initialize_type_map + @type_map_initialized = true + end end # @private @@ -966,6 +975,11 @@ def initialize(...) @local_tz = nil @max_identifier_length = nil + # Build the OID type map up front so the guarded build in #configure_connection + # has a map to populate. AR 7.2 connects lazily, so the catalog sweep that + # fills this map runs on first use, once per adapter (see #configure_connection). + @type_map = Type::HashLookupTypeMap.new + @use_insert_returning = @config.key?(:insert_returning) ? self.class.type_cast_config_to_boolean(@config[:insert_returning]) : true end diff --git a/lib/arjdbc/postgresql/oid_types.rb b/lib/arjdbc/postgresql/oid_types.rb index cd6e6cff9..a9b87d86a 100644 --- a/lib/arjdbc/postgresql/oid_types.rb +++ b/lib/arjdbc/postgresql/oid_types.rb @@ -67,6 +67,45 @@ def assert_valid_registration(oid, oid_type) # @private module OIDTypes + + # Shared cache of the (expensive) pg_type catalog query results, keyed by + # server identity. Lets new connections build their own (isolated) OID type + # map without re-running the catalog sweep against the database — the + # adapter-side complement to the driver's shared metadata cache. + # See docs/lazy-connection-remediation.md (A2). + @type_records_cache = {} + @type_records_mutex = Mutex.new + + class << self + # Cached catalog row-sets for +key+, or nil. Reads happen under the + # mutex; the (expensive) catalog queries that produce the rows run + # OUTSIDE the lock in the caller, so a slow query can't block other + # connections or deadlock if loading re-enters. + def get_type_records(key) + return nil if key.nil? + @type_records_mutex.synchronize { @type_records_cache[key] } + end + + # Cache +records+ for +key+ (first writer wins). A brief cold-start + # window may run the sweep more than once; it converges immediately. + # The structure is deep-frozen so the shared copy is provably immutable: + # any number of threads may read it concurrently and #run only ever reads + # (it rejects/extracts into fresh arrays), so no lock is needed on reads. + def store_type_records(key, records) + return if key.nil? + records.each { |set| set.each(&:freeze).freeze }.freeze + @type_records_mutex.synchronize { @type_records_cache[key] ||= records } + end + + # Invalidate the shared rows for +key+ (or all keys) so the next full + # load re-queries the catalog — used when types change (extensions/enums). + def clear_type_records_cache(key = nil) + @type_records_mutex.synchronize do + key ? @type_records_cache.delete(key) : @type_records_cache.clear + end + end + end + def get_oid_type(oid, fmod, column_name, sql_type = '') # :nodoc: # Note: type_map is storing a bunch of oid type prefixed with a namespace even # if they are not namespaced (e.g. ""."oidvector"). builtin types which are @@ -97,6 +136,10 @@ def get_oid_type(oid, fmod, column_name, sql_type = '') # :nodoc: def reload_type_map @lock.synchronize do + # Drop the shared catalog rows for this database so the next full load + # re-queries the catalog (types may have changed: extensions/enums). + OIDTypes.clear_type_records_cache(oid_cache_key) + if @type_map type_map.clear else @@ -204,13 +247,54 @@ def register_class_with_precision(...) def load_additional_types(oids = nil) # :nodoc: initializer = ArjdbcTypeMapInitializer.new(type_map) - load_types_queries(initializer, oids) do |query| - execute_and_clear(query, "SCHEMA", []) do |records| - initializer.run(records) + + if oids + # Lazy single-OID lookups (an unknown type seen at query time) always + # hit the catalog; they're rare and specific to the OID in question. + load_types_queries(initializer, oids) do |query| + execute_and_clear(query, "SCHEMA", []) do |records| + initializer.run(records) + end + end + else + # Full load. Replay the shared catalog rows when available; otherwise + # run the catalog queries once and cache the rows for other connections + # to the same database. #run reads but never mutates the rows, so each + # connection safely populates its own type_map from the shared copy. + if (cached = OIDTypes.get_type_records(oid_cache_key)) + cached.each { |records| initializer.run(records) } + else + # The three catalog queries are built lazily: each WHERE..IN clause + # depends on the types #run registered from the previous query, so we + # must run() between yields (not after) — while capturing the rows. + sets = [] + load_types_queries(initializer, nil) do |query| + execute_and_clear(query, "SCHEMA", []) do |records| + rows = records.to_a + sets << rows + initializer.run(rows) + end + end + OIDTypes.store_type_records(oid_cache_key, sets) end end end + # Identifies the database whose pg_type catalog a cached row-set describes. + # pg_type lives in pg_catalog: its contents are per-database and independent + # of search_path, so the cache is scoped by database NAME, qualified with + # host/port to disambiguate same-named databases on different servers. + # + # The JDBC URL is included as a final component so url-only configs (where + # the discrete :host/:port/:database keys may be absent) can never collide + # two different databases onto a nil key. When :database isn't given + # explicitly we recover it from the URL's path so the name still scopes it. + def oid_cache_key + database = @config[:database] || @config[:dbname] || + @config[:url].to_s[%r{//[^/]*/([^?]+)}, 1] + [@config[:host], @config[:port], database, @config[:url]] + end + def load_types_queries(initializer, oids) query = <<~SQL SELECT t.oid, t.typname, t.typelem, t.typdelim, t.typinput, r.rngsubtype, t.typtype, t.typbasetype From e7b25d450c39252ef811e6545d504449fc3a5750 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Thu, 25 Jun 2026 11:22:53 -0700 Subject: [PATCH 6/6] pin i18n to 1.15.1+ --- Gemfile | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Gemfile b/Gemfile index 57bdcd19f..705678aee 100644 --- a/Gemfile +++ b/Gemfile @@ -1,9 +1,9 @@ source "https://rubygems.org" if ENV['RAILS'] # Use local clone of Rails - rails_dir = ENV['RAILS'] + rails_dir = ENV['RAILS'] activerecord_dir = ::File.join(rails_dir, 'activerecord') - + if !::File.exist?(rails_dir) && !::File.exist?(activerecord_dir) raise "ENV['RAILS'] set but does not point at a valid rails clone" end @@ -21,7 +21,7 @@ if ENV['RAILS'] # Use local clone of Rails elsif ENV['AR_VERSION'] # Use specific version of AR and not .gemspec version version = ENV['AR_VERSION'] - + if !version.eql?('false') # Don't bundle any versions of AR; use LOAD_PATH # Specified as raw number. Use normal gem require. if version =~ /^([0-9.])+(_)?(rc|RC|beta|BETA|PR|pre)*([0-9.])*$/ @@ -41,7 +41,7 @@ elsif ENV['AR_VERSION'] # Use specific version of AR and not .gemspec version gem 'actionpack', require: false gem 'actionview', require: false end - + end end else @@ -54,9 +54,7 @@ else end end -# Cap i18n below 1.15.0: that release requires Ruby 3.2+, which we can't assume -# across the JRuby versions we support. -gem 'i18n', '< 1.15.0', require: nil +gem 'i18n', '>= 1.15.1', require: nil gem 'rake', require: nil @@ -101,7 +99,7 @@ end group :test do # for testing against different version(s) - if sqlite_version = ENV['JDBC_SQLITE_VERSION'] + if sqlite_version = ENV['JDBC_SQLITE_VERSION'] gem 'jdbc-sqlite3', sqlite_version, require: nil, platform: :jruby end