Draft: fix COMMIT/ROLLBACK retry data-loss footgun (PostgreSQL & MySQL)

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,6 +54,8 @@ else
   end
 end
 
+gem 'i18n', '>= 1.15.1', require: nil
+
 gem 'rake', require: nil
 
 group :test do
@@ -97,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
 

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

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,14 +66,24 @@ 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
       end
 
       ########################## 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, 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.
       # @param name the save-point name
@@ -74,7 +92,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 +105,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 +118,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

lib/arjdbc/mysql/adapter.rb

@@ -252,7 +252,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 +298,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?

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
@@ -593,7 +602,10 @@ 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.
+      execute('SHOW client_min_messages', 'SCHEMA').first&.values&.first
     end
 
     # Set the client message level.
@@ -963,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

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