Merge pull request #1218 from skunkworker/pr/retry-correctness

Fix COMMIT/ROLLBACK/savepoint retry data-loss; harden MySQL connection-loss detection

Author: headius

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?

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

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