Unwrap CacheNotFound for RemoteActionCache Using retrier causes CacheNotFoundException to be wrapped in a RetryException, which is handled through undesirable means by the users of RemoteActionCache#download via IOException. This must be unwrapped and thrown directly as a part of the download interface for users, so that users can distinguish these degraded cache entries specifically. Reverting the behavior of remotelyReExecuteOrphanedCachedActions, which was adapted to verify download retries, and should be separated into its own test. Closes #5917. PiperOrigin-RevId: 209638152
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index 3c73c4e..4cf1af0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.concurrent.ThreadSafety; +import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.Utils; @@ -68,6 +69,10 @@ ((SettableFuture<byte[]>) EMPTY_BYTES).set(new byte[0]); } + public static boolean causedByCacheMiss(RetryException t) { + return t.getCause() instanceof CacheNotFoundException; + } + protected final RemoteOptions options; protected final DigestUtil digestUtil; private final Retrier retrier;
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java index a5eff1a..bf1bb2a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -36,6 +36,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.hash.HashingOutputStream; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.actions.ActionInput; @@ -189,6 +190,9 @@ @Override protected ListenableFuture<Void> downloadBlob(Digest digest, OutputStream out) { + if (digest.getSizeBytes() == 0) { + return Futures.immediateFuture(null); + } String resourceName = ""; if (!options.remoteInstanceName.isEmpty()) { resourceName += options.remoteInstanceName + "/";
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 7c6cc50..619a67b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.exec.SpawnCache; import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; +import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; @@ -137,7 +138,10 @@ .build(); return SpawnCache.success(spawnResult); } - } catch (CacheNotFoundException e) { + } catch (RetryException e) { + if (!AbstractRemoteActionCache.causedByCacheMiss(e)) { + throw e; + } // There's a cache miss. Fall back to local execution. } catch (IOException e) { String errorMsg = e.getMessage();
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 65570b6..18eb6bb 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -185,7 +185,10 @@ .setCacheHit(true) .setRunnerName("remote cache hit") .build(); - } catch (CacheNotFoundException e) { + } catch (RetryException e) { + if (!AbstractRemoteActionCache.causedByCacheMiss(e)) { + throw e; + } // No cache hit, so we fall through to local or remote execution. // We set acceptCachedResult to false in order to force the action re-execution. acceptCachedResult = false;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java index 84db066..f87e78b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -94,6 +94,7 @@ import java.util.Set; import java.util.SortedMap; import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.Nullable; import org.junit.After; import org.junit.AfterClass; @@ -1048,10 +1049,10 @@ @Override public void read(ReadRequest request, StreamObserver<ReadResponse> responseObserver) { - // First read is a retriable error, next read succeeds. + // First read is a cache miss, next read succeeds. if (first) { first = false; - responseObserver.onError(Status.UNAVAILABLE.asRuntimeException()); + responseObserver.onError(Status.NOT_FOUND.asRuntimeException()); } else { responseObserver.onNext( ReadResponse.newBuilder().setData(ByteString.copyFromUtf8("stdout")).build()); @@ -1078,10 +1079,12 @@ }; } }); + AtomicInteger numExecuteCalls = new AtomicInteger(); serviceRegistry.addService( new ExecutionImplBase() { @Override public void execute(ExecuteRequest request, StreamObserver<Operation> responseObserver) { + numExecuteCalls.incrementAndGet(); assertThat(request.getSkipCacheLookup()).isTrue(); // Action will be re-executed. responseObserver.onNext( Operation.newBuilder() @@ -1107,7 +1110,8 @@ SpawnResult result = client.exec(simpleSpawn, simplePolicy); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); - assertThat(result.isCacheHit()).isTrue(); + assertThat(result.isCacheHit()).isFalse(); assertThat(outErr.outAsLatin1()).isEqualTo("stdout"); + assertThat(numExecuteCalls.get()).isEqualTo(1); } }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 4b6d312..105a881 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -743,7 +743,10 @@ ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(cachedResult); - doThrow(CacheNotFoundException.class) + Retrier.RetryException downloadFailure = + new Retrier.RetryException( + "", 1, new CacheNotFoundException(Digest.getDefaultInstance(), digestUtil)); + doThrow(downloadFailure) .when(cache) .download(eq(cachedResult), any(Path.class), any(FileOutErr.class)); ActionResult execResult = ActionResult.newBuilder().setExitCode(31).build();