From 32fcec877c7af9623b887a3c144eca7516696c8b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 28 Jan 2026 02:36:08 -0800 Subject: [PATCH] Fix a deadlock in `RemotePathChecker` when using a disk cache (https://github.com/bazelbuild/bazel/pull/28416) The call to `RemoteActionFileSystem#getInputStream` in `RemotePathChecker#isRemote` could result in a deadlock since it is executed on the same executor that is also used to prefetch the file from the disk cache. This is fixed by replacing the blocking call with a future. Along the way the method is renamed to avoid confusion due to two different meanings of "remote" (not being available in the disk cache vs. not being materialized at the exec path). Fixing this bug makes it possible to run all BwoB tests with disk cache. Related to #28408 Closes #28416. PiperOrigin-RevId: 862135346 Change-Id: Ic6a0b190be8f4769788078afe481496996f82013 --- .../lib/remote/RemoteActionFileSystem.java | 36 ++++---- .../lib/remote/RemoteExecutionCache.java | 85 +++++++++++-------- .../BuildWithoutTheBytesIntegrationTest.java | 11 +++ .../build/lib/remote/CombinedCacheTest.java | 4 +- 4 files changed, 80 insertions(+), 56 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index b47645a653626a..5b2305040f6566 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -17,12 +17,15 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.util.concurrent.Futures.immediateFailedFuture; +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; +import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; @@ -373,7 +376,10 @@ public boolean delete(PathFragment path) throws IOException { @Override public InputStream getInputStream(PathFragment path) throws IOException { try { - downloadIfRemote(path); + getFromFuture(downloadIfRemote(path)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException(String.format("Received interrupt while fetching file '%s'", path), e); } catch (BulkTransferException e) { var newlyLostInputs = e.getLostArtifacts(inputArtifactData::getInput); if (!newlyLostInputs.isEmpty()) { @@ -384,29 +390,23 @@ public InputStream getInputStream(PathFragment path) throws IOException { return localFs.getPath(path).getInputStream(); } - private void downloadIfRemote(PathFragment path) throws IOException { - if (!isRemote(path)) { - return; + /** Downloads the file at {@code path} if it is remote. */ + public ListenableFuture downloadIfRemote(PathFragment path) { + try { + if (!isRemote(path)) { + return immediateVoidFuture(); + } + } catch (IOException e) { + return immediateFailedFuture(e); } PathFragment execPath = path.relativeTo(execRoot); ActionInput input = inputArtifactData.getInput(execPath); if (input == null) { // TODO(tjgq): Also look up the remote output tree. - return; - } - - try { - getFromFuture( - inputFetcher.prefetchFiles( - action, - ImmutableList.of(input), - inputArtifactData, - Priority.CRITICAL, - Reason.INPUTS)); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new IOException(String.format("Received interrupt while fetching file '%s'", path), e); + return immediateVoidFuture(); } + return inputFetcher.prefetchFiles( + action, ImmutableList.of(input), inputArtifactData, Priority.CRITICAL, Reason.INPUTS); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java index a8f88a801faf1f..ea883fdb42e8b8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java @@ -17,6 +17,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.util.concurrent.Futures.immediateFailedFuture; import static com.google.common.util.concurrent.Futures.immediateFuture; +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable; import static com.google.devtools.build.lib.remote.util.RxFutures.toSingle; @@ -33,6 +34,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.flogger.GoogleLogger; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.profiler.Profiler; @@ -74,33 +76,41 @@ public class RemoteExecutionCache extends CombinedCache implements MerkleTreeUpl private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); /** - * An interface used to check whether a given {@link Path} is stored in a remote or a disk cache. + * An interface used to check whether a given {@link Path} is available without contacting the + * remote cache, i.e., it is present on the local disk, perhaps after being downloaded from the + * disk cache. */ public interface RemotePathChecker { - boolean isRemote(RemoteActionExecutionContext context, Path path) throws IOException; + ListenableFuture isAvailableLocally(RemoteActionExecutionContext context, Path path); } private RemotePathChecker remotePathChecker = new RemotePathChecker() { @Override - public boolean isRemote(RemoteActionExecutionContext context, Path path) - throws IOException { + public ListenableFuture isAvailableLocally( + RemoteActionExecutionContext context, Path path) { var fs = path.getFileSystem(); - if (fs instanceof RemoteActionFileSystem remoteActionFileSystem) { - if (remoteActionFileSystem.isRemote(path)) { - if (context.getReadCachePolicy().allowDiskCache()) { - try (var inputStream = path.getInputStream()) { - // If the file exists in the disk cache, download it and continue the upload. - return false; - } catch (IOException e) { - logger.atWarning().withCause(e).log( - "Failed to get input stream for %s", path.getPathString()); - } - } - return true; - } + if (!(fs instanceof RemoteActionFileSystem remoteActionFileSystem)) { + return immediateFuture(true); + } + // If the file is available in the disk cache, we can attempt to download it from there. + ListenableFuture downloadFromDiskCache = immediateVoidFuture(); + if (context.getReadCachePolicy().allowDiskCache()) { + downloadFromDiskCache = + Futures.catchingAsync( + remoteActionFileSystem.downloadIfRemote(path.asFragment()), + IOException.class, + e -> { + logger.atWarning().withCause(e).log( + "Failed to download %s", path.getPathString()); + return immediateVoidFuture(); + }, + directExecutor()); } - return false; + return Futures.transform( + downloadFromDiskCache, + unused -> remoteActionFileSystem.getHostFileSystem().exists(path.asFragment()), + directExecutor()); } }; @@ -182,25 +192,26 @@ public ListenableFuture uploadFile( RemotePathResolver remotePathResolver, Digest digest, Path path) { - try { - if (remotePathChecker.isRemote(context, path)) { - // If we get here, the remote input was determined to exist in the remote or disk - // cache at some point before action execution, but reported to be missing when - // querying the remote for missing action inputs; possibly because it was evicted in - // the interim. - if (remotePathResolver != null) { - throw new CacheNotFoundException( - digest, remotePathResolver.localPathToExecPath(path.asFragment())); - } else { - // This path should only be taken for RemoteRepositoryRemoteExecutor, which has no - // way to handle lost inputs. - throw new CacheNotFoundException(digest, path.getPathString()); - } - } - } catch (IOException e) { - return immediateFailedFuture(e); - } - return remoteCacheClient.uploadFile(context, digest, path); + return Futures.transformAsync( + remotePathChecker.isAvailableLocally(context, path), + isAvailableLocally -> { + if (!isAvailableLocally) { + // If we get here, the remote input was determined to exist in the remote or disk + // cache at some point before action execution, but reported to be missing when + // querying the remote for missing action inputs; possibly because it was evicted in + // the interim. + if (remotePathResolver != null) { + throw new CacheNotFoundException( + digest, remotePathResolver.localPathToExecPath(path.asFragment())); + } else { + // This path should only be taken for RemoteRepositoryRemoteExecutor, which has no + // way to handle lost inputs. + throw new CacheNotFoundException(digest, path.getPathString()); + } + } + return remoteCacheClient.uploadFile(context, digest, path); + }, + directExecutor()); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 8e88af79e0c3da..31da671eadae48 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -39,8 +39,10 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; +import java.util.UUID; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -51,6 +53,8 @@ public class BuildWithoutTheBytesIntegrationTest extends BuildWithoutTheBytesIntegrationTestBase { @ClassRule @Rule public static final WorkerInstance worker = IntegrationTestUtils.createWorker(); + @TestParameter public boolean useDiskCache; + @Override protected ImmutableList getStartupOptions() { // Some tests require the ability to create symlinks on Windows. @@ -74,6 +78,10 @@ protected void setupOptions() throws Exception { // The default behavior is to require the target path to exist and make a deep copy. addOptions("--action_env=MSYS=winsymlinks:native"); } + + if (useDiskCache) { + addOptions("--disk_cache=" + UUID.randomUUID()); + } } @Override @@ -127,6 +135,9 @@ protected void assertOutputContains(String content, String contains) throws Exce @Override protected void evictAllBlobs() throws Exception { worker.reset(); + if (useDiskCache) { + addOptions("--disk_cache=" + UUID.randomUUID()); + } } @Override diff --git a/src/test/java/com/google/devtools/build/lib/remote/CombinedCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/CombinedCacheTest.java index c98aa4d723b481..73898ad8b1989c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/CombinedCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/CombinedCacheTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer; import static java.nio.charset.StandardCharsets.UTF_8; @@ -350,7 +351,8 @@ public void ensureInputsPresent_missingInputs_exceptionHasLostInputs() throws Ex RemoteCacheClient cacheProtocol = spy(new InMemoryCacheClient()); RemoteExecutionCache remoteCache = spy(newRemoteExecutionCache(cacheProtocol)); remoteCache.setRemotePathChecker( - (context, path) -> path.relativeTo(execRoot).equals(PathFragment.create("foo"))); + (context, path) -> + immediateFuture(!path.relativeTo(execRoot).equals(PathFragment.create("foo")))); Path path = execRoot.getRelative("foo"); FileSystemUtils.writeContentAsLatin1(path, "bar");