remote: Don't upload failed action to cache. Fixes #3452

Also, restructure the code for better read- and testability.

Change-Id: Ibdd0413f89e4687b836b768a9e7d6315234cb825
PiperOrigin-RevId: 163322658

2 files changed, 72 insertions, 14 deletions

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 1943f8288a..ff24657f93 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
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.remote;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -54,7 +55,7 @@ import javax.annotation.Nullable;
 
 /** A client for the remote execution service. */
 @ThreadSafe
-final class RemoteSpawnRunner implements SpawnRunner {
+class RemoteSpawnRunner implements SpawnRunner {
   private final Path execRoot;
   private final RemoteOptions options;
   // TODO(olaola): This will be set on a per-action basis instead.
@@ -127,7 +128,8 @@ final class RemoteSpawnRunner implements SpawnRunner {
       }
 
       if (remoteExecutor == null) {
-        return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
+        return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
+            remoteCache, actionKey);
       }
 
       // Upload the command and all the inputs into the remote cache.
@@ -142,7 +144,8 @@ final class RemoteSpawnRunner implements SpawnRunner {
       ExecuteResponse reply = remoteExecutor.executeRemotely(request.build());
       result = reply.getResult();
       if (options.remoteLocalFallback && result.getExitCode() != 0) {
-        return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
+        return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
+            remoteCache, actionKey);
       }
       remoteCache.download(result, execRoot, policy.getFileOutErr());
       return new SpawnResult.Builder()
@@ -151,7 +154,8 @@ final class RemoteSpawnRunner implements SpawnRunner {
           .build();
     } catch (IOException e) {
       if (options.remoteLocalFallback) {
-        return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
+        return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
+            remoteCache, actionKey);
       }
 
       String message = "";
@@ -206,7 +210,7 @@ final class RemoteSpawnRunner implements SpawnRunner {
     return command.build();
   }
 
-  Map<Path, Long> getInputCtimes(SortedMap<PathFragment, ActionInput> inputMap) {
+  private Map<Path, Long> getInputCtimes(SortedMap<PathFragment, ActionInput> inputMap) {
     HashMap<Path, Long>  ctimes = new HashMap<>();
     for (Map.Entry<PathFragment, ActionInput> e : inputMap.entrySet()) {
       ActionInput input = e.getValue();
@@ -227,23 +231,36 @@ final class RemoteSpawnRunner implements SpawnRunner {
   }
 
   /**
-   * Fallback: execute the spawn locally. If an ActionKey is provided, try to upload results to
-   * remote action cache.
+   * Execute a {@link Spawn} locally, using {@link #fallbackRunner}.
+   *
+   * <p>If possible also upload the {@link SpawnResult} to a remote cache.
    */
   private SpawnResult execLocally(
       Spawn spawn,
       SpawnExecutionPolicy policy,
       SortedMap<PathFragment, ActionInput> inputMap,
-      RemoteActionCache remoteCache,
-      ActionKey actionKey)
-      throws ExecException, IOException, InterruptedException {
-    if (!options.remoteUploadLocalResults || !Spawns.mayBeCached(spawn) || remoteCache == null
-        || actionKey == null) {
-      // This is an optimization to not compute the ctimes in case remote upload is disabled.
-      return fallbackRunner.exec(spawn, policy);
+      boolean uploadToCache,
+      @Nullable RemoteActionCache remoteCache,
+      @Nullable ActionKey actionKey) throws ExecException, IOException, InterruptedException {
+    if (uploadToCache && Spawns.mayBeCached(spawn) && remoteCache != null && actionKey != null) {
+      return execLocallyAndUpload(spawn, policy, inputMap, remoteCache, actionKey);
     }
+    return fallbackRunner.exec(spawn, policy);
+  }
+
+  @VisibleForTesting
+  SpawnResult execLocallyAndUpload(
+      Spawn spawn,
+      SpawnExecutionPolicy policy,
+      SortedMap<PathFragment, ActionInput> inputMap,
+      RemoteActionCache remoteCache,
+      ActionKey actionKey) throws ExecException, IOException, InterruptedException {
     Map<Path, Long> ctimesBefore = getInputCtimes(inputMap);
     SpawnResult result = fallbackRunner.exec(spawn, policy);
+    if (!Status.SUCCESS.equals(result.status()) || result.exitCode() != 0) {
+      // Don't upload failed actions.
+      return result;
+    }
     Map<Path, Long> ctimesAfter = getInputCtimes(inputMap);
     for (Map.Entry<Path, Long> e : ctimesBefore.entrySet()) {
       // Skip uploading to remote cache, because an input was modified during execution.
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 a7718ff0f2..dfa344d32f 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
@@ -15,7 +15,9 @@ package com.google.devtools.build.lib.remote;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyZeroInteractions;
 import static org.mockito.Mockito.when;
@@ -30,6 +32,8 @@ import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.SimpleSpawn;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.exec.SpawnInputExpander;
+import com.google.devtools.build.lib.exec.SpawnResult;
+import com.google.devtools.build.lib.exec.SpawnResult.Status;
 import com.google.devtools.build.lib.exec.SpawnRunner;
 import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
@@ -54,6 +58,7 @@ import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 
 /** Tests for {@link com.google.devtools.build.lib.remote.RemoteSpawnRunner} */
@@ -172,6 +177,42 @@ public class RemoteSpawnRunnerTest {
         any(FileOutErr.class));
   }
 
+  @Test
+  @SuppressWarnings("unchecked")
+  public void failedActionShouldNotBeUploaded() throws Exception {
+    // Test that the outputs of a failed locally executed action are not uploaded to a remote
+    // cache.
+
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteUploadLocalResults = true;
+
+    RemoteSpawnRunner runner =
+        spy(new RemoteSpawnRunner(execRoot, options, localRunner, true, cache, null));
+
+    Spawn spawn = new SimpleSpawn(
+        new FakeOwner("foo", "bar"),
+        /*arguments=*/ ImmutableList.of(),
+        /*environment=*/ ImmutableMap.of(),
+        /*executionInfo=*/ ImmutableMap.of(),
+        /*inputs=*/ ImmutableList.of(),
+        /*outputs=*/ ImmutableList.<ActionInput>of(),
+        ResourceSet.ZERO);
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    SpawnResult res = Mockito.mock(SpawnResult.class);
+    when(res.exitCode()).thenReturn(1);
+    when(res.status()).thenReturn(Status.EXECUTION_FAILED);
+    when(localRunner.exec(eq(spawn), eq(policy))).thenReturn(res);
+
+    assertThat(runner.exec(spawn, policy)).isSameAs(res);
+
+    verify(localRunner).exec(eq(spawn), eq(policy));
+    verify(runner).execLocallyAndUpload(eq(spawn), eq(policy), any(SortedMap.class), eq(cache),
+        any(ActionKey.class));
+    verify(cache, never()).upload(any(ActionKey.class), any(Path.class), any(Collection.class),
+        any(FileOutErr.class));
+  }
+
   // TODO(buchgr): Extract a common class to be used for testing.
   class FakeSpawnExecutionPolicy implements SpawnExecutionPolicy {