[I] retry pattern executes one extra attempt after maxAttempts exhausted [pekko]

["He-Pin (via GitHub)" <gi...@apache.org>]

He-Pin opened a new issue, #3233:
URL: https://github.com/apache/pekko/issues/3233

   ### Motivation
   
   `pekko.pattern.retry` invokes the `attempt` function `N+1` times when `attempts=N` is specified. This off-by-one error means users get one more side-effecting invocation than expected — an extra network call, database query, or other operation that the `attempts` parameter was supposed to cap.
   
   ### Current behavior
   
   In `RetrySupport.scala`, the private `retry` method (line 382–396) has an `if/else` branch:
   
   ```scala
   // RetrySupport.scala:382-396
   if (maxAttempts - attempted > 0) {
     val result = tryAttempt()
     // ... on failure, recurse via doRetry(attempted + 1)
   } else {
     tryAttempt()  // <-- executes after maxAttempts is exhausted
   }
   ```
   
   When `maxAttempts - attempted <= 0` (all attempts used), the `else` branch still calls `tryAttempt()`, producing one extra invocation.
   
   Trace with `attempts=5`, all failures:
   
   | Recursive call | `attempted` | Guard `5 - attempted > 0` | `tryAttempt()` invoked? |
   |---|---|---|---|
   | 1 | 0 | 5 > 0 — true | Yes (attempt #1) |
   | 2 | 1 | 4 > 0 — true | Yes (attempt #2) |
   | 3 | 2 | 3 > 0 — true | Yes (attempt #3) |
   | 4 | 3 | 2 > 0 — true | Yes (attempt #4) |
   | 5 | 4 | 1 > 0 — true | Yes (attempt #5) |
   | 6 | 5 | 0 > 0 — **false** | **Yes (attempt #6 — extra)** |
   
   The existing tests in `RetrySpec.scala` encode this behavior. For example, the test "return a failure for a Future that would have succeeded but retries were exhausted" (line 89) uses `attempts=5` and expects the error message `"6"` — proving 6 invocations.
   
   Edge case: `attempts=0` still calls `tryAttempt()` once, which contradicts the meaning of "zero attempts."
   
   ### Expected behavior
   
   `attempts=N` should invoke the attempt function exactly N times. When all attempts are exhausted, the method should return the result of the last failed attempt rather than executing one more time.
   
   The parameter is named `attempts` (not `retries`) and is documented as `@param attempts the maximum number of attempts`, which should mean N total invocations.
   
   ### Code evidence
   
   - `actor/src/main/scala/org/apache/pekko/pattern/RetrySupport.scala:394-396` — the `else` branch that calls `tryAttempt()` after maxAttempts is exhausted
   - `actor/src/main/scala/org/apache/pekko/pattern/RetrySupport.scala:382` — the guard `if (maxAttempts - attempted > 0)` that allows exactly `maxAttempts` entries into the if-branch, then falls through to the extra else-branch invocation
   - `actor-tests/src/test/scala/org/apache/pekko/pattern/RetrySpec.scala:89-93` — test expects `"6"` with `attempts=5`, confirming N+1 behavior
   - `actor-tests/src/test/scala/org/apache/pekko/pattern/RetrySpec.scala:128-133` — test expects `"1000"` with `attempts=999`, confirming N+1 behavior
   
   ### Reproduction
   
   ```scala
   import java.util.concurrent.atomic.AtomicInteger
   import scala.concurrent.{Await, Future}
   import scala.concurrent.duration._
   
   val counter = new AtomicInteger(0)
   val result = retry(
     attempt = () => { counter.incrementAndGet(); Future.failed(new RuntimeException("fail")) },
     attempts = 3,
     delay = 1.milli
   )(system.dispatcher, system.scheduler)
   
   Await.ready(result, 5.seconds)
   assert(counter.get() == 3)  // FAILS: actual value is 4
   ```
   
   ### Impact
   
   - Module: `pekko-actor`, package `org.apache.pekko.pattern`
   - Affects all overloads of `retry` (with/without delay, with/without `shouldRetry` predicate, with/without exponential backoff), since they all delegate to the same private `retry` method
   - Users who rely on `attempts` to limit side effects (rate-limited APIs, billing-per-request services, idempotency boundaries) will see one more invocation than allowed
   - **Note on binary compatibility**: fixing this will require updating the existing tests in `RetrySpec.scala` that encode the N+1 behavior


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org

Re: [I] retry pattern executes one extra attempt after maxAttempts exhausted [pekko]

["He-Pin (via GitHub)" <gi...@apache.org>]

He-Pin closed issue #3233: retry pattern executes one extra attempt after maxAttempts exhausted
URL: https://github.com/apache/pekko/issues/3233


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org