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

Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry.

Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread.

Fixes bazelbuild#14543.

Closes bazelbuild#15016.

PiperOrigin-RevId: 434448255
(cherry picked from commit 5b54588)

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;
@@ -1120,24 +1121,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("Expected output " + output + " was not created locally.");
+            }
+            outputFiles.add(outputPath);
+          }
+
+          return UploadManifest.create(
+              remoteOptions,
+              digestUtil,
+              remotePathResolver,
+              action.actionKey,
+              action.action,
+              action.command,
+              outputFiles.build(),
+              action.spawnExecutionContext.getFileOutErr(),
+              spawnResult.exitCode());
+        });
+  }
+
   @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. */
@@ -1149,42 +1178,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

@@ -1365,6 +1365,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

@@ -3499,4 +3499,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",
+)
+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"