Fixes #6219. Don't rethrow any remote cache failures on either download or upload, only warn. Added more tests. TESTED=new unit tests RELNOTES: Fix regression #6219, remote cache failures PiperOrigin-RevId: 214614941
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 4cf1af0..34c9cc6 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,7 +34,6 @@ 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; @@ -69,7 +68,7 @@ ((SettableFuture<byte[]>) EMPTY_BYTES).set(new byte[0]); } - public static boolean causedByCacheMiss(RetryException t) { + public static boolean causedByCacheMiss(IOException t) { return t.getCause() instanceof CacheNotFoundException; }
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 410ca80..27294f0 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,7 +33,6 @@ 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; @@ -138,18 +137,15 @@ .build(); return SpawnCache.success(spawnResult); } - } 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(); - if (isNullOrEmpty(errorMsg)) { - errorMsg = e.getClass().getSimpleName(); + if (!AbstractRemoteActionCache.causedByCacheMiss(e)) { + String errorMsg = e.getMessage(); + if (isNullOrEmpty(errorMsg)) { + errorMsg = e.getClass().getSimpleName(); + } + errorMsg = "Error reading from the remote cache:\n" + errorMsg; + report(Event.warn(errorMsg)); } - errorMsg = "Error reading from the remote cache:\n" + errorMsg; - report(Event.warn(errorMsg)); } finally { withMetadata.detach(previous); }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index df821cb..cba05a7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -24,6 +24,8 @@ import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.Command; +import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.OutputFile; import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -49,6 +51,7 @@ import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; +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; @@ -409,4 +412,126 @@ assertThat(progressUpdates) .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache")); } + + @Test + public void printWarningIfDownloadFails() throws Exception { + doThrow(new RetryException("reason", 0, io.grpc.Status.UNAVAILABLE.asRuntimeException())) + .when(remoteCache) + .getCachedActionResult(any(ActionKey.class)); + + CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); + assertThat(entry.hasResult()).isFalse(); + SpawnResult result = + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .build(); + ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file")); + + Mockito.doAnswer( + new Answer<Void>() { + @Override + public Void answer(InvocationOnMock invocation) { + RequestMetadata meta = TracingMetadataUtils.fromCurrentContext(); + assertThat(meta.getCorrelatedInvocationsId()).isEqualTo("build-req-id"); + assertThat(meta.getToolInvocationId()).isEqualTo("command-id"); + return null; + } + }) + .when(remoteCache) + .upload( + any(ActionKey.class), + any(Action.class), + any(Command.class), + any(Path.class), + eq(outputFiles), + eq(outErr), + eq(true)); + entry.store(result); + verify(remoteCache) + .upload( + any(ActionKey.class), + any(Action.class), + any(Command.class), + any(Path.class), + eq(outputFiles), + eq(outErr), + eq(true)); + + assertThat(eventHandler.getEvents()).hasSize(1); + Event evt = eventHandler.getEvents().get(0); + assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); + assertThat(evt.getMessage()).contains("Error"); + assertThat(evt.getMessage()).contains("reading"); + assertThat(evt.getMessage()).contains("reason"); + assertThat(progressUpdates) + .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache")); + } + + @Test + public void orphanedCachedResultIgnored() throws Exception { + Digest digest = digestUtil.computeAsUtf8("bla"); + ActionResult actionResult = + ActionResult.newBuilder() + .addOutputFiles(OutputFile.newBuilder().setPath("/random/file").setDigest(digest)) + .build(); + when(remoteCache.getCachedActionResult(any(ActionKey.class))) + .thenAnswer( + new Answer<ActionResult>() { + @Override + public ActionResult answer(InvocationOnMock invocation) { + RequestMetadata meta = TracingMetadataUtils.fromCurrentContext(); + assertThat(meta.getCorrelatedInvocationsId()).isEqualTo("build-req-id"); + assertThat(meta.getToolInvocationId()).isEqualTo("command-id"); + return actionResult; + } + }); + doThrow(new RetryException("cache miss", 0, new CacheNotFoundException(digest, digestUtil))) + .when(remoteCache) + .download(actionResult, execRoot, outErr); + + CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); + assertThat(entry.hasResult()).isFalse(); + SpawnResult result = + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .build(); + ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file")); + + Mockito.doAnswer( + new Answer<Void>() { + @Override + public Void answer(InvocationOnMock invocation) { + RequestMetadata meta = TracingMetadataUtils.fromCurrentContext(); + assertThat(meta.getCorrelatedInvocationsId()).isEqualTo("build-req-id"); + assertThat(meta.getToolInvocationId()).isEqualTo("command-id"); + return null; + } + }) + .when(remoteCache) + .upload( + any(ActionKey.class), + any(Action.class), + any(Command.class), + any(Path.class), + eq(outputFiles), + eq(outErr), + eq(true)); + entry.store(result); + verify(remoteCache) + .upload( + any(ActionKey.class), + any(Action.class), + any(Command.class), + any(Path.class), + eq(outputFiles), + eq(outErr), + eq(true)); + assertThat(progressUpdates) + .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache")); + assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed. + } }