Reimplement RemoteSpawnStrategy on top of RemoteSpawnRunner

It is intentional that RemoteSpawnStrategy no longer contains any remote
execution specific code. My intent is to merge all SpawnStrategy
implementations into a single class (similar to the new RemoteSpawnStrategy),
and delegate all the specific work to SpawnRunner implementations. However,
we're not there yet, and we still need to be able to look up SpawnStrategy
implementations by name through the annotations, so we still need separate
classes for now.

We might also want to have a shared test suite for all SpawnRunner instances
that checks for basic compliance with the specification.

Progress on #1531.

PiperOrigin-RevId: 161377751

4 files changed, 125 insertions, 216 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD
index 6d42b4320f..4df5c718e8 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib:vfs",
         "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/standalone",
         "//src/main/java/com/google/devtools/common/options",
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
index af15906e97..01d99aefd2 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -11,7 +11,6 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-
 package com.google.devtools.build.lib.remote;
 
 import com.google.common.base.Preconditions;
@@ -19,20 +18,24 @@ import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionContext;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.ResourceManager;
-import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.exec.ActionContextProvider;
 import com.google.devtools.build.lib.exec.ActionInputPrefetcher;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
+import com.google.devtools.build.lib.exec.SpawnRunner;
+import com.google.devtools.build.lib.exec.apple.XCodeLocalEnvProvider;
+import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
+import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
-import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy;
+import com.google.devtools.build.lib.util.OS;
 
 /**
  * Provide a remote execution context.
  */
 final class RemoteActionContextProvider extends ActionContextProvider {
   private final CommandEnvironment env;
+  private RemoteSpawnRunner spawnRunner;
   private RemoteSpawnStrategy spawnStrategy;
 
   RemoteActionContextProvider(CommandEnvironment env) {
@@ -43,17 +46,6 @@ final class RemoteActionContextProvider extends ActionContextProvider {
   public void init(
       ActionInputFileCache actionInputFileCache, ActionInputPrefetcher actionInputPrefetcher) {
     ExecutionOptions executionOptions = env.getOptions().getOptions(ExecutionOptions.class);
-    LocalExecutionOptions localExecutionOptions =
-        env.getOptions().getOptions(LocalExecutionOptions.class);
-    SpawnActionContext fallbackStrategy =
-        new StandaloneSpawnStrategy(
-            env.getExecRoot(),
-            actionInputPrefetcher,
-            localExecutionOptions,
-            executionOptions.verboseFailures,
-            env.getRuntime().getProductName(),
-            ResourceManager.instance());
-
     RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class);
     AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class);
     ChannelOptions channelOptions = ChannelOptions.create(authAndTlsOptions);
@@ -84,14 +76,33 @@ final class RemoteActionContextProvider extends ActionContextProvider {
     } else {
       remoteExecutor = null;
     }
+    spawnRunner = new RemoteSpawnRunner(
+        env.getExecRoot(),
+        remoteOptions,
+        createFallbackRunner(actionInputPrefetcher),
+        remoteCache,
+        remoteExecutor);
     spawnStrategy =
         new RemoteSpawnStrategy(
+            "remote",
+            spawnRunner,
+            executionOptions.verboseFailures);
+  }
+
+  private SpawnRunner createFallbackRunner(ActionInputPrefetcher actionInputPrefetcher) {
+    LocalExecutionOptions localExecutionOptions =
+        env.getOptions().getOptions(LocalExecutionOptions.class);
+    LocalEnvProvider localEnvProvider = OS.getCurrent() == OS.DARWIN
+        ? new XCodeLocalEnvProvider()
+        : LocalEnvProvider.UNMODIFIED;
+    return
+        new LocalSpawnRunner(
             env.getExecRoot(),
-            remoteOptions,
-            remoteCache,
-            remoteExecutor,
-            executionOptions.verboseFailures,
-            fallbackStrategy);
+            actionInputPrefetcher,
+            localExecutionOptions,
+            ResourceManager.instance(),
+            env.getRuntime().getProductName(),
+            localEnvProvider);
   }
 
   @Override
@@ -101,9 +112,10 @@ final class RemoteActionContextProvider extends ActionContextProvider {
 
   @Override
   public void executionPhaseEnding() {
-    if (spawnStrategy != null) {
-      spawnStrategy.close();
-      spawnStrategy = null;
+    if (spawnRunner != null) {
+      spawnRunner.close();
     }
+    spawnRunner = null;
+    spawnStrategy = null;
   }
 }
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 7b6971623c..7ba5e5edf7 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
@@ -42,6 +42,7 @@ import java.util.Collection;
 import java.util.List;
 import java.util.SortedMap;
 import java.util.TreeSet;
+import javax.annotation.Nullable;
 
 /** A client for the remote execution service. */
 @ThreadSafe
@@ -52,15 +53,15 @@ final class RemoteSpawnRunner implements SpawnRunner {
   private final Platform platform;
   private final SpawnRunner fallbackRunner;
 
-  private final RemoteActionCache remoteCache;
-  private final GrpcRemoteExecutor remoteExecutor;
+  @Nullable private final RemoteActionCache remoteCache;
+  @Nullable private final GrpcRemoteExecutor remoteExecutor;
 
   RemoteSpawnRunner(
       Path execRoot,
       RemoteOptions options,
       SpawnRunner fallbackRunner,
-      RemoteActionCache remoteCache,
-      GrpcRemoteExecutor remoteExecutor) {
+      @Nullable RemoteActionCache remoteCache,
+      @Nullable GrpcRemoteExecutor remoteExecutor) {
     this.execRoot = execRoot;
     this.options = options;
     this.platform = options.parseRemotePlatformOverride();
@@ -210,4 +211,11 @@ final class RemoteSpawnRunner implements SpawnRunner {
     }
     return result;
   }
+
+  /** Release resources associated with this spawn runner. */
+  public void close() {
+    if (remoteCache != null) {
+      remoteCache.close();
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
index 15545bdc44..9a1c415589 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
@@ -11,42 +11,35 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-
 package com.google.devtools.build.lib.remote;
 
-import com.google.common.collect.ImmutableMap;
+import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.ActionStatusMessage;
+import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.Spawns;
-import com.google.devtools.build.lib.actions.UserExecException;
-import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.exec.SpawnExecException;
 import com.google.devtools.build.lib.exec.SpawnInputExpander;
-import com.google.devtools.build.lib.remote.Digests.ActionKey;
-import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
+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;
 import com.google.devtools.build.lib.rules.fileset.FilesetActionContext;
+import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy;
 import com.google.devtools.build.lib.util.CommandFailureUtils;
-import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.remoteexecution.v1test.Action;
-import com.google.devtools.remoteexecution.v1test.ActionResult;
-import com.google.devtools.remoteexecution.v1test.Command;
-import com.google.devtools.remoteexecution.v1test.Digest;
-import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
-import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
-import com.google.devtools.remoteexecution.v1test.Platform;
-import com.google.protobuf.Duration;
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
 import java.util.SortedMap;
-import java.util.TreeSet;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Strategy that uses a distributed cache for sharing action input and output files. Optionally this
@@ -57,99 +50,15 @@ import java.util.TreeSet;
   contextType = SpawnActionContext.class
 )
 final class RemoteSpawnStrategy implements SpawnActionContext {
-  private final Path execRoot;
-  private final SpawnActionContext fallbackStrategy;
+  private final SpawnInputExpander spawnInputExpander = new SpawnInputExpander(/*strict=*/false);
+  private final String strategyName;
+  private final SpawnRunner spawnRunner;
   private final boolean verboseFailures;
-  private final RemoteOptions remoteOptions;
-  // TODO(olaola): This will be set on a per-action basis instead.
-  private final Platform platform;
-  private final SpawnInputExpander spawnInputExpander = new SpawnInputExpander(/*strict=*/ false);
-
-  private final RemoteActionCache remoteCache;
-  private final GrpcRemoteExecutor workExecutor;
 
-  RemoteSpawnStrategy(
-      Path execRoot,
-      RemoteOptions remoteOptions,
-      RemoteActionCache remoteCache,
-      GrpcRemoteExecutor remoteExecutor,
-      boolean verboseFailures,
-      SpawnActionContext fallbackStrategy) {
-    this.execRoot = execRoot;
-    this.fallbackStrategy = fallbackStrategy;
+  RemoteSpawnStrategy(String strategyName, SpawnRunner spawnRunner, boolean verboseFailures) {
+    this.strategyName = strategyName;
+    this.spawnRunner = spawnRunner;
     this.verboseFailures = verboseFailures;
-    this.remoteOptions = remoteOptions;
-    this.platform = remoteOptions.parseRemotePlatformOverride();
-    this.remoteCache = remoteCache;
-    this.workExecutor = remoteExecutor;
-  }
-
-  /** Release resources associated with this spawn strategy. */
-  public void close() {
-    if (remoteCache != null) {
-      remoteCache.close();
-    }
-  }
-
-  private Action buildAction(
-      Collection<? extends ActionInput> outputs,
-      Digest command,
-      Digest inputRoot,
-      long timeoutSeconds) {
-    Action.Builder action = Action.newBuilder();
-    action.setCommandDigest(command);
-    action.setInputRootDigest(inputRoot);
-    // Somewhat ugly: we rely on the stable order of outputs here for remote action caching.
-    for (ActionInput output : outputs) {
-      // TODO: output directories should be handled here, when they are supported.
-      action.addOutputFiles(output.getExecPathString());
-    }
-    if (platform != null) {
-      action.setPlatform(platform);
-    }
-    if (timeoutSeconds > 0) {
-      action.setTimeout(Duration.newBuilder().setSeconds(timeoutSeconds));
-    }
-    return action.build();
-  }
-
-  private Command buildCommand(List<String> arguments, ImmutableMap<String, String> environment) {
-    Command.Builder command = Command.newBuilder();
-    command.addAllArguments(arguments);
-    // Sorting the environment pairs by variable name.
-    TreeSet<String> variables = new TreeSet<>(environment.keySet());
-    for (String var : variables) {
-      command.addEnvironmentVariablesBuilder().setName(var).setValue(environment.get(var));
-    }
-    return command.build();
-  }
-
-  /**
-   * Fallback: execute the spawn locally. If an ActionKey is provided, try to upload results to
-   * remote action cache.
-   */
-  private void execLocally(
-      Spawn spawn,
-      ActionExecutionContext actionExecutionContext,
-      RemoteActionCache remoteCache,
-      ActionKey actionKey)
-      throws ExecException, IOException, InterruptedException {
-    fallbackStrategy.exec(spawn, actionExecutionContext);
-    if (remoteOptions.remoteUploadLocalResults && remoteCache != null && actionKey != null) {
-      ArrayList<Path> outputFiles = new ArrayList<>();
-      for (ActionInput output : spawn.getOutputFiles()) {
-        Path outputFile = execRoot.getRelative(output.getExecPathString());
-        // Ignore non-existent files.
-        // TODO(ulfjack): This is not ideal - in general, all spawn strategies should stat the
-        // output files and return a list of existing files. We shouldn't re-stat the files here.
-        if (!outputFile.exists()) {
-          continue;
-        }
-        outputFiles.add(outputFile);
-      }
-      remoteCache.upload(
-          actionKey, execRoot, outputFiles, actionExecutionContext.getFileOutErr());
-    }
   }
 
   @Override
@@ -157,109 +66,88 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
     return "remote";
   }
 
-  /** Executes the given {@code spawn}. */
   @Override
-  public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
+  public void exec(final Spawn spawn, final ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
-    String mnemonic = spawn.getMnemonic();
-
-    if (!spawn.isRemotable() || remoteCache == null) {
-      fallbackStrategy.exec(spawn, actionExecutionContext);
+    if (!spawn.isRemotable()) {
+      StandaloneSpawnStrategy standaloneStrategy =
+          Preconditions.checkNotNull(
+              actionExecutionContext.getContext(StandaloneSpawnStrategy.class));
+      standaloneStrategy.exec(spawn, actionExecutionContext);
       return;
     }
+
     if (actionExecutionContext.reportsSubcommands()) {
       actionExecutionContext.reportSubcommand(spawn);
     }
-    actionExecutionContext
-        .getEventBus()
-        .post(ActionStatusMessage.runningStrategy(spawn.getResourceOwner(), "remote"));
+    SpawnExecutionPolicy policy = new SpawnExecutionPolicy() {
+      @Override
+      public ActionInputFileCache getActionInputFileCache() {
+        return actionExecutionContext.getActionInputFileCache();
+      }
 
-    ActionKey actionKey = null;
-    try {
-      // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
-      ActionInputFileCache inputFileCache = actionExecutionContext.getActionInputFileCache();
-      TreeNodeRepository repository = new TreeNodeRepository(execRoot, inputFileCache);
-      SortedMap<PathFragment, ActionInput> inputMap =
-          spawnInputExpander.getInputMapping(
-              spawn,
-              actionExecutionContext.getArtifactExpander(),
-              actionExecutionContext.getActionInputFileCache(),
-              actionExecutionContext.getContext(FilesetActionContext.class));
-      TreeNode inputRoot = repository.buildFromActionInputs(inputMap);
-      repository.computeMerkleDigests(inputRoot);
-      Command command = buildCommand(spawn.getArguments(), spawn.getEnvironment());
-      Action action =
-          buildAction(
-              spawn.getOutputFiles(),
-              Digests.computeDigest(command),
-              repository.getMerkleDigest(inputRoot),
-              Spawns.getTimeoutSeconds(spawn));
+      @Override
+      public void lockOutputFiles() throws InterruptedException {
+        // This is only needed for the dynamic spawn strategy, which we still need to actually
+        // implement.
+      }
 
-      // Look up action cache, and reuse the action output if it is found.
-      actionKey = Digests.computeActionKey(action);
-      ActionResult result =
-          this.remoteOptions.remoteAcceptCached
-              ? remoteCache.getCachedActionResult(actionKey)
-              : null;
-      boolean acceptCachedResult = this.remoteOptions.remoteAcceptCached;
-      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.
+      @Override
+      public long getTimeoutMillis() {
         try {
-          remoteCache.download(result, execRoot, actionExecutionContext.getFileOutErr());
-          return;
-        } catch (CacheNotFoundException e) {
-          acceptCachedResult = false; // Retry the action remotely and invalidate the results.
+          return TimeUnit.SECONDS.toMillis(Spawns.getTimeoutSeconds(spawn));
+        } catch (ExecException e) {
+          // The exec info is set internally, so we can never fail to parse the timeout.
+          throw new RuntimeException(e);
         }
       }
 
-      if (workExecutor == null) {
-        execLocally(spawn, actionExecutionContext, remoteCache, actionKey);
-        return;
+      @Override
+      public FileOutErr getFileOutErr() {
+        return actionExecutionContext.getFileOutErr();
       }
 
-      // 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(remoteOptions.remoteInstanceName)
-              .setAction(action)
-              .setTotalInputFileCount(inputMap.size())
-              .setSkipCacheLookup(!acceptCachedResult);
-      ExecuteResponse reply = workExecutor.executeRemotely(request.build());
-      result = reply.getResult();
-      if (remoteOptions.remoteLocalFallback && result.getExitCode() != 0) {
-        execLocally(spawn, actionExecutionContext, remoteCache, actionKey);
-        return;
+      @Override
+      public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException {
+        return spawnInputExpander.getInputMapping(
+            spawn,
+            actionExecutionContext.getArtifactExpander(),
+            actionExecutionContext.getActionInputFileCache(),
+            actionExecutionContext.getContext(FilesetActionContext.class));
       }
-      remoteCache.download(result, execRoot, actionExecutionContext.getFileOutErr());
-      if (result.getExitCode() != 0) {
-        String cwd = actionExecutionContext.getExecRoot().getPathString();
-        String message =
-            CommandFailureUtils.describeCommandFailure(
-                verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
-        throw new UserExecException(message + ": Exit " + result.getExitCode());
-      }
-    } catch (CacheNotFoundException | IOException e) {
-      Exception reportedCause = e;
-      // TODO(olaola): handle this exception by reuploading / reexecuting the action remotely.
-      if (remoteOptions.remoteLocalFallback) {
-        actionExecutionContext.getEventHandler().handle(
-            Event.warn(mnemonic + " remote work failed (" + e + ")"));
-        try {
-          execLocally(spawn, actionExecutionContext, remoteCache, actionKey);
-          return;
-        } catch (IOException e2) {
-          reportedCause = e2;
+
+      @Override
+      public void report(ProgressStatus state) {
+        EventBus eventBus = actionExecutionContext.getEventBus();
+        switch (state) {
+          case EXECUTING:
+            eventBus.post(
+                ActionStatusMessage.runningStrategy(spawn.getResourceOwner(), strategyName));
+            break;
+          case SCHEDULING:
+            eventBus.post(ActionStatusMessage.schedulingStrategy(spawn.getResourceOwner()));
+            break;
+          default:
+            break;
         }
       }
+    };
+
+    SpawnResult result;
+    try {
+      result = spawnRunner.exec(spawn, policy);
+    } catch (IOException e) {
+      throw new EnvironmentalExecException("Unexpected IO error.", e);
+    }
+
+    if ((result.status() != Status.SUCCESS) || (result.exitCode() != 0)) {
+      // TODO(ulfjack): Return SpawnResult from here and let the upper layers worry about error
+      // handling and reporting.
       String cwd = actionExecutionContext.getExecRoot().getPathString();
       String message =
           CommandFailureUtils.describeCommandFailure(
               verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
-      throw new UserExecException(message, reportedCause);
+      throw new SpawnExecException(message, result, /*catastrophe=*/ false);
     }
   }