Remote: Don't upload action result if declared outputs are not created.

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

@@ -58,6 +58,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -1123,24 +1124,52 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
+  private Single<UploadManifest> buildUploadManifestAsync(
+      RemoteAction action, SpawnResult spawnResult) {
+    return Single.fromCallable(
+        () -> {
+          ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
+          for (ActionInput outputFile : action.spawn.getOutputFiles()) {
+            Path outputPath = execRoot.getRelative(outputFile.getExecPath());
+            if (!outputPath.exists()) {
+              String output;
+              if (outputFile instanceof Artifact) {
+                output = ((Artifact) outputFile).prettyPrint();
+              } else {
+                output = outputFile.getExecPathString();
+              }
+              throw new IOException("Output " + output + " was not created.");
+            }
+            outputFiles.add(outputPath);
+          }
+
+          return UploadManifest.create(
+              remoteOptions,
+              digestUtil,
+              remotePathResolver,
+              action.actionKey,
+              action.action,
+              action.command,
+              outputFiles.build(),
+              action.spawnExecutionContext.getFileOutErr(),
+              /* exitCode= */ 0);
+        });
+  }
+
   @VisibleForTesting
   UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult)
-      throws ExecException, IOException {
-    Collection<Path> outputFiles =
-        action.spawn.getOutputFiles().stream()
-            .map((inp) -> execRoot.getRelative(inp.getExecPath()))
-            .collect(ImmutableList.toImmutableList());
-
-    return UploadManifest.create(
-        remoteOptions,
-        digestUtil,
-        remotePathResolver,
-        action.actionKey,
-        action.action,
-        action.command,
-        outputFiles,
-        action.spawnExecutionContext.getFileOutErr(),
-        /* exitCode= */ 0);
+      throws IOException, ExecException, InterruptedException {
+    try {
+      return buildUploadManifestAsync(action, spawnResult).blockingGet();
+    } catch (RuntimeException e) {
+      Throwable cause = e.getCause();
+      if (cause != null) {
+        Throwables.throwIfInstanceOf(cause, IOException.class);
+        Throwables.throwIfInstanceOf(cause, ExecException.class);
+        Throwables.throwIfInstanceOf(cause, InterruptedException.class);
+      }
+      throw e;
+    }
   }
 
   /** Upload outputs of a remote action which was executed locally to remote cache. */
@@ -1152,42 +1181,43 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
         SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0,
         "shouldn't upload outputs of failed local action");
 
-    try {
-      UploadManifest manifest = buildUploadManifest(action, spawnResult);
-      if (remoteOptions.remoteCacheAsync) {
-        Single.using(
-                remoteCache::retain,
-                remoteCache ->
-                    manifest.uploadAsync(
-                        action.getRemoteActionExecutionContext(), remoteCache, reporter),
-                RemoteCache::release)
-            .subscribeOn(scheduler)
-            .subscribe(
-                new SingleObserver<ActionResult>() {
-                  @Override
-                  public void onSubscribe(@NonNull Disposable d) {
-                    backgroundTaskPhaser.register();
-                  }
-
-                  @Override
-                  public void onSuccess(@NonNull ActionResult actionResult) {
-                    backgroundTaskPhaser.arriveAndDeregister();
-                  }
-
-                  @Override
-                  public void onError(@NonNull Throwable e) {
-                    backgroundTaskPhaser.arriveAndDeregister();
-                    reportUploadError(e);
-                  }
-                });
-      } else {
-        try (SilentCloseable c =
-            Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) {
-          manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter);
-        }
+    if (remoteOptions.remoteCacheAsync) {
+      Single.using(
+              remoteCache::retain,
+              remoteCache ->
+                  buildUploadManifestAsync(action, spawnResult)
+                      .flatMap(
+                          manifest ->
+                              manifest.uploadAsync(
+                                  action.getRemoteActionExecutionContext(), remoteCache, reporter)),
+              RemoteCache::release)
+          .subscribeOn(scheduler)
+          .subscribe(
+              new SingleObserver<ActionResult>() {
+                @Override
+                public void onSubscribe(@NonNull Disposable d) {
+                  backgroundTaskPhaser.register();
+                }
+
+                @Override
+                public void onSuccess(@NonNull ActionResult actionResult) {
+                  backgroundTaskPhaser.arriveAndDeregister();
+                }
+
+                @Override
+                public void onError(@NonNull Throwable e) {
+                  backgroundTaskPhaser.arriveAndDeregister();
+                  reportUploadError(e);
+                }
+              });
+    } else {
+      try (SilentCloseable c =
+          Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) {
+        UploadManifest manifest = buildUploadManifest(action, spawnResult);
+        manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter);
+      } catch (IOException e) {
+        reportUploadError(e);
       }
-    } catch (IOException e) {
-      reportUploadError(e);
     }
   }
 

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

@@ -350,14 +350,15 @@ ActionResult getActionResult() {
   /** Uploads outputs and action result (if exit code is 0) to remote cache. */
   public ActionResult upload(
       RemoteActionExecutionContext context, RemoteCache remoteCache, ExtendedEventHandler reporter)
-      throws IOException, InterruptedException {
+      throws IOException, InterruptedException, ExecException {
     try {
       return uploadAsync(context, remoteCache, reporter).blockingGet();
     } catch (RuntimeException e) {
       Throwable cause = e.getCause();
       if (cause != null) {
         throwIfInstanceOf(cause, InterruptedException.class);
         throwIfInstanceOf(cause, IOException.class);
+        throwIfInstanceOf(cause, ExecException.class);
       }
       throw e;
     }

src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

@@ -1367,6 +1367,27 @@ public void uploadOutputs_firesUploadEvents() throws Exception {
                 spawn.getResourceOwner(), "ac/" + action.getActionId()));
   }
 
+  @Test
+  public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception {
+    Path file = execRoot.getRelative("outputs/file");
+    Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file);
+    RemoteExecutionService service = newRemoteExecutionService();
+    Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputFile));
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteAction action = service.buildRemoteAction(spawn, context);
+    SpawnResult spawnResult =
+        new SpawnResult.Builder()
+            .setExitCode(0)
+            .setStatus(SpawnResult.Status.SUCCESS)
+            .setRunnerName("test")
+            .build();
+
+    service.uploadOutputs(action, spawnResult);
+
+    // assert
+    assertThat(cache.getNumFindMissingDigests()).isEmpty();
+  }
+
   @Test
   public void uploadInputsIfNotPresent_deduplicateFindMissingBlobCalls() throws Exception {
     int taskCount = 100;

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

@@ -3535,4 +3535,27 @@ EOF
   [[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files"
 }
 
+function test_missing_outputs_dont_upload_action_result() {
+  # Test that if declared outputs are not created, even the exit code of action
+  # is 0, we treat this as failed and don't upload action result.
+  # See https://github.com/bazelbuild/bazel/issues/14543.
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\"",
+)
+EOF
+
+  bazel build \
+      --remote_cache=grpc://localhost:${worker_port} \
+      //a:foo >& $TEST_log && fail "Should failed to build"
+
+  remote_cas_files="$(count_remote_cas_files)"
+  [[ "$remote_cas_files" == 0 ]] || fail "Expected 0 remote cas entries, not $remote_cas_files"
+  remote_ac_files="$(count_remote_ac_files)"
+  [[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files"
+}
+
 run_suite "Remote execution and remote cache tests"