From f5857830bb68bd05ffc257506575ed37a8128933 Mon Sep 17 00:00:00 2001
From: Chi Wang <chiwang@google.com>
Date: Mon, 21 Mar 2022 03:50:55 -0700
Subject: [PATCH] Remote: Don't check TreeArtifact output

With a151116f7f671efea9a95f3e251561e53e535cca, we ignore action cache entry when checking the remote cache if a mandatory output of the Spawn is missing.

However, this breaks Spawns that declare a directory output but don't generate anything under it. Since remote servers typically don't create directory ahead of time, and if the action itself doesn't create files under the directory, that directory will be eliminated from the action cache entry.

This PR fixes that by not checking TreeArtifact output.

Closes #15077.

PiperOrigin-RevId: 436163970
---
 .../lib/remote/RemoteExecutionService.java    |  7 +++
 .../bazel/remote/remote_execution_test.sh     | 57 +++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index b6ccc3adc071d4..94f23185cfb189 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -1014,6 +1014,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     // Check that all mandatory outputs are created.
     for (ActionInput output : action.spawn.getOutputFiles()) {
       if (action.spawn.isMandatoryOutput(output)) {
+        // Don't check output that is tree artifact since spawn could generate nothing under that
+        // directory. Remote server typically doesn't create directory ahead of time resulting in
+        // empty tree artifact missing from action cache entry.
+        if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
+          continue;
+        }
+
         Path localPath = execRoot.getRelative(output.getExecPath());
         if (!metadata.files.containsKey(localPath)
             && !metadata.directories.containsKey(localPath)
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index e8088b1552fb2c..a1afb968a274da 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1001,6 +1001,63 @@ EOF
            || fail "Failed to run //a:starlark_output_dir_test with remote execution"
 }
 
+function generate_empty_treeartifact_build() {
+  mkdir -p a
+  cat > a/BUILD <<'EOF'
+load(":output_dir.bzl", "gen_output_dir")
+gen_output_dir(
+    name = "output-dir",
+    outdir = "dir",
+)
+EOF
+  cat > a/output_dir.bzl <<'EOF'
+def _gen_output_dir_impl(ctx):
+    output_dir = ctx.actions.declare_directory(ctx.attr.outdir)
+    ctx.actions.run_shell(
+        outputs = [output_dir],
+        inputs = [],
+        command = "",
+        arguments = [output_dir.path],
+    )
+    return [
+        DefaultInfo(files = depset(direct = [output_dir])),
+    ]
+
+gen_output_dir = rule(
+    implementation = _gen_output_dir_impl,
+    attrs = {
+        "outdir": attr.string(mandatory = True),
+    },
+)
+EOF
+}
+
+function test_empty_treeartifact_works_with_remote_execution() {
+  # Test that empty tree artifact works with remote execution
+  generate_empty_treeartifact_build
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    //a:output-dir >& $TEST_log || fail "Failed to build"
+}
+
+function test_empty_treeartifact_works_with_remote_cache() {
+  # Test that empty tree artifact works with remote cache
+  generate_empty_treeartifact_build
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //a:output-dir >& $TEST_log || fail "Failed to build"
+
+  bazel clean
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //a:output-dir >& $TEST_log || fail "Failed to build"
+
+  expect_log "remote cache hit"
+}
+
 function test_downloads_minimal() {
   # Test that genrule outputs are not downloaded when using
   # --remote_download_minimal