Repository: bazelbuild/bazel
Issue: remote: treat failed actions as a cache miss
Labels: type: bug
P1
team-Remote-Exec

If a remote cache returns a failed action (exit code != 0) then Bazel should treat this as a cache miss and attempt to re-execute the action. See for details https://gitlab.com/BuildGrid/buildgrid/issues/151

Issue comments:
We need to fix this for remote execution and caching across all protocols. In the case of remote execution, we'll also need to set `skip_cache_lookup` to `true` in order to ensure re-execution.

Pull request review comments:
So "we don't cache failed actions" is no longer correct for Bazel? If so, this comment is no longer true (even if Bazel won't cache failed actions itself).
s/missed cache/cache miss/
Why do we need to test for the metadata?
why do you think the asserts below this line are valuable for the test?
I think the cache and execution priority aren't important for this test, no?
// Failed actions are treated as a cache miss mostly in order to avoid caching flaky actions (tests).
Afaik these are all default values no?
upload should be called because `uploadLocalResults` is `true`. Generally, if a failed action was cached which then succeeds locally we want to overwrite the failed result with the successful result in the cache. This would typically be used for flaky tests.
Right, we don't. Cleaned up.
We need to verify, that current action indeed treated as a cash miss => we do not attempt to download anything. 
Asserts in line 528-529 are obsolete, removing them.
Whether or not something is being downloaded seems orthogonal to what should be tested here. I'd argue that all this test should verify is that it's indeed a cache miss. We then have a separate unit test that verifies what should/should not happen on a cache miss.
`when(res.exitCode()).thenReturn(1);
    when(res.status()).thenReturn(Status.EXECUTION_FAILED);`
because of these 2 lines the local action also failed => no upload expected to the cache.

But I think successful use case makes more sense here, so changed to cache miss + successful local execution => cache.upload(...)
Fair point. Removed.
nit: instead of mocking the spawn you can just use SpawnResult.Builder()

Patch that resolved the issue:
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 f86de075f83ad4..7d9d2bde32eb2c 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
@@ -135,8 +135,9 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
         try (SilentCloseable c = Profiler.instance().profile("RemoteCache.getCachedActionResult")) {
           result = remoteCache.getCachedActionResult(actionKey);
         }
-        if (result != null) {
-          // We don't cache failed actions, so we know the outputs exist.
+        if (result != null && result.getExitCode() == 0) {
+          // In case if failed action returned (exit code != 0) we treat it as a cache miss
+          // Otherwise, we know that result exists.
           try (SilentCloseable c = Profiler.instance().profile("RemoteCache.download")) {
             remoteCache.download(result, execRoot, context.getFileOutErr());
           }
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 f0625ed2a68b31..1bbe8f0c262afd 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
@@ -191,21 +191,20 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
         }
         if (cachedResult != null) {
           if (cachedResult.getExitCode() != 0) {
-            // The remote cache must never serve a failed action.
-            throw new EnvironmentalExecException(
-                "The remote cache is in an invalid state as it"
-                    + " served a failed action. Hash of the action: "
-                    + actionKey.getDigest());
-          }
-          try (SilentCloseable c = Profiler.instance().profile("Remote.downloadRemoteResults")) {
-            return downloadRemoteResults(cachedResult, context.getFileOutErr())
-                .setCacheHit(true)
-                .setRunnerName("remote cache hit")
-                .build();
-          } catch (CacheNotFoundException 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.
+            // Failed actions are treated as a cache miss mostly in order to avoid caching flaky actions (tests).
+            // Set acceptCachedResult to false in order to force the action re-execution
             acceptCachedResult = false;
+          } else {
+            try (SilentCloseable c = Profiler.instance().profile("Remote.downloadRemoteResults")) {
+              return downloadRemoteResults(cachedResult, context.getFileOutErr())
+                  .setCacheHit(true)
+                  .setRunnerName("remote cache hit")
+                  .build();
+            } catch (CacheNotFoundException 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;
+            }
           }
         }
       } catch (IOException e) {


Test patch:
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 d50adcf11d191e..da2ff6f327a5dc 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
@@ -513,4 +513,14 @@ public Void answer(InvocationOnMock invocation) {
         .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
     assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed.
   }
+
+  @Test
+  public void failedCacheActionAsCacheMiss() throws Exception {
+    ActionResult actionResult = ActionResult.newBuilder().setExitCode(1).build();
+    when(remoteCache.getCachedActionResult(any(ActionKey.class))).thenReturn(actionResult);
+
+    CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy);
+
+    assertThat(entry.hasResult()).isFalse();
+  }
 }
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 328a707a1cd7f7..07a585f52bfbff 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
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.remote;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
@@ -47,7 +46,6 @@
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
-import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
@@ -330,17 +328,14 @@ public void failedLocalActionShouldNotBeUploaded() throws Exception {
   }
 
   @Test
-  public void dontAcceptFailedCachedAction() throws Exception {
-    // Test that bazel fails if the remote cache serves a failed action.
+  @SuppressWarnings("unchecked")
+  public void treatFailedCachedActionAsCacheMiss_local() throws Exception {
+    // Test that bazel treats failed cache action as a cache miss and attempts to execute action locally
 
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
-
     ActionResult failedAction = ActionResult.newBuilder().setExitCode(1).build();
     when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(failedAction);
 
-    Spawn spawn = newSimpleSpawn();
-    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
-
     RemoteSpawnRunner runner =
         spy(
             new RemoteSpawnRunner(
@@ -349,7 +344,7 @@ public void dontAcceptFailedCachedAction() throws Exception {
                 Options.getDefaults(ExecutionOptions.class),
                 new AtomicReference<>(localRunner),
                 true,
-                /*cmdlineReporter=*/ null,
+                /* cmdlineReporter= */ null,
                 "build-req-id",
                 "command-id",
                 cache,
@@ -357,8 +352,79 @@ public void dontAcceptFailedCachedAction() throws Exception {
                 retrier,
                 digestUtil,
                 logDir));
+    Spawn spawn = newSimpleSpawn();
+    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+
+    SpawnResult succeeded =
+        new SpawnResult.Builder()
+            .setStatus(Status.SUCCESS)
+            .setExitCode(0)
+            .setRunnerName("test")
+            .build();
+    when(localRunner.exec(eq(spawn), eq(policy))).thenReturn(succeeded);
 
-    assertThrows(EnvironmentalExecException.class, () -> runner.exec(spawn, policy));
+    runner.exec(spawn, policy);
+
+    verify(localRunner).exec(eq(spawn), eq(policy));
+    verify(runner)
+        .execLocallyAndUpload(
+            eq(spawn),
+            eq(policy),
+            any(SortedMap.class),
+            eq(cache),
+            any(ActionKey.class),
+            any(Action.class),
+            any(Command.class),
+            /* uploadLocalResults= */ eq(true));
+    verify(cache)
+        .upload(
+            any(ActionKey.class),
+            any(Action.class),
+            any(Command.class),
+            any(Path.class),
+            any(Collection.class),
+            any(FileOutErr.class));
+    verify(cache, never())
+        .download(
+            any(ActionResult.class),
+            any(Path.class),
+            eq(outErr));
+  }
+
+  @Test
+  public void treatFailedCachedActionAsCacheMiss_remote() throws Exception {
+    // Test that bazel treats failed cache action as a cache miss and attempts to execute action remotely
+
+    ActionResult failedAction = ActionResult.newBuilder().setExitCode(1).build();
+    when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(failedAction);
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(
+            execRoot,
+            options,
+            Options.getDefaults(ExecutionOptions.class),
+            new AtomicReference<>(localRunner),
+            true,
+            /* cmdlineReporter= */ null,
+            "build-req-id",
+            "command-id",
+            cache,
+            executor,
+            retrier,
+            digestUtil,
+            logDir);
+
+    ExecuteResponse succeeded = ExecuteResponse.newBuilder().setResult(
+        ActionResult.newBuilder().setExitCode(0).build()).build();
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenReturn(succeeded);
+    Spawn spawn = newSimpleSpawn();
+    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+
+    runner.exec(spawn, policy);
+
+    ArgumentCaptor<ExecuteRequest> requestCaptor = ArgumentCaptor.forClass(ExecuteRequest.class);
+    verify(executor).executeRemotely(requestCaptor.capture());
+    assertThat(requestCaptor.getValue().getSkipCacheLookup()).isTrue();
   }
 
   @Test