From 2b92a3111e83a4d14934059afd0f51161a41276f Mon Sep 17 00:00:00 2001
From: Chi Wang <chiwang@google.com>
Date: Fri, 1 Apr 2022 07:03:24 -0700
Subject: [PATCH] Remote: Don't check declared outputs for failed action

Fix a bug that Bazel print message

```
Invalid action cache entry ...: expected output ... does not exist.
```

instead of the underlying error message when remote action failed.

Closes #15151.

PiperOrigin-RevId: 438815494
---
 .../lib/remote/RemoteExecutionService.java    | 40 ++++++++++---------
 .../bazel/remote/remote_execution_test.sh     | 19 +++++++++
 2 files changed, 40 insertions(+), 19 deletions(-)

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 94f23185cfb189..d00110ad267e9e 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
@@ -1011,26 +1011,28 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       metadata = parseActionResultMetadata(action, result);
     }
 
-    // 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;
-        }
+    if (result.success()) {
+      // 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)
-            && !metadata.symlinks.containsKey(localPath)) {
-          throw new IOException(
-              "Invalid action cache entry "
-                  + action.actionKey.getDigest().getHash()
-                  + ": expected output "
-                  + prettyPrint(output)
-                  + " does not exist.");
+          Path localPath = execRoot.getRelative(output.getExecPath());
+          if (!metadata.files.containsKey(localPath)
+              && !metadata.directories.containsKey(localPath)
+              && !metadata.symlinks.containsKey(localPath)) {
+            throw new IOException(
+                "Invalid action cache entry "
+                    + action.actionKey.getDigest().getHash()
+                    + ": expected output "
+                    + prettyPrint(output)
+                    + " does not exist.");
+          }
         }
       }
     }
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index d2c0c9918b8b3d..ae03c48cfcc6ee 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3649,4 +3649,23 @@ EOF
   [[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files"
 }
 
+function test_failed_action_dont_check_declared_outputs() {
+  # Test that if action failed, outputs are not checked
+
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "exit 1",
+)
+EOF
+
+  bazel build \
+      --remote_executor=grpc://localhost:${worker_port} \
+      //a:foo >& $TEST_log && fail "Should failed to build"
+
+  expect_log "Executing genrule .* failed: (Exit 1):"
+}
+
 run_suite "Remote execution and remote cache tests"