[8.6.0] Refactor TLS error handling (Backport fix for #28459) by Ashutosh0x · Pull Request #28461 · bazelbuild/bazel

Ashutosh0x · 2026-01-28T08:22:45Z

This PR provides a robust fix for the TLS error handling logic originally proposed in #28459. It addresses the review feedback by inspecting the exception cause chain for certificate errors instead of checking exception messages strings.

Supersedes #28459.

gemini-code-assist

Code Review

This pull request is a major update, bumping the Bazel version to 9.0.0 and overhauling CI configurations, dependencies, and build settings. It also introduces a substantial amount of new documentation and tooling. My review identifies a few high-severity concerns in the new configurations that could impact CI stability and build correctness. These include potentially incorrect test scoping in CI, improper flag application in .bazelrc, and a possible version mismatch in a dependency patch. None of the comments were dropped or modified based on the provided rules, as they did not directly apply to the specific issues raised.

.bazelci/postsubmit.yml

.bazelrc

MODULE.bazel

fmeum · 2026-01-28T09:43:07Z

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java

+          while (t != null) {
+            if (t instanceof CertificateException || t instanceof CertPathValidatorException) {
+              String message = "TLS error: " + e.getMessage();
+              eventHandler.handle(Event.progress(message));


It doesn't look right to report one event per cause. Could we report just the outer exception's message?

Ashutosh0x · 2026-01-28T10:06:32Z

Addressed @fmeum's review feedback:

Changes made:

Refactored the SSL exception handling to first inspect the entire cause chain using a for-loop
Set a boolean flag when a certificate error is found
Report the TLS error event only once after the loop completes
Use the outer SSLException's message as requested

The code now clearly shows that:

We check the cause chain first (for-loop)
We report a single event with the outer exception's message
We throw the unrecoverable exception

This addresses the concern about potentially reporting one event per cause.

Ashutosh0x · 2026-01-28T10:08:32Z

Regarding the gemini-code-assist bot review comments:

The bot appears to have analyzed files that are not part of this PR diff. This PR only modifies \HttpConnector.java\ to fix TLS error handling.

To clarify:

This PR does NOT change \MODULE.bazel, .bazelrc, or .bazelci/postsubmit.yml\
The
ules_jvm_external\ patch concern is not applicable - the codebase correctly uses version \6.0\ with
ules_jvm_external_6.0.patch\

The bot's comments about version mismatches (6.5/6.6) appear to be hallucinated.

gemini-code-assist · 2026-01-28T10:08:49Z

Thank you for bringing this to my attention, @Ashutosh0x. I apologize for the incorrect review comments regarding files outside the scope of this pull request. My analysis mistakenly included files that were not part of the diff, leading to irrelevant suggestions.

I understand that this PR specifically modifies src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java to improve TLS error handling. I will ensure that my future reviews are strictly limited to the changes within the provided diffs and relevant context.

Your feedback is valuable and helps me improve. I am an AI and sometimes make mistakes, and I appreciate you pointing out these inaccuracies.

Ashutosh0x · 2026-01-28T10:14:52Z

Thank you for bringing this to my attention, @Ashutosh0x. I apologize for the incorrect review comments regarding files outside the scope of this pull request. My analysis mistakenly included files that were not part of the diff, leading to irrelevant suggestions.

I understand that this PR specifically modifies src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java to improve TLS error handling. I will ensure that my future reviews are strictly limited to the changes within the provided diffs and relevant context.

Your feedback is valuable and helps me improve. I am an AI and sometimes make mistakes, and I appreciate you pointing out these inaccuracies.

@fmeum Ai can make mistakes 😂

Ashutosh0x · 2026-01-28T12:12:57Z

Ive refactored the TLS error handling to properly inspect the exception cause chain for certificate errors, ensuring we only fail over when it's appropriate. I also fixed the missing SSLException import and removed the accidentally committed merge conflict markers. The CI should be much cleaner now. @fmeum, could you take another look?

meteorcloudy · 2026-01-28T13:29:58Z

@Ashutosh0x Can you explain what's the difference between this PR and #28459?
Two suggestions:

Keep the PR description clear about which change you are backporting and what extra change you made if any.
Looks like we are also missing the test

@fmeum

Address @fmeum's review feedback: refactored the SSL exception handling to first check the entire cause chain for certificate errors using a for loop, then report a single event only if a certificate error is found. This ensures we report just the outer exception's message once, rather than appearing to report one event per cause. Changes: - Use a for-loop instead of while to inspect the exception cause chain - Set a boolean flag when a certificate error is found - Report the TLS error event only once after the loop completes - Use the outer SSLException's message as requested

Ashutosh0x · 2026-01-28T14:43:44Z

@meteorcloudy Thank you for the review and suggestions!

Difference from #28459:
This PR backports the fix from #28459 (now merged to master) to the release-8.6.0 branch. The core logic is the same:

Inspects the exception cause chain for CertificateException or CertPathValidatorException
Reports a single TLS error event (addressing @fmeum's feedback)
Throws UnrecoverableHttpException to prevent retries on permanent certificate errors

Tests Added:
I've now added unit tests to HttpConnectorTest.java covering:

ssLException_withCertificateCause_throwsUnrecoverableHttpException - verifies immediate failover on CertificateException
ssLException_withCertPathValidatorCause_throwsUnrecoverableHttpException - verifies immediate failover on CertPathValidatorException
ssLException_withoutCertificateCause_retries - ensures transient SSL errors (e.g., 'Connection reset') still trigger retries

All 36 HttpConnectorTest tests pass locally.

Ready for another review!

iancha1992 · 2026-01-28T19:26:56Z

@Ashutosh0x Don't we also need to make changes for
src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java? Like in https://github.com/bazelbuild/bazel/pull/28459/changes

@iancha1992

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.

Ashutosh0x · 2026-01-29T02:20:17Z

@iancha1992 Thank you for the review. I've added the missing changes to HttpDownloaderTest.java from PR #28459.

Changes made in commit 34ad9b1:

Updated imports to use FileSystemUtils instead of DataInputStream
Replaced the local readFile() method with FileSystemUtils.readContent()
Added new test downloadFrom2UrlsFirstTlsErrorSecondOk() to verify TLS error failover behavior

This test validates that when the first URL encounters a TLS/SSL error, the downloader correctly fails over to the second URL and completes the download successfully.

The PR is now fully aligned with #28459.

meteorcloudy · 2026-01-30T10:03:14Z

merging #28459 instead.

Ashutosh0x requested a review from a team as a code owner January 28, 2026 08:22

gemini-code-assist bot reviewed Jan 28, 2026

View reviewed changes

.bazelci/postsubmit.yml Outdated Show resolved Hide resolved

.bazelrc Outdated Show resolved Hide resolved

MODULE.bazel Outdated Show resolved Hide resolved

Ashutosh0x force-pushed the fix-pr-28459-tls-handling branch 3 times, most recently from 03c9588 to 1d8bce1 Compare January 28, 2026 09:11

fmeum reviewed Jan 28, 2026

View reviewed changes

Ashutosh0x force-pushed the fix-pr-28459-tls-handling branch from 1d8bce1 to 7ac214c Compare January 28, 2026 10:06

meteorcloudy removed team-Performance Issues for Performance teams team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-Java Issues for Java rules team-Rules-CPP Issues for C++ rules team-Rules-Python Native rules for Python labels Jan 28, 2026

meteorcloudy changed the title ~~Refactor TLS error handling (Backport fix for #28459)~~ [8.6.0] Refactor TLS error handling (Backport fix for #28459) Jan 28, 2026

Ashutosh0x force-pushed the fix-pr-28459-tls-handling branch from 7ac214c to e932303 Compare January 28, 2026 14:43

This was referenced Jan 28, 2026

[8.6.0] [Repository Downloader] Fail over immediately on TLS errors #28379

Closed

[8.6.0] [Repository Downloader] Fail over immediately on TLS errors (https://github.com/bazelbuild/bazel/pull/28347) #28459

Merged

iancha1992 requested a review from meteorcloudy January 29, 2026 16:56

iancha1992 enabled auto-merge January 29, 2026 16:56

meteorcloudy closed this Jan 30, 2026

auto-merge was automatically disabled January 30, 2026 10:03
Pull request was closed

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jan 30, 2026

ofek mentioned this pull request Feb 4, 2026

Fix UAF in scan preloading and add OOM protection bytedance/bolt#167

Merged

Conversation

Ashutosh0x commented Jan 28, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmeum Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Ashutosh0x commented Jan 28, 2026

Uh oh!

Ashutosh0x commented Jan 28, 2026

Uh oh!

gemini-code-assist bot commented Jan 28, 2026

Uh oh!

Ashutosh0x commented Jan 28, 2026

Uh oh!

Ashutosh0x commented Jan 28, 2026

Uh oh!

meteorcloudy commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ashutosh0x commented Jan 28, 2026

Uh oh!

iancha1992 commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ashutosh0x commented Jan 29, 2026

Uh oh!

meteorcloudy commented Jan 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

meteorcloudy commented Jan 28, 2026 •

edited

Loading

iancha1992 commented Jan 28, 2026 •

edited

Loading