remote: don't upload failed action outputs. Fixes bazelbuild#7232

For historical reasons we are currently uploading the outputs
of failed actionsi (exit code != 0). This was mostly so that
test.log and test.xml of failed tests would be uploaded when
BES was enabled. However, we have had local file uploads in
BES for a while now and thus this (unexpected) feature is now
obsolete.

PTAL @benjaminp

Closes bazelbuild#7243.

PiperOrigin-RevId: 231201641

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

@@ -93,20 +93,18 @@ public AbstractRemoteActionCache(RemoteOptions options, DigestUtil digestUtil, R
       throws IOException, InterruptedException;
 
   /**
-   * Upload the result of a locally executed action to the cache by uploading any necessary files,
-   * stdin / stdout, as well as adding an entry for the given action key to the cache if
-   * uploadAction is true.
+   * Upload the result of a locally executed action to the remote cache.
    *
-   * @throws IOException if the remote cache is unavailable.
+   * @throws IOException if there was an error uploading to the remote cache
+   * @throws ExecException if uploading any of the action outputs is not supported
    */
   abstract void upload(
       DigestUtil.ActionKey actionKey,
       Action action,
       Command command,
       Path execRoot,
       Collection<Path> files,
-      FileOutErr outErr,
-      boolean uploadAction)
+      FileOutErr outErr)
       throws ExecException, IOException, InterruptedException;
 
   /**

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

@@ -308,14 +308,10 @@ public void upload(
       Command command,
       Path execRoot,
       Collection<Path> files,
-      FileOutErr outErr,
-      boolean uploadAction)
+      FileOutErr outErr)
       throws ExecException, IOException, InterruptedException {
     ActionResult.Builder result = ActionResult.newBuilder();
-    upload(execRoot, actionKey, action, command, files, outErr, uploadAction, result);
-    if (!uploadAction) {
-      return;
-    }
+    upload(execRoot, actionKey, action, command, files, outErr, result);
     try {
       retrier.execute(
           () ->
@@ -338,7 +334,6 @@ void upload(
       Command command,
       Collection<Path> files,
       FileOutErr outErr,
-      boolean uploadAction,
       ActionResult.Builder result)
       throws ExecException, IOException, InterruptedException {
     UploadManifest manifest =
@@ -349,9 +344,7 @@ void upload(
             options.incompatibleRemoteSymlinks,
             options.allowSymlinkUpload);
     manifest.addFiles(files);
-    if (uploadAction) {
-      manifest.addAction(actionKey, action, command);
-    }
+    manifest.addAction(actionKey, action, command);
 
     List<Chunker> filesToUpload = new ArrayList<>();
 

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

@@ -189,8 +189,12 @@ public boolean willStore() {
         }
 
         @Override
-        public void store(SpawnResult result)
-            throws ExecException, InterruptedException, IOException {
+        public void store(SpawnResult result) throws ExecException, InterruptedException {
+          boolean uploadResults = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+          if (!uploadResults) {
+            return;
+          }
+
           if (options.experimentalGuardAgainstConcurrentChanges) {
             try (SilentCloseable c =
                 Profiler.instance().profile("RemoteCache.checkForConcurrentModifications")) {
@@ -200,13 +204,13 @@ public void store(SpawnResult result)
               return;
             }
           }
-          boolean uploadAction = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+
           Context previous = withMetadata.attach();
           Collection<Path> files =
               RemoteSpawnRunner.resolveActionInputs(execRoot, spawn.getOutputFiles());
           try (SilentCloseable c = Profiler.instance().profile("RemoteCache.upload")) {
             remoteCache.upload(
-                actionKey, action, command, execRoot, files, context.getFileOutErr(), uploadAction);
+                actionKey, action, command, execRoot, files, context.getFileOutErr());
           } catch (IOException e) {
             String errorMsg = e.getMessage();
             if (isNullOrEmpty(errorMsg)) {

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

@@ -528,20 +528,23 @@ SpawnResult execLocallyAndUpload(
     Map<Path, Long> ctimesBefore = getInputCtimes(inputMap);
     SpawnResult result = execLocally(spawn, context);
     Map<Path, Long> ctimesAfter = getInputCtimes(inputMap);
+    uploadLocalResults =
+        uploadLocalResults && Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+    if (!uploadLocalResults) {
+      return result;
+    }
+
     for (Map.Entry<Path, Long> e : ctimesBefore.entrySet()) {
       // Skip uploading to remote cache, because an input was modified during execution.
       if (!ctimesAfter.get(e.getKey()).equals(e.getValue())) {
         return result;
       }
     }
-    if (!uploadLocalResults) {
-      return result;
-    }
-    boolean uploadAction = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+
     Collection<Path> outputFiles = resolveActionInputs(execRoot, spawn.getOutputFiles());
     try (SilentCloseable c = Profiler.instance().profile("Remote.upload")) {
       remoteCache.upload(
-          actionKey, action, command, execRoot, outputFiles, context.getFileOutErr(), uploadAction);
+          actionKey, action, command, execRoot, outputFiles, context.getFileOutErr());
     } catch (IOException e) {
       if (verboseFailures) {
         report(Event.debug("Upload to remote cache failed: " + e.getMessage()));

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

@@ -96,11 +96,10 @@ public void upload(
       Command command,
       Path execRoot,
       Collection<Path> files,
-      FileOutErr outErr,
-      boolean uploadAction)
+      FileOutErr outErr)
       throws ExecException, IOException, InterruptedException {
     ActionResult.Builder result = ActionResult.newBuilder();
-    upload(result, actionKey, action, command, execRoot, files, uploadAction);
+    upload(result, actionKey, action, command, execRoot, files, /* uploadAction= */ true);
     if (outErr.getErrorPath().exists()) {
       Digest stderr = uploadFileContents(outErr.getErrorPath());
       result.setStderrDigest(stderr);
@@ -109,9 +108,7 @@ public void upload(
       Digest stdout = uploadFileContents(outErr.getOutputPath());
       result.setStdoutDigest(stdout);
     }
-    if (uploadAction) {
-      blobStore.putActionResult(actionKey.getDigest().getHash(), result.build().toByteArray());
-    }
+    blobStore.putActionResult(actionKey.getDigest().getHash(), result.build().toByteArray());
   }
 
   public void upload(

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

@@ -732,8 +732,7 @@ void upload(
         Command command,
         Path execRoot,
         Collection<Path> files,
-        FileOutErr outErr,
-        boolean uploadAction)
+        FileOutErr outErr)
         throws ExecException, IOException, InterruptedException {
       throw new UnsupportedOperationException();
     }

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

@@ -81,6 +81,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
+import java.util.List;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -655,21 +656,18 @@ public void testUploadDirectory() throws Exception {
           public void findMissingBlobs(
               FindMissingBlobsRequest request,
               StreamObserver<FindMissingBlobsResponse> responseObserver) {
-            assertThat(request.getBlobDigestsList())
-                .containsExactly(fooDigest, quxDigest, barDigest);
+            assertThat(request.getBlobDigestsList()).containsAllOf(fooDigest, quxDigest, barDigest);
             // Nothing is missing.
             responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance());
             responseObserver.onCompleted();
           }
         });
 
-    ActionResult.Builder result = ActionResult.newBuilder();
-    client.upload(
-        execRoot, null, null, null, ImmutableList.<Path>of(fooFile, barDir), outErr, false, result);
+    ActionResult result = uploadDirectory(client, ImmutableList.<Path>of(fooFile, barDir));
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
     expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
-    assertThat(result.build()).isEqualTo(expectedResult.build());
+    assertThat(result).isEqualTo(expectedResult.build());
   }
 
   @Test
@@ -686,19 +684,17 @@ public void testUploadDirectoryEmpty() throws Exception {
           public void findMissingBlobs(
               FindMissingBlobsRequest request,
               StreamObserver<FindMissingBlobsResponse> responseObserver) {
-            assertThat(request.getBlobDigestsList()).containsExactly(barDigest);
+            assertThat(request.getBlobDigestsList()).contains(barDigest);
             // Nothing is missing.
             responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance());
             responseObserver.onCompleted();
           }
         });
 
-    ActionResult.Builder result = ActionResult.newBuilder();
-    client.upload(
-        execRoot, null, null, null, ImmutableList.<Path>of(barDir), outErr, false, result);
+    ActionResult result = uploadDirectory(client, ImmutableList.<Path>of(barDir));
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
-    assertThat(result.build()).isEqualTo(expectedResult.build());
+    assertThat(result).isEqualTo(expectedResult.build());
   }
 
   @Test
@@ -738,19 +734,27 @@ public void findMissingBlobs(
               FindMissingBlobsRequest request,
               StreamObserver<FindMissingBlobsResponse> responseObserver) {
             assertThat(request.getBlobDigestsList())
-                .containsExactly(quxDigest, barDigest, wobbleDigest);
+                .containsAllOf(quxDigest, barDigest, wobbleDigest);
             // Nothing is missing.
             responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance());
             responseObserver.onCompleted();
           }
         });
 
-    ActionResult.Builder result = ActionResult.newBuilder();
-    client.upload(
-        execRoot, null, null, null, ImmutableList.<Path>of(barDir), outErr, false, result);
+    ActionResult result = uploadDirectory(client, ImmutableList.of(barDir));
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
-    assertThat(result.build()).isEqualTo(expectedResult.build());
+    assertThat(result).isEqualTo(expectedResult.build());
+  }
+
+  private ActionResult uploadDirectory(GrpcRemoteCache client, List<Path> outputs)
+      throws Exception {
+    ActionResult.Builder result = ActionResult.newBuilder();
+    Action action = Action.getDefaultInstance();
+    ActionKey actionKey = DIGEST_UTIL.computeActionKey(action);
+    Command cmd = Command.getDefaultInstance();
+    client.upload(execRoot, actionKey, action, cmd, outputs, outErr, result);
+    return result.build();
   }
 
   @Test
@@ -789,7 +793,6 @@ public void findMissingBlobs(
         command,
         ImmutableList.<Path>of(fooFile, barFile),
         outErr,
-        true,
         result);
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
@@ -840,7 +843,6 @@ public void findMissingBlobs(
         command,
         ImmutableList.<Path>of(fooFile, barFile),
         outErr,
-        true,
         result);
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
@@ -853,47 +855,6 @@ public void findMissingBlobs(
     assertThat(numGetMissingCalls.get()).isEqualTo(4);
   }
 
-  @Test
-  public void testUploadUploadsOnlyOutputs() throws Exception {
-    final GrpcRemoteCache client = newClient();
-    final Digest fooDigest =
-        fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz");
-    final Digest barDigest =
-        fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar"), "x");
-    serviceRegistry.addService(
-        new ContentAddressableStorageImplBase() {
-          @Override
-          public void findMissingBlobs(
-              FindMissingBlobsRequest request,
-              StreamObserver<FindMissingBlobsResponse> responseObserver) {
-            // This checks we will try to upload the actual outputs.
-            assertThat(request.getBlobDigestsList()).containsExactly(fooDigest, barDigest);
-            responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance());
-            responseObserver.onCompleted();
-          }
-        });
-    serviceRegistry.addService(
-        new ActionCacheImplBase() {
-          @Override
-          public void updateActionResult(
-              UpdateActionResultRequest request, StreamObserver<ActionResult> responseObserver) {
-            fail("Update action result was expected to not be called.");
-          }
-        });
-
-    ActionKey emptyKey = DIGEST_UTIL.computeActionKey(Action.getDefaultInstance());
-    Path fooFile = execRoot.getRelative("a/foo");
-    Path barFile = execRoot.getRelative("bar");
-    client.upload(
-        emptyKey,
-        Action.getDefaultInstance(),
-        Command.getDefaultInstance(),
-        execRoot,
-        ImmutableList.<Path>of(fooFile, barFile),
-        outErr,
-        false);
-  }
-
   @Test
   public void testUploadCacheMissesWithRetries() throws Exception {
     final GrpcRemoteCache client = newClient();
@@ -1014,8 +975,7 @@ public void onError(Throwable t) {
         Command.getDefaultInstance(),
         execRoot,
         ImmutableList.<Path>of(fooFile, barFile, bazFile),
-        outErr,
-        true);
+        outErr);
     // 4 times for the errors, 3 times for the successful uploads.
     Mockito.verify(mockByteStreamImpl, Mockito.times(7))
         .write(Mockito.<StreamObserver<WriteResponse>>anyObject());