remote: Refactor RemoteSpawnRunner to distinquish between remote

executor, remote cache and local executor errors.

This is a no-op refactoring and clears the way to integrate a change
that no longer uploads to the remote cache if a previous remote cache
interaction failed.

RELNOTES: None.
PiperOrigin-RevId: 164966432

1 files changed, 90 insertions, 52 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 f6d79ad45c..f09d496a31 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
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.exec.SpawnResult.Status;
 import com.google.devtools.build.lib.exec.SpawnRunner;
 import com.google.devtools.build.lib.remote.Digests.ActionKey;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
+import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.remoteexecution.v1test.Action;
@@ -107,73 +108,110 @@ class RemoteSpawnRunner implements SpawnRunner {
 
     // Look up action cache, and reuse the action output if it is found.
     ActionKey actionKey = Digests.computeActionKey(action);
+    boolean acceptCachedResult = options.remoteAcceptCached && Spawns.mayBeCached(spawn);
+    boolean uploadLocalResults = options.remoteUploadLocalResults;
+
     try {
-      boolean acceptCachedResult = options.remoteAcceptCached && Spawns.mayBeCached(spawn);
-      ActionResult result =
+      // Try to lookup the action in the action cache.
+      ActionResult cachedResult =
           acceptCachedResult
               ? remoteCache.getCachedActionResult(actionKey)
               : null;
-      if (result != null) {
-        // We don't cache failed actions, so we know the outputs exist.
-        // For now, download all outputs locally; in the future, we can reuse the digests to
-        // just update the TreeNodeRepository and continue the build.
+      if (cachedResult != null) {
         try {
-          remoteCache.download(result, execRoot, policy.getFileOutErr());
-          return new SpawnResult.Builder()
-              .setStatus(Status.SUCCESS)  // Even if the action failed with non-zero exit code.
-              .setExitCode(result.getExitCode())
-              .build();
+          return downloadRemoteResults(cachedResult, policy.getFileOutErr());
         } catch (CacheNotFoundException e) {
-          acceptCachedResult = false; // Retry the action remotely and invalidate the results.
+          // Intentionally left empty. No cache hit, so we fall through to local or
+          // remote execution.
         }
       }
+    } catch (IOException e) {
+      return execLocallyOrFail(spawn, policy, inputMap, actionKey,
+          options.remoteUploadLocalResults, e);
+    }
 
-      if (remoteExecutor == null) {
-        return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
-            remoteCache, actionKey);
-      }
+    if (remoteExecutor == null) {
+      // Remote execution is disabled and so execute the spawn on the local machine.
+      return execLocally(spawn, policy, inputMap, uploadLocalResults, remoteCache, actionKey);
+    }
 
+    try {
       // Upload the command and all the inputs into the remote cache.
       remoteCache.ensureInputsPresent(repository, execRoot, inputRoot, command);
-      // TODO(olaola): set BuildInfo and input total bytes as well.
-      ExecuteRequest.Builder request =
-          ExecuteRequest.newBuilder()
-              .setInstanceName(options.remoteInstanceName)
-              .setAction(action)
-              .setTotalInputFileCount(inputMap.size())
-              .setSkipCacheLookup(!acceptCachedResult);
-      ExecuteResponse reply = remoteExecutor.executeRemotely(request.build());
-      result = reply.getResult();
-      if (options.remoteLocalFallback && result.getExitCode() != 0) {
-        return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
-            remoteCache, actionKey);
-      }
-      remoteCache.download(result, execRoot, policy.getFileOutErr());
-      return new SpawnResult.Builder()
-          .setStatus(Status.SUCCESS)  // Even if the action failed with non-zero exit code.
-          .setExitCode(result.getExitCode())
-          .build();
     } catch (IOException e) {
-      if (options.remoteLocalFallback) {
-        return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
-            remoteCache, actionKey);
-      }
+      return execLocallyOrFail(spawn, policy, inputMap, actionKey,
+          options.remoteUploadLocalResults, e);
+    }
 
-      String message = "";
-      if (e instanceof RetryException
-          && ((RetryException) e).causedByStatusCode(Code.UNAVAILABLE)) {
-        message = "The remote executor/cache is unavailable";
-      } else if (e instanceof CacheNotFoundException) {
-        message = "Failed to download from remote cache";
-      } else {
-        message = "Error in remote cache/executor";
-      }
-      // TODO(olaola): reuse the ErrorMessage class for these errors.
-      if (verboseFailures) {
-        message += "\n" + Throwables.getStackTraceAsString(e);
-      }
-      throw new EnvironmentalExecException(message, e, true);
+    final ActionResult result;
+    try {
+      result = executeRemotely(action, inputMap.size(), acceptCachedResult);
+    } catch (IOException e) {
+      return execLocallyOrFail(spawn, policy, inputMap, actionKey, uploadLocalResults, e);
+    }
+
+    boolean executionFailed = result.getExitCode() != 0;
+    if (options.remoteLocalFallback && executionFailed) {
+      return execLocally(spawn, policy, inputMap, options.remoteUploadLocalResults,
+          remoteCache, actionKey);
+    }
+
+    try {
+      return downloadRemoteResults(result, policy.getFileOutErr());
+    } catch (IOException e) {
+      return execLocallyOrFail(spawn, policy, inputMap, actionKey,
+          options.remoteUploadLocalResults, e);
+    }
+  }
+
+  private SpawnResult downloadRemoteResults(ActionResult result, FileOutErr outErr)
+      throws ExecException, IOException, InterruptedException {
+    remoteCache.download(result, execRoot, outErr);
+    return new SpawnResult.Builder()
+        .setStatus(Status.SUCCESS)  // Even if the action failed with non-zero exit code.
+        .setExitCode(result.getExitCode())
+        .build();
+  }
+
+  private ActionResult executeRemotely(Action action, int numInputFiles,
+      boolean acceptCachedResult) throws IOException, InterruptedException {
+    // TODO(olaola): set BuildInfo and input total bytes as well.
+    ExecuteRequest.Builder request =
+        ExecuteRequest.newBuilder()
+            .setInstanceName(options.remoteInstanceName)
+            .setAction(action)
+            .setTotalInputFileCount(numInputFiles)
+            .setSkipCacheLookup(!acceptCachedResult);
+    ExecuteResponse reply = remoteExecutor.executeRemotely(request.build());
+    return reply.getResult();
+  }
+
+  private SpawnResult execLocallyOrFail(Spawn spawn, SpawnExecutionPolicy policy,
+      SortedMap<PathFragment, ActionInput> inputMap, ActionKey actionKey,
+      boolean uploadLocalResults, IOException cause)
+      throws ExecException, InterruptedException, IOException {
+    if (options.remoteLocalFallback) {
+      return execLocally(spawn, policy, inputMap, uploadLocalResults,
+          remoteCache, actionKey);
+    }
+    throw new EnvironmentalExecException(errorMessage(cause), cause, true);
+  }
+
+  private String errorMessage(IOException e) {
+    String message = "";
+    if (e instanceof RetryException
+        && ((RetryException) e).causedByStatusCode(Code.UNAVAILABLE)) {
+      message = "The remote executor/cache is unavailable";
+    } else if (e instanceof CacheNotFoundException) {
+      message = "Failed to download from remote cache";
+    } else {
+      message = "Error in remote cache/executor";
+    }
+    // TODO(olaola): reuse the ErrorMessage class for these errors.
+    if (verboseFailures) {
+      message += "\n" + Throwables.getStackTraceAsString(e);
     }
+    return message;
   }
 
   static Action buildAction(