[8.5.0] Do not download in-memory outputs

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -1368,6 +1368,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
         }
 
         if (isInMemoryOutputFile) {
+          // Download into memory only; do not write to disk.
+          remoteOutputChecker.skipDownload(inMemoryOutputPath);
           if (file.contents.isEmpty()) {
             // As the contents field doesn't have presence information, we use the digest size to
             // distinguish between an empty file and one that wasn't inlined.

src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java

@@ -42,14 +42,16 @@
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListSet;
 import java.util.function.Predicate;
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
 /**
- * An {@link RemoteArtifactChecker} that checks the TTL of remote metadata and decides which outputs
- * to download.
+ * An {@link OutputChecker} that decides which outputs to download taking into account the output
+ * mode and the TTL of remote metadata.
  */
 public class RemoteOutputChecker implements RemoteArtifactChecker {
   private enum CommandMode {
@@ -63,10 +65,11 @@ private enum CommandMode {
   private final Clock clock;
   private final CommandMode commandMode;
   private final RemoteOutputsMode outputsMode;
-  private final ImmutableList<Predicate<String>> patternsToDownload;
   @Nullable private final RemoteOutputChecker lastRemoteOutputChecker;
 
+  private final ImmutableList<Predicate<String>> patternsToDownload;
   private final ConcurrentArtifactPathTrie pathsToDownload = new ConcurrentArtifactPathTrie();
+  private final Set<PathFragment> pathsToSkip = ConcurrentHashMap.newKeySet();
 
   public RemoteOutputChecker(
       Clock clock,
@@ -250,10 +253,22 @@ private void addOutputsToDownload(Iterable<? extends ActionInput> files) {
     }
   }
 
+  /** Marks a file for download. */
   public void addOutputToDownload(ActionInput file) {
     pathsToDownload.add(file);
   }
 
+  /**
+   * Marks a file as not for download, regardless of the output mode.
+   *
+   * <p>This is used by {@link RemoteExecutionService} to skip downloading in-memory outputs.
+   *
+   * @param execPath the exec path of the file that is not to be downloaded.
+   */
+  public void skipDownload(PathFragment execPath) {
+    pathsToSkip.add(execPath);
+  }
+
   private boolean shouldAddTopLevelTarget(@Nullable ConfiguredTarget configuredTarget) {
     return switch (commandMode) {
       // Always download outputs of toplevel targets in run mode.
@@ -290,6 +305,9 @@ public boolean shouldDownloadOutput(ActionInput output, RemoteFileArtifactValue
 
   /** Returns whether a remote {@link ActionInput} with the given path should be downloaded. */
   public boolean shouldDownloadOutput(PathFragment execPath) {
+    if (pathsToSkip.contains(execPath)) {
+      return false;
+    }
     return outputsMode == RemoteOutputsMode.ALL
         || pathsToDownload.contains(execPath)
         || matchesPattern(execPath);

src/test/shell/bazel/remote/build_without_the_bytes_test.sh

@@ -1781,6 +1781,90 @@ EOF
   expect_log "START.*: \[.*\] Executing genrule //a:dep"
 }
 
+function test_inmemory_dotd_files() {
+  # Reject multiple downloads of the same digest as a regression test for
+  # https://github.com/bazelbuild/bazel/issues/22387.
+  stop_worker
+  start_worker --error_on_duplicate_downloads
+
+  add_rules_cc MODULE.bazel
+  cat > BUILD <<'EOF'
+load("@rules_cc//cc:cc_library.bzl", "cc_library")
+cc_library(name="foo", srcs=["foo.c"])
+EOF
+  touch foo.c
+
+  # Populate remote cache with .d file.
+  bazel build \
+      --remote_upload_local_results \
+      --remote_cache=grpc://localhost:"${worker_port}" \
+      //:foo &> "${TEST_log}" \
+      || fail "Expected success from uploading invocation"
+
+  # Search for alternative .d file names for compatibility across platforms.
+  local -r dotd_file="$(find -L bazel-out -type f -name 'foo*.d' | head -n1)"
+  if [[ -z "$dotd_file" ]]; then
+    fail ".d file not found under bazel-out"
+  fi
+  local -r dotd_hash="$(sha256sum ${dotd_file} | cut -d' ' -f1)"
+
+  # Delete output tree.
+  bazel clean
+
+  # Build while downloading .d file into memory.
+  bazel build \
+      --experimental_inmemory_dotd_files \
+      --remote_download_all \
+      --remote_cache=grpc://localhost:"${worker_port}" \
+      //:foo &> "$TEST_log" \
+      || fail "Expected success from downloading invocation"
+
+  # The .d file should not have been downloaded to disk.
+  assert_not_exists ${dotd_file}
+}
+
+function test_inmemory_dotd_files_disabled() {
+  # Reject multiple downloads of the same digest as a regression test for
+  # https://github.com/bazelbuild/bazel/issues/22387.
+  stop_worker
+  start_worker --error_on_duplicate_downloads
+
+  add_rules_cc MODULE.bazel
+  cat > BUILD <<'EOF'
+load("@rules_cc//cc:cc_library.bzl", "cc_library")
+cc_library(name="foo", srcs=["foo.c"])
+EOF
+  touch foo.c
+
+  # Populate remote cache with .d file.
+  bazel build \
+      --remote_upload_local_results \
+      --remote_cache=grpc://localhost:"${worker_port}" \
+      //:foo &> "$TEST_log" \
+      || fail "Expected success from uploading invocation"
+
+  # Search for alternative .d file names for compatibility across platforms.
+  local -r dotd_file="$(find -L bazel-bin -type f -name 'foo*.d' | head -n1)"
+  if [[ -z "$dotd_file" ]]; then
+    fail ".d file not found under bazel-out"
+  fi
+  local -r dotd_hash="$(sha256sum ${dotd_file} | cut -d' ' -f1)"
+
+  # Delete output tree.
+  bazel clean
+
+  # Build while downloading .d file onto disk.
+  bazel build \
+      --noexperimental_inmemory_dotd_files \
+      --remote_download_all \
+      --remote_cache=grpc://localhost:"${worker_port}" \
+      //:foo &> "$TEST_log" \
+      || fail "Expected success from downloading invocation"
+
+  # The .d file should have been downloaded to disk.
+  assert_exists ${dotd_file}
+}
+
 function test_remote_download_regex() {
   mkdir -p a
 
@@ -1835,6 +1919,7 @@ EOF
         --remote_executor=grpc://localhost:${worker_port} \
         --remote_download_minimal \
         --remote_download_regex=".*" \
+        --experimental_inmemory_jdeps_files=false \
         //a:test >& $TEST_log || fail "Failed to build"
 
   [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!"

src/test/shell/bazel/remote/remote_utils.sh

@@ -108,4 +108,4 @@ function append_remote_cas_files() {
 
 function delete_remote_cas_files() {
   rm -rf "$cas_path/cas"
-}
+}

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java

@@ -24,6 +24,7 @@
 import build.bazel.remote.execution.v2.FileNode;
 import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy;
 import build.bazel.remote.execution.v2.SymlinkNode;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.remote.CombinedCache;
@@ -36,6 +37,7 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.protobuf.ByteString;
 import java.io.IOException;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
@@ -44,13 +46,21 @@ class OnDiskBlobStoreCache extends CombinedCache {
   private static final ExecutorService executorService =
       MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(1));
 
-  public OnDiskBlobStoreCache(RemoteOptions options, Path cacheDir, DigestUtil digestUtil)
+  private record DigestAndInvocation(Digest digest, String invocationId) {}
+
+  private final RemoteWorkerOptions remoteWorkerOptions;
+  private final ConcurrentHashMap<DigestAndInvocation, Integer>
+      numberOfDownloadsPerDigestAndInvocation = new ConcurrentHashMap<>();
+
+  public OnDiskBlobStoreCache(
+      Path cacheDir, DigestUtil digestUtil, RemoteOptions remoteOptions, RemoteWorkerOptions remoteWorkerOptions)
       throws IOException {
     super(
         /* remoteCacheClient= */ null,
         new DiskCacheClient(cacheDir, digestUtil, executorService, /* verifyDownloads= */ true),
-        options,
+        remoteOptions,
         digestUtil);
+    this.remoteWorkerOptions = remoteWorkerOptions;
   }
 
   @Override
@@ -92,6 +102,30 @@ public void downloadTree(
     }
   }
 
+  @Override
+  public ListenableFuture<byte[]> downloadBlob(
+      RemoteActionExecutionContext context, Digest digest) {
+    if (remoteWorkerOptions.errorOnDuplicateDownloads) {
+      // Only populate numberOfDownloadsPerDigestAndInvocation when fakeErrorForDuplicatedDownloads
+      // is enabled to avoid unnecessary unbounded memory growth.
+      int numberOfDownloads =
+          numberOfDownloadsPerDigestAndInvocation.merge(
+              new DigestAndInvocation(digest, context.getRequestMetadata().getToolInvocationId()),
+              1,
+              Integer::sum);
+      if (numberOfDownloads > 1) {
+        return Futures.immediateFailedFuture(
+            new IOException(
+                String.format(
+                    "Duplicate download of blob digest %s for invocation id %s",
+                    DigestUtil.toString(digest),
+                    context.getRequestMetadata().getToolInvocationId())));
+      }
+    }
+
+    return super.downloadBlob(context, digest);
+  }
+
   public DigestUtil getDigestUtil() {
     return digestUtil;
   }

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java

@@ -321,7 +321,7 @@ public static void main(String[] args) throws Exception {
     Path casPath =
         remoteWorkerOptions.casPath != null ? fs.getPath(remoteWorkerOptions.casPath) : null;
     DigestUtil digestUtil = new DigestUtil(SyscallCache.NO_CACHE, fs.getDigestFunction());
-    OnDiskBlobStoreCache cache = new OnDiskBlobStoreCache(remoteOptions, casPath, digestUtil);
+    OnDiskBlobStoreCache cache = new OnDiskBlobStoreCache(casPath, digestUtil, remoteOptions, remoteWorkerOptions);
     ListeningScheduledExecutorService retryService =
         MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1));
     RemoteWorker worker = new RemoteWorker(fs, remoteWorkerOptions, cache, sandboxPath, digestUtil);

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java

@@ -198,6 +198,16 @@ public class RemoteWorkerOptions extends OptionsBase {
               + " testing only.")
   public boolean unavailable;
 
+  @Option(
+      name = "error_on_duplicate_downloads",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "If true, each individual digest is allowed to be downloaded at most once per tool"
+              + " invocation id. This is useful for testing only.")
+  public boolean errorOnDuplicateDownloads;
+
   private static final int MAX_JOBS = 16384;
 
   /**