Repository: bazelbuild/bazel
Issue: remote: don't upload failed action outputs
Labels: type: bug
P2
team-Remote-Exec

For historical reasons we are currently uploading the outputs of failed actions. 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. 

Issue comments:
cc @benjaminp 

Pull request review comments:
The javadoc needs a update.
In the `RemoteSpawnCache`, the equivalent condition was moved before the concurrent modification check. It probably doesn't matter, but it doesn't hurt to be consistent either way.
Is it worthwhile to keep the `containsExactly` by adding the `actionKey` to the list here?
I'm wondering if there should be a helper for the above three lines. They appear a lot.
done
done
I'd argue no, as this test checks that directory uploads work. it's a protocol detail that the action and command are also stored in the CAS that should be verified elsewhere.
yep. done
done

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
index 6b0b96c1b7eba8..0d742f5b198910 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
@@ -93,11 +93,10 @@ 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,
@@ -105,8 +104,7 @@ abstract void upload(
       Command command,
       Path execRoot,
       Collection<Path> files,
-      FileOutErr outErr,
-      boolean uploadAction)
+      FileOutErr outErr)
       throws ExecException, IOException, InterruptedException;
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
index 30dc103f23d04e..1e1ae401fb3cb9 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -297,20 +297,16 @@ public void onCompleted() {
   }
 
   @Override
-  public void upload(
+  public void
+  upload(
       ActionKey actionKey,
       Action action,
       Command command,
       Path execRoot,
       Collection<Path> files,
-      FileOutErr outErr,
-      boolean uploadAction)
-      throws ExecException, IOException, InterruptedException {
+      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(
           () ->
@@ -333,7 +329,6 @@ void upload(
       Command command,
       Collection<Path> files,
       FileOutErr outErr,
-      boolean uploadAction,
       ActionResult.Builder result)
       throws ExecException, IOException, InterruptedException {
     UploadManifest manifest =
@@ -344,9 +339,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<>();
 
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index 66a0b23ab41fc7..77fd0cb71e7abf 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -190,7 +190,12 @@ public boolean willStore() {
 
         @Override
         public void store(SpawnResult result)
-            throws ExecException, InterruptedException, IOException {
+            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 +205,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)) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 55d0cdd3a4635a..14ea4b2ae1ff00 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/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()));
diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
index 6610e4b2d716cd..bb0e7d58b0e06b 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
@@ -95,11 +95,9 @@ public void upload(
       Command command,
       Path execRoot,
       Collection<Path> files,
-      FileOutErr outErr,
-      boolean uploadAction)
-      throws ExecException, IOException, InterruptedException {
+      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);
@@ -108,9 +106,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(
diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
index b9450244ccd976..b35e447fe59636 100644
--- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
+++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
@@ -329,7 +329,7 @@ private ActionResult execute(Digest actionDigest, Path execRoot)
     ActionResult.Builder result = ActionResult.newBuilder();
     boolean setResult = exitCode == 0 && !action.getDoNotCache();
     try {
-      cache.upload(result, actionKey, action, command, execRoot, outputs, setResult);
+        cache.upload(result, actionKey, action, command, execRoot, outputs, setResult);
     } catch (ExecException e) {
       if (errStatus == null) {
         errStatus =


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
index da5e8218dca333..f4f6026a3efec6 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
+++ b/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();
     }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
index 92894f8928cebf..b047842f024211 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
+++ b/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;
@@ -656,20 +657,18 @@ public void findMissingBlobs(
               FindMissingBlobsRequest request,
               StreamObserver<FindMissingBlobsResponse> responseObserver) {
             assertThat(request.getBlobDigestsList())
-                .containsExactly(fooDigest, quxDigest, barDigest);
+                .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 +685,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 +735,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.newBuilder().build();
+    ActionKey actionKey = DIGEST_UTIL.computeActionKey(action);
+    Command cmd = Command.newBuilder().build();
+    client.upload(execRoot, actionKey, action, cmd, outputs, outErr, result);
+    return result.build();
   }
 
   @Test
@@ -789,7 +794,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 +844,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 +856,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 +976,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());
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index f0005a8bbe7e7a..c3defa1dd37344 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -239,8 +239,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             any(Collection.class),
-            any(FileOutErr.class),
-            any(Boolean.class));
+            any(FileOutErr.class));
     assertThat(result.setupSuccess()).isTrue();
     assertThat(result.exitCode()).isEqualTo(0);
     assertThat(result.isCacheHit()).isTrue();
@@ -279,8 +278,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
     entry.store(result);
     verify(remoteCache)
         .upload(
@@ -289,8 +287,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
     assertThat(progressUpdates)
         .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
   }
@@ -326,9 +323,8 @@ public void noCacheSpawns() throws Exception {
   }
 
   @Test
-  public void noCacheSpawnsNoResultStore() throws Exception {
-    // Only successful action results are uploaded to the remote cache. The artifacts, however,
-    // are uploaded regardless.
+  public void failedActionsAreNotUploaded() throws Exception {
+    // Only successful action results are uploaded to the remote cache.
     CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy);
     verify(remoteCache).getCachedActionResult(any(ActionKey.class));
     assertThat(entry.hasResult()).isFalse();
@@ -340,15 +336,14 @@ public void noCacheSpawnsNoResultStore() throws Exception {
             .build();
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
     entry.store(result);
-    verify(remoteCache)
+    verify(remoteCache, never())
         .upload(
             any(ActionKey.class),
             any(Action.class),
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(false));
+            eq(outErr));
     assertThat(progressUpdates)
         .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
   }
@@ -373,8 +368,7 @@ public void printWarningIfUploadFails() throws Exception {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
 
     entry.store(result);
     verify(remoteCache)
@@ -384,8 +378,7 @@ public void printWarningIfUploadFails() throws Exception {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
 
     assertThat(eventHandler.getEvents()).hasSize(1);
     Event evt = eventHandler.getEvents().get(0);
@@ -428,8 +421,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
     entry.store(result);
     verify(remoteCache)
         .upload(
@@ -438,8 +430,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
 
     assertThat(eventHandler.getEvents()).hasSize(1);
     Event evt = eventHandler.getEvents().get(0);
@@ -498,8 +489,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
     entry.store(result);
     verify(remoteCache)
         .upload(
@@ -508,8 +498,7 @@ public Void answer(InvocationOnMock invocation) {
             any(Command.class),
             any(Path.class),
             eq(outputFiles),
-            eq(outErr),
-            eq(true));
+            eq(outErr));
     assertThat(progressUpdates)
         .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
     assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed.
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index 93df866fd00026..6cfc635cd16471 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -220,8 +220,7 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception {
             any(Command.class),
             any(Path.class),
             any(Collection.class),
-            any(FileOutErr.class),
-            any(Boolean.class));
+            any(FileOutErr.class));
     verifyZeroInteractions(localRunner);
   }
 
@@ -276,9 +275,8 @@ public void nonCachableSpawnsShouldNotBeCached_local() throws Exception {
 
   @Test
   @SuppressWarnings("unchecked")
-  public void failedActionShouldOnlyUploadOutputs() throws Exception {
-    // Test that the outputs of a failed locally executed action are uploaded to a remote cache,
-    // but the action result itself is not.
+  public void failedLocalActionShouldNotBeUploaded() throws Exception {
+    // Test that the outputs of a locally executed action that failed are not uploaded.
 
     options.remoteUploadLocalResults = true;
 
@@ -319,16 +317,15 @@ public void failedActionShouldOnlyUploadOutputs() throws Exception {
             any(ActionKey.class),
             any(Action.class),
             any(Command.class),
-            eq(true));
-    verify(cache)
+            /* uploadLocalResults= */ eq(true));
+    verify(cache, never())
         .upload(
             any(ActionKey.class),
             any(Action.class),
             any(Command.class),
             any(Path.class),
             any(Collection.class),
-            any(FileOutErr.class),
-            eq(false));
+            any(FileOutErr.class));
   }
 
   @Test
@@ -405,8 +402,7 @@ public void printWarningIfCacheIsDown() throws Exception {
             any(Command.class),
             any(Path.class),
             any(Collection.class),
-            any(FileOutErr.class),
-            eq(true));
+            any(FileOutErr.class));
 
     SpawnResult res =
         new SpawnResult.Builder()
diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
index d63cea5ff60503..3d75d1d467ac7c 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
@@ -35,7 +35,9 @@
 import com.google.devtools.build.lib.remote.Retrier.Backoff;
 import com.google.devtools.build.lib.remote.blobstore.ConcurrentMapBlobStore;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
+import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -44,6 +46,7 @@
 import com.google.devtools.common.options.Options;
 import io.grpc.Context;
 import java.io.IOException;
+import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.Executors;
@@ -373,7 +376,7 @@ public void testUploadDirectory() throws Exception {
         cmd,
         execRoot,
         ImmutableList.<Path>of(fooFile, barDir),
-        true);
+        /* uploadAction= */ true);
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
     expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
@@ -399,13 +402,12 @@ public void testUploadDirectoryEmpty() throws Exception {
     final ConcurrentMap<String, byte[]> map = new ConcurrentHashMap<>();
     final SimpleBlobStoreActionCache client = newClient(map);
 
-    ActionResult.Builder result = ActionResult.newBuilder();
-    client.upload(result, null, null, null, execRoot, ImmutableList.<Path>of(barDir), false);
+    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());
 
-    assertThat(map.keySet()).containsExactly(barDigest.getHash());
+    assertThat(map.keySet()).contains(barDigest.getHash());
   }
 
   @Test
@@ -442,14 +444,23 @@ public void testUploadDirectoryNested() throws Exception {
     quxFile.setExecutable(true);
     final Path barDir = execRoot.getRelative("bar");
 
-    ActionResult.Builder result = ActionResult.newBuilder();
-    client.upload(result, null, null, null, execRoot, ImmutableList.<Path>of(barDir), false);
+    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());
 
     assertThat(map.keySet())
-        .containsExactly(wobbleDigest.getHash(), quxDigest.getHash(), barDigest.getHash());
+        .containsAllOf(wobbleDigest.getHash(), quxDigest.getHash(), barDigest.getHash());
+  }
+
+  private ActionResult uploadDirectory(SimpleBlobStoreActionCache client, List<Path> outputs)
+      throws Exception {
+    ActionResult.Builder result = ActionResult.newBuilder();
+    Action action = Action.newBuilder().build();
+    ActionKey actionKey = DIGEST_UTIL.computeActionKey(action);
+    Command cmd = Command.newBuilder().build();
+    client.upload(result, actionKey, action, cmd, execRoot, outputs, /* uploadAction= */ true);
+    return result.build();
   }
 
   @Test
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 02570d95de4448..c49d1d54d20087 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -248,58 +248,20 @@ function is_file_uploaded() {
   if [ -e "$cas_path/${h:0:64}" ]; then return 0; else return 1; fi
 }
 
-function test_failing_cc_test_grpc_cache() {
+function test_failed_test_outputs_not_uploaded() {
+  # Test that outputs of a failed test/action are not uploaded to the remote
+  # cache. This is a regression test for https://github.com/bazelbuild/bazel/issues/7232
   mkdir -p a
   cat > a/BUILD <<EOF
 package(default_visibility = ["//visibility:public"])
 cc_test(
-name = 'test',
-srcs = [ 'test.cc' ],
+  name = 'test',
+  srcs = [ 'test.cc' ],
 )
 EOF
   cat > a/test.cc <<EOF
 #include <iostream>
 int main() { std::cout << "Fail me!" << std::endl; return 1; }
-EOF
-  bazel test \
-      --remote_cache=localhost:${worker_port} \
-      --test_output=errors \
-      //a:test >& $TEST_log \
-      && fail "Expected test failure" || true
-   $(is_file_uploaded bazel-testlogs/a/test/test.log) \
-     || fail "Expected test log to be uploaded to remote execution"
-   $(is_file_uploaded bazel-testlogs/a/test/test.xml) \
-     || fail "Expected test xml to be uploaded to remote execution"
-}
-
-function test_failing_cc_test_remote_spawn_cache() {
-  mkdir -p a
-  cat > a/BUILD <<EOF
-package(default_visibility = ["//visibility:public"])
-cc_test(
-name = 'test',
-srcs = [ 'test.cc' ],
-)
-EOF
-  cat > a/test.cc <<EOF
-#include <iostream>
-int main() { std::cout << "Fail me!" << std::endl; return 1; }
-EOF
-  bazel test \
-      --remote_cache=localhost:${worker_port} \
-      --test_output=errors \
-      //a:test >& $TEST_log \
-      && fail "Expected test failure" || true
-   $(is_file_uploaded bazel-testlogs/a/test/test.log) \
-     || fail "Expected test log to be uploaded to remote execution"
-   $(is_file_uploaded bazel-testlogs/a/test/test.xml) \
-     || fail "Expected test xml to be uploaded to remote execution"
-  # Check that logs are uploaded regardless of the spawn being cacheable.
-  # Re-running a changed test that failed once renders the test spawn uncacheable.
-  rm -f a/test.cc
-  cat > a/test.cc <<EOF
-#include <iostream>
-int main() { std::cout << "Fail me again!" << std::endl; return 1; }
 EOF
   bazel test \
       --remote_cache=localhost:${worker_port} \