Removing local fallback on failed action commands.

We should only fall back if a remote execution error occurred, not if the command itself failed.

TESTED=better unit tests
RELNOTES: None
PiperOrigin-RevId: 172406687
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 e02de94..b5534f9 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
@@ -350,7 +350,7 @@
   }
 
   @Test
-  public void fallbackFails() throws Exception {
+  public void noRemoteExecutorFallbackFails() throws Exception {
     // Errors from the fallback runner should be propogated out of the remote runner.
 
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
@@ -388,6 +388,80 @@
   }
 
   @Test
+  public void remoteCacheErrorFallbackFails() throws Exception {
+    // Errors from the fallback runner should be propogated out of the remote runner.
+
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteUploadLocalResults = true;
+    options.remoteLocalFallback = true;
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(
+            execRoot,
+            options,
+            localRunner,
+            true,
+            /*cmdlineReporter=*/ null,
+            "build-req-id",
+            "command-id",
+            cache,
+            null);
+
+    Spawn spawn = newSimpleSpawn();
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    when(cache.getCachedActionResult(any(ActionKey.class))).thenThrow(new IOException());
+
+    IOException err = new IOException("local execution error");
+    when(localRunner.exec(eq(spawn), eq(policy))).thenThrow(err);
+
+    try {
+      runner.exec(spawn, policy);
+      fail("expected IOException to be raised");
+    } catch (IOException e) {
+      assertThat(e).isSameAs(err);
+    }
+
+    verify(localRunner).exec(eq(spawn), eq(policy));
+  }
+
+  @Test
+  public void testLocalFallbackFailureRemoteExecutorFailure() throws Exception {
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteLocalFallback = true;
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(
+            execRoot,
+            options,
+            localRunner,
+            true,
+            /*cmdlineReporter=*/ null,
+            "build-req-id",
+            "command-id",
+            cache,
+            executor);
+
+    when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(null);
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new IOException());
+
+    Spawn spawn = newSimpleSpawn();
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    IOException err = new IOException("local execution error");
+    when(localRunner.exec(eq(spawn), eq(policy))).thenThrow(err);
+
+    try {
+      runner.exec(spawn, policy);
+      fail("expected IOException to be raised");
+    } catch (IOException e) {
+      assertThat(e).isSameAs(err);
+    }
+
+    verify(localRunner).exec(eq(spawn), eq(policy));
+  }
+
+  @Test
   public void cacheDownloadFailureTriggersRemoteExecution() throws Exception {
     // If downloading a cached action fails, remote execution should be tried.
 
@@ -461,6 +535,78 @@
   }
 
   @Test
+  public void testRemoteExecutionTimeoutDoesNotTriggerFallback() throws Exception {
+    // If remote execution times out the SpawnResult status should be TIMEOUT, regardess of local
+    // fallback option.
+
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteLocalFallback = true;
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(
+            execRoot,
+            options,
+            localRunner,
+            true,
+            /*cmdlineReporter=*/ null,
+            "build-req-id",
+            "command-id",
+            cache,
+            executor);
+
+    ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build();
+    when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(null);
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(new TimeoutException());
+
+    Spawn spawn = newSimpleSpawn();
+
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    SpawnResult res = runner.exec(spawn, policy);
+    assertThat(res.status()).isEqualTo(Status.TIMEOUT);
+
+    verify(executor).executeRemotely(any(ExecuteRequest.class));
+    verify(cache, never()).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class));
+    verify(localRunner, never()).exec(eq(spawn), eq(policy));
+  }
+
+  @Test
+  public void testRemoteExecutionCommandFailureDoesNotTriggerFallback() throws Exception {
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteLocalFallback = true;
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(
+            execRoot,
+            options,
+            localRunner,
+            true,
+            /*cmdlineReporter=*/ null,
+            "build-req-id",
+            "command-id",
+            cache,
+            executor);
+
+    ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build();
+    when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(null);
+    ExecuteResponse failed = ExecuteResponse.newBuilder().setResult(
+        ActionResult.newBuilder().setExitCode(33).build()).build();
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenReturn(failed);
+
+    Spawn spawn = newSimpleSpawn();
+
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    SpawnResult res = runner.exec(spawn, policy);
+    assertThat(res.status()).isEqualTo(Status.SUCCESS);  // Even though the command failed.
+    assertThat(res.exitCode()).isEqualTo(33);
+
+    verify(executor).executeRemotely(any(ExecuteRequest.class));
+    verify(cache, never()).download(eq(cachedResult), eq(execRoot), any(FileOutErr.class));
+    verify(localRunner, never()).exec(eq(spawn), eq(policy));
+  }
+
+  @Test
   public void testExitCode_executorfailure() throws Exception {
     // If we get a failure due to the remote cache not working, the exit code should be
     // ExitCode.REMOTE_ERROR.