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

The pull request aims to classify UnknownHostException as a transient failure to trigger retry logic. While DownloadManager.java correctly adds UnknownHostException to its retryable exceptions, HttpConnector.java wraps this exception in an UnrecoverableHttpException. This counteracts the intended retry mechanism, as UnrecoverableHttpException is not considered retryable by the DownloadManager. This inconsistency needs to be resolved to ensure the retry logic functions as intended.

Comment on lines +163 to +165
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

This change wraps UnknownHostException in an UnrecoverableHttpException. However, the PR description states that UnknownHostException should trigger retry logic, and DownloadManager.java has been updated to consider UnknownHostException as retryable. By wrapping it in UnrecoverableHttpException, the DownloadManager will not retry, as UnrecoverableHttpException is not listed as a retryable exception. This contradicts the stated goal of the pull request. To enable retries for UnknownHostException, it should either be re-thrown directly or wrapped in an exception that DownloadManager recognizes as retryable.

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

@meteorcloudy meteorcloudy enabled auto-merge January 9, 2026 13:56
@meteorcloudy meteorcloudy added this pull request to the merge queue Jan 9, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2026
@iancha1992 iancha1992 added this pull request to the merge queue Jan 9, 2026
Merged via the queue into bazelbuild:release-8.6.0 with commit cd4e655 Jan 9, 2026
47 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.

4 participants