Add Proc as a type for ActiveJob retry_on attempts

[rosew]

Motivation / Background

This Pull Request has been created because I needed retry_on attempts: to use more complex logic than an Integer.

In my specific case I was creating a concern for Jobs that would use retry_on to allow jobs to quietly fail and retry. The concern was going to use a method to allow attempts to be customized in individual jobs.

    included do
      retry_on(QuietRetryError, wait: :exponentially_longer, attempts: quiet_retry_attempts) do
        # ...
      end
    end

However this ran into a state where the method needed to be defined and run when the concern was included instead of being available later in the job. It would be very fragile to have to define the method before including the concern. This lead to me to looking at if attempts accepted anything besides an integer. It did not but I saw there was already a pattern in place for wait to accept a Proc. I thought if that pattern was repeated for attempts than it would allow more flexibility for configuring attempts.

My original thought was to have the Proc for attempts return a number similar to what the Proc for wait does. However, on thinking about it I decided that still did not leave much flexibility because attempts would still be determined by a number. Instead I switched it to return true/false so if the next attempt occurred could be determined by whatever logic was implemented in the Proc.

Detail

This Pull Request changes ActiveJob retry_on attempts so it will accept a Proc.

ActiveJob retry_on allowed the wait option to be set as a Proc that calculates the wait but did not give the same option for attempts. attempts could be set to only a number of times to attempt or unlimited. However, jobs sometimes need different logic than number of executions to determine if they should try again.

This PR adds support for setting attempts to a Proc that returns either true or false. On true the retry will occur.

As a an example:

retry_on ProcRetryError, attempts: ->(executions) { executions < 2 }

Additional information

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• There are no typos in commit messages and comments.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Feature branch is up-to-date with main (if not - rebase it).
• Pull request only contains one commit for bug fixes and small features. If it's a larger feature, multiple commits are permitted but must be descriptive.
• Tests are added if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
• PR is not in a draft state.
• CI is passing.

[sambostock]

I wonder if it is clear from the name that it is meant to return a boolean? I could see someone returning 0 and running into trouble because it is truthy.

Would it make sense to have an alternate mutually exclusive parameter with a different name, rather than reusing attempts? Something like attempt_again or reattempt?

[rosew]

Oh, that is a good point. It does become less clear. Originally I was just going to make the proc return a number to be compared... but then I thought it would be good to add more flexibility by making it return true/false.

So two questions:

• What do you think of still allowing a Proc for attempts but making that proc return a number so it still gives flexibility to generate that number from something instead of passing in an Int?
• If that happens do you think I should also add support for :reattempt that is true/false? I don't need it for what I'm doing but I'd be happy to add it for the greater flexibility for others in the future. And if I do should it be part of this PR or a separate one?

[WillHalto]

Hi @rosew great PR! I just opened #46261 and was pointed over to this. I'd be in strong support of:

What do you think of still allowing a Proc for attempts but making that proc return a number so it still gives flexibility to generate that number from something instead of passing in an Int?

That would give good flexibility for most cases imo.

In #46261 I had intended to pass the job instance to the attempts proc, to allow callers to dynamically specify the desired number of attempts by doing something like

class MyJob < ActiveJob::Base
    retry_on CustomRetryException, attempts: ->(job){ job.arguments.last[:retries] || 5 }
    def perform(retries:)
        # job implementation
    end
end
MyJob.perform_later(retries: 10)
MyJob.perform_later(retries: 1)

What are your thoughts on doing something like that?

[rosew]

The main reason I passed in executions was because it mirrored the behavior of the other Proc accepted for delay. I don't have a strong opinion either way on if this would be a good case to break from the current pattern or not.

I do see how my method can allow it to be customized at the Class level where yours can allow it to be customized at the instance level.

I'd love thoughts from rails maintainers on how close we should stick to the other proc pattern and/or if there are reasons not to pass the into the proc.