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 https://github.com/bazelbuild/bazel/commit/562fcf9f5dfd14daea718f77da95b43b1400689b. To reproduce: run a failing test vs a BES service, the test log would not be uploaded.

TESTED=unit tests
PiperOrigin-RevId: 169143428

3 files changed, 79 insertions, 28 deletions

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 96b74d2a5b..fe31aa729d 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
@@ -38,6 +38,7 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils;
 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;
@@ -413,6 +414,40 @@ public class GrpcRemoteCacheTest {
   }
 
   @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();
     final Digest fooDigest =
@@ -526,7 +561,8 @@ public class GrpcRemoteCacheTest {
                 };
               }
             });
-    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());
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 43e4de4f54..160c20fa6d 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
@@ -189,7 +189,11 @@ public class RemoteSpawnCacheTest {
             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 class RemoteSpawnCacheTest {
     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 class RemoteSpawnCacheTest {
     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);
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 a6c8218f01..940c9d3afd 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
@@ -147,8 +147,13 @@ public class RemoteSpawnRunnerTest {
 
     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 class RemoteSpawnRunnerTest {
 
     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 class RemoteSpawnRunnerTest {
     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 class RemoteSpawnRunnerTest {
     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);