Keep executions for each specific exception (#34352)

* Keep executions for each specific declaration

Fixes #34337

ActiveJob used the global executions counter to control the number of
times a job should be retried. The problem with this approach was that
in case a job raised different exceptions during its executions they
weren't retried the number of times defined by their `attemps` number.

**Example:**

Having the following job:
```ruby
class BuggyJob < ActiveJob::Base
  retry_on CustomException, attemps: 3
  retry_on OtherException, attempts: 3
end
```
If the job raised `CustomException` in the first two executions and then
it raised `OtherException`, the job wasn't retried anymore because the
global executions counter was already indicating 3 attempts.

With this patch each `retry_on` declaration has its specific counter so
that the first two executions that raise `CustomException` don't affect
the retries count that future exceptions may have.

* Revert "clarifies documentation around the attempts arugment to retry_on"

This reverts commit 86aa8f8c5631f77ed9a208e5107003c01512133e.

5 files changed, 51 insertions, 4 deletions

diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md
index e5cbff4a88..8e18faeb02 100644
--- a/activejob/CHANGELOG.md
+++ b/activejob/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Keep executions for each specific declaration
+
+    Each `retry_on` declaration has now its own specific executions counter. Before it was
+    shared between all executions of a job.
+
+    *Alberto Almagro*
+
 *   Allow all assertion helpers that have a `only` and `except` keyword to accept
     Procs.
 
diff --git a/activejob/lib/active_job/core.rb b/activejob/lib/active_job/core.rb
index 698153636b..4ab62f89b0 100644
--- a/activejob/lib/active_job/core.rb
+++ b/activejob/lib/active_job/core.rb
@@ -28,6 +28,12 @@ module ActiveJob
     # Number of times this job has been executed (which increments on every retry, like after an exception).
     attr_accessor :executions
 
+    # Hash that contains the number of times this job handled errors for each specific retry_on declaration.
+    # Keys are the string representation of the exceptions listed in the retry_on declaration,
+    # while its associated value holds the number of executions where the corresponding retry_on
+    # declaration handled one of its listed exceptions.
+    attr_accessor :exception_executions
+
     # I18n.locale to be used during the job.
     attr_accessor :locale
 
@@ -75,6 +81,7 @@ module ActiveJob
       @queue_name = self.class.queue_name
       @priority   = self.class.priority
       @executions = 0
+      @exception_executions = Hash.new(0)
     end
 
     # Returns a hash with the job data that can safely be passed to the
@@ -88,6 +95,7 @@ module ActiveJob
         "priority"   => priority,
         "arguments"  => serialize_arguments_if_needed(arguments),
         "executions" => executions,
+        "exception_executions" => exception_executions,
         "locale"     => I18n.locale.to_s,
         "timezone"   => Time.zone.try(:name)
       }
@@ -126,6 +134,7 @@ module ActiveJob
       self.priority             = job_data["priority"]
       self.serialized_arguments = job_data["arguments"]
       self.executions           = job_data["executions"]
+      self.exception_executions = job_data["exception_executions"]
       self.locale               = job_data["locale"] || I18n.locale.to_s
       self.timezone             = job_data["timezone"] || Time.zone.try(:name)
     end
diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb
index bc9e168971..53984a4e49 100644
--- a/activejob/lib/active_job/exceptions.rb
+++ b/activejob/lib/active_job/exceptions.rb
@@ -9,7 +9,6 @@ module ActiveJob
 
     module ClassMethods
       # Catch the exception and reschedule job for re-execution after so many seconds, for a specific number of attempts.
-      # The number of attempts includes the total executions of a job, not just the retried executions.
       # If the exception keeps getting raised beyond the specified number of attempts, the exception is allowed to
       # bubble up to the underlying queuing system, which may have its own retry mechanism or place it in a
       # holding queue for inspection.
@@ -22,8 +21,7 @@ module ActiveJob
       #   as a computing proc that the number of executions so far as an argument, or as a symbol reference of
       #   <tt>:exponentially_longer</tt>, which applies the wait algorithm of <tt>(executions ** 4) + 2</tt>
       #   (first wait 3s, then 18s, then 83s, etc)
-      # * <tt>:attempts</tt> - Re-enqueues the job the specified number of times (default: 5 attempts),
-      #   attempts here refers to the total number of times the job is executed, not just retried executions
+      # * <tt>:attempts</tt> - Re-enqueues the job the specified number of times (default: 5 attempts)
       # * <tt>:queue</tt> - Re-enqueues the job on a different queue
       # * <tt>:priority</tt> - Re-enqueues the job with a different priority
       #
@@ -46,7 +44,9 @@ module ActiveJob
       #  end
       def retry_on(*exceptions, wait: 3.seconds, attempts: 5, queue: nil, priority: nil)
         rescue_from(*exceptions) do |error|
-          if executions < attempts
+          exception_executions[exceptions.to_s] += 1
+
+          if exception_executions[exceptions.to_s] < attempts
             retry_job wait: determine_delay(wait), queue: queue, priority: priority, error: error
           else
             if block_given?
diff --git a/activejob/test/cases/exceptions_test.rb b/activejob/test/cases/exceptions_test.rb
index 37bb65538a..b5328b8d6a 100644
--- a/activejob/test/cases/exceptions_test.rb
+++ b/activejob/test/cases/exceptions_test.rb
@@ -30,6 +30,28 @@ class ExceptionsTest < ActiveJob::TestCase
     end
   end
 
+  test "keeps the same attempts counter when several exceptions are listed in the same declaration" do
+    exceptions_to_raise = %w(FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo
+                             SecondRetryableErrorOfTwo SecondRetryableErrorOfTwo)
+
+    assert_raises SecondRetryableErrorOfTwo do
+      perform_enqueued_jobs do
+        ExceptionRetryJob.perform_later(exceptions_to_raise)
+      end
+    end
+  end
+
+  test "keeps a separate attempts counter for each individual declaration" do
+    exceptions_to_raise = %w(FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo
+                             DefaultsError DefaultsError)
+
+    assert_nothing_raised do
+      perform_enqueued_jobs do
+        ExceptionRetryJob.perform_later(exceptions_to_raise)
+      end
+    end
+  end
+
   test "failed retry job when exception kept occurring against defaults" do
     perform_enqueued_jobs do
       begin
diff --git a/activejob/test/jobs/retry_job.rb b/activejob/test/jobs/retry_job.rb
index 68dc17e16c..2d19d4c41e 100644
--- a/activejob/test/jobs/retry_job.rb
+++ b/activejob/test/jobs/retry_job.rb
@@ -39,3 +39,12 @@ class RetryJob < ActiveJob::Base
     end
   end
 end
+
+class ExceptionRetryJob < ActiveJob::Base
+  retry_on FirstRetryableErrorOfTwo, SecondRetryableErrorOfTwo, attempts: 4
+  retry_on DefaultsError
+
+  def perform(exceptions)
+    raise exceptions.shift.constantize.new unless exceptions.empty?
+  end
+end