feat: improved retry handling #186
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update overhauls the retry strategy in the Java SDK, introducing RFC 9110-compliant parsing of the Retry-After header, exponential backoff with jitter, and differentiated retry logic for state-affecting and non-state-affecting operations. The maximum retries are capped at 15, and error handling now exposes the Retry-After value. Comprehensive tests and documentation have been added and updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpRequestAttempt
participant RetryStrategy
participant ExponentialBackoff
participant RetryAfterHeaderParser
participant Server
Client->>HttpRequestAttempt: Send HTTP request
loop up to maxRetries
HttpRequestAttempt->>Server: Perform HTTP request
Server-->>HttpRequestAttempt: HTTP response (status, headers)
alt Retry-After header present
HttpRequestAttempt->>RetryAfterHeaderParser: Parse Retry-After
RetryAfterHeaderParser-->>HttpRequestAttempt: Optional<Duration> delay
end
HttpRequestAttempt->>RetryStrategy: shouldRetry(request, status, hasRetryAfter)
RetryStrategy-->>HttpRequestAttempt: true/false
alt Should retry
HttpRequestAttempt->>RetryStrategy: calculateRetryDelay(retryAfter, retryCount)
RetryStrategy->>ExponentialBackoff: calculateDelay(retryCount)
ExponentialBackoff-->>RetryStrategy: Duration (with jitter)
RetryStrategy-->>HttpRequestAttempt: Duration (delay)
HttpRequestAttempt->>HttpRequestAttempt: Wait for delay, increment retryCount
else Exit loop
HttpRequestAttempt-->>Client: Return response or throw error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Suggested labels
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (34.55%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
============================================
+ Coverage 33.73% 34.55% +0.81%
- Complexity 1005 1041 +36
============================================
Files 182 185 +3
Lines 6900 6983 +83
Branches 778 789 +11
============================================
+ Hits 2328 2413 +85
+ Misses 4467 4464 -3
- Partials 105 106 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (1)
8-11: Remove misleading auto-generated comment.Same issue as the previous file - this is clearly hand-written code, not auto-generated.
src/test/java/dev/openfga/sdk/util/RetryAfterHeaderParserTest.java (1)
8-11: Remove misleading auto-generated comment.Same issue as other files - this is hand-written test code, not auto-generated.
🧹 Nitpick comments (3)
src/main/java/dev/openfga/sdk/util/RetryAfterHeaderParser.java (1)
8-11: Remove misleading auto-generated comment.The header comment states this class is auto-generated and should not be edited manually, but this is clearly a manually written utility class that will require maintenance and updates.
- * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). - * https://openapi-generator.tech - * Do not edit the class manually.src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (1)
75-98: Consider refactoring to eliminate code duplication.This method duplicates the entire logic from the first
calculateDelaymethod. Consider extracting the common logic to reduce maintenance burden.public static Duration calculateDelay(int retryCount) { - if (retryCount < 0) { - return Duration.ZERO; - } - - // Calculate base delay: 2^retryCount * 100ms - long baseDelayMs = (long) Math.pow(2, retryCount) * BASE_DELAY_MS; - - // Cap at maximum delay - long maxDelayMs = MAX_DELAY_SECONDS * 1000L; - if (baseDelayMs > maxDelayMs) { - baseDelayMs = maxDelayMs; - } - - // Add jitter: random value between baseDelay and 2 * baseDelay - long minDelayMs = baseDelayMs; - long maxDelayMsWithJitter = Math.min(baseDelayMs * 2, maxDelayMs); - - // Generate random delay within the jitter range - long jitterRange = maxDelayMsWithJitter - minDelayMs; - long actualDelayMs = minDelayMs + (jitterRange > 0 ? (long) (RANDOM.nextDouble() * (jitterRange + 1)) : 0); - - return Duration.ofMillis(actualDelayMs); + return calculateDelay(retryCount, RANDOM); }src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java (1)
30-319: Consider adding tests for additional retry scenarios.The current test suite provides excellent coverage of the core retry logic. Consider adding tests for these scenarios to achieve complete coverage:
- HTTP-date format Retry-After headers - The PR mentions RFC 9110 compliance which includes HTTP-date format support
- Other state-affecting methods - Test PUT, PATCH, DELETE to ensure they follow the same retry rules as POST
- Retry-After boundary values - Test edge cases like 1 second, 1800 seconds (30 minutes max), and out-of-range values
- FgaError Retry-After exposure - Verify that the Retry-After header value is properly exposed in FgaError objects as mentioned in the PR objectives
Example test for HTTP-date format:
@Test void shouldHandleHttpDateRetryAfterHeader() throws Exception { // Test with HTTP-date format: "Fri, 31 Dec 1999 23:59:59 GMT" String httpDate = DateTimeFormatter.RFC_1123_DATE_TIME .format(ZonedDateTime.now().plusSeconds(2)); wireMockServer.stubFor(get(urlEqualTo("/test")) .willReturn(aResponse() .withStatus(429) .withHeader("Retry-After", httpDate) .withBody("{\"error\":\"rate limited\"}"))); // ... rest of test }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)README.md(4 hunks)src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java(3 hunks)src/main/java/dev/openfga/sdk/api/configuration/Configuration.java(3 hunks)src/main/java/dev/openfga/sdk/errors/FgaError.java(3 hunks)src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java(1 hunks)src/main/java/dev/openfga/sdk/util/RetryAfterHeaderParser.java(1 hunks)src/main/java/dev/openfga/sdk/util/RetryStrategy.java(1 hunks)src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java(9 hunks)src/test/java/dev/openfga/sdk/api/auth/OAuth2ClientTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java(12 hunks)src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java(1 hunks)src/test/java/dev/openfga/sdk/util/RetryAfterHeaderParserTest.java(1 hunks)src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/test/java/dev/openfga/sdk/util/RetryAfterHeaderParserTest.java (1)
src/main/java/dev/openfga/sdk/util/RetryAfterHeaderParser.java (1)
RetryAfterHeaderParser(30-104)
src/main/java/dev/openfga/sdk/util/RetryStrategy.java (2)
src/main/java/dev/openfga/sdk/errors/HttpStatusCode.java (1)
HttpStatusCode(3-38)src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (1)
ExponentialBackoff(26-99)
src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java (1)
src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (1)
ExponentialBackoff(26-99)
🔇 Additional comments (57)
src/test/java/dev/openfga/sdk/api/auth/OAuth2ClientTest.java (1)
143-148: LGTM - Test correctly reflects new retry strategy.The addition of the
Retry-Afterheader to the 500 response correctly aligns with the new retry behavior where POST requests only retry on 5xx errors when this header is present. The comment clearly documents this breaking change.src/main/java/dev/openfga/sdk/errors/FgaError.java (3)
20-20: LGTM - Clean field addition.The new
retryAfterHeaderfield follows the established pattern of other fields in the class.
64-69: LGTM - Proper header extraction logic.The implementation correctly uses
Optionalto safely extract theRetry-Afterheader and only sets it when present. This avoids null pointer issues and follows good Java practices.
137-143: LGTM - Standard getter/setter implementation.The getter and setter methods follow standard Java conventions and enable client code to access the server-suggested retry delay as mentioned in the PR objectives.
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java (9)
270-271: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, as documented in the breaking change comment.
452-453: LGTM - Correctly implements new retry behavior for DELETE requests.The test correctly verifies that DELETE requests no longer retry on 5xx errors without a
Retry-Afterheader, consistent with the new retry strategy for state-affecting operations.
675-676: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, maintaining consistency with other state-affecting operations.
1085-1086: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, consistent with the new retry strategy.
1315-1316: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, maintaining consistency across all write operations.
1430-1431: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, consistent with the new retry strategy for state-affecting operations.
1557-1558: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, maintaining consistency with other write operations.
1672-1673: LGTM - Correctly implements new retry behavior for POST requests.The test correctly verifies that POST requests no longer retry on 5xx errors without a
Retry-Afterheader, consistent with the new retry strategy.
1924-1925: LGTM - Correctly implements new retry behavior for PUT requests.The test correctly verifies that PUT requests no longer retry on 5xx errors without a
Retry-Afterheader, completing the consistent implementation across all state-affecting HTTP methods (POST, DELETE, PUT).src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (12)
389-390: LGTM! Test correctly reflects the new retry behavior.The test has been properly updated to reflect that POST requests no longer retry on 5xx errors without a
Retry-Afterheader. The explanatory comment clearly documents this breaking change.
576-577: LGTM! Test correctly reflects the new retry behavior.The test has been properly updated to reflect that DELETE requests no longer retry on 5xx errors without a
Retry-Afterheader. The explanatory comment clearly documents this breaking change.
860-861: LGTM! Test correctly reflects the new retry behavior.The test has been properly updated to reflect that POST requests no longer retry on 5xx errors without a
Retry-Afterheader. The explanatory comment clearly documents this breaking change.
1558-1559: LGTM! Test correctly reflects the new retry behavior.The test has been properly updated to reflect that POST requests for write operations no longer retry on 5xx errors without a
Retry-Afterheader. The explanatory comment clearly documents this breaking change.
1873-1874: Verify if batch check operations should retry on 5xx errors.The
clientBatchCheckoperation is also semantically a non-state-affecting operation (batch authorization checking doesn't modify state) despite using the POST HTTP method. This reinforces the pattern that the implementation may be classifying operations by HTTP method rather than semantic meaning.This is consistent with the previous concerns about read and check operations. The verification script from the previous comment should cover this case as well.
2100-2101: LGTM! Test correctly documents 429 retry behavior.The comment appropriately clarifies that HTTP 429 (rate limiting) errors continue to retry regardless of the HTTP method, which is not affected by the breaking changes. This serves as a good contrast to the 5xx error behavior changes.
2218-2219: Verify if expand operations should retry on 5xx errors.The
expandoperation is semantically a non-state-affecting operation (it expands relationships for analysis/debugging purposes) despite using the POST HTTP method. This follows the same pattern as read/check operations where the implementation may be classifying by HTTP method rather than semantic meaning.This should be covered by the verification script from the earlier comments about read/check operations.
2325-2326: Verify if listObjects operations should retry on 5xx errors.The
listObjectsoperation is semantically a non-state-affecting operation (it lists objects based on authorization queries) despite using the POST HTTP method. This continues the pattern of potential misclassification by HTTP method rather than semantic operation type.This should be covered by the verification script from the earlier comments about read/check operations.
2572-2573: Verify if listRelations operations should retry on 5xx errors.The
listRelationsoperation is semantically a non-state-affecting operation (it checks which relations a user has with an object) despite using the POST HTTP method. This continues the pattern of potential misclassification by HTTP method rather than semantic operation type.This should be covered by the verification script from the earlier comments about read/check operations.
2911-2912: LGTM! Test correctly reflects the new retry behavior.The test has been properly updated to reflect that PUT requests for write assertions no longer retry on 5xx errors without a
Retry-Afterheader. This is correct sincewriteAssertionsis genuinely a state-affecting operation that modifies the authorization model's assertions.
1130-1131: I wasn’t able to locate where the retry strategy classifies “read” vs. “write” operations in the codebase—particularly how it treats the POST-based read endpoint. Can you please confirm whether the implementation is:
- Classifying retries purely by HTTP method (POST → write)
- Or using semantic operation type (read endpoint → retry on 5xx despite POST)
This will determine if the test change (no longer retrying 5xx on read) aligns with the intended behavior.
1676-1677: Confirm retry behavior for check (POST) operationsThe current retry logic in RetryStrategy.shouldRetry() uses isStateAffectingMethod(method) to classify all POST requests (including check calls) as state-affecting, so they only retry on 5xx when a Retry-After header is present. If the check() API is intended to be treated as non-state-affecting (and thus always retry on 5xx), you’ll need to adjust:
- RetryStrategy.isStateAffectingMethod() in src/main/java/dev/openfga/sdk/util/RetryStrategy.java to exempt check operations
- Add or update unit tests in RetryStrategyTest.java to cover check (POST) semantics
Please verify whether check() should behave like other non-mutating operations and update the classification logic accordingly.
src/main/java/dev/openfga/sdk/util/RetryAfterHeaderParser.java (3)
45-60: LGTM! Well-structured parsing logic with proper fallback.The method correctly tries integer parsing first, then falls back to HTTP-date parsing. The null/empty validation and trimming are handled appropriately.
65-78: LGTM! Robust integer parsing with proper validation.The range validation (1-1800 seconds) correctly implements the 30-minute maximum as specified in the requirements. Exception handling gracefully returns empty Optional for invalid input.
84-103: LGTM! RFC 9110 compliant HTTP-date parsing.The implementation correctly uses RFC_1123_DATE_TIME formatter and validates the calculated duration within the same 1-1800 second range. The use of
Instant.now()for current time calculation is appropriate for retry scenarios.src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (1)
42-65: LGTM! Correct exponential backoff implementation with proper jitter.The mathematical formula correctly implements
2^retryCount * 100ms, with appropriate capping at 120 seconds. The jitter calculation between base and 2*base delay helps prevent thundering herd problems.CHANGELOG.md (1)
5-26: Comprehensive and well-structured changelog.The changelog effectively documents all breaking changes, new features, and provides clear migration guidance. The technical details section helps users understand the implementation specifics.
src/test/java/dev/openfga/sdk/util/RetryAfterHeaderParserTest.java (1)
25-214: Excellent comprehensive test coverage.The test suite thoroughly covers all scenarios:
- Valid and invalid integer parsing (including boundary values)
- HTTP-date parsing with proper timing tolerance
- Edge cases (null, empty, whitespace, non-numeric values)
- Input sanitization (whitespace trimming)
The use of tolerance in HTTP-date tests (lines 113) properly accounts for execution time variations.
src/main/java/dev/openfga/sdk/api/configuration/Configuration.java (3)
39-40: LGTM! Well-defined retry configuration constants.The constants clearly establish the default (3) and maximum allowable (15) retry counts, which aligns with the PR objectives and provides clear boundaries for configuration.
57-57: LGTM! Proper initialization in constructor.The constructor correctly initializes maxRetries to the default value, maintaining consistency with other configuration properties.
271-277: LGTM! Robust validation with clear error messages.The validation correctly prevents negative values and enforces the maximum allowable retries limit. The error messages are clear and informative, helping developers understand the constraints.
src/main/java/dev/openfga/sdk/util/RetryStrategy.java (3)
51-71: LGTM! Excellent implementation of RFC 9110-compliant retry logic.The method correctly implements the differentiated retry behavior for state-affecting vs non-state-affecting operations, properly handles the 429 always-retry case, and correctly excludes 501 errors from retry attempts.
80-88: LGTM! Clean implementation of delay calculation priority.The method correctly prioritizes server-specified
Retry-Afterdelays over exponential backoff, which aligns with RFC 9110 recommendations and best practices.
97-99: LGTM! Correct identification of state-affecting methods.The method properly identifies state-affecting HTTP methods and handles case-insensitive comparison appropriately.
src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java (3)
24-36: LGTM! Excellent test for base delay calculation.The test correctly verifies that retry count 0 produces a delay between 100ms and 200ms (base delay with jitter), using a fixed Random seed for deterministic testing.
69-80: LGTM! Proper validation of maximum delay capping.The test correctly verifies that high retry counts are capped at the 120-second maximum, preventing unbounded delay growth.
140-157: LGTM! Comprehensive progression validation.The test systematically validates the exponential progression pattern across multiple retry counts, ensuring both the base calculation and jitter ranges are correct. The use of
setSeed(42)for each iteration ensures consistent test results.src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (2)
105-121: LGTM! Excellent integration of the new retry strategy.The updated retry logic properly integrates all the new utilities:
- Parses
Retry-Afterheader using the dedicated parser- Uses
RetryStrategy.shouldRetryfor RFC 9110-compliant retry decisions- Calculates delays using
RetryStrategy.calculateRetryDelay- Maintains proper retry count checking
The integration maintains the existing flow while incorporating the enhanced retry behavior.
168-176: LGTM! Robust delay validation and fallback logic.The method properly handles edge cases by validating the retry delay and providing sensible fallbacks. This ensures the retry mechanism remains robust even with invalid delay values.
README.md (2)
968-991: LGTM! Comprehensive and clear retry behavior documentation.The documentation excellently explains the new RFC 9110-compliant retry strategy, clearly differentiates between read and write operation retry behavior, and prominently highlights breaking changes. This will greatly assist users in understanding and migrating to the new retry logic.
1005-1006: LGTM! Practical configuration and error handling examples.The examples clearly demonstrate how to configure the new retry behavior and access the
Retry-Afterheader information in error handling, providing users with actionable guidance.Also applies to: 1019-1034
src/test/java/dev/openfga/sdk/util/RetryStrategyTest.java (4)
62-94: LGTM! Critical tests validating breaking changes for state-affecting operations.These tests properly validate the new retry behavior where state-affecting methods (POST, PUT, PATCH, DELETE) only retry on 5xx errors when a
Retry-Afterheader is present. The explicit "Breaking change" comments help document the behavior change.
153-171: LGTM! Comprehensive validation of non-state-affecting method behavior.The test properly validates that read operations (GET, HEAD, OPTIONS) maintain backward-compatible retry behavior on 5xx errors, regardless of
Retry-Afterheader presence.
246-261: LGTM! Important case-insensitive method handling test.This test ensures the retry strategy works correctly regardless of HTTP method case, providing robustness against variations in method naming.
205-243: LGTM! Thorough validation of delay calculation priority.The tests correctly validate that
Retry-Afterheader values are prioritized over exponential backoff, and that the fallback exponential backoff produces delays within the expected jitter ranges.src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java (9)
30-55: LGTM! Well-structured test setup.The test setup follows best practices with proper WireMock configuration, reasonable test delays, and correct resource cleanup in the teardown method.
57-90: LGTM! Comprehensive test for 429 retry behavior.The test correctly validates the core retry functionality with proper scenario sequencing and verification of both success response and retry count.
92-125: LGTM! Correctly tests server error retry for GET requests.The test properly validates the differentiated retry behavior where GET requests retry on 5xx errors when a Retry-After header is present.
127-151: LGTM! Critical test for breaking change behavior.This test correctly validates the breaking change where POST requests (state-affecting operations) do not retry on 5xx errors without a Retry-After header. The comment clearly documents this behavioral change.
153-186: LGTM! Validates positive case for POST retry behavior.This test correctly complements the previous test by showing that POST requests do retry on 5xx errors when a Retry-After header is present, completing the validation of state-affecting operation retry logic.
188-215: LGTM! Correctly tests 501 exception handling.The test properly validates that 501 (Not Implemented) responses are never retried, even with a Retry-After header present, which aligns with the RFC requirements and PR objectives.
217-244: LGTM! Properly validates retry limit enforcement.The test correctly verifies that retry attempts respect the configured maximum retries, with the proper calculation of total requests (initial + maxRetries).
285-318: LGTM! Validates graceful handling of invalid Retry-After values.The test correctly verifies that the system gracefully handles invalid Retry-After header values by falling back to exponential backoff, which is important for robust error handling.
246-283: Requesting HttpRequestAttempt retry logic for precise verificationTo confirm which
retryCountvalue is passed intoExponentialBackoff.calculateDelay(and thus whether the first retry uses a 100 ms or 200 ms base), please share the snippet fromHttpRequestAttempt.java(around where retries are scheduled) and theRetryStrategy.javablock that invokescalculateDelay. Specifically, we need to see:
- How
retryCountis initialized and incremented inHttpRequestAttempt.- The exact call into
ExponentialBackoff.calculateDelay(...)(viaRetryStrategy).This will show whether the first retry uses
retryCount=0(100 ms–200 ms) orretryCount=1(200 ms–400 ms).
There was a problem hiding this comment.
Lovely PR + tests + documentation! ❤️
Just a small change (updated the issue).
We later decided to retry on 5xx on all requests regardless of whether they change state or not.
Can we:
- For all (429s + 5xxs) except 501:
- honor Retry-After when sent by the server, falling back to exponential backoff when it's not.
- For network errors (e.g. no internet connectivity/LB closes the connection/server disconnects) [these don't necessarily have an http reponse and status as the server didn't reply)
- Retry using exponential backoff
| - **BREAKING**: FgaError now exposes Retry-After header value via getRetryAfterHeader() method | ||
|
|
||
| ### Technical Details | ||
| - Implements RFC 9110 compliant Retry-After header parsing (supports both integer seconds and HTTP-date formats) |
There was a problem hiding this comment.
Oh, we can support HTTP-date format? That's cool. I thought during Resiliency Week discussions with @rhamzeh that there was difficulty in doing that.
There was a problem hiding this comment.
🤔 Was there? This is how we built it for the Go SDKs.
The only problem was a personal one, date based retries are bad b/c you can never be sure the client and server are on the same clock and don't have any drift, but we support that in Go https://github.com/openfga/go-sdk/blob/main/internal/utils/retryutils/retryutils.go#L60-L63
There was a problem hiding this comment.
Yes, personally I feel the same. Without tight control and a common NTP server, there are always clock drift issues. But I have no issues with supporting it, though. I must have been misremembering our conversation about HTTP-date.
Updated to remove state-specific handling and retry on network issues. Also tested some basic and retry functionality using a test app created (not included) to exercise that code, worked as expected. |
| * @param statusCode The HTTP response status code | ||
| * @param hasRetryAfterHeader Whether the response contains a valid Retry-After header | ||
| * @param hasRetryAfterHeader Whether the response contains a valid Retry-After header (kept for API compatibility) | ||
| * @return true if the request should be retried, false otherwise | ||
| */ | ||
| public static boolean shouldRetry(HttpRequest request, int statusCode, boolean hasRetryAfterHeader) { |
There was a problem hiding this comment.
Do we need to maintain compatibility? We haven't merged the PR yet - so it's fine to remove that
There was a problem hiding this comment.
You're absolutely right! 💥
🤣
Removed unneeded params. Interesting how changes made in a long-running manner on a branch can get AI kinda confused. Will need to keep that in mind for the future.
There was a problem hiding this comment.
Excellent catch - this appears to be a scenario missed in this PR (and not something we support yet in go or python?). I'll need to revisit the changes here to support the per-request configuration along with the global configuration and default behavior.
There was a problem hiding this comment.
I've updated the PR to honor the min retry scenario and add tests here: 9cfce52
| void shouldNotRetryWith500WithoutRetryAfterHeaderForPostRequest() throws Exception { | ||
| // Given - Breaking change: POST requests should NOT retry on 5xx without Retry-After | ||
| void shouldRetryWith500WithoutRetryAfterHeaderForPostRequest() throws Exception { | ||
| // Given - Simplified logic: POST requests should retry on 5xx errors |
There was a problem hiding this comment.
Can we remove the simplified parts in this and other comments?
README.md
Outdated
| @@ -125,7 +125,7 @@ libraryDependencies += "dev.openfga" % "openfga-sdk" % "0.8.3" | |||
|
|
|||
| We strongly recommend you initialize the `OpenFgaClient` only once and then re-use it throughout your app, otherwise you will incur the cost of having to re-initialize multiple times or at every request, the cost of reduced connection pooling and re-use, and would be particularly costly in the client credentials flow, as that flow will be preformed on every request. | |||
|
|
|||
| > The `Client` will by default retry API requests up to 3 times. Rate limiting (429) errors are always retried. Server errors (5xx) are retried for read operations, but write operations only retry when the server provides a `Retry-After` header. | |||
| > The `Client` will by default retry API requests up to 3 times. Rate limiting (429) errors are always retried. Server errors (5xx) are retried for all operations, with intelligent delay calculation using `Retry-After` headers when provided or exponential backoff as fallback. | |||
There was a problem hiding this comment.
This might seem nitpicky - but can we remove intelligent here and in other comments? 😅
| @@ -1913,7 +1921,8 @@ DEFAULT_STORE_ID, DEFAULT_AUTH_MODEL_ID, new WriteAssertionsRequest()) | |||
| .get()); | |||
|
|
|||
| // Then | |||
| mockHttpClient.verify().put(putUrl).called(1 + DEFAULT_MAX_RETRIES); | |||
| // Simplified logic: PUT requests now retry on 5xx errors (1 initial + 3 retries = 4 total) | |||
| mockHttpClient.verify().put(putUrl).called(4); | |||
There was a problem hiding this comment.
Would it be possible to add a test w/ a network error here?
There was a problem hiding this comment.
And if I can ask, can we have a client request where the token exchange request fails once, to make sure all is recovering properly?
There was a problem hiding this comment.
Would it be possible to add a test w/ a network error here?
The http mocking lib makes this hard to do here, since it wraps excdeptions with an IllegalStateException. I think the tests we have in HttpRequestAttemptRetryTest in wiremock, including covers that scenario adequately and better simulates network issues
And if I can ask, can we have a client request where the token exchange request fails once, to make sure all is recovering properly?
We have exchangeOAuth2TokenWithRetriesSuccess and exchangeOAuth2TokenWithRetriesFailure in OAuth2ClientTest - are those missing a scenario that should be tested for?
| @@ -1662,7 +1669,8 @@ public void listObjects_500() throws Exception { | |||
| .get()); | |||
|
|
|||
| // Then | |||
| mockHttpClient.verify().post(postUrl).called(1 + DEFAULT_MAX_RETRIES); | |||
| // Simplified logic: POST requests now retry on 5xx errors (1 initial + 3 retries = 4 total) | |||
There was a problem hiding this comment.
Prob keeping these as they were (1 + DEFAULT_MAX_RETRIES) is more maintainable as we change the value of DEFAULT_MAX_RETRIES
| @@ -140,11 +140,11 @@ public void exchangeOAuth2TokenWithRetriesSuccess(WireMockRuntimeInfo wm) throws | |||
| .willReturn(jsonResponse("rate_limited", 429)) | |||
| .willSetStateTo("rate limited once")); | |||
|
|
|||
| // Then return 500 | |||
| // Then return 500 with Retry-After header (breaking change: POST requests need Retry-After for 5xx retries) | |||
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
jimmyjames
force-pushed
the
feat/retry-after
branch
from
22ab88e to
f5895f8
Compare
jimmyjames
force-pushed
the
feat/retry-after
branch
from
bb57ebf to
3432536
Compare
Retry-After header support with enhanced retry strategy
Summary
This PR implements comprehensive RFC 9110 compliant retry behavior with support for the
Retry-Afterheader, enhanced exponential backoff with jitter, network error recovery, and improved configuration validation with strict input validation.Fixes #155
Key Features
🔄 RFC 9110 Compliant Retry-After Header Support
📈 Enhanced Exponential Backoff with Jitter
2^retryCount * minimumRetryDelaywith random jitter🛡️ Unified Retry Strategy
Retry-Afterheader when presentRetry-Afternot available🔧 Configuration Enhancements
minimumRetryDelayconfigurationRetry-Afterheader value exposed inFgaErrorobjectsBreaking Changes⚠️
1. Strict Configuration Validation
Previous:
minimumRetryDelayaccepted null values and negative durationsNew: Throws
IllegalArgumentExceptionfor null or negative values2. Maximum Retry Validation
Previous: No validation on
maxRetriesvalueNew: Throws
IllegalArgumentExceptionifmaxRetries > 153. Enhanced Error Information
New:
FgaErrornow exposesRetry-Afterheader valueExample Usage
Basic Configuration
Retry Behavior Examples
Scenario 1: Rate Limiting (429)
Scenario 2: Server Error with Retry-After Header
Scenario 3: Server Error without Retry-After Header
Scenario 4: Network Error Recovery
Scenario 5: Exponential Backoff Sequence
Implementation Details
New Utility Classes
RetryAfterHeaderParser: RFC 9110 compliant header parsing for both seconds and HTTP-date formatsExponentialBackoff: Exponential backoff calculation with jitter and maximum delay capRetryStrategy: Centralized retry decision logic for different error typesEnhanced Classes
HttpRequestAttempt: Updated retry logic with network error handling and proper minimum delay enforcementConfiguration: Added strict validation forminimumRetryDelayandmaxRetriesparametersFgaError: ExposesRetry-Afterheader value for improved observabilityComprehensive Test Coverage
Retry-Afterresponses and network error simulationMigration Guide
Update configuration validation: Ensure
minimumRetryDelayvalues are non-null and positiveValidate retry configuration: Ensure
maxRetriesvalues are ≤ 15Update error handling (optional): Use new
getRetryAfterHeader()method for custom retry logicBackward Compatibility
getRetryAfterHeader()methodPerformance and Reliability Improvements
Retry-AfterheaderTesting
All existing tests pass (348+ tests) plus comprehensive new test coverage:
New Test Coverage
minimumRetryDelayandmaxRetriesTest Categories
References