Repository: bazelbuild/bazel
Issue: Input prefetcher handles partial tree artifacts and nested artifacts incorrectly
Labels: type: bug
P2
team-Remote-Exec

It's possible for artifacts to nest (i.e., to have a declare_file or declare_directory that descends from another declare_directory). If two nested inputs are presented to the input prefetcher, the common files will be downloaded twice and race against each other, potentially corrupting the input tree.

https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java;l=289;drc=05b97393ff7604f237d9d31bd63714f27fbe83f2
https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java;l=230;drc=05b97393ff7604f237d9d31bd63714f27fbe83f2

Issue comments:
In addition, it's possible for an action to consume only one of the files in a tree artifact (see `SpawnActionTemplate`), in which case the prefetcher will try to fetch the entire tree artifact; this currently fails because the metadata for the entire tree artifact isn't present (see https://github.com/bazelbuild/bazel/issues/16987).
Reopening because https://github.com/bazelbuild/bazel/commit/25ba76c1b8d420ebae43c1f59890ae46153c2240 was just a workaround, and we have yet to fix it properly.
@tjgq Should this be cherry-picked?
Yes, but I'm not sure 6.1.0 is still accepting cherry-picks. If we decide to do an rc3, I'll cherry-pick it there. Otherwise, it will have to go into 6.2.0.
@bazel-io fork 6.2.0

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java
index 0435d17eb3c35b..fb2b07c9ae4cde 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java
@@ -28,6 +28,11 @@ public ListenableFuture<Void> prefetchFiles(
           // Do nothing.
           return immediateVoidFuture();
         }
+
+        @Override
+        public boolean supportsPartialTreeArtifactInputs() {
+          return true;
+        }
       };
 
   /**
@@ -39,4 +44,10 @@ public ListenableFuture<Void> prefetchFiles(
    */
   ListenableFuture<Void> prefetchFiles(
       Iterable<? extends ActionInput> inputs, MetadataProvider metadataProvider);
+
+  /**
+   * Whether the prefetcher is able to fetch individual files in a tree artifact without fetching
+   * the entire tree artifact.
+   */
+  boolean supportsPartialTreeArtifactInputs();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
index 5ae39e9172d9f1..d128e83a6f6bad 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
@@ -61,6 +61,12 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher {
     this.remoteCache = Preconditions.checkNotNull(remoteCache);
   }
 
+  @Override
+  public boolean supportsPartialTreeArtifactInputs() {
+    // This prefetcher is unable to fetch only individual files inside a tree artifact.
+    return false;
+  }
+
   @Override
   protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOException {
     if (!(input instanceof EmptyActionInput)) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 952e2dd573c641..4a116c0fc2b482 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -1253,7 +1253,8 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
           topLevelFilesets,
           input,
           value,
-          env);
+          env,
+          skyframeActionExecutor.supportsPartialTreeArtifactInputs());
     }
 
     if (actionExecutionFunctionExceptionHandler != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index f024131f12f511..412659253f3ea4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
 import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
@@ -47,7 +48,8 @@ static void addToMap(
       Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
       Artifact key,
       SkyValue value,
-      Environment env)
+      Environment env,
+      boolean prefetcherSupportsPartialTreeArtifacts)
       throws InterruptedException {
     addToMap(
         inputMap,
@@ -58,7 +60,8 @@ static void addToMap(
         key,
         value,
         env,
-        MetadataConsumerForMetrics.NO_OP);
+        MetadataConsumerForMetrics.NO_OP,
+        prefetcherSupportsPartialTreeArtifacts);
   }
 
   /**
@@ -74,7 +77,8 @@ static void addToMap(
       Artifact key,
       SkyValue value,
       Environment env,
-      MetadataConsumerForMetrics consumer)
+      MetadataConsumerForMetrics consumer,
+      boolean prefetcherSupportsPartialTreeArtifacts)
       throws InterruptedException {
     if (value instanceof RunfilesArtifactValue) {
       // Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that
@@ -120,16 +124,35 @@ static void addToMap(
           /*depOwner=*/ key);
       consumer.accumulate(treeArtifactValue);
     } else if (value instanceof ActionExecutionValue) {
-      FileArtifactValue metadata = ((ActionExecutionValue) value).getExistingFileArtifactValue(key);
-      inputMap.put(key, metadata, key);
-      if (key.isFileset()) {
-        ImmutableList<FilesetOutputSymlink> filesets = getFilesets(env, (SpecialArtifact) key);
-        if (filesets != null) {
-          topLevelFilesets.put(key, filesets);
-          consumer.accumulate(filesets);
-        }
+      if (!prefetcherSupportsPartialTreeArtifacts && key instanceof TreeFileArtifact) {
+        // If we're unable to prefetch individual files in a tree artifact, include the full tree
+        // artifact in the action inputs. This makes actions that consume partial tree artifacts
+        // (such as the ones generated by SpawnActionTemplate or CppCompileActionTemplate) less
+        // efficient, but is needed until https://github.com/bazelbuild/bazel/issues/16333 is fixed.
+        SpecialArtifact treeArtifact = key.getParent();
+        TreeArtifactValue treeArtifactValue =
+            ((ActionExecutionValue) value).getTreeArtifactValue(treeArtifact);
+        expandTreeArtifactAndPopulateArtifactData(
+            treeArtifact,
+            treeArtifactValue,
+            expandedArtifacts,
+            archivedTreeArtifacts,
+            inputMap,
+            /* depOwner= */ treeArtifact);
+        consumer.accumulate(treeArtifactValue);
       } else {
-        consumer.accumulate(metadata);
+        FileArtifactValue metadata =
+            ((ActionExecutionValue) value).getExistingFileArtifactValue(key);
+        inputMap.put(key, metadata, key);
+        if (key.isFileset()) {
+          ImmutableList<FilesetOutputSymlink> filesets = getFilesets(env, (SpecialArtifact) key);
+          if (filesets != null) {
+            topLevelFilesets.put(key, filesets);
+            consumer.accumulate(filesets);
+          }
+        } else {
+          consumer.accumulate(metadata);
+        }
       }
     } else {
       Preconditions.checkArgument(value instanceof FileArtifactValue, "Unexpected value %s", value);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index df7cb7534d80ce..1dfec25289ade8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -218,7 +218,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                 input,
                 artifactValue,
                 env,
-                currentConsumer);
+                currentConsumer,
+                skyframeActionExecutor.supportsPartialTreeArtifactInputs());
             if (!allArtifactsAreImportant && importantArtifactSet.contains(input)) {
               // Calling #addToMap a second time with `input` and `artifactValue` will perform no-op
               // updates to the secondary collections passed in (eg. expandedArtifacts,
@@ -233,7 +234,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                   input,
                   artifactValue,
                   env,
-                  MetadataConsumerForMetrics.NO_OP);
+                  MetadataConsumerForMetrics.NO_OP,
+                  skyframeActionExecutor.supportsPartialTreeArtifactInputs());
             }
           }
         }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 6329c467285eb3..342d97715e8944 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -320,6 +320,10 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) {
         .test(action.getMnemonic());
   }
 
+  boolean supportsPartialTreeArtifactInputs() {
+    return actionInputPrefetcher.supportsPartialTreeArtifactInputs();
+  }
+
   boolean publishTargetSummaries() {
     return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary;
   }


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
index c806bc3b5861a2..5f4bedca32fb94 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -879,6 +879,7 @@ public void testSharedActionsRacing() throws Exception {
     // is running. This way, both actions will check the action cache beforehand and try to update
     // the action cache post-build.
     final CountDownLatch inputsRequested = new CountDownLatch(2);
+    skyframeExecutor.configureActionExecutor(/* fileCache= */ null, ActionInputPrefetcher.NONE);
     skyframeExecutor
         .getEvaluator()
         .injectGraphTransformerForTesting(
@@ -1231,6 +1232,7 @@ public void sharedActionTemplate() throws Exception {
     ActionTemplate<DummyAction> template2 =
         new DummyActionTemplate(baseOutput, sharedOutput2, ActionOwner.SYSTEM_ACTION_OWNER);
     ActionLookupValue shared2Ct = createActionLookupValue(template2, shared2);
+    skyframeExecutor.configureActionExecutor(/* fileCache= */ null, ActionInputPrefetcher.NONE);
     // Inject the "configured targets" into the graph.
     skyframeExecutor
         .getDifferencerForTesting()
@@ -1645,6 +1647,7 @@ public void analysisEventsNotStoredInExecution() throws Exception {
             createActionLookupValue(action2, lc2),
             null,
             NestedSetBuilder.create(Order.STABLE_ORDER, Event.warn("analysis warning 2")));
+    skyframeExecutor.configureActionExecutor(/* fileCache= */ null, ActionInputPrefetcher.NONE);
     skyframeExecutor
         .getDifferencerForTesting()
         .inject(ImmutableMap.of(lc1, ctValue1, lc2, ctValue2));
diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
index 477a2f0bd8335f..3680f845c1846f 100755
--- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
+++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -70,14 +70,7 @@ else
   declare -r EXE_EXT=""
 fi
 
-function test_cc_tree() {
-  if [[ "$PLATFORM" == "darwin" ]]; then
-    # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
-    # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
-    # action executors in order to select the appropriate Xcode toolchain.
-    return 0
-  fi
-
+function setup_cc_tree() {
   mkdir -p a
   cat > a/BUILD <<EOF
 load(":tree.bzl", "mytree")
@@ -93,11 +86,40 @@ def _tree_impl(ctx):
 
 mytree = rule(implementation = _tree_impl)
 EOF
+}
+
+function test_cc_tree_remote_executor() {
+  if [[ "$PLATFORM" == "darwin" ]]; then
+    # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
+    # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
+    # action executors in order to select the appropriate Xcode toolchain.
+    return 0
+  fi
+
+  setup_cc_tree
+
   bazel build \
       --remote_executor=grpc://localhost:${worker_port} \
       --remote_download_minimal \
       //a:tree_cc >& "$TEST_log" \
-      || fail "Failed to build //a:tree_cc with minimal downloads"
+      || fail "Failed to build //a:tree_cc with remote executor and minimal downloads"
+}
+
+function test_cc_tree_remote_cache() {
+  if [[ "$PLATFORM" == "darwin" ]]; then
+    # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
+    # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
+    # action executors in order to select the appropriate Xcode toolchain.
+    return 0
+  fi
+
+  setup_cc_tree
+
+  bazel build \
+      --remote_cache=grpc://localhost:${worker_port} \
+      --remote_download_minimal \
+      //a:tree_cc >& "$TEST_log" \
+      || fail "Failed to build //a:tree_cc with remote cache and minimal downloads"
 }
 
 function test_cc_include_scanning_and_minimal_downloads() {