[7.0.0] Fix crash when looking up tree metadata while prefetching the…

… output of an action template expansion. (#20004)

An action resulting from an action template expansion produces only one
of the files in a tree artifact. If, in addition, the file is an
in-memory output but its download was requested by the
--remote_download_* flags, it will be prefetched when finalizing the
action, at which point the prefetcher will look at the metadata for the
full tree to determine whether the file should be materialized as a
symlink into a different location. This results in a crash because the
full tree metadata isn't yet available.

As a fix, the prefetcher now tolerates the absence of tree metadata when
these conditions apply. This is still correct because an action template
expansion will never materialize its outputs as symlinks.

Fixes #19988.

Closes #20001.

Commit
8305a2c

PiperOrigin-RevId: 578176873
Change-Id: I64f765ca5f8852d86150ad866a5fc5930092d661

Co-authored-by: Tiago Quelhas <[email protected]>

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

@@ -333,15 +333,15 @@ private Single<TransferResult> prefetchFile(
         return Single.just(TransferResult.ok());
       }
 
-      @Nullable Symlink symlink = maybeGetSymlink(input, metadata, metadataSupplier);
+      @Nullable Symlink symlink = maybeGetSymlink(action, input, metadata, metadataSupplier);
 
       if (symlink != null) {
         checkState(execPath.startsWith(symlink.getLinkExecPath()));
         execPath =
             symlink.getTargetExecPath().getRelative(execPath.relativeTo(symlink.getLinkExecPath()));
       }
 
-      @Nullable PathFragment treeRootExecPath = maybeGetTreeRoot(input, metadataSupplier);
+      @Nullable PathFragment treeRootExecPath = maybeGetTreeRoot(action, input, metadataSupplier);
 
       Completable result =
           downloadFileNoCheckRx(
@@ -374,17 +374,25 @@ private Single<TransferResult> prefetchFile(
    * FileArtifactValue#getMaterializationExecPath()} field in their metadata.
    */
   @Nullable
-  private PathFragment maybeGetTreeRoot(ActionInput input, MetadataSupplier metadataSupplier)
+  private PathFragment maybeGetTreeRoot(
+      ActionExecutionMetadata action, ActionInput input, MetadataSupplier metadataSupplier)
       throws IOException, InterruptedException {
     if (!(input instanceof TreeFileArtifact)) {
       return null;
     }
-    SpecialArtifact treeArtifact = ((TreeFileArtifact) input).getParent();
-    FileArtifactValue treeMetadata =
-        checkNotNull(
-            metadataSupplier.getMetadata(treeArtifact),
-            "input %s belongs to a tree artifact whose metadata is missing",
-            input);
+    TreeFileArtifact treeFile = (TreeFileArtifact) input;
+    SpecialArtifact treeArtifact = treeFile.getParent();
+    FileArtifactValue treeMetadata = metadataSupplier.getMetadata(treeArtifact);
+    if (treeMetadata == null) {
+      if (!treeFile.isChildOfDeclaredDirectory() && action.getOutputs().contains(treeFile)) {
+        // If this file is produced by an action template, the full tree artifact metadata might
+        // not be available yet. However, we know with certainty that the file is not materialized
+        // as a symlink.
+        return null;
+      }
+      throw new IllegalStateException(
+          String.format("input %s belongs to a tree artifact whose metadata is missing", treeFile));
+    }
     return treeMetadata.getMaterializationExecPath().orElse(treeArtifact.getExecPath());
   }
 
@@ -399,17 +407,27 @@ private PathFragment maybeGetTreeRoot(ActionInput input, MetadataSupplier metada
    */
   @Nullable
   private Symlink maybeGetSymlink(
-      ActionInput input, FileArtifactValue metadata, MetadataSupplier metadataSupplier)
+      ActionExecutionMetadata action,
+      ActionInput input,
+      FileArtifactValue metadata,
+      MetadataSupplier metadataSupplier)
       throws IOException, InterruptedException {
     if (input instanceof TreeFileArtifact) {
-      // Check whether the entire tree artifact should be prefetched into a separate location.
-      SpecialArtifact treeArtifact = ((TreeFileArtifact) input).getParent();
-      FileArtifactValue treeMetadata =
-          checkNotNull(
-              metadataSupplier.getMetadata(treeArtifact),
-              "input %s belongs to a tree artifact whose metadata is missing",
-              input);
-      return maybeGetSymlink(treeArtifact, treeMetadata, metadataSupplier);
+      TreeFileArtifact treeFile = (TreeFileArtifact) input;
+      SpecialArtifact treeArtifact = treeFile.getParent();
+      FileArtifactValue treeMetadata = metadataSupplier.getMetadata(treeArtifact);
+      if (treeMetadata == null) {
+        if (!treeFile.isChildOfDeclaredDirectory() && action.getOutputs().contains(treeFile)) {
+          // If this file is produced by an action template, the full tree artifact metadata might
+          // not be available yet. However, we know with certainty that the file is not materialized
+          // as a symlink.
+          return null;
+        }
+        throw new IllegalStateException(
+            String.format(
+                "input %s belongs to a tree artifact whose metadata is missing", treeFile));
+      }
+      return maybeGetSymlink(action, treeArtifact, treeMetadata, metadataSupplier);
     }
     PathFragment execPath = input.getExecPath();
     PathFragment materializationExecPath = metadata.getMaterializationExecPath().orElse(execPath);

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

@@ -88,7 +88,26 @@ mytree = rule(implementation = _tree_impl)
 EOF
 }
 
-function test_cc_tree_remote_executor() {
+function test_cc_tree_remote_executor_download_all() {
+  # Regression test for https://github.com/bazelbuild/bazel/issues/19988.
+
+  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_all \
+      //a:tree_cc >& "$TEST_log" \
+      || fail "Failed to build //a:tree_cc with remote executor and full downloads"
+}
+
+function test_cc_tree_remote_executor_download_minimal() {
   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
@@ -105,7 +124,7 @@ function test_cc_tree_remote_executor() {
       || fail "Failed to build //a:tree_cc with remote executor and minimal downloads"
 }
 
-function test_cc_tree_remote_cache() {
+function test_cc_tree_remote_cache_download_minimal() {
   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
@@ -122,7 +141,7 @@ function test_cc_tree_remote_cache() {
       || fail "Failed to build //a:tree_cc with remote cache and minimal downloads"
 }
 
-function test_cc_tree_prefetching() {
+function test_cc_tree_prefetching_download_minimal() {
   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