Set oldest_content_accepted for remote downloader requests without the checksum This PR address the #23932 issue with remote downloader. Closes #23970. PiperOrigin-RevId: 686180819 Change-Id: Ia36c5793622bd1ac1fdaa9ef1326ddc385a5a01f
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 dccb139..16c71bd 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
@@ -16,6 +16,7 @@ srcs = glob(["*.java"]), deps = [ "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote:ReferenceCountedChannel", "//src/main/java/com/google/devtools/build/lib/remote:Retrier", @@ -27,6 +28,7 @@ "//third_party:guava", "//third_party:jsr305", "//third_party/grpc-java:grpc-jar", + "@protobuf//:protobuf_java_util", "@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 8cf643b..9720347 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
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.HashOutputStream; +import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.ReferenceCountedChannel; @@ -38,6 +39,7 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.vfs.Path; +import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.Channel; import io.grpc.StatusRuntimeException; @@ -45,6 +47,7 @@ import java.io.OutputStream; import java.net.URISyntaxException; import java.net.URL; +import java.time.Duration; import java.util.List; import java.util.Map; import java.util.Optional; @@ -235,6 +238,12 @@ .setName(QUALIFIER_CHECKSUM_SRI) .setValue(checksum.get().toSubresourceIntegrity()) .build()); + } else { + // If no checksum is provided, never accept cached content. + // Timestamp is offset by an hour to account for clock skew. + requestBuilder.setOldestContentAccepted( + Timestamps.fromMillis( + BlazeClock.instance().now().plus(Duration.ofHours(1)).toEpochMilli())); } if (!Strings.isNullOrEmpty(canonicalId)) {
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 6f45cf4..d5cc6da 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
@@ -24,6 +24,7 @@ "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/common", @@ -44,6 +45,7 @@ "//third_party:truth", "//third_party/grpc-java:grpc-jar", "@protobuf//:protobuf_java", + "@protobuf//:protobuf_java_util", "@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 1e2ba06..b6ca8c4 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
@@ -42,6 +42,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException; +import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.ChannelConnectionWithServerCapabilitiesFactory; import com.google.devtools.build.lib.remote.ReferenceCountedChannel; @@ -54,6 +55,7 @@ import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.TestUtils; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -61,6 +63,7 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; +import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.ManagedChannel; import io.grpc.Server; @@ -73,6 +76,7 @@ import java.io.InputStream; import java.net.URI; import java.net.URL; +import java.time.Duration; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -90,6 +94,8 @@ @RunWith(JUnit4.class) public class GrpcRemoteDownloaderTest { + private static final ManualClock clock = new ManualClock(); + private static final DigestUtil DIGEST_UTIL = new DigestUtil(SyscallCache.NO_CACHE, DigestHashFunction.SHA256); @@ -117,6 +123,8 @@ context = RemoteActionExecutionContext.create(metadata); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + + BlazeClock.setClock(clock); } @After @@ -212,6 +220,8 @@ .isEqualTo( FetchBlobRequest.newBuilder() .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .setOldestContentAccepted( + Timestamps.fromMillis(clock.advance(Duration.ofHours(1)))) .addUris("http://example.com/content.txt") .build()); responseObserver.onNext( @@ -474,4 +484,29 @@ Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) .build()); } + + @Test + public void testFetchBlobRequest_withoutChecksum() throws Exception { + FetchBlobRequest request = + GrpcRemoteDownloader.newFetchBlobRequest( + "instance name", + false, + ImmutableList.of(new URI("http://example.com/").toURL()), + Optional.<Checksum>empty(), + "canonical ID", + DIGEST_UTIL.getDigestFunction(), + ImmutableMap.of(), + StaticCredentials.EMPTY); + + assertThat(request) + .isEqualTo( + FetchBlobRequest.newBuilder() + .setInstanceName("instance name") + .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .setOldestContentAccepted(Timestamps.fromMillis(clock.advance(Duration.ofHours(1)))) + .addUris("http://example.com/") + .addQualifiers( + Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) + .build()); + } }