Repository navigation
fix(auth): retry dropped token connections - #14873
huong8373tt-beep wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds automatic retry logic using ExponentialBackOff and HttpBackOffIOExceptionHandler to handle transient IOExceptions in ImpersonatedCredentials, StsRequestHandler, and UserCredentials, along with corresponding unit tests. The review feedback suggests refactoring the duplicated retry configuration code into a reusable utility method. Additionally, it highlights a discrepancy in the test setup where HTTP error responses are mocked as thrown IOExceptions, causing incorrect retry behavior in tests compared to production.
| request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES); | ||
| ExponentialBackOff backoff = | ||
| new ExponentialBackOff.Builder() | ||
| .setInitialIntervalMillis(OAuth2Utils.INITIAL_RETRY_INTERVAL_MILLIS) | ||
| .setRandomizationFactor(OAuth2Utils.RETRY_RANDOMIZATION_FACTOR) | ||
| .setMultiplier(OAuth2Utils.RETRY_MULTIPLIER) | ||
| .build(); | ||
| request.setIOExceptionHandler(new HttpBackOffIOExceptionHandler(backoff)); |
There was a problem hiding this comment.
The exact same block of code for configuring ExponentialBackOff and HttpBackOffIOExceptionHandler is duplicated across ImpersonatedCredentials.java, StsRequestHandler.java, and UserCredentials.java. To improve maintainability and ensure consistency, consider refactoring this setup into a reusable helper method in a dedicated utility class (e.g., OAuth2Utils.initializeRetrySettings(HttpRequest request)) rather than duplicating it locally, avoiding tight coupling and inverted dependencies.
References
- When sharing constants or configuration logic across different modules, extract them into a dedicated utility class rather than duplicating them locally or importing them from high-level wrappers, to avoid tight coupling and inverted dependencies.
There was a problem hiding this comment.
Addressed in c476a5f0: the shared ExponentialBackOff / HttpBackOffIOExceptionHandler setup now lives in OAuth2Utils.setDefaultRetrySettings(HttpRequest) and is reused by UserCredentials, ImpersonatedCredentials, and StsRequestHandler. The three focused test classes pass (101 tests).
| IOException error = | ||
| TestUtils.buildHttpResponseException( | ||
| "invalidRequest", /* errorDescription= */ null, /* errorUri= */ null)); | ||
| "invalidRequest", /* errorDescription= */ null, /* errorUri= */ null); | ||
| transport.addResponseErrorSequence(error, error, error, error); |
There was a problem hiding this comment.
There is a subtle discrepancy between the test environment and production behavior here. In production, an HTTP error response (like 400 Bad Request representing invalidRequest) is returned by the transport as a standard HTTP response and is not thrown as an IOException by the low-level transport. Therefore, HttpBackOffIOExceptionHandler will not intercept it, and no retries will occur for HTTP error responses. However, in this test, TestUtils.buildHttpResponseException is queued in addResponseErrorSequence, which causes the mock transport to throw it directly from LowLevelHttpRequest.execute(). Because HttpResponseException extends IOException, the HttpBackOffIOExceptionHandler treats it as a network-level IOException and retries it, forcing us to queue the error 4 times to exhaust the retry budget. While queuing the error 4 times is a functional workaround to make the test pass, it is misleading because it implies that HTTP error responses are retried in production. If the mock transport supports it, consider configuring it to return a mock LowLevelHttpResponse with a 400 status code and the error JSON body instead of throwing an HttpResponseException directly.
There was a problem hiding this comment.
Addressed in c476a5f0: this test now returns an actual HTTP 400 LowLevelHttpResponse with the OAuth error JSON. Only the dedicated dropped-connection tests use the IOException path, so the tests now match production retry behavior.
|
Addressed the review feedback in commit
Validation after the change:
Slai Agent Runtime performed the investigation, implementation, and test execution. The account owner authorized this contribution and is responsible for its content. |
Summary
Retries dropped connections for OAuth2 token requests made by
UserCredentials,ImpersonatedCredentials, andStsRequestHandlerusing the existing auth-library exponential-backoff settings.The existing STS error tests now queue failures through the full retry budget, preserving their assertions that terminal OAuth and I/O failures are still surfaced.
Fixes #14772.
Validation
mvn -Dmaven.repo.local=E:\toolchains\m2repo -Dtest=UserCredentialsTest,ImpersonatedCredentialsTest,StsRequestHandlerTest -DfailIfNoTests=false -Dcheckstyle.skip=true -Dfmt.skip=true testmvn -Dmaven.repo.local=E:\toolchains\m2repo -Dcheckstyle.skip=true fmt:formatgit diff --checkDisclosure
Slai Agent Runtime performed the investigation, implementation, and test execution. The account owner authorized this contribution and is responsible for its content.