Remote: Don't check declared outputs for failed action (#15181)

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 Co-authored-by: Chi Wang <[email protected]>
bazelbuild · Apr 5, 2022 · d418245 · d418245
1 parent df153df
commit d418245
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 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
@@ -1010,26 +1010,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
@@ -3579,4 +3579,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"