Uploading failed action outputs to the remote cache, because even if …

…the tests fails, we still want to be able to download the logs and other outputs from CAS.

This fixes a bug introduced by 562fcf9. To reproduce: run a failing test vs a BES service, the test log would not be uploaded.

TESTED=unit tests
PiperOrigin-RevId: 169143428

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

@@ -281,10 +281,18 @@ private void readBlob(Digest digest, OutputStream stream)
   }
 
   @Override
-  public void upload(ActionKey actionKey, Path execRoot, Collection<Path> files, FileOutErr outErr)
+  public void upload(
+      ActionKey actionKey,
+      Path execRoot,
+      Collection<Path> files,
+      FileOutErr outErr,
+      boolean uploadAction)
       throws IOException, InterruptedException {
     ActionResult.Builder result = ActionResult.newBuilder();
     upload(execRoot, files, outErr, result);
+    if (!uploadAction) {
+      return;
+    }
     try {
       retrier.execute(
           () ->

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

@@ -75,9 +75,15 @@ void download(ActionResult result, Path execRoot, FileOutErr outErr)
 
   /**
    * 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.
+   * stdin / stdout, as well as adding an entry for the given action key to the cache if
+   * uploadAction is true.
    */
-  void upload(ActionKey actionKey, Path execRoot, Collection<Path> files, FileOutErr outErr)
+  void upload(
+      ActionKey actionKey,
+      Path execRoot,
+      Collection<Path> files,
+      FileOutErr outErr,
+      boolean uploadAction)
       throws IOException, InterruptedException;
 
   /** Release resources associated with the cache. The cache may not be used after calling this. */

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

@@ -128,11 +128,9 @@ public boolean willStore() {
         @Override
         public void store(SpawnResult result, Collection<Path> files)
             throws InterruptedException, IOException {
-          if (result.status() != Status.SUCCESS || result.exitCode() != 0) {
-            return;
-          }
           try {
-            remoteCache.upload(actionKey, execRoot, files, policy.getFileOutErr());
+            boolean uploadAction = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+            remoteCache.upload(actionKey, execRoot, files, policy.getFileOutErr(), uploadAction);
           } catch (IOException e) {
             if (verboseFailures) {
               report(Event.debug("Upload to remote cache failed: " + e.getMessage()));

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

@@ -329,10 +329,6 @@ SpawnResult execLocallyAndUpload(
       ActionKey actionKey) throws ExecException, IOException, InterruptedException {
     Map<Path, Long> ctimesBefore = getInputCtimes(inputMap);
     SpawnResult result = fallbackRunner.exec(spawn, policy);
-    if (!Status.SUCCESS.equals(result.status()) || result.exitCode() != 0) {
-      // Don't upload failed actions.
-      return result;
-    }
     Map<Path, Long> ctimesAfter = getInputCtimes(inputMap);
     for (Map.Entry<Path, Long> e : ctimesBefore.entrySet()) {
       // Skip uploading to remote cache, because an input was modified during execution.
@@ -342,7 +338,8 @@ SpawnResult execLocallyAndUpload(
     }
     List<Path> outputFiles = listExistingOutputFiles(execRoot, spawn);
     try {
-      remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr());
+      boolean uploadAction = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
+      remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr(), uploadAction);
     } 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

@@ -170,8 +170,12 @@ private void downloadOutErr(ActionResult result, FileOutErr outErr)
 
   @Override
   public void upload(
-      ActionKey actionKey, Path execRoot, Collection<Path> files, FileOutErr outErr)
-          throws IOException, InterruptedException {
+      ActionKey actionKey,
+      Path execRoot,
+      Collection<Path> files,
+      FileOutErr outErr,
+      boolean uploadAction)
+      throws IOException, InterruptedException {
     ActionResult.Builder result = ActionResult.newBuilder();
     upload(result, execRoot, files);
     if (outErr.getErrorPath().exists()) {
@@ -182,8 +186,10 @@ public void upload(
       Digest stdout = uploadFileContents(outErr.getOutputPath());
       result.setStdoutDigest(stdout);
     }
-    blobStore.putActionResult(
-        actionKey.getDigest().getHash(), new ByteArrayInputStream(result.build().toByteArray()));
+    if (uploadAction) {
+      blobStore.putActionResult(
+          actionKey.getDigest().getHash(), new ByteArrayInputStream(result.build().toByteArray()));
+    }
   }
 
   public void upload(ActionResult.Builder result, Path execRoot, Collection<Path> files)

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

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.common.options.Options;
+import com.google.devtools.remoteexecution.v1test.Action;
 import com.google.devtools.remoteexecution.v1test.ActionCacheGrpc.ActionCacheImplBase;
 import com.google.devtools.remoteexecution.v1test.ActionResult;
 import com.google.devtools.remoteexecution.v1test.ContentAddressableStorageGrpc.ContentAddressableStorageImplBase;
@@ -412,6 +413,40 @@ public void findMissingBlobs(
     assertThat(result.build()).isEqualTo(expectedResult.build());
   }
 
+  @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 = Digests.computeActionKey(Action.getDefaultInstance());
+    Path fooFile = execRoot.getRelative("a/foo");
+    Path barFile = execRoot.getRelative("bar");
+    client.upload(emptyKey, execRoot, ImmutableList.<Path>of(fooFile, barFile), outErr, false);
+  }
+
   @Test
   public void testUploadCacheMissesWithRetries() throws Exception {
     final GrpcRemoteCache client = newClient();
@@ -526,7 +561,8 @@ public void onError(Throwable t) {
                 };
               }
             });
-    client.upload(actionKey, execRoot, ImmutableList.<Path>of(fooFile, barFile, bazFile), outErr);
+    client.upload(
+        actionKey, execRoot, ImmutableList.<Path>of(fooFile, barFile, bazFile), outErr, true);
     // 4 times for the errors, 3 times for the successful uploads.
     Mockito.verify(mockByteStreamImpl, Mockito.times(7))
         .write(Mockito.<StreamObserver<WriteResponse>>anyObject());

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

@@ -189,7 +189,11 @@ public void cacheHit() throws Exception {
             any(Command.class));
     verify(remoteCache, never())
         .upload(
-            any(ActionKey.class), any(Path.class), any(Collection.class), any(FileOutErr.class));
+            any(ActionKey.class),
+            any(Path.class),
+            any(Collection.class),
+            any(FileOutErr.class),
+            any(Boolean.class));
     assertThat(result.setupSuccess()).isTrue();
     assertThat(result.exitCode()).isEqualTo(0);
     // We expect the CachedLocalSpawnRunner to _not_ write to outErr at all.
@@ -205,11 +209,7 @@ public void cacheMiss() throws Exception {
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
     entry.store(result, outputFiles);
     verify(remoteCache)
-        .upload(
-            any(ActionKey.class),
-            any(Path.class),
-            eq(outputFiles),
-            eq(outErr));
+        .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
   }
 
   @Test
@@ -219,18 +219,13 @@ public void printWarningIfUploadFails() throws Exception {
     SpawnResult result = new SpawnResult.Builder().setExitCode(0).setStatus(Status.SUCCESS).build();
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
 
-    doThrow(new IOException("cache down")).when(remoteCache).upload(any(ActionKey.class),
-        any(Path.class),
-        eq(outputFiles),
-        eq(outErr));
+    doThrow(new IOException("cache down"))
+        .when(remoteCache)
+        .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
 
     entry.store(result, outputFiles);
     verify(remoteCache)
-        .upload(
-            any(ActionKey.class),
-            any(Path.class),
-            eq(outputFiles),
-            eq(outErr));
+        .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
 
     assertThat(eventHandler.getEvents()).hasSize(1);
     Event evt = eventHandler.getEvents().get(0);

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

@@ -147,8 +147,13 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception {
 
     verify(cache, never())
         .getCachedActionResult(any(ActionKey.class));
-    verify(cache, never()).upload(any(ActionKey.class), any(Path.class), any(Collection.class),
-          any(FileOutErr.class));
+    verify(cache, never())
+        .upload(
+            any(ActionKey.class),
+            any(Path.class),
+            any(Collection.class),
+            any(FileOutErr.class),
+            any(Boolean.class));
     verifyZeroInteractions(localRunner);
   }
 
@@ -187,15 +192,20 @@ public void nonCachableSpawnsShouldNotBeCached_local() throws Exception {
 
     verify(cache, never())
         .getCachedActionResult(any(ActionKey.class));
-    verify(cache, never()).upload(any(ActionKey.class), any(Path.class), any(Collection.class),
-        any(FileOutErr.class));
+    verify(cache, never())
+        .upload(
+            any(ActionKey.class),
+            any(Path.class),
+            any(Collection.class),
+            any(FileOutErr.class),
+            any(Boolean.class));
   }
 
   @Test
   @SuppressWarnings("unchecked")
-  public void failedActionShouldNotBeUploaded() throws Exception {
-    // Test that the outputs of a failed locally executed action are not uploaded to a remote
-    // cache.
+  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.
 
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteUploadLocalResults = true;
@@ -217,8 +227,13 @@ public void failedActionShouldNotBeUploaded() throws Exception {
     verify(localRunner).exec(eq(spawn), eq(policy));
     verify(runner).execLocallyAndUpload(eq(spawn), eq(policy), any(SortedMap.class), eq(cache),
         any(ActionKey.class));
-    verify(cache, never()).upload(any(ActionKey.class), any(Path.class), any(Collection.class),
-        any(FileOutErr.class));
+    verify(cache)
+        .upload(
+            any(ActionKey.class),
+            any(Path.class),
+            any(Collection.class),
+            any(FileOutErr.class),
+            eq(false));
   }
 
   @Test
@@ -267,9 +282,14 @@ public void printWarningIfCacheIsDown() throws Exception {
     when(cache.getCachedActionResult(any(ActionKey.class)))
         .thenThrow(new IOException("cache down"));
 
-    doThrow(new IOException("cache down")).when(cache)
-        .upload(any(ActionKey.class), any(Path.class), any(Collection.class),
-            any(FileOutErr.class));
+    doThrow(new IOException("cache down"))
+        .when(cache)
+        .upload(
+            any(ActionKey.class),
+            any(Path.class),
+            any(Collection.class),
+            any(FileOutErr.class),
+            eq(true));
 
     SpawnResult res = new SpawnResult.Builder().setStatus(Status.SUCCESS).setExitCode(0).build();
     when(localRunner.exec(eq(spawn), eq(policy))).thenReturn(res);