From 78b89a0136a83d303d4d88373d6e510f85a81fbb Mon Sep 17 00:00:00 2001
From: Benjamin Peterson <benjamin@engflow.com>
Date: Mon, 9 Aug 2021 20:21:01 -0700
Subject: [PATCH] remote: Do not retry bytestream read requests if the file is
 complete.

Previously, an error at the end of the Read stream would result in a pointless retry request for 0 bytes of data.

Closes #13808.

PiperOrigin-RevId: 389785691
---
 .../build/lib/remote/GrpcCacheClient.java     |  8 +++++++
 .../build/lib/remote/GrpcCacheClientTest.java | 22 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java
index 29ee7acf7d9d27..a1d74b604ec26b 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java
@@ -366,6 +366,14 @@ public void onNext(ReadResponse readResponse) {
 
               @Override
               public void onError(Throwable t) {
+                if (offset.get() == digest.getSizeBytes()) {
+                  // If the file was fully downloaded, it doesn't matter if there was an error at
+                  // the end of the stream.
+                  logger.atInfo().withCause(t).log(
+                      "ignoring error because file was fully received");
+                  onCompleted();
+                  return;
+                }
                 Status status = Status.fromThrowable(t);
                 if (status.getCode() == Status.Code.NOT_FOUND) {
                   future.setException(new CacheNotFoundException(digest));
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
index 4999dc73105a32..3faa64560ef7e9 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
@@ -962,6 +962,28 @@ public void read(ReadRequest request, StreamObserver<ReadResponse> responseObser
     Mockito.verify(mockBackoff, Mockito.never()).nextDelayMillis(any(Exception.class));
   }
 
+  @Test
+  public void downloadBlobDoesNotRetryZeroLengthRequests()
+      throws IOException, InterruptedException {
+    Backoff mockBackoff = Mockito.mock(Backoff.class);
+    final GrpcCacheClient client =
+        newClient(Options.getDefaults(RemoteOptions.class), () -> mockBackoff);
+    final Digest digest = DIGEST_UTIL.computeAsUtf8("abcdefg");
+    serviceRegistry.addService(
+        new ByteStreamImplBase() {
+          @Override
+          public void read(ReadRequest request, StreamObserver<ReadResponse> responseObserver) {
+            assertThat(request.getResourceName()).contains(digest.getHash());
+            assertThat(request.getReadOffset()).isEqualTo(0);
+            ByteString data = ByteString.copyFromUtf8("abcdefg");
+            responseObserver.onNext(ReadResponse.newBuilder().setData(data).build());
+            responseObserver.onError(Status.INTERNAL.asException());
+          }
+        });
+    assertThat(new String(downloadBlob(context, client, digest), UTF_8)).isEqualTo("abcdefg");
+    Mockito.verify(mockBackoff, Mockito.never()).nextDelayMillis(any(Exception.class));
+  }
+
   @Test
   public void downloadBlobPassesThroughDeadlineExceededWithoutProgress() throws IOException {
     Backoff mockBackoff = Mockito.mock(Backoff.class);