Make SSL/TLS errors non-retriable to enable URL fallback #28227
+40
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes bazelbuild#28158 SSL/TLS errors such as expired certificates or hostname mismatches are not transient - retrying the same URL will not resolve them. Previously, these errors were retried up to 8 times before failing, wasting time when alternative mirror URLs were available. This change catches SSLException and throws UnrecoverableHttpException, which signals HttpConnectorMultiplexer to immediately try the next available URL instead of retrying the failing one. This is consistent with how UnknownHostException is already handled.
github-actions
bot
added
team-ExternalDeps
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of retrying on SSL/TLS errors by treating them as unrecoverable, which will allow fallback to other URLs. The implementation is straightforward. My main concern is the lack of automated tests for this new behavior. While manual verification is planned, adding an automated test case would be crucial to prevent future regressions for this important networking logic. Additionally, the new code for handling SSLException duplicates logic from the existing UnknownHostException handler. I've left a comment with a suggestion to improve consistency and noted this as a candidate for future refactoring to improve maintainability.
src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java
Outdated
Show resolved
Hide resolved
- Use format() helper method for error message construction for consistency with other logging in the file - Add unit test sslError_throwsUnrecoverableHttpException() that verifies SSL errors are treated as non-retriable
The test scenario (HTTPS connection to plain HTTP server) triggers different exception types on Windows vs Linux/macOS. Skip the test on Windows since the actual SSL certificate handling is tested on other platforms.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #28158
UnrecoverableHttpExceptionforSSLException, allowingHttpConnectorMultiplexerto immediately try alternative URLsUnknownHostExceptionProblem
When a TLS certificate expires on a mirror server, Bazel retries the same URL up to 8 times before failing. This wastes time when alternative mirror URLs are available that could succeed.
Solution
Catch
SSLExceptionand throwUnrecoverableHttpExceptioninstead of retrying. This signals the multiplexer to fallback to the next available URL immediately.Changes
HttpConnector.java: Added catch block forSSLExceptionthat throwsUnrecoverableHttpExceptionHttpConnectorTest.java: AddedsslError_throwsUnrecoverableHttpException()testTest plan
sslError_throwsUnrecoverableHttpException()that verifies SSL errors throwUnrecoverableHttpException