Merge pull request rails#46046 from adrianna-chang-shopify/ac-db-retries-with-timeout

Take into account timeout limit when retrying queries

Author: byroot

activerecord/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Allow applications to set retry deadline for query retries.
+
+    Building on the work done in #44576 and #44591, we extend the logic that automatically
+    reconnects database connections to take into account a timeout limit. We won't retry
+    a query if a given amount of time has elapsed since the query was first attempted. This
+    value defaults to nil, meaning that all retryable queries are retried regardless of time elapsed,
+    but this can be changed via the `retry_deadline` option in the database config.
+
+    *Adrianna Chang*
+
 *   Fix a case where the query cache can return wrong values. See #46044
 
     *Aaron Patterson*

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -169,6 +169,14 @@ def connection_retries
         (@config[:connection_retries] || 1).to_i
       end
 
+      def retry_deadline
+        if @config[:retry_deadline]
+          @config[:retry_deadline].to_f
+        else
+          nil
+        end
+      end
+
       def default_timezone
         @default_timezone || ActiveRecord.default_timezone
       end
@@ -582,6 +590,7 @@ def active?
       # instead.
       def reconnect!(restore_transactions: false)
         retries_available = connection_retries
+        deadline = retry_deadline && Process.clock_gettime(Process::CLOCK_MONOTONIC) + retry_deadline
 
         @lock.synchronize do
           reconnect
@@ -596,8 +605,9 @@ def reconnect!(restore_transactions: false)
           end
         rescue => original_exception
           translated_exception = translate_exception_class(original_exception, nil, nil)
+          retry_deadline_exceeded = deadline && deadline < Process.clock_gettime(Process::CLOCK_MONOTONIC)
 
-          if retries_available > 0
+          if !retry_deadline_exceeded && retries_available > 0
             retries_available -= 1
 
             if retryable_connection_error?(translated_exception)
@@ -872,7 +882,8 @@ def reconnect_can_restore_state?
         #
         # If +allow_retry+ is true, a connection-related exception will
         # cause an automatic reconnect and re-run of the block, up to
-        # the connection's configured +connection_retries+ setting.
+        # the connection's configured +connection_retries+ setting
+        # and the configured +retry_deadline+ limit.
         #
         # If +uses_transaction+ is false, the block will be run without
         # ensuring virtual transactions have been materialized in the DB
@@ -901,6 +912,7 @@ def with_raw_connection(allow_retry: false, uses_transaction: true)
             materialize_transactions if uses_transaction
 
             retries_available = allow_retry ? connection_retries : 0
+            deadline = retry_deadline && Process.clock_gettime(Process::CLOCK_MONOTONIC) + retry_deadline
             reconnectable = reconnect_can_restore_state?
 
             if @verified
@@ -927,8 +939,9 @@ def with_raw_connection(allow_retry: false, uses_transaction: true)
               result
             rescue => original_exception
               translated_exception = translate_exception_class(original_exception, nil, nil)
+              retry_deadline_exceeded = deadline && deadline < Process.clock_gettime(Process::CLOCK_MONOTONIC)
 
-              if retries_available > 0
+              if !retry_deadline_exceeded && retries_available > 0
                 retries_available -= 1
 
                 if retryable_query_error?(translated_exception)

activerecord/test/cases/adapter_test.rb

@@ -651,6 +651,45 @@ def teardown
         assert_operator Post.count, :>, 0
       end
 
+      test "can reconnect and retry queries under limit when retry deadline is set" do
+        attempts = 0
+        @connection.stub(:retry_deadline, 0.1) do
+          @connection.send(:with_raw_connection, allow_retry: true) do
+            if attempts == 0
+              attempts += 1
+              raise ActiveRecord::ConnectionFailed.new("Something happened to the connection")
+            end
+          end
+        end
+      end
+
+      test "does not reconnect and retry queries when retries are disabled" do
+        assert_raises(ActiveRecord::ConnectionFailed) do
+          attempts = 0
+          @connection.send(:with_raw_connection) do
+            if attempts == 0
+              attempts += 1
+              raise ActiveRecord::ConnectionFailed.new("Something happened to the connection")
+            end
+          end
+        end
+      end
+
+      test "does not reconnect and retry queries that exceed retry deadline" do
+        assert_raises(ActiveRecord::ConnectionFailed) do
+          attempts = 0
+          @connection.stub(:retry_deadline, 0.1) do
+            @connection.send(:with_raw_connection, allow_retry: true) do
+              if attempts == 0
+                sleep(0.2)
+                attempts += 1
+                raise ActiveRecord::ConnectionFailed.new("Something happened to the connection")
+              end
+            end
+          end
+        end
+      end
+
       private
         def raw_transaction_open?(connection)
           case connection.class::ADAPTER_NAME

guides/source/configuring.md

@@ -2822,6 +2822,33 @@ development:
   use_metadata_table: false
 ```
 
+#### Configuring Retry Behaviour
+
+By default, Rails will automatically reconnect to the database server and retry certain queries
+if something goes wrong. Only safely retryable (idempotent) queries will be retried. The number
+of retries can be specified in your the database configuration via `connection_retries`, or disabled
+by setting the value to 0. The default number of retries is 1.
+
+```yaml
+development:
+  adapter: mysql2
+  connection_retries: 3
+```
+
+The database config also allows a `retry_deadline` to be configured. If a `retry_deadline` is configured,
+an otherwise-retryable query will _not_ be retried if the specified time has elapsed while the query was
+first tried. For example, a `retry_deadline` of 5 seconds means that if 5 seconds have passed since a query
+was first attempted, we won't retry the query, even if it is idempotent and there are `connection_retries` left.
+
+This value defaults to nil, meaning that all retryable queries are retried regardless of time elapsed.
+The value for this config should be specified in seconds.
+
+```yaml
+development:
+  adapter: mysql2
+  retry_deadline: 5 # Stop retrying queries after 5 seconds
+```
+
 ### Creating Rails Environments
 
 By default Rails ships with three environments: "development", "test", and "production". While these are sufficient for most use cases, there are circumstances when you want more environments.