Repository: kagkarlsson/db-scheduler (java)

Issue:
Implementations of FailureHandler do not use Clock
# Prerequisites
We are using `ManualScheduler` and `SettableClock` in our tests to control the task lifecycle and everything was working as expected before we started to use `MaxRetriesFailureHandler` and `ExponentialBackoffFailureHandler`.


`ExponentialBackoffFailureHandler` does not use `Clock` to get the current timestamp, but uses `Instant.now()` instead. This leads to a mixed behaviour in the tests and contradicts with the happy path execution timestamps which provided by `SettableClock`.

## Expected Behavior
Expected that `FailureHandlers` would use `Clock` (SettableClock) to schedule the next execution.

## Current Behavior

Currently the `Instant.now()` is used to calculate the next execution time.
https://github.com/kagkarlsson/db-scheduler/blob/8ffe82547ad7c81db02a82547e5f0477d0580e6a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java#L91

## Context
- DB-Scheduler Version : 1.16
- Java Version : 17
- Spring Boot (check for Yes) : [X]
- Database and Version :  




Discussion:

Patch:
diff --git a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java
--- a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java
+++ b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java
@@ -52,9 +52,10 @@ public void onFailure(
           round(
               sleepDuration.toMillis()
                   * pow(exponentialRate, executionComplete.getExecution().consecutiveFailures));
-      Instant nextTry = Instant.now().plusMillis(retryDurationMs);
+      Instant nextTry = executionComplete.getTimeDone().plusMillis(retryDurationMs);
       LOG.debug(
-          "Execution failed. Retrying task {} at {}",
+          "Execution failed {}. Retrying task {} at {}",
+          executionComplete.getTimeDone(),
           executionComplete.getExecution().taskInstance,
           nextTry);
       executionOperations.reschedule(executionComplete, nextTry);
@@ -101,7 +102,7 @@ public OnFailureRetryLater(Duration sleepDuration) {
     @Override
     public void onFailure(
         ExecutionComplete executionComplete, ExecutionOperations<T> executionOperations) {
-      Instant nextTry = Instant.now().plus(sleepDuration);
+      Instant nextTry = executionComplete.getTimeDone().plus(sleepDuration);
       LOG.debug(
           "Execution failed. Retrying task {} at {}",
           executionComplete.getExecution().taskInstance,


Test patch:
diff --git a/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerTest.java b/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerTest.java
--- a/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerTest.java
+++ b/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerTest.java
@@ -262,7 +262,7 @@ public void should_reschedule_failure_on_exponential_backoff_with_default_rate()
     scheduler.executeDue();
 
     // Simulate 30 minutes worth of time to validate we did not process more than we should
-    for (int minuteWorthOfTime = 1; minuteWorthOfTime <= 30; minuteWorthOfTime++) {
+    for (int minuteWorthOfTime = 1; minuteWorthOfTime <= 80; minuteWorthOfTime++) {
       clock.set(clock.now().plus(ofMinutes(1)));
       scheduler.executeDue();
     }
@@ -278,10 +278,10 @@ public void should_reschedule_failure_on_exponential_backoff_with_default_rate()
       Duration actualExponentialBackoffDuration = ofMillis(retryDurationMs);
       assertThat(
           scheduleTimeDifferenceFromFirstCall.getSeconds(),
-          greaterThanOrEqualTo(expectedSleepDuration.getSeconds()));
+          greaterThanOrEqualTo(expectedSleepDuration.minusSeconds(1).getSeconds()));
       assertThat(
           scheduleTimeDifferenceFromFirstCall.getSeconds(),
-          is(actualExponentialBackoffDuration.getSeconds()));
+          greaterThanOrEqualTo(actualExponentialBackoffDuration.minusSeconds(1).getSeconds()));
     }
   }
 
@@ -312,7 +312,7 @@ public void should_reschedule_failure_on_exponential_backoff_with_defined_rate()
     scheduler.executeDue();
 
     // Simulate 30 minutes worth of time to validate we did not process more than we should
-    for (int minuteWorthOfTime = 1; minuteWorthOfTime <= 30; minuteWorthOfTime++) {
+    for (int minuteWorthOfTime = 1; minuteWorthOfTime <= 80; minuteWorthOfTime++) {
       clock.set(clock.now().plus(ofMinutes(1)));
       scheduler.executeDue();
     }
@@ -320,19 +320,21 @@ public void should_reschedule_failure_on_exponential_backoff_with_defined_rate()
     assertThat(executionTimes.size(), is(10));
     // Skip first execution of this b/c it was not using the exponential backoff but the first
     // attempted call before failure
+    Duration lastScheduleTimeDifferenceFromFirstCall = ZERO;
     for (int i = 1, executionTimesSize = executionTimes.size(); i < executionTimesSize; i++) {
       final Instant executionTime = executionTimes.get(i);
       long retryDurationMs =
           Math.round(expectedSleepDuration.toMillis() * Math.pow(customRate, i - 1));
 
-      Duration scheduleTimeDifferenceFromFirstCall = between(firstExecution, executionTime);
-      Duration actualExponentialBackoffDuration = ofMillis(retryDurationMs);
+      Duration expectedTimeDifferenceFromFirstCall =
+          ofMillis(retryDurationMs).plus(lastScheduleTimeDifferenceFromFirstCall);
+      lastScheduleTimeDifferenceFromFirstCall = between(firstExecution, executionTime);
       assertThat(
-          scheduleTimeDifferenceFromFirstCall.getSeconds(),
-          greaterThanOrEqualTo(expectedSleepDuration.getSeconds()));
+          lastScheduleTimeDifferenceFromFirstCall.getSeconds(),
+          greaterThanOrEqualTo(expectedSleepDuration.minusSeconds(1).getSeconds()));
       assertThat(
-          scheduleTimeDifferenceFromFirstCall.getSeconds(),
-          is(actualExponentialBackoffDuration.getSeconds()));
+          lastScheduleTimeDifferenceFromFirstCall.getSeconds(),
+          greaterThanOrEqualTo(expectedTimeDifferenceFromFirstCall.minusSeconds(1).getSeconds()));
     }
   }
 }