chore: bring retry behavior inline with RetryHelper

BenWhitehead · BenWhitehead · commit 3d2520dd6686 · 2025-03-14T17:38:20.000-04:00
Add test to ensure same number of attempts for the same retry settings.
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/RetryContext.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/RetryContext.java
@@ -129,7 +129,7 @@ <T extends Throwable> void recordError(T t, OnSuccess onSuccess, OnFailure<T> on
         maxAttempts = Integer.MAX_VALUE;
       }
       boolean shouldRetry = algorithm.shouldRetry(t, null);
-      Duration elapsedOverall = backoff.getCumulativeBackoff().plus(elapsed);
+      Duration cumulativeBackoff = backoff.getCumulativeBackoff();
       BackoffResult nextBackoff = backoff.nextBackoff(elapsed);
       String msgPrefix = null;
       if (shouldRetry && failureCount >= maxAttempts) {
@@ -167,7 +167,7 @@ <T extends Throwable> void recordError(T t, OnSuccess onSuccess, OnFailure<T> on
                 maxAttempts == Integer.MAX_VALUE
                     ? ""
                     : String.format(", maxAttempts: %d", maxAttempts),
-                elapsedOverall,
+                cumulativeBackoff,
                 nextBackoff.errorString(),
                 Durations.eq(backoff.getTimeout(), Durations.EFFECTIVE_INFINITY)
                     ? ""
@@ -291,6 +291,7 @@ public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit)
       return new DirectScheduledFuture(unit, delay, command);
     }
 
+    // <editor-fold desc="UnsupportedOperations">
     @Override
     public <V> ScheduledFuture<V> schedule(Callable<V> callable, long delay, TimeUnit unit) {
       throw new UnsupportedOperationException();
@@ -377,6 +378,7 @@ public <T> T invokeAny(Collection<? extends Callable<T>> tasks, long timeout, Ti
     public void execute(Runnable command) {
       throw new UnsupportedOperationException();
     }
+    // </editor-fold>
 
     private final class DirectScheduledFuture implements ScheduledFuture<Object> {
 
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/RetryContextTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/RetryContextTest.java
@@ -23,22 +23,28 @@
 import com.google.api.core.NanoClock;
 import com.google.api.gax.grpc.GrpcStatusCode;
 import com.google.api.gax.retrying.BasicResultRetryAlgorithm;
+import com.google.api.gax.retrying.ResultRetryAlgorithm;
 import com.google.api.gax.retrying.RetrySettings;
 import com.google.api.gax.rpc.ApiException;
 import com.google.api.gax.rpc.ApiExceptionFactory;
 import com.google.api.gax.rpc.ResourceExhaustedException;
+import com.google.cloud.RetryHelper;
+import com.google.cloud.RetryHelper.RetryHelperException;
 import com.google.cloud.storage.Backoff.Jitterer;
 import com.google.cloud.storage.RetryContext.OnFailure;
 import com.google.cloud.storage.RetryContext.OnSuccess;
 import com.google.cloud.storage.Retrying.RetryingDependencies;
+import com.google.common.base.Stopwatch;
 import io.grpc.Status.Code;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 import org.junit.Before;
@@ -73,7 +79,7 @@ public void retryable_when_maxAttemptBudget_consumed() {
               Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
           assertThat(suppressedMessages)
               .containsExactly(
-                  "Operation failed to complete within attempt budget (attempts: 1, maxAttempts: 1, elapsed: PT0.001S, nextBackoff: PT3S)");
+                  "Operation failed to complete within attempt budget (attempts: 1, maxAttempts: 1, elapsed: PT0S, nextBackoff: PT3S)");
         });
   }
 
@@ -110,7 +116,7 @@ public void retryable_maxAttemptBudget_still_available() {
               Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
           assertThat(suppressedMessages)
               .containsExactly(
-                  "Operation failed to complete within attempt budget (attempts: 3, maxAttempts: 3, elapsed: PT15.003S, nextBackoff: PT3S) previous failures follow in order of occurrence",
+                  "Operation failed to complete within attempt budget (attempts: 3, maxAttempts: 3, elapsed: PT9.002S, nextBackoff: PT3S) previous failures follow in order of occurrence",
                   "{unavailable}",
                   "{internal}");
         });
@@ -133,7 +139,7 @@ public void nonretryable_regardlessOfAttemptBudget() {
               Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
           assertThat(suppressedMessages)
               .containsExactly(
-                  "Unretryable error (attempts: 1, maxAttempts: 3, elapsed: PT0.001S, nextBackoff: PT3S)");
+                  "Unretryable error (attempts: 1, maxAttempts: 3, elapsed: PT0S, nextBackoff: PT3S)");
         });
   }
 
@@ -167,7 +173,7 @@ public boolean shouldRetry(Throwable previousThrowable, Object previousResponse)
               Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
           assertThat(suppressedMessages)
               .containsExactly(
-                  "Unretryable error (attempts: 3, maxAttempts: 6, elapsed: PT15.003S, nextBackoff: PT3S) previous failures follow in order of occurrence",
+                  "Unretryable error (attempts: 3, maxAttempts: 6, elapsed: PT9.002S, nextBackoff: PT3S) previous failures follow in order of occurrence",
                   "{unavailable}",
                   "{internal}");
         });
@@ -204,7 +210,7 @@ public boolean shouldRetry(Throwable previousThrowable, Object previousResponse)
               Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
           assertThat(suppressedMessages)
               .containsExactly(
-                  "Unretryable error (attempts: 1, maxAttempts: 6, elapsed: PT9.003S, nextBackoff: PT3S)");
+                  "Unretryable error (attempts: 1, maxAttempts: 6, elapsed: PT9.002S, nextBackoff: PT3S)");
         });
   }
 
@@ -252,7 +258,7 @@ public void retryable_when_timeoutBudget_consumed() {
               Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
           assertThat(suppressedMessages)
               .containsExactly(
-                  "Operation failed to complete within backoff budget (attempts: 3, elapsed: PT24S, nextBackoff: EXHAUSTED, timeout: PT24S) previous failures follow in order of occurrence",
+                  "Operation failed to complete within backoff budget (attempts: 3, elapsed: PT15S, nextBackoff: EXHAUSTED, timeout: PT24S) previous failures follow in order of occurrence",
                   "{unavailable 1}",
                   "{unavailable 2}");
         });
@@ -315,6 +321,65 @@ public void recordErrorWhileAlreadyInBackoffTruncatesExistingBackoffAndReevaluat
     }
   }
 
+  @Test
+  public void similarToRetryingHelper() {
+    RetrySettings retrySettings =
+        StorageOptions.getDefaultRetrySettings()
+            .toBuilder()
+            .setTotalTimeoutDuration(Duration.ofMillis(3_125))
+            .setInitialRetryDelayDuration(Duration.ofNanos(12_500_000))
+            .setRetryDelayMultiplier(2.0)
+            .setMaxRetryDelayDuration(Duration.ofSeconds(2))
+            .setMaxAttempts(6)
+            .setJittered(false)
+            .build();
+    ResultRetryAlgorithm<?> alg =
+        new BasicResultRetryAlgorithm<Object>() {
+          @Override
+          public boolean shouldRetry(Throwable previousThrowable, Object previousResponse) {
+            return previousThrowable instanceof Invocation;
+          }
+        };
+    ApiClock clock = NanoClock.getDefaultClock();
+
+    RetryContext ctx =
+        RetryContext.of(
+            RetryContext.directScheduledExecutorService(),
+            RetryingDependencies.simple(clock, retrySettings),
+            alg,
+            Jitterer.noJitter());
+
+    List<Duration> retryHelperSplits = new ArrayList<>();
+    Stopwatch retryHelperStopwatch = Stopwatch.createStarted();
+    try {
+      RetryHelper.runWithRetries(
+          () -> {
+            retryHelperSplits.add(retryHelperStopwatch.elapsed());
+            throw new Invocation();
+          },
+          retrySettings,
+          alg,
+          clock);
+    } catch (RetryHelperException ignore) {
+    }
+
+    List<Duration> retryContextSplits = new ArrayList<>();
+    Stopwatch retryContextStopwatch = Stopwatch.createStarted();
+    ctx.reset();
+    AtomicBoolean attemptAgain = new AtomicBoolean(false);
+    do {
+      attemptAgain.set(false);
+      try {
+        retryContextSplits.add(retryContextStopwatch.elapsed());
+        throw new Invocation();
+      } catch (Exception e) {
+        ctx.recordError(e, () -> attemptAgain.set(true), noop -> {});
+      }
+    } while (attemptAgain.get());
+
+    assertThat(retryContextSplits.size()).isEqualTo(retryHelperSplits.size());
+  }
+
   private static ApiException apiException(Code code, String message) {
     return ApiExceptionFactory.createException(message, null, GrpcStatusCode.of(code), false);
   }
@@ -392,4 +457,10 @@ public void release() {
       cdl.countDown();
     }
   }
+
+  static final class Invocation extends Exception {
+    private Invocation() {
+      super();
+    }
+  }
 }
