Skip to content

Conversation

@bazel-io
Copy link
Member

@bazel-io bazel-io commented Jan 9, 2026

UnknownHostException can represent a transient failure in DNS.
Permit this exception to trigger the retry logic for the download loop.

Closes #28023.

PiperOrigin-RevId: 854154072
Change-Id: I84bca74b00af1bbbb322ecc3f8697ed252dbfe64

Commit 8fe8d55

UnknownHostException can represent a transient failure in DNS.
Permit this exception to trigger the retry logic for the download loop.

Closes bazelbuild#28023.

PiperOrigin-RevId: 854154072
Change-Id: I84bca74b00af1bbbb322ecc3f8697ed252dbfe64
@bazel-io bazel-io requested a review from a team as a code owner January 9, 2026 13:37
@bazel-io bazel-io added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Jan 9, 2026
@bazel-io bazel-io requested a review from meteorcloudy January 9, 2026 13:37
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to make UnknownHostException retryable during repository downloads, which is a good improvement for handling transient DNS failures. The approach is to classify UnknownHostException as retryable in the DownloadManager and adjust exception handling in HttpConnector.

However, the implementation in HttpConnector incorrectly uses addSuppressed instead of initCause to wrap the original exception. This prevents the DownloadManager's retry logic from detecting the UnknownHostException as it only checks the exception cause chain. I've left a comment with a suggested fix.

Comment on lines +161 to +163
IOException httpException = new UnrecoverableHttpException(message);
httpException.addSuppressed(e);
throw httpException;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The retry logic in DownloadManager.shouldRetryDownload inspects the exception's cause chain to find a retryable exception. Using addSuppressed(e) will not place the original UnknownHostException in the cause chain, so it won't be detected for a retry.

To ensure the exception can be caught by the retry logic, you should use initCause(e) to properly chain the exceptions.

Suggested change
IOException httpException = new UnrecoverableHttpException(message);
httpException.addSuppressed(e);
throw httpException;
IOException httpException = new UnrecoverableHttpException(message);
httpException.initCause(e);
throw httpException;

@meteorcloudy meteorcloudy enabled auto-merge January 9, 2026 13:56
@meteorcloudy meteorcloudy added this pull request to the merge queue Jan 9, 2026
Merged via the queue into bazelbuild:release-9.0.0 with commit 236ca13 Jan 9, 2026
46 checks passed
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants