From abcd97db100477dea87e84f62bfcb0e1f1562cbf Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 8 Aug 2025 10:56:12 -0500 Subject: [PATCH 1/4] fix: prefer retry-after, improved retry strategy --- .../sdk/api/client/HttpRequestAttempt.java | 62 +++---- .../dev/openfga/sdk/util/RetryStrategy.java | 11 +- .../client/HttpRequestAttemptRetryTest.java | 157 +++++++++++++----- .../openfga/sdk/util/RetryStrategyTest.java | 4 +- 4 files changed, 156 insertions(+), 78 deletions(-) diff --git a/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java b/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java index fd3d03ac..550ab2cb 100644 --- a/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java +++ b/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 { private final ApiClient apiClient; @@ -99,10 +96,11 @@ private CompletableFuture> 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> handleNetworkError(Throwable throwable, int retryNumber) { @@ -114,9 +112,7 @@ private CompletableFuture> 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,32 @@ private CompletableFuture> 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> delayedRetry( + Duration retryDelay, int nextRetryNumber, Throwable previousError) { + // Use CompletableFuture.delayedExecutor() to delay the retry attempt itself + return CompletableFuture.supplyAsync( + () -> { + // We don't need a return value, just the delay + return null; + }, + CompletableFuture.delayedExecutor(retryDelay.toNanos(), TimeUnit.NANOSECONDS)) + .thenCompose(ignored -> { + // Reuse the existing HttpClient instead of creating a new one for efficiency + HttpClient reusableClient = apiClient.getHttpClient(); + return attemptHttpRequest(reusableClient, nextRetryNumber, previousError); + }); } private CompletableFuture> processHttpResponse( @@ -150,7 +169,6 @@ private CompletableFuture> processHttpResponse( // Check if we should retry based on the new strategy if (RetryStrategy.shouldRetry(statusCode)) { return handleHttpErrorRetry(retryAfterDelay, retryNumber, error); - } else { } } @@ -196,18 +214,6 @@ private CompletableFuture deserializeResponse(HttpResponse 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 { private final PrintStream out; private final String target; diff --git a/src/main/java/dev/openfga/sdk/util/RetryStrategy.java b/src/main/java/dev/openfga/sdk/util/RetryStrategy.java index 921f2e4a..bbb2516c 100644 --- a/src/main/java/dev/openfga/sdk/util/RetryStrategy.java +++ b/src/main/java/dev/openfga/sdk/util/RetryStrategy.java @@ -61,19 +61,14 @@ 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 retryAfterDelay, int retryCount, Duration minimumRetryDelay) { - // If Retry-After header is present and valid, use it + // If Retry-After header is present, use it (takes precedence over minimum retry delay) 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; + return retryAfterDelay.get(); } // Otherwise, use exponential backoff with jitter, respecting minimum retry delay diff --git a/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java b/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java index 8ada4afa..4500f63a 100644 --- a/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java +++ b/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java @@ -325,34 +325,122 @@ void shouldHandleInvalidRetryAfterHeader() throws Exception { } @Test - void shouldRetryOnConnectionTimeout() throws Exception { + void shouldRetryNetworkErrorsWithExponentialBackoff() throws Exception { // Given - Capture port before stopping server int serverPort = wireMockServer.port(); wireMockServer.stop(); - // Create configuration with shorter timeout for faster test - ClientConfiguration timeoutConfig = new ClientConfiguration() + // Create configuration with specific delays for timing test + ClientConfiguration networkConfig = new ClientConfiguration() .apiUrl("http://localhost:" + serverPort) .maxRetries(2) - .minimumRetryDelay(Duration.ofMillis(10)); + .minimumRetryDelay(Duration.ofMillis(100)); // 100ms base delay HttpRequest request = HttpRequest.newBuilder() .uri(java.net.URI.create("http://localhost:" + serverPort + "/test")) .GET() - .timeout(Duration.ofMillis(100)) // Short timeout + .timeout(Duration.ofMillis(50)) // Short timeout to force connection error .build(); HttpRequestAttempt attempt = - new HttpRequestAttempt<>(request, "test", Void.class, apiClient, timeoutConfig); + new HttpRequestAttempt<>(request, "test", Void.class, apiClient, networkConfig); - // When & Then + Instant startTime = Instant.now(); + + // When ExecutionException exception = assertThrows( ExecutionException.class, () -> attempt.attemptHttpRequest().get()); - // Should fail after retries with network error + Instant endTime = Instant.now(); + Duration totalTime = Duration.between(startTime, endTime); + + // Then assertThat(exception.getCause()).isInstanceOf(ApiException.class); - ApiException apiException = (ApiException) exception.getCause(); - assertThat(apiException.getCause()).isNotNull(); // Should have underlying network error + + // With exponential backoff: 100ms (1st retry) + ~200ms (2nd retry) = ~300ms total + // Allow some tolerance for execution overhead + assertThat(totalTime.toMillis()) + .isGreaterThan(200) // Should be at least ~300ms + .isLessThan(1000); // But not excessive + } + + @Test + void shouldHonorNetworkErrorRetryDelayTiming() throws Exception { + // Given - Capture port before stopping server to force network error + int serverPort = wireMockServer.port(); + wireMockServer.stop(); + + // Create configuration with specific minimum retry delay + ClientConfiguration networkConfig = new ClientConfiguration() + .apiUrl("http://localhost:" + serverPort) + .maxRetries(1) // Only 1 retry to test precise timing + .minimumRetryDelay(Duration.ofMillis(500)); // 500ms delay + + HttpRequest request = HttpRequest.newBuilder() + .uri(java.net.URI.create("http://localhost:" + serverPort + "/test")) + .GET() + .timeout(Duration.ofMillis(50)) // Short timeout to force connection error quickly + .build(); + + HttpRequestAttempt attempt = + new HttpRequestAttempt<>(request, "test", Void.class, apiClient, networkConfig); + + Instant startTime = Instant.now(); + + // When + ExecutionException exception = assertThrows( + ExecutionException.class, () -> attempt.attemptHttpRequest().get()); + + Instant endTime = Instant.now(); + Duration totalTime = Duration.between(startTime, endTime); + + // Then + assertThat(exception.getCause()).isInstanceOf(ApiException.class); + + // Should take approximately 500ms for the retry delay (plus small overhead) + // Per GitHub issue #155: jitter range is [base, 2*base], so 500ms base can become up to 1000ms + // Network error retry timing is now working correctly + assertThat(totalTime.toMillis()) + .isGreaterThan(450) // Should be at least ~500ms (base delay) + .isLessThan(1200); // Allow for up to 1000ms jitter + execution overhead + } + + @Test + void shouldUseExponentialBackoffForNetworkErrorsWithPreciseTiming() throws Exception { + // Given - Use invalid hostname to simulate DNS failure (more reliable than port) + ClientConfiguration dnsConfig = new ClientConfiguration() + .apiUrl("http://invalid-hostname-that-does-not-exist.local") + .maxRetries(2) + .minimumRetryDelay(Duration.ofMillis(200)); // 200ms base delay + + HttpRequest request = HttpRequest.newBuilder() + .uri(java.net.URI.create("http://invalid-hostname-that-does-not-exist.local/test")) + .GET() + .timeout(Duration.ofSeconds(1)) // Reasonable timeout + .build(); + + HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, dnsConfig); + + Instant startTime = Instant.now(); + + // When + ExecutionException exception = assertThrows( + ExecutionException.class, () -> attempt.attemptHttpRequest().get()); + + Instant endTime = Instant.now(); + Duration totalTime = Duration.between(startTime, endTime); + + // Then + assertThat(exception.getCause()).isInstanceOf(ApiException.class); + + // With exponential backoff from 200ms base: + // 1st retry: ~200ms * 2^0 = ~200ms + // 2nd retry: ~200ms * 2^1 = ~400ms + // Total: ~600ms (plus jitter and overhead) + // Network error retry timing is now working correctly after refactoring + assertThat(totalTime.toMillis()) + .isGreaterThan(500) // Should be at least ~600ms + .isLessThan(1200); // But not excessive (allowing for jitter) } @Test @@ -382,47 +470,34 @@ void shouldRetryOnUnknownHost() throws Exception { } @Test - void shouldRetryNetworkErrorsWithExponentialBackoff() throws Exception { + void shouldRetryOnConnectionTimeout() throws Exception { // Given - Capture port before stopping server int serverPort = wireMockServer.port(); wireMockServer.stop(); - long startTime = System.currentTimeMillis(); - - ClientConfiguration networkConfig = new ClientConfiguration() + // Create configuration with shorter timeout for faster test + ClientConfiguration timeoutConfig = new ClientConfiguration() .apiUrl("http://localhost:" + serverPort) - .maxRetries(3) - .minimumRetryDelay(Duration.ofMillis(50)); // Longer delay to measure timing + .maxRetries(2) + .minimumRetryDelay(Duration.ofMillis(10)); HttpRequest request = HttpRequest.newBuilder() .uri(java.net.URI.create("http://localhost:" + serverPort + "/test")) .GET() - .timeout(Duration.ofMillis(100)) + .timeout(Duration.ofMillis(100)) // Short timeout .build(); HttpRequestAttempt attempt = - new HttpRequestAttempt<>(request, "test", Void.class, apiClient, networkConfig); + new HttpRequestAttempt<>(request, "test", Void.class, apiClient, timeoutConfig); // When & Then ExecutionException exception = assertThrows( ExecutionException.class, () -> attempt.attemptHttpRequest().get()); - // Verify timing shows exponential backoff was used for network errors - long duration = System.currentTimeMillis() - startTime; - assertThat(duration).isGreaterThan(100); // Should take time due to retries + backoff - - // Should fail with network error after all retries + // Should fail after retries with network error assertThat(exception.getCause()).isInstanceOf(ApiException.class); ApiException apiException = (ApiException) exception.getCause(); assertThat(apiException.getCause()).isNotNull(); // Should have underlying network error - - // Verify telemetry attributes were set for network error retries - assertThat(attempt.getTelemetryAttributes()) - .containsKey(dev.openfga.sdk.telemetry.Attributes.HTTP_REQUEST_RESEND_COUNT); - String resendCount = - attempt.getTelemetryAttributes().get(dev.openfga.sdk.telemetry.Attributes.HTTP_REQUEST_RESEND_COUNT); - assertThat(resendCount).isNotNull(); - assertThat(Integer.parseInt(resendCount)).isGreaterThan(0); // Should have retry count > 0 } @Test @@ -465,7 +540,7 @@ void shouldRespectGlobalMinimumRetryDelayWithExponentialBackoff() throws Excepti } @Test - void shouldRespectGlobalMinimumRetryDelayWhenRetryAfterIsSmaller() throws Exception { + void shouldUseRetryAfterHeaderEvenWhenSmallerThanGlobalMinimumDelay() throws Exception { // Given - Server responds with Retry-After header smaller than minimum delay wireMockServer.stubFor(get(urlEqualTo("/test")) .inScenario("retry-scenario-global-1") @@ -486,11 +561,11 @@ void shouldRespectGlobalMinimumRetryDelayWhenRetryAfterIsSmaller() throws Except .GET() .build(); - // Use global configuration with larger minimum retry delay (should take precedence over Retry-After) + // Use global configuration with larger minimum retry delay (should NOT take precedence over Retry-After) ClientConfiguration globalConfig = new ClientConfiguration() .apiUrl("http://localhost:" + wireMockServer.port()) .maxRetries(2) - .minimumRetryDelay(Duration.ofSeconds(3)); // Should override Retry-After: 1 + .minimumRetryDelay(Duration.ofSeconds(3)); // Should NOT override Retry-After: 1 HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, globalConfig); @@ -504,8 +579,9 @@ void shouldRespectGlobalMinimumRetryDelayWhenRetryAfterIsSmaller() throws Except Duration totalTime = Duration.between(startTime, endTime); // Then - // Should have respected the minimum retry delay (3 seconds) instead of Retry-After (1 second) - assertThat(totalTime.toMillis()).isGreaterThan(2800); // Should be at least ~3 seconds + // Should have respected the Retry-After header (1 second) instead of minimum retry delay (3 seconds) + assertThat(totalTime.toMillis()).isGreaterThan(800); // Should be at least ~1 second + assertThat(totalTime.toMillis()).isLessThan(2500); // But less than 2.5 seconds (well below the 3s minimum) // Verify both requests were made wireMockServer.verify(2, getRequestedFor(urlEqualTo("/test"))); @@ -635,7 +711,7 @@ void shouldRespectPerRequestMaxRetriesOverride() throws Exception { } @Test - void shouldRespectPerRequestMinimumRetryDelayWhenRetryAfterIsSmaller() throws Exception { + void shouldUseRetryAfterHeaderEvenWhenSmallerThanMinimumDelay() throws Exception { // Given - Server responds with Retry-After header smaller than minimum delay wireMockServer.stubFor(get(urlEqualTo("/test")) .inScenario("retry-scenario-per-request-1") @@ -656,7 +732,7 @@ void shouldRespectPerRequestMinimumRetryDelayWhenRetryAfterIsSmaller() throws Ex .GET() .build(); - // Override with larger minimum retry delay (should take precedence over Retry-After) + // Override with larger minimum retry delay (should NOT take precedence over Retry-After) dev.openfga.sdk.api.configuration.Configuration overriddenConfig = configuration.override( new dev.openfga.sdk.api.configuration.ConfigurationOverride().minimumRetryDelay(Duration.ofSeconds(3))); @@ -672,8 +748,9 @@ void shouldRespectPerRequestMinimumRetryDelayWhenRetryAfterIsSmaller() throws Ex Duration totalTime = Duration.between(startTime, endTime); // Then - // Should have respected the minimum retry delay (3 seconds) instead of Retry-After (1 second) - assertThat(totalTime.toMillis()).isGreaterThan(2800); // Should be at least ~3 seconds + // Should have respected the Retry-After header (1 second) instead of minimum retry delay (3 seconds) + assertThat(totalTime.toMillis()).isGreaterThan(800); // Should be at least ~1 second + assertThat(totalTime.toMillis()).isLessThan(2500); // But less than 2.5 seconds (well below the 3s minimum) // Verify both requests were made wireMockServer.verify(2, getRequestedFor(urlEqualTo("/test"))); diff --git a/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java b/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java index b5ca6edf..a65a2bd3 100644 --- a/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java +++ b/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java @@ -35,7 +35,7 @@ void calculateRetryDelay_withRetryAfterHeader_shouldUseRetryAfterValue() { } @Test - void calculateRetryDelay_withRetryAfterSmallerThanMinimum_shouldUseMinimum() { + void calculateRetryDelay_withRetryAfterSmallerThanMinimum_shouldUseRetryAfter() { // Given Optional retryAfterDelay = Optional.of(Duration.ofMillis(50)); int retryCount = 1; @@ -45,7 +45,7 @@ void calculateRetryDelay_withRetryAfterSmallerThanMinimum_shouldUseMinimum() { Duration result = RetryStrategy.calculateRetryDelay(retryAfterDelay, retryCount, minimumRetryDelay); // Then - assertThat(result).isEqualTo(Duration.ofMillis(200)); + assertThat(result).isEqualTo(Duration.ofMillis(50)); } @Test From 3e6489bee3a0ca936a2f4a6638e9a775216d3210 Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 8 Aug 2025 13:24:48 -0500 Subject: [PATCH 2/4] fix: test optimization --- .../client/HttpRequestAttemptRetryTest.java | 73 +++++++++---------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java b/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java index 4500f63a..d4cc87e8 100644 --- a/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java +++ b/src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java @@ -67,7 +67,7 @@ void shouldRetryWith429AndRetryAfterHeader() throws Exception { .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(429) - .withHeader("Retry-After", "1") + .withHeader("Retry-After", "0.05") // Fast retry for test performance - timing not verified .withBody("{\"error\":\"rate limited\"}")) .willSetStateTo("First Retry")); @@ -102,7 +102,7 @@ void shouldRetryWith500AndRetryAfterHeaderForGetRequest() throws Exception { .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(500) - .withHeader("Retry-After", "1") + .withHeader("Retry-After", "0.05") // Fast retry for test performance - timing not verified .withBody("{\"error\":\"server error\"}")) .willSetStateTo("First Retry")); @@ -164,7 +164,7 @@ void shouldRetryWith500WithRetryAfterHeaderForPostRequest() throws Exception { .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(500) - .withHeader("Retry-After", "1") + .withHeader("Retry-After", "0.05") // Fast retry for test performance - timing not verified .withBody("{\"error\":\"server error\"}")) .willSetStateTo("First Retry")); @@ -195,10 +195,7 @@ void shouldRetryWith500WithRetryAfterHeaderForPostRequest() throws Exception { void shouldNotRetryWith501() throws Exception { // Given wireMockServer.stubFor(get(urlEqualTo("/test")) - .willReturn(aResponse() - .withStatus(501) - .withHeader("Retry-After", "1") - .withBody("{\"error\":\"not implemented\"}"))); + .willReturn(aResponse().withStatus(501).withBody("{\"error\":\"not implemented\"}"))); HttpRequest request = HttpRequest.newBuilder() .uri(java.net.URI.create("http://localhost:" + wireMockServer.port() + "/test")) @@ -224,10 +221,7 @@ void shouldNotRetryWith501() throws Exception { void shouldRespectMaxRetries() throws Exception { // Given wireMockServer.stubFor(get(urlEqualTo("/test")) - .willReturn(aResponse() - .withStatus(429) - .withHeader("Retry-After", "1") - .withBody("{\"error\":\"rate limited\"}"))); + .willReturn(aResponse().withStatus(429).withBody("{\"error\":\"rate limited\"}"))); HttpRequest request = HttpRequest.newBuilder() .uri(java.net.URI.create("http://localhost:" + wireMockServer.port() + "/test")) @@ -416,7 +410,7 @@ void shouldUseExponentialBackoffForNetworkErrorsWithPreciseTiming() throws Excep HttpRequest request = HttpRequest.newBuilder() .uri(java.net.URI.create("http://invalid-hostname-that-does-not-exist.local/test")) .GET() - .timeout(Duration.ofSeconds(1)) // Reasonable timeout + .timeout(Duration.ofMillis(500)) // Reasonable timeout .build(); HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, dnsConfig); @@ -515,7 +509,7 @@ void shouldRespectGlobalMinimumRetryDelayWithExponentialBackoff() throws Excepti ClientConfiguration globalConfig = new ClientConfiguration() .apiUrl("http://localhost:" + wireMockServer.port()) .maxRetries(2) - .minimumRetryDelay(Duration.ofSeconds(2)); // Should act as floor for exponential backoff + .minimumRetryDelay(Duration.ofMillis(100)); // Should act as floor for exponential backoff HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, globalConfig); @@ -535,8 +529,8 @@ void shouldRespectGlobalMinimumRetryDelayWithExponentialBackoff() throws Excepti // Verify that it retried the expected number of times wireMockServer.verify(1 + globalConfig.getMaxRetries(), getRequestedFor(urlEqualTo("/test"))); - // With 2 retries and minimum 2-second delays, total time should be at least 4 seconds - assertThat(totalTime.toMillis()).isGreaterThan(3500); // Should be at least ~4 seconds + // With 2 retries and minimum 100ms delays, total time should be at least 200ms + assertThat(totalTime.toMillis()).isGreaterThan(150); // Should be at least ~200ms } @Test @@ -547,7 +541,7 @@ void shouldUseRetryAfterHeaderEvenWhenSmallerThanGlobalMinimumDelay() throws Exc .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(429) - .withHeader("Retry-After", "1") // 1 second + .withHeader("Retry-After", "0.05") // 50ms .withBody("{\"error\":\"rate limited\"}")) .willSetStateTo("After-First-Request")); @@ -565,7 +559,7 @@ void shouldUseRetryAfterHeaderEvenWhenSmallerThanGlobalMinimumDelay() throws Exc ClientConfiguration globalConfig = new ClientConfiguration() .apiUrl("http://localhost:" + wireMockServer.port()) .maxRetries(2) - .minimumRetryDelay(Duration.ofSeconds(3)); // Should NOT override Retry-After: 1 + .minimumRetryDelay(Duration.ofMillis(150)); // Should NOT override Retry-After HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, globalConfig); @@ -579,9 +573,9 @@ void shouldUseRetryAfterHeaderEvenWhenSmallerThanGlobalMinimumDelay() throws Exc Duration totalTime = Duration.between(startTime, endTime); // Then - // Should have respected the Retry-After header (1 second) instead of minimum retry delay (3 seconds) - assertThat(totalTime.toMillis()).isGreaterThan(800); // Should be at least ~1 second - assertThat(totalTime.toMillis()).isLessThan(2500); // But less than 2.5 seconds (well below the 3s minimum) + // Should have respected the Retry-After header (50ms) instead of minimum retry delay (150ms) + assertThat(totalTime.toMillis()).isGreaterThan(30); // Should be at least ~50ms + assertThat(totalTime.toMillis()).isLessThan(400); // But less than 400ms (well below the 150ms minimum) // Verify both requests were made wireMockServer.verify(2, getRequestedFor(urlEqualTo("/test"))); @@ -595,7 +589,7 @@ void shouldUseRetryAfterWhenLargerThanGlobalMinimumDelay() throws Exception { .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(429) - .withHeader("Retry-After", "2") // 2 seconds + .withHeader("Retry-After", "0.1") // 100ms .withBody("{\"error\":\"rate limited\"}")) .willSetStateTo("After-First-Request")); @@ -613,7 +607,7 @@ void shouldUseRetryAfterWhenLargerThanGlobalMinimumDelay() throws Exception { ClientConfiguration globalConfig = new ClientConfiguration() .apiUrl("http://localhost:" + wireMockServer.port()) .maxRetries(2) - .minimumRetryDelay(Duration.ofMillis(500)); // Should NOT override Retry-After: 2 + .minimumRetryDelay(Duration.ofMillis(50)); // Should NOT override Retry-After: 100ms HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, globalConfig); @@ -627,13 +621,12 @@ void shouldUseRetryAfterWhenLargerThanGlobalMinimumDelay() throws Exception { Duration totalTime = Duration.between(startTime, endTime); // Then - // Should have respected the Retry-After header (2 seconds) over minimum delay (500ms) + // Should have respected the Retry-After header (100ms) over minimum delay (50ms) // Note: Using generous bounds due to timing variability in test environments System.out.println("Actual retry duration: " + totalTime.toMillis() + " ms"); - assertThat(totalTime.toMillis()) - .isGreaterThan(1200); // Should be at least ~2 seconds (with larger CI tolerance) - assertThat(totalTime.toMillis()).isLessThan(15000); // But not excessive + assertThat(totalTime.toMillis()).isGreaterThan(50); // Should be at least ~100ms (with tolerance) + assertThat(totalTime.toMillis()).isLessThan(1000); // But not excessive // Verify both requests were made wireMockServer.verify(2, getRequestedFor(urlEqualTo("/test"))); @@ -651,8 +644,9 @@ void shouldRespectPerRequestMinimumRetryDelayOverride() throws Exception { .build(); // Override with larger minimum retry delay using per-request configuration - dev.openfga.sdk.api.configuration.Configuration overriddenConfig = configuration.override( - new dev.openfga.sdk.api.configuration.ConfigurationOverride().minimumRetryDelay(Duration.ofSeconds(2))); + dev.openfga.sdk.api.configuration.Configuration overriddenConfig = + configuration.override(new dev.openfga.sdk.api.configuration.ConfigurationOverride() + .minimumRetryDelay(Duration.ofMillis(100))); HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, overriddenConfig); @@ -672,8 +666,8 @@ void shouldRespectPerRequestMinimumRetryDelayOverride() throws Exception { // Verify that it retried the expected number of times wireMockServer.verify(1 + overriddenConfig.getMaxRetries(), getRequestedFor(urlEqualTo("/test"))); - // With 3 retries and minimum 2-second delays, total time should be at least 6 seconds - assertThat(totalTime.toMillis()).isGreaterThan(5500); // Should be at least ~6 seconds + // With 3 retries and minimum 100ms delays, total time should be at least 300ms + assertThat(totalTime.toMillis()).isGreaterThan(250); // Should be at least ~300ms } @Test @@ -718,7 +712,7 @@ void shouldUseRetryAfterHeaderEvenWhenSmallerThanMinimumDelay() throws Exception .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(429) - .withHeader("Retry-After", "1") // 1 second + .withHeader("Retry-After", "0.05") // 50ms .withBody("{\"error\":\"rate limited\"}")) .willSetStateTo("After-First-Request")); @@ -733,8 +727,9 @@ void shouldUseRetryAfterHeaderEvenWhenSmallerThanMinimumDelay() throws Exception .build(); // Override with larger minimum retry delay (should NOT take precedence over Retry-After) - dev.openfga.sdk.api.configuration.Configuration overriddenConfig = configuration.override( - new dev.openfga.sdk.api.configuration.ConfigurationOverride().minimumRetryDelay(Duration.ofSeconds(3))); + dev.openfga.sdk.api.configuration.Configuration overriddenConfig = + configuration.override(new dev.openfga.sdk.api.configuration.ConfigurationOverride() + .minimumRetryDelay(Duration.ofMillis(150))); HttpRequestAttempt attempt = new HttpRequestAttempt<>(request, "test", Void.class, apiClient, overriddenConfig); @@ -748,9 +743,9 @@ void shouldUseRetryAfterHeaderEvenWhenSmallerThanMinimumDelay() throws Exception Duration totalTime = Duration.between(startTime, endTime); // Then - // Should have respected the Retry-After header (1 second) instead of minimum retry delay (3 seconds) - assertThat(totalTime.toMillis()).isGreaterThan(800); // Should be at least ~1 second - assertThat(totalTime.toMillis()).isLessThan(2500); // But less than 2.5 seconds (well below the 3s minimum) + // Should have respected the Retry-After header (50ms) instead of minimum retry delay (150ms) + assertThat(totalTime.toMillis()).isGreaterThan(30); // Should be at least ~50ms + assertThat(totalTime.toMillis()).isLessThan(400); // But less than 400ms (well below the 150ms minimum) // Verify both requests were made wireMockServer.verify(2, getRequestedFor(urlEqualTo("/test"))); @@ -764,7 +759,7 @@ void shouldNotOverrideRetryAfterWhenItIsLargerThanMinimumDelayPerRequest() throw .whenScenarioStateIs("Started") .willReturn(aResponse() .withStatus(429) - .withHeader("Retry-After", "1") // 1 second delay + .withHeader("Retry-After", "0.05") // 50ms .withBody("{\"error\":\"rate limited\"}")) .willSetStateTo("retry-attempted")); @@ -798,9 +793,9 @@ void shouldNotOverrideRetryAfterWhenItIsLargerThanMinimumDelayPerRequest() throw Duration totalTime = Duration.between(startTime, endTime); // Then - // Should have respected the Retry-After header (1 second) for the single retry + // Should have respected the Retry-After header (50ms) for the single retry // Note: actual timing may vary in test environments, so we use generous bounds - assertThat(totalTime.toMillis()).isGreaterThan(800); // Should be at least ~1 second + assertThat(totalTime.toMillis()).isGreaterThan(30); // Should be at least ~50ms assertThat(totalTime.toMillis()).isLessThan(10000); // But not excessive (was sometimes 4x in CI) // Verify initial request + 1 retry = 2 total requests From 153c570980471735e2e202775877e127b98d9f8f Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 8 Aug 2025 13:59:27 -0500 Subject: [PATCH 3/4] fix: rename method and use runAsync instead of supplyAsync --- .../openfga/sdk/api/client/HttpRequestAttempt.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java b/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java index 550ab2cb..d3f2332f 100644 --- a/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java +++ b/src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java @@ -80,10 +80,10 @@ public CompletableFuture> 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(); } @@ -140,16 +140,14 @@ private CompletableFuture> handleHttpErrorRetry( private CompletableFuture> delayedRetry( Duration retryDelay, int nextRetryNumber, Throwable previousError) { // Use CompletableFuture.delayedExecutor() to delay the retry attempt itself - return CompletableFuture.supplyAsync( + return CompletableFuture.runAsync( () -> { - // We don't need a return value, just the delay - return null; + // No-op task, we only care about the delay timing }, CompletableFuture.delayedExecutor(retryDelay.toNanos(), TimeUnit.NANOSECONDS)) .thenCompose(ignored -> { - // Reuse the existing HttpClient instead of creating a new one for efficiency - HttpClient reusableClient = apiClient.getHttpClient(); - return attemptHttpRequest(reusableClient, nextRetryNumber, previousError); + // Get HttpClient when needed (just returns cached instance) + return attemptHttpRequest(getHttpClient(), nextRetryNumber, previousError); }); } From 1defea46db38ca9b2936b93abda132da32cb36ba Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 8 Aug 2025 14:26:44 -0500 Subject: [PATCH 4/4] review: fix hot-loop scenario --- .../dev/openfga/sdk/util/RetryStrategy.java | 7 ++- .../openfga/sdk/util/RetryStrategyTest.java | 45 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/openfga/sdk/util/RetryStrategy.java b/src/main/java/dev/openfga/sdk/util/RetryStrategy.java index bbb2516c..0a0a5345 100644 --- a/src/main/java/dev/openfga/sdk/util/RetryStrategy.java +++ b/src/main/java/dev/openfga/sdk/util/RetryStrategy.java @@ -66,9 +66,12 @@ public static boolean shouldRetry(int statusCode) { */ public static Duration calculateRetryDelay( Optional retryAfterDelay, int retryCount, Duration minimumRetryDelay) { - // If Retry-After header is present, use it (takes precedence over minimum retry delay) + // If Retry-After header is present, use it but enforce minimum delay floor if (retryAfterDelay.isPresent()) { - return retryAfterDelay.get(); + 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 diff --git a/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java b/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java index a65a2bd3..035d9931 100644 --- a/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java +++ b/src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java @@ -120,4 +120,49 @@ void shouldRetry_with404_shouldReturnFalse() { void shouldRetry_with501_shouldReturnFalse() { assertThat(RetryStrategy.shouldRetry(501)).isFalse(); } + + @Test + void calculateRetryDelay_withZeroRetryAfter_shouldEnforceMinimumDelay() { + // Given - Server sends Retry-After: 0 (problematic!) + Optional retryAfterDelay = Optional.of(Duration.ZERO); + int retryCount = 1; + Duration minimumRetryDelay = Duration.ofMillis(100); + + // When + Duration result = RetryStrategy.calculateRetryDelay(retryAfterDelay, retryCount, minimumRetryDelay); + + // Then - Should enforce minimum delay to prevent hot-loop retries + // Current code will FAIL this test by returning Duration.ZERO + assertThat(result.toMillis()).isGreaterThanOrEqualTo(1); // At least 1ms to prevent hot-loops + } + + @Test + void calculateRetryDelay_withNegativeRetryAfter_shouldEnforceMinimumDelay() { + // Given - Server sends malformed negative delay (very problematic!) + Optional retryAfterDelay = Optional.of(Duration.ofMillis(-500)); + int retryCount = 1; + Duration minimumRetryDelay = Duration.ofMillis(100); + + // When + Duration result = RetryStrategy.calculateRetryDelay(retryAfterDelay, retryCount, minimumRetryDelay); + + // Then - Should enforce minimum delay to handle malformed server responses + // Current code will FAIL this test by returning negative duration + assertThat(result.toMillis()).isGreaterThanOrEqualTo(1); // At least 1ms for safety + } + + @Test + void calculateRetryDelay_withVerySmallRetryAfter_shouldEnforceMinimumDelay() { + // Given - Server sends extremely small delay (could cause near-hot-loop) + Optional retryAfterDelay = Optional.of(Duration.ofNanos(500)); // 0.0005ms + int retryCount = 1; + Duration minimumRetryDelay = Duration.ofMillis(100); + + // When + Duration result = RetryStrategy.calculateRetryDelay(retryAfterDelay, retryCount, minimumRetryDelay); + + // Then - Should enforce reasonable minimum delay + // Current code will FAIL this test by returning tiny delay + assertThat(result.toMillis()).isGreaterThanOrEqualTo(1); // At least 1ms for system stability + } }