Disable ActiveJob retry jitter when given zero/falsey value (rails#38003

) * Add failing ActiveJob exceptions test for "disable retry jitter" Thanks to @kaspth for the starting point. * Update ActiveJob retry jitter to correctly use zero value * Simplify "disable retry jitter" test We don't need to repeat this many times. Fewer is shorter. * Refactor determine_delay with jitter * Fix indentation * Close the curtains and give JITTER_DEFAULT some privacy * Use .zero? instead of == to check jitter value * Add ActiveJob test for explicit zero jitter Co-authored-by: Kasper Timm Hansen <[email protected]> Co-authored-by: Cliff Pruitt <[email protected]>
sugi · Dec 17, 2019 · db0bc5e · db0bc5e
1 parent f8bda7c
commit db0bc5e
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 10 deletions.
diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb
@@ -53,7 +53,7 @@ module ClassMethods
       #      # Might raise Net::OpenTimeout or Timeout::Error when the remote service is down
       #    end
       #  end
-      def retry_on(*exceptions, wait: 3.seconds, attempts: 5, queue: nil, priority: nil, jitter: nil)
+      def retry_on(*exceptions, wait: 3.seconds, attempts: 5, queue: nil, priority: nil, jitter: JITTER_DEFAULT)
         rescue_from(*exceptions) do |error|
           executions = executions_for(exceptions)
           if executions < attempts
@@ -126,17 +126,21 @@ def retry_job(options = {})
     end
 
     private
-      def determine_delay(seconds_or_duration_or_algorithm:, executions:, jitter: nil)
-        jitter ||= self.class.retry_jitter
+      JITTER_DEFAULT = Object.new
+      private_constant :JITTER_DEFAULT
+
+      def determine_delay(seconds_or_duration_or_algorithm:, executions:, jitter: JITTER_DEFAULT)
+        jitter = jitter == JITTER_DEFAULT ? self.class.retry_jitter : (jitter || 0.0)
+
         case seconds_or_duration_or_algorithm
         when :exponentially_longer
-          ((executions**4) + (Kernel.rand((executions**4) * jitter))) + 2
-        when ActiveSupport::Duration
-          duration = seconds_or_duration_or_algorithm.to_i
-          duration + Kernel.rand(duration * jitter)
-        when Integer
-          seconds = seconds_or_duration_or_algorithm
-          seconds + (Kernel.rand(seconds * jitter).ceil)
+          delay = executions**4
+          delay_jitter = determine_jitter_for_delay(delay, jitter)
+          delay + delay_jitter + 2
+        when ActiveSupport::Duration, Integer
+          delay = seconds_or_duration_or_algorithm.to_i
+          delay_jitter = determine_jitter_for_delay(delay, jitter)
+          delay + delay_jitter
         when Proc
           algorithm = seconds_or_duration_or_algorithm
           algorithm.call(executions)
@@ -145,6 +149,11 @@ def determine_delay(seconds_or_duration_or_algorithm:, executions:, jitter: nil)
         end
       end
 
+      def determine_jitter_for_delay(delay, jitter)
+        return 0.0 if jitter.zero?
+        Kernel.rand(delay * jitter)
+      end
+
       def executions_for(exceptions)
         if exception_executions
           exception_executions[exceptions.to_s] = (exception_executions[exceptions.to_s] || 0) + 1

diff --git a/activejob/test/cases/exceptions_test.rb b/activejob/test/cases/exceptions_test.rb
@@ -154,6 +154,38 @@ class ExceptionsTest < ActiveSupport::TestCase
     ActiveJob::Base.retry_jitter = old_jitter
   end
 
+  test "retry jitter disabled with nil" do
+    travel_to Time.now
+
+    Kernel.stub(:rand, ->(arg) { arg }) do
+      RetryJob.perform_later "DisabledJitterError", 3, :log_scheduled_at
+
+      assert_equal [
+        "Raised DisabledJitterError for the 1st time",
+        "Next execution scheduled at #{(Time.now + 3.seconds).to_f}",
+        "Raised DisabledJitterError for the 2nd time",
+        "Next execution scheduled at #{(Time.now + 3.seconds).to_f}",
+        "Successfully completed job"
+      ], JobBuffer.values
+    end
+  end
+
+  test "retry jitter disabled with zero" do
+    travel_to Time.now
+
+    Kernel.stub(:rand, ->(arg) { arg }) do
+      RetryJob.perform_later "ZeroJitterError", 3, :log_scheduled_at
+
+      assert_equal [
+        "Raised ZeroJitterError for the 1st time",
+        "Next execution scheduled at #{(Time.now + 3.seconds).to_f}",
+        "Raised ZeroJitterError for the 2nd time",
+        "Next execution scheduled at #{(Time.now + 3.seconds).to_f}",
+        "Successfully completed job"
+      ], JobBuffer.values
+    end
+  end
+
   test "custom wait retrying job" do
     travel_to Time.now
 

diff --git a/activejob/test/jobs/retry_job.rb b/activejob/test/jobs/retry_job.rb
@@ -4,6 +4,8 @@
 require "active_support/core_ext/integer/inflections"
 
 class DefaultsError < StandardError; end
+class DisabledJitterError < StandardError; end
+class ZeroJitterError < StandardError; end
 class FirstRetryableErrorOfTwo < StandardError; end
 class SecondRetryableErrorOfTwo < StandardError; end
 class LongWaitError < StandardError; end
@@ -18,6 +20,8 @@ class CustomDiscardableError < StandardError; end
 
 class RetryJob < ActiveJob::Base
   retry_on DefaultsError
+  retry_on DisabledJitterError, jitter: nil
+  retry_on ZeroJitterError, jitter: 0.0
   retry_on FirstRetryableErrorOfTwo, SecondRetryableErrorOfTwo, attempts: 4
   retry_on LongWaitError, wait: 1.hour, attempts: 10
   retry_on ShortWaitTenAttemptsError, wait: 1.second, attempts: 10