From 1faf3d7baffc8b5935b1a282f0d6fa1b63fb941e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 12 Feb 2025 02:06:55 -0800 Subject: [PATCH] Don't fetch empty digest on failure status in `FetchBlobResponse` 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 --- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloader.java | 5 ++ .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloaderTest.java | 46 +++++++++++++++++++ 4 files changed, 53 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD index 1ea404a3597e80..81342ce2904e7f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 103239beb5c4d8..44dcc8164d277a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/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( diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index 3dee6d7f751131..a244f1a9e291df 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/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", diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index 81deba20f69559..9a8ae64ddf3203 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/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 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);