diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java index d6bf6656f362e4..04bc766ce42939 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java @@ -110,13 +110,13 @@ public class RepositoryOptions extends OptionsBase { @Option( name = "experimental_repository_downloader_retries", - defaultValue = "0", + defaultValue = "5", documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.UNKNOWN}, metadataTags = {OptionMetadataTag.EXPERIMENTAL}, help = - "The maximum number of attempts to retry a download error. If set to 0, retries are" - + " disabled.") + "The maximum number of attempts to retry a download error while fetching external" + + " repositories and modules. If set to 0, retries are disabled.") public int repositoryDownloaderRetries; @Option( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java index c0f42b8735b55c..500a852ecfac87 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java @@ -36,9 +36,10 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InterruptedIOException; -import java.net.SocketException; +import java.net.SocketTimeoutException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; @@ -334,6 +335,13 @@ private Path downloadInExecutor( clientEnv, type); break; + } catch (SocketTimeoutException e) { + // SocketTimeoutExceptions are subclasses of InterruptedIOException, but they do not + // necessarily indicate an external interruption. Treat them like ordinary IOExceptions so + // they can be retried. + if (!shouldRetryDownload(e, attempt)) { + throw e; + } } catch (InterruptedIOException e) { throw new InterruptedException(e.getMessage()); } catch (IOException e) { @@ -358,11 +366,18 @@ private boolean shouldRetryDownload(IOException e, int attempt) { return false; } + if (isPermanentlyUnretryableException(e)) { + return false; + } + if (isRetryableException(e)) { return true; } for (var suppressed : e.getSuppressed()) { + if (isPermanentlyUnretryableException(suppressed)) { + continue; + } if (isRetryableException(suppressed)) { return true; } @@ -371,8 +386,13 @@ private boolean shouldRetryDownload(IOException e, int attempt) { return false; } + private boolean isPermanentlyUnretryableException(Throwable e) { + return e instanceof UnrecoverableHttpException || e instanceof FileNotFoundException; + } + private boolean isRetryableException(Throwable e) { - return e instanceof ContentLengthMismatchException || e instanceof SocketException; + // Broad retry policy: retry on most IOExceptions, but not on those we treat as permanent. + return e instanceof IOException && !isPermanentlyUnretryableException(e); } /** @@ -460,6 +480,11 @@ public byte[] downloadAndReadOneUrlForBzlmod( eventHandler, clientEnv); break; + } catch (SocketTimeoutException e) { + // See comment in #downloadInExecutor. + if (!shouldRetryDownload(e, attempt)) { + throw e; + } } catch (InterruptedIOException e) { throw new InterruptedException(e.getMessage()); } catch (IOException e) { diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManagerTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManagerTest.java new file mode 100644 index 00000000000000..c19bc3255fc9f0 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManagerTest.java @@ -0,0 +1,208 @@ +// Copyright 2026 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.bazel.repository.downloader; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.google.auth.Credentials; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.JavaIoFileSystem; +import com.google.devtools.build.lib.vfs.Path; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.net.URL; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.Phaser; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.After; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class DownloadManagerTest { + + @Rule public final TemporaryFolder workingDir = new TemporaryFolder(); + + private final ExecutorService executor = Executors.newFixedThreadPool(1); + + @After + public void after() { + executor.shutdown(); + } + + private static DownloadManager newDownloadManagerForTest(Downloader downloader) { + RepositoryCache repositoryCache = mock(RepositoryCache.class); + when(repositoryCache.isEnabled()).thenReturn(false); + HttpDownloader bzlmodHttpDownloader = mock(HttpDownloader.class); + return new DownloadManager(repositoryCache, downloader, bzlmodHttpDownloader); + } + + @Test + public void retriesOnGenericIOException() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + Downloader throwingDownloader = + new Downloader() { + @Override + public void download( + List urls, + Map> headers, + Credentials credentials, + Optional checksum, + String canonicalId, + Path output, + ExtendedEventHandler eventHandler, + Map clientEnv, + Optional type) + throws IOException { + attempts.incrementAndGet(); + throw new IOException("boom"); + } + }; + + DownloadManager downloadManager = newDownloadManagerForTest(throwingDownloader); + downloadManager.setRetries(2); + + JavaIoFileSystem fs = new JavaIoFileSystem(DigestHashFunction.SHA256); + Path out = fs.getPath(workingDir.newFile().getAbsolutePath()); + + Future f = + downloadManager.startDownload( + executor, + ImmutableList.of(new URL("http://example.invalid/file")), + ImmutableMap.of(), + ImmutableMap.of(), + Optional.empty(), + "canonical", + Optional.empty(), + out, + /* eventHandler= */ mock(ExtendedEventHandler.class), + ImmutableMap.of(), + "ctx", + new Phaser()); + + assertThrows(IOException.class, () -> downloadManager.finalizeDownload(f)); + assertThat(attempts.get()).isEqualTo(3); // 1 initial + 2 retries + } + + @Test + public void doesNotRetryOnUnrecoverableHttpException() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + Downloader throwingDownloader = + new Downloader() { + @Override + public void download( + List urls, + Map> headers, + Credentials credentials, + Optional checksum, + String canonicalId, + Path output, + ExtendedEventHandler eventHandler, + Map clientEnv, + Optional type) + throws IOException { + attempts.incrementAndGet(); + throw new UnrecoverableHttpException("nope"); + } + }; + + DownloadManager downloadManager = newDownloadManagerForTest(throwingDownloader); + downloadManager.setRetries(5); + + JavaIoFileSystem fs = new JavaIoFileSystem(DigestHashFunction.SHA256); + Path out = fs.getPath(workingDir.newFile().getAbsolutePath()); + + Future f = + downloadManager.startDownload( + executor, + ImmutableList.of(new URL("http://example.invalid/file")), + ImmutableMap.of(), + ImmutableMap.of(), + Optional.empty(), + "canonical", + Optional.empty(), + out, + /* eventHandler= */ mock(ExtendedEventHandler.class), + ImmutableMap.of(), + "ctx", + new Phaser()); + + assertThrows(IOException.class, () -> downloadManager.finalizeDownload(f)); + assertThat(attempts.get()).isEqualTo(1); + } + + @Test + public void doesNotRetryOnFileNotFoundException() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + Downloader throwingDownloader = + new Downloader() { + @Override + public void download( + List urls, + Map> headers, + Credentials credentials, + Optional checksum, + String canonicalId, + Path output, + ExtendedEventHandler eventHandler, + Map clientEnv, + Optional type) + throws IOException { + attempts.incrementAndGet(); + throw new FileNotFoundException("missing"); + } + }; + + DownloadManager downloadManager = newDownloadManagerForTest(throwingDownloader); + downloadManager.setRetries(5); + + JavaIoFileSystem fs = new JavaIoFileSystem(DigestHashFunction.SHA256); + Path out = fs.getPath(workingDir.newFile().getAbsolutePath()); + + Future f = + downloadManager.startDownload( + executor, + ImmutableList.of(new URL("http://example.invalid/file")), + ImmutableMap.of(), + ImmutableMap.of(), + Optional.empty(), + "canonical", + Optional.empty(), + out, + /* eventHandler= */ mock(ExtendedEventHandler.class), + ImmutableMap.of(), + "ctx", + new Phaser()); + + assertThrows(IOException.class, () -> downloadManager.finalizeDownload(f)); + assertThat(attempts.get()).isEqualTo(1); + } +} +