`DefaultClientRetryPolicy` potential overflow

[goulashify]

Hi!

Love the quality across the library!

Issue
This is very much a tentative bug report (I'm a novice in Go), and even if my interpretation is correct, this probably won't affect any sane real-world use cases due to a low base/error count.

That said, wouldn't this still be able to overflow? (for example if retrySecondsWithoutJitter returns maxDurationSeconds - 1)

Proposed fix
I'd add a clamping step after applying the jitter.

Adjacent simplificaton
Here the comments drifted from implementation, if I understood this correctly, attempt == (errorCount + 1) in all cases a job is retried or rescued and the real reason is backward compatibility with versions that wouldn't restore the pre-fetch attempt value on snooze. This is the comment we used in our port:

// In modern versions of River this is the same number as `attempt`, however in older version snoozing a job wouldn't
// restore its attempt count to the pre-fetch value, and that would lead to incorrect retry durations when jobs are
// first snoozed, then retried. To avoid this and keep backward compatibility, the number of errors are used instead.

No real dent, just wanted to chip in where I could. :)

Thanks for your work,
– Dani

[brandur]

Thanks Dani.

I didn't run the numbers, but I think overflow on jitter calculation is unlikely just due to the way exponentials tend to work. Say N^4 is the number that takes us past maxDurationSeconds. (N-1)^4 will be a much smaller number than N^4. Enough so that adding a 10% jitter will never take it past maxDurationSeconds.

I guess the only exception is if (N-1)^4 is right at the edge of maxDurationSeconds so that it's within 10%. I'm not sure whether that's the case for our chosen numbers, but I suppose it doesn't hurt to protect against it so I opened #1023.

Here the comments drifted from implementation, if I understood this correctly, attempt == (errorCount + 1) in all cases a job is retried or rescued and the real reason is backward compatibility with versions that wouldn't restore the pre-fetch attempt value on snooze. This is the comment we used in our port:

Ah yeah. Actually, if you read this paragraph just above, it was updated to capture the new snoozing behavior:

https://github.com/riverqueue/river/blob/9f16f8fa69ccd99d273cc86e037523b548bb91c3/retry_policy.go/#L39-L44

But you're right, that comment below is out-of-date. I pushed your version of the comment into #1023 as well.

[goulashify]

Thanks for the detailed explanation Brandur!