[8.2.0] Don't fetch empty digest on failure status in `FetchBlobRespo…

…nse` (#25277)

When the `GrpcRemoteDownloader` receives a `FetchBlobResponse` with an
error status (instead of a connection level status error), it must fail
and fall back to the HTTP downloader if configured instead of ignoring
the status and attempting to fetch the (usually empty) digest from the
cache. The previous behavior could result in errors being masked and
resulting in an empty file being downloaded if the remote cache accepts
empty `Digest` messages.

Closes #25244.

PiperOrigin-RevId: 725963696
Change-Id: I9f643b944b9fff9264c72f41485564ff1fab71d9

Commit
e0cd2f2

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Co-authored-by: Ian (Hee) Cha <heec@google.com>

src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD

@@ -29,6 +29,7 @@ java_library(
         "//third_party:jsr305",
         "//third_party/grpc-java:grpc-jar",
         "@com_google_protobuf//:protobuf_java_util",
+        "@googleapis//google/rpc:rpc_java_proto",
         "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_grpc",
         "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto",
         "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",

src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java

@@ -40,9 +40,11 @@
 import com.google.devtools.build.lib.remote.util.Utils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.protobuf.util.Timestamps;
+import com.google.rpc.Code;
 import io.grpc.CallCredentials;
 import io.grpc.Channel;
 import io.grpc.StatusRuntimeException;
+import io.grpc.protobuf.StatusProto;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.net.URISyntaxException;
@@ -160,6 +162,9 @@ public void download(
                       channel ->
                           fetchBlockingStub(remoteActionExecutionContext, channel)
                               .fetchBlob(request)));
+      if (response.getStatus().getCode() != Code.OK_VALUE) {
+        throw StatusProto.toStatusRuntimeException(response.getStatus());
+      }
       final Digest blobDigest = response.getBlobDigest();
 
       retrier.execute(

src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD

@@ -47,6 +47,7 @@ java_library(
         "//third_party/grpc-java:grpc-jar",
         "@com_google_protobuf//:protobuf_java",
         "@com_google_protobuf//:protobuf_java_util",
+        "@googleapis//google/rpc:rpc_java_proto",
         "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_grpc",
         "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto",
         "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",

src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java

@@ -65,6 +65,8 @@
 import com.google.devtools.common.options.Options;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.util.Timestamps;
+import com.google.rpc.Code;
+import com.google.rpc.Status;
 import io.grpc.CallCredentials;
 import io.grpc.ManagedChannel;
 import io.grpc.Server;
@@ -277,6 +279,50 @@ public void fetchBlob(
     assertThat(downloaded).isEqualTo(content);
   }
 
+  @Test
+  public void testStatusHandling() throws Exception {
+    final byte[] content = "example content".getBytes(UTF_8);
+    serviceRegistry.addService(
+        new FetchImplBase() {
+          @Override
+          public void fetchBlob(
+              FetchBlobRequest request, StreamObserver<FetchBlobResponse> responseObserver) {
+            assertThat(request)
+                .isEqualTo(
+                    FetchBlobRequest.newBuilder()
+                        .setDigestFunction(DIGEST_UTIL.getDigestFunction())
+                        .setOldestContentAccepted(
+                            Timestamps.fromMillis(clock.advance(Duration.ofHours(1))))
+                        .addUris("http://example.com/content.txt")
+                        .build());
+            responseObserver.onNext(
+                FetchBlobResponse.newBuilder()
+                    .setStatus(
+                        Status.newBuilder()
+                            .setCode(Code.PERMISSION_DENIED_VALUE)
+                            .setMessage("permission denied")
+                            .build())
+                    .setUri("http://example.com/content.txt")
+                    .build());
+            responseObserver.onCompleted();
+          }
+        });
+    final RemoteCacheClient cacheClient = new InMemoryCacheClient();
+    final GrpcRemoteDownloader downloader =
+        newDownloader(cacheClient, /* fallbackDownloader= */ null);
+    // Add a cache entry for the empty Digest to verify that the implementation checks the status
+    // before fetching the digest.
+    getFromFuture(cacheClient.uploadBlob(context, Digest.getDefaultInstance(), ByteString.EMPTY));
+
+    var exception =
+        assertThrows(
+            IOException.class,
+            () ->
+                downloadBlob(
+                    downloader, new URL("http://example.com/content.txt"), Optional.empty()));
+    assertThat(exception).hasMessageThat().contains("permission denied");
+  }
+
   @Test
   public void testPropagateChecksum() throws Exception {
     final byte[] content = "example content".getBytes(UTF_8);