fix: retry improvements (#198)

* fix: prefer retry-after, improved retry strategy

* fix: test optimization

* fix: rename method and use runAsync instead of supplyAsync

* review: fix hot-loop scenario

Author: jimmyjames

src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java

@@ -12,16 +12,13 @@
 import dev.openfga.sdk.util.RetryStrategy;
 import java.io.IOException;
 import java.io.PrintStream;
-import java.net.http.HttpClient;
-import java.net.http.HttpRequest;
-import java.net.http.HttpResponse;
+import java.net.http.*;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
-import java.time.Duration;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Optional;
+import java.time.*;
+import java.util.*;
 import java.util.concurrent.*;
+import java.util.function.Function;
 
 public class HttpRequestAttempt<T> {
     private final ApiClient apiClient;
@@ -83,10 +80,10 @@ public CompletableFuture<ApiResponse<T>> attemptHttpRequest() throws ApiExceptio
         addTelemetryAttribute(Attributes.HTTP_REQUEST_METHOD, request.method());
         addTelemetryAttribute(Attributes.USER_AGENT, configuration.getUserAgent());
 
-        return attemptHttpRequest(createClient(), 0, null);
+        return attemptHttpRequest(getHttpClient(), 0, null);
     }
 
-    private HttpClient createClient() {
+    private HttpClient getHttpClient() {
         return apiClient.getHttpClient();
     }
 
@@ -99,10 +96,11 @@ private CompletableFuture<ApiResponse<T>> attemptHttpRequest(
                         // Handle network errors (no HTTP response received)
                         return handleNetworkError(throwable, retryNumber);
                     }
-                    // No network error, proceed with normal HTTP response handling
+
+                    // Handle HTTP response (including error status codes)
                     return processHttpResponse(response, retryNumber, previousError);
                 })
-                .thenCompose(future -> future);
+                .thenCompose(Function.identity());
     }
 
     private CompletableFuture<ApiResponse<T>> handleNetworkError(Throwable throwable, int retryNumber) {
@@ -114,9 +112,7 @@ private CompletableFuture<ApiResponse<T>> handleNetworkError(Throwable throwable
             // Add telemetry for network error retry
             addTelemetryAttribute(Attributes.HTTP_REQUEST_RESEND_COUNT, String.valueOf(retryNumber + 1));
 
-            // Create delayed client and retry asynchronously without blocking
-            HttpClient delayingClient = getDelayedHttpClient(retryDelay);
-            return attemptHttpRequest(delayingClient, retryNumber + 1, throwable);
+            return delayedRetry(retryDelay, retryNumber + 1, throwable);
         } else {
             // Max retries exceeded, fail with the network error
             return CompletableFuture.failedFuture(new ApiException(throwable));
@@ -129,9 +125,30 @@ private CompletableFuture<ApiResponse<T>> handleHttpErrorRetry(
         Duration retryDelay =
                 RetryStrategy.calculateRetryDelay(retryAfterDelay, retryNumber, configuration.getMinimumRetryDelay());
 
-        // Create delayed client and retry asynchronously without blocking
-        HttpClient delayingClient = getDelayedHttpClient(retryDelay);
-        return attemptHttpRequest(delayingClient, retryNumber + 1, error);
+        return delayedRetry(retryDelay, retryNumber + 1, error);
+    }
+
+    /**
+     * Performs a delayed retry using CompletableFuture.delayedExecutor().
+     * This method centralizes the common delay logic used by both network error and HTTP error retries.
+     *
+     * @param retryDelay The duration to wait before retrying
+     * @param nextRetryNumber The next retry attempt number (1-based)
+     * @param previousError The previous error that caused the retry
+     * @return CompletableFuture that completes after the delay with the retry attempt
+     */
+    private CompletableFuture<ApiResponse<T>> delayedRetry(
+            Duration retryDelay, int nextRetryNumber, Throwable previousError) {
+        // Use CompletableFuture.delayedExecutor() to delay the retry attempt itself
+        return CompletableFuture.runAsync(
+                        () -> {
+                            // No-op task, we only care about the delay timing
+                        },
+                        CompletableFuture.delayedExecutor(retryDelay.toNanos(), TimeUnit.NANOSECONDS))
+                .thenCompose(ignored -> {
+                    // Get HttpClient when needed (just returns cached instance)
+                    return attemptHttpRequest(getHttpClient(), nextRetryNumber, previousError);
+                });
     }
 
     private CompletableFuture<ApiResponse<T>> processHttpResponse(
@@ -150,7 +167,6 @@ private CompletableFuture<ApiResponse<T>> processHttpResponse(
                 // Check if we should retry based on the new strategy
                 if (RetryStrategy.shouldRetry(statusCode)) {
                     return handleHttpErrorRetry(retryAfterDelay, retryNumber, error);
-                } else {
                 }
             }
 
@@ -196,18 +212,6 @@ private CompletableFuture<T> deserializeResponse(HttpResponse<String> response)
         }
     }
 
-    private HttpClient getDelayedHttpClient(Duration retryDelay) {
-        if (retryDelay == null || retryDelay.isZero() || retryDelay.isNegative()) {
-            // Fallback to minimum retry delay if invalid
-            retryDelay = configuration.getMinimumRetryDelay();
-        }
-
-        return apiClient
-                .getHttpClientBuilder()
-                .executor(CompletableFuture.delayedExecutor(retryDelay.toNanos(), TimeUnit.NANOSECONDS))
-                .build();
-    }
-
     private static class BodyLogger implements Flow.Subscriber<ByteBuffer> {
         private final PrintStream out;
         private final String target;

src/main/java/dev/openfga/sdk/util/RetryStrategy.java

@@ -61,19 +61,17 @@ public static boolean shouldRetry(int statusCode) {
      *
      * @param retryAfterDelay Optional delay from Retry-After header
      * @param retryCount Current retry attempt (0-based)
-     * @param minimumRetryDelay Base delay for exponential backoff
+     * @param minimumRetryDelay Minimum delay to enforce (only used when no Retry-After header present)
      * @return Duration representing the delay before the next retry
      */
     public static Duration calculateRetryDelay(
             Optional<Duration> retryAfterDelay, int retryCount, Duration minimumRetryDelay) {
-        // If Retry-After header is present and valid, use it
+        // If Retry-After header is present, use it but enforce minimum delay floor
         if (retryAfterDelay.isPresent()) {
-            Duration retryAfterValue = retryAfterDelay.get();
-            // Honor minimum retry delay if configured and greater than Retry-After value
-            if (minimumRetryDelay != null && minimumRetryDelay.compareTo(retryAfterValue) > 0) {
-                return minimumRetryDelay;
-            }
-            return retryAfterValue;
+            Duration serverDelay = retryAfterDelay.get();
+            // Clamp to minimum 1ms to prevent hot-loop retries and handle malformed server responses
+            Duration minimumSafeDelay = Duration.ofMillis(1);
+            return serverDelay.compareTo(minimumSafeDelay) < 0 ? minimumSafeDelay : serverDelay;
         }
 
         // Otherwise, use exponential backoff with jitter, respecting minimum retry delay