Remote: Do not upload empty output to remote cache.

An unintended side-effect of change cc2b3ec is that zero-sized blob won't be uploaded to gRPC cache. However, the behavior is existed for a long time and the REAPI spec is also updated accordingly. This change makes the behavior explicit and brings it to other remote cache backends.

Context #11063.

Fixes #13349.

Closes #13594.

PiperOrigin-RevId: 384457129

Author: coeuvre

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

@@ -139,6 +139,38 @@ public ActionResult downloadActionResult(
     return getFromFuture(cacheProtocol.downloadActionResult(context, actionKey, inlineOutErr));
   }
 
+  /**
+   * Upload a local file to the remote cache.
+   *
+   * @param context the context for the action.
+   * @param digest the digest of the file.
+   * @param file the file to upload.
+   */
+  public final ListenableFuture<Void> uploadFile(
+      RemoteActionExecutionContext context, Digest digest, Path file) {
+    if (digest.getSizeBytes() == 0) {
+      return COMPLETED_SUCCESS;
+    }
+
+    return cacheProtocol.uploadFile(context, digest, file);
+  }
+
+  /**
+   * Upload sequence of bytes to the remote cache.
+   *
+   * @param context the context for the action.
+   * @param digest the digest of the file.
+   * @param data the BLOB to upload.
+   */
+  public final ListenableFuture<Void> uploadBlob(
+      RemoteActionExecutionContext context, Digest digest, ByteString data) {
+    if (digest.getSizeBytes() == 0) {
+      return COMPLETED_SUCCESS;
+    }
+
+    return cacheProtocol.uploadBlob(context, digest, data);
+  }
+
   /**
    * Upload the result of a locally executed action to the remote cache.
    *
@@ -219,14 +251,14 @@ private void uploadOutputs(
     for (Digest digest : digestsToUpload) {
       Path file = digestToFile.get(digest);
       if (file != null) {
-        uploads.add(cacheProtocol.uploadFile(context, digest, file));
+        uploads.add(uploadFile(context, digest, file));
       } else {
         ByteString blob = digestToBlobs.get(digest);
         if (blob == null) {
           String message = "FindMissingBlobs call returned an unknown digest: " + digest;
           throw new IOException(message);
         }
-        uploads.add(cacheProtocol.uploadBlob(context, digest, blob));
+        uploads.add(uploadBlob(context, digest, blob));
       }
     }
 
@@ -317,6 +349,15 @@ public void onFailure(Throwable t) {
     return outerF;
   }
 
+  private ListenableFuture<Void> downloadBlob(
+      RemoteActionExecutionContext context, Digest digest, OutputStream out) {
+    if (digest.getSizeBytes() == 0) {
+      return COMPLETED_SUCCESS;
+    }
+
+    return cacheProtocol.downloadBlob(context, digest, out);
+  }
+
   private static Path toTmpDownloadPath(Path actualPath) {
     return actualPath.getParentDirectory().getRelative(actualPath.getBaseName() + ".tmp");
   }
@@ -662,7 +703,14 @@ public void onFailure(Throwable t) {
     return outerF;
   }
 
-  private List<ListenableFuture<FileMetadata>> downloadOutErr(
+  /**
+   * Download the stdout and stderr of an executed action.
+   *
+   * @param context the context for the action.
+   * @param result the result of the action.
+   * @param outErr the {@link OutErr} that the stdout and stderr will be downloaded to.
+   */
+  public final List<ListenableFuture<FileMetadata>> downloadOutErr(
       RemoteActionExecutionContext context, ActionResult result, OutErr outErr) {
     List<ListenableFuture<FileMetadata>> downloads = new ArrayList<>();
     if (!result.getStdoutRaw().isEmpty()) {
@@ -675,8 +723,7 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(
     } else if (result.hasStdoutDigest()) {
       downloads.add(
           Futures.transform(
-              cacheProtocol.downloadBlob(
-                  context, result.getStdoutDigest(), outErr.getOutputStream()),
+              downloadBlob(context, result.getStdoutDigest(), outErr.getOutputStream()),
               (d) -> null,
               directExecutor()));
     }
@@ -690,8 +737,7 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(
     } else if (result.hasStderrDigest()) {
       downloads.add(
           Futures.transform(
-              cacheProtocol.downloadBlob(
-                  context, result.getStderrDigest(), outErr.getErrorStream()),
+              downloadBlob(context, result.getStderrDigest(), outErr.getErrorStream()),
               (d) -> null,
               directExecutor()));
     }

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

@@ -610,6 +610,7 @@ public void uploadSymlinkInDirectoryNoAllowError() throws Exception {
     assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload");
   }
 
+  // TODO(chiwang): Cleanup the tests, e.g. use java test data builder pattern.
   @Test
   public void downloadRelativeFileSymlink() throws Exception {
     RemoteCache cache = newRemoteCache();
@@ -1364,6 +1365,27 @@ public void testDownloadEmptyBlobAndFile() throws Exception {
     assertThat(file.getFileSize()).isEqualTo(0);
   }
 
+  @Test
+  public void downloadOutErr_empty_doNotPerformDownload() throws Exception {
+    // Test that downloading empty stdout/stderr does not try to perform a download.
+
+    InMemoryRemoteCache remoteCache = newRemoteCache();
+    Digest emptyDigest = digestUtil.compute(new byte[0]);
+    ActionResult.Builder result = ActionResult.newBuilder();
+    result.setStdoutDigest(emptyDigest);
+    result.setStderrDigest(emptyDigest);
+
+    RemoteCache.waitForBulkTransfer(
+        remoteCache.downloadOutErr(
+            context,
+            result.build(),
+            new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))),
+        true);
+
+    assertThat(remoteCache.getNumSuccessfulDownloads()).isEqualTo(0);
+    assertThat(remoteCache.getNumFailedDownloads()).isEqualTo(0);
+  }
+
   @Test
   public void testDownloadFileWithSymlinkTemplate() throws Exception {
     // Test that when a symlink template is provided, we don't actually download files to disk.
@@ -1627,6 +1649,50 @@ public void testUploadDirectory() throws Exception {
     assertThat(remoteCache.findMissingDigests(context, toQuery)).isEmpty();
   }
 
+  @Test
+  public void upload_emptyBlobAndFile_doNotPerformUpload() throws Exception {
+    // Test that uploading an empty BLOB/file does not try to perform an upload.
+    InMemoryRemoteCache remoteCache = newRemoteCache();
+    Digest emptyDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), "");
+    Path file = execRoot.getRelative("file");
+
+    Utils.getFromFuture(remoteCache.uploadBlob(context, emptyDigest, ByteString.EMPTY));
+    assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))
+        .containsExactly(emptyDigest);
+
+    Utils.getFromFuture(remoteCache.uploadFile(context, emptyDigest, file));
+    assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))
+        .containsExactly(emptyDigest);
+  }
+
+  @Test
+  public void upload_emptyOutputs_doNotPerformUpload() throws Exception {
+    // Test that uploading an empty output does not try to perform an upload.
+
+    // arrange
+    Digest emptyDigest =
+        fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/test/wobble"), "");
+    Path file = execRoot.getRelative("bar/test/wobble");
+    InMemoryRemoteCache remoteCache = newRemoteCache();
+    Action action = Action.getDefaultInstance();
+    ActionKey actionDigest = digestUtil.computeActionKey(action);
+    Command cmd = Command.getDefaultInstance();
+
+    // act
+    remoteCache.upload(
+        context,
+        remotePathResolver,
+        actionDigest,
+        action,
+        cmd,
+        ImmutableList.of(file),
+        new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr")));
+
+    // assert
+    assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))
+        .containsExactly(emptyDigest);
+  }
+
   @Test
   public void testUploadEmptyDirectory() throws Exception {
     // Test that uploading an empty directory works.

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java

@@ -18,7 +18,6 @@
 import build.bazel.remote.execution.v2.Directory;
 import build.bazel.remote.execution.v2.DirectoryNode;
 import build.bazel.remote.execution.v2.FileNode;
-import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.remote.RemoteCache;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
@@ -27,7 +26,6 @@
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.Utils;
 import com.google.devtools.build.lib.vfs.Path;
-import com.google.protobuf.ByteString;
 import java.io.IOException;
 
 /** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */
@@ -61,16 +59,6 @@ public void downloadTree(
     }
   }
 
-  public ListenableFuture<Void> uploadFile(
-      RemoteActionExecutionContext context, Digest digest, Path file) {
-    return cacheProtocol.uploadFile(context, digest, file);
-  }
-
-  public ListenableFuture<Void> uploadBlob(
-      RemoteActionExecutionContext context, Digest digest, ByteString data) {
-    return cacheProtocol.uploadBlob(context, digest, data);
-  }
-
   public void uploadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult)
       throws IOException, InterruptedException {