[8.6.0] [Repository Downloader] Fail over immediately on TLS errors (https://github.com/bazelbuild/bazel/pull/28347) #28459
Conversation
…ld#28347) When a mirror URL fails with an SSLException (e.g. expired certificate), the downloader should fail over to the next mirror immediately instead of retrying the failing URL multiple times. Fixes bazelbuild#28158 RELNOTES: Bazel now fails over immediately to mirror URLs if a TLS handshake error occurs. CC @lberki @fmeum @Wyv Closes bazelbuild#28347. PiperOrigin-RevId: 861936486 Change-Id: If40b966224c1e0687dd258adb2c03bbf055b8c40
bazel-io
added
team-ExternalDeps
bazel-io
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 introduces a crucial improvement by ensuring that the repository downloader fails over immediately when encountering TLS handshake errors, such as expired certificates. This change enhances the reliability of downloads by preventing unnecessary retries on unrecoverable errors. A new test case has been added to validate this behavior, and the code has been refactored to use FileSystemUtils.readContent for file operations.
| if (e.getMessage() != null | ||
| && (e.getMessage().contains("certificate") | ||
| || e.getMessage().contains("CertPathValidatorException"))) { |
There was a problem hiding this comment.
Relying on string matching (e.getMessage().contains("certificate") || e.getMessage().contains("CertPathValidatorException")) to identify permanent TLS errors is fragile. Exception messages can vary across Java versions or different SSL/TLS providers, potentially leading to incorrect error classification and retry behavior. A more robust approach would be to inspect the cause chain of the SSLException for specific exception types related to certificate validation (e.g., java.security.cert.CertificateException or javax.net.ssl.SSLHandshakeException with a CertificateException in its cause).
iancha1992
requested review from
meteorcloudy
and removed request for
meteorcloudy
|
Might be replaced by #28461? |
This commit adds the missing test case from PR bazelbuild#28459 as requested by @iancha1992. The changes include: - Updated imports to use FileSystemUtils instead of DataInputStream - Replaced the readFile() method with FileSystemUtils.readContent() - Added downloadFrom2UrlsFirstTlsErrorSecondOk() test to verify TLS error failover behavior These changes ensure that the test coverage for TLS error handling is complete and consistent with the main PR bazelbuild#28459.
|
I would merge this one since it's prepared by a deterministic bot instead of a undeterministric one. |
|
Thanks, @meteorcloudy appreciate it! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2ea95c4
github-actions
bot
removed
the
awaiting-review
4 participants
When a mirror URL fails with an SSLException (e.g. expired certificate), the downloader should fail over to the next mirror immediately instead of retrying the failing URL multiple times.
Fixes #28158
RELNOTES: Bazel now fails over immediately to mirror URLs if a TLS handshake error occurs.
CC @lberki @fmeum @Wyv
Closes #28347.
PiperOrigin-RevId: 861936486
Change-Id: If40b966224c1e0687dd258adb2c03bbf055b8c40
Commit 5594c2a