Skip to content
Snippets Groups Projects
Unverified Commit e73391e6 authored by bazel.build machine account's avatar bazel.build machine account Committed by GitHub
Browse files

[8.2.0] Don't fetch empty digest on failure status in `FetchBlobResponse` (#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
https://github.com/bazelbuild/bazel/commit/e0cd2f245491f93065e5e04106a9b6e7eb029292



Co-authored-by: default avatarFabian Meumertzheim <fabian@meumertzhe.im>
Co-authored-by: default avatarIan (Hee) Cha <heec@google.com>
parent e1e5c940
No related merge requests found
...@@ -29,6 +29,7 @@ java_library( ...@@ -29,6 +29,7 @@ java_library(
"//third_party:jsr305", "//third_party:jsr305",
"//third_party/grpc-java:grpc-jar", "//third_party/grpc-java:grpc-jar",
"@com_google_protobuf//:protobuf_java_util", "@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_grpc",
"@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",
......
...@@ -40,9 +40,11 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; ...@@ -40,9 +40,11 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils;
import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Path;
import com.google.protobuf.util.Timestamps; import com.google.protobuf.util.Timestamps;
import com.google.rpc.Code;
import io.grpc.CallCredentials; import io.grpc.CallCredentials;
import io.grpc.Channel; import io.grpc.Channel;
import io.grpc.StatusRuntimeException; import io.grpc.StatusRuntimeException;
import io.grpc.protobuf.StatusProto;
import java.io.IOException; import java.io.IOException;
import java.io.OutputStream; import java.io.OutputStream;
import java.net.URISyntaxException; import java.net.URISyntaxException;
...@@ -160,6 +162,9 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader { ...@@ -160,6 +162,9 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader {
channel -> channel ->
fetchBlockingStub(remoteActionExecutionContext, channel) fetchBlockingStub(remoteActionExecutionContext, channel)
.fetchBlob(request))); .fetchBlob(request)));
if (response.getStatus().getCode() != Code.OK_VALUE) {
throw StatusProto.toStatusRuntimeException(response.getStatus());
}
final Digest blobDigest = response.getBlobDigest(); final Digest blobDigest = response.getBlobDigest();
retrier.execute( retrier.execute(
......
...@@ -47,6 +47,7 @@ java_library( ...@@ -47,6 +47,7 @@ java_library(
"//third_party/grpc-java:grpc-jar", "//third_party/grpc-java:grpc-jar",
"@com_google_protobuf//:protobuf_java", "@com_google_protobuf//:protobuf_java",
"@com_google_protobuf//:protobuf_java_util", "@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_grpc",
"@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",
......
...@@ -65,6 +65,8 @@ import com.google.devtools.build.lib.vfs.SyscallCache; ...@@ -65,6 +65,8 @@ import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.common.options.Options; import com.google.devtools.common.options.Options;
import com.google.protobuf.ByteString; import com.google.protobuf.ByteString;
import com.google.protobuf.util.Timestamps; import com.google.protobuf.util.Timestamps;
import com.google.rpc.Code;
import com.google.rpc.Status;
import io.grpc.CallCredentials; import io.grpc.CallCredentials;
import io.grpc.ManagedChannel; import io.grpc.ManagedChannel;
import io.grpc.Server; import io.grpc.Server;
...@@ -277,6 +279,50 @@ public class GrpcRemoteDownloaderTest { ...@@ -277,6 +279,50 @@ public class GrpcRemoteDownloaderTest {
assertThat(downloaded).isEqualTo(content); 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 @Test
public void testPropagateChecksum() throws Exception { public void testPropagateChecksum() throws Exception {
final byte[] content = "example content".getBytes(UTF_8); final byte[] content = "example content".getBytes(UTF_8);
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment