Remote: Register "remote" strategy even if remote execution is not av…

…ailable.

So that you can set a `--spawn_strategy` that includes `remote` without setting `--remote_executor`. In addition, if `--remote_local_fallback` is set and `remote_executor` is unreachable when build starts, Bazel will fallback to next available strategy.

Fixes #13340 and #13487.

Closes #13490.

PiperOrigin-RevId: 378339904

src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java

@@ -42,7 +42,7 @@
 final class RemoteActionContextProvider implements ExecutorLifecycleListener {
 
   private final CommandEnvironment env;
-  private final RemoteCache cache;
+  @Nullable private final RemoteCache cache;
   @Nullable private final RemoteExecutionClient executor;
   @Nullable private final ListeningScheduledExecutorService retryScheduler;
   private final DigestUtil digestUtil;
@@ -52,19 +52,27 @@ final class RemoteActionContextProvider implements ExecutorLifecycleListener {
 
   private RemoteActionContextProvider(
       CommandEnvironment env,
-      RemoteCache cache,
+      @Nullable RemoteCache cache,
       @Nullable RemoteExecutionClient executor,
       @Nullable ListeningScheduledExecutorService retryScheduler,
       DigestUtil digestUtil,
       @Nullable Path logDir) {
     this.env = Preconditions.checkNotNull(env, "env");
-    this.cache = Preconditions.checkNotNull(cache, "cache");
+    this.cache = cache;
     this.executor = executor;
     this.retryScheduler = retryScheduler;
     this.digestUtil = digestUtil;
     this.logDir = logDir;
   }
 
+  public static RemoteActionContextProvider createForPlaceholder(
+      CommandEnvironment env,
+      ListeningScheduledExecutorService retryScheduler,
+      DigestUtil digestUtil) {
+    return new RemoteActionContextProvider(
+        env, /*cache=*/ null, /*executor=*/ null, retryScheduler, digestUtil, /*logDir=*/ null);
+  }
+
   public static RemoteActionContextProvider createForRemoteCaching(
       CommandEnvironment env,
       RemoteCache cache,
@@ -125,12 +133,7 @@ private RemoteExecutionService getRemoteExecutionService() {
    *
    * @param registryBuilder builder with which to register the strategy
    */
-  public void registerRemoteSpawnStrategyIfApplicable(
-      SpawnStrategyRegistry.Builder registryBuilder) {
-    if (executor == null) {
-      return; // Can't use a spawn strategy without executor.
-    }
-
+  public void registerRemoteSpawnStrategy(SpawnStrategyRegistry.Builder registryBuilder) {
     boolean verboseFailures =
         checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures;
     RemoteSpawnRunner spawnRunner =
@@ -168,6 +171,10 @@ RemoteCache getRemoteCache() {
     return cache;
   }
 
+  RemoteExecutionClient getRemoteExecutionClient() {
+    return executor;
+  }
+
   void setFilesToDownload(ImmutableSet<ActionInput> topLevelOutputs) {
     this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload");
   }
@@ -181,7 +188,9 @@ public void executionPhaseStarting(
 
   @Override
   public void executionPhaseEnding() {
-    cache.close();
+    if (cache != null) {
+      cache.close();
+    }
     if (executor != null) {
       executor.close();
     }

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -14,6 +14,8 @@
 package com.google.devtools.build.lib.remote;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
 import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath;
 import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload;
@@ -29,7 +31,6 @@
 import build.bazel.remote.execution.v2.LogFile;
 import build.bazel.remote.execution.v2.Platform;
 import build.bazel.remote.execution.v2.RequestMetadata;
-import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -82,7 +83,7 @@ public class RemoteExecutionService {
   private final String commandId;
   private final DigestUtil digestUtil;
   private final RemoteOptions remoteOptions;
-  private final RemoteCache remoteCache;
+  @Nullable private final RemoteCache remoteCache;
   @Nullable private final RemoteExecutionClient remoteExecutor;
   private final ImmutableSet<ActionInput> filesToDownload;
 
@@ -93,7 +94,7 @@ public RemoteExecutionService(
       String commandId,
       DigestUtil digestUtil,
       RemoteOptions remoteOptions,
-      RemoteCache remoteCache,
+      @Nullable RemoteCache remoteCache,
       @Nullable RemoteExecutionClient remoteExecutor,
       ImmutableSet<ActionInput> filesToDownload) {
     this.execRoot = execRoot;
@@ -214,6 +215,21 @@ public NetworkTime getNetworkTime() {
     }
   }
 
+  /** Returns {@code true} if the result of spawn may be cached remotely. */
+  public boolean mayBeCachedRemotely(Spawn spawn) {
+    return remoteCache != null && Spawns.mayBeCached(spawn) && Spawns.mayBeCachedRemotely(spawn);
+  }
+
+  /** Returns {@code true} if the result of spawn may be cached. */
+  public boolean mayBeCached(Spawn spawn) {
+    return remoteCache != null && Spawns.mayBeCached(spawn);
+  }
+
+  /** Returns {@code true} if the spawn may be executed remotely. */
+  public boolean mayBeExecutedRemotely(Spawn spawn) {
+    return remoteCache != null && remoteExecutor != null && Spawns.mayBeExecutedRemotely(spawn);
+  }
+
   /** Creates a new {@link RemoteAction} instance from spawn. */
   public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context)
       throws IOException, UserExecException, ForbiddenActionInputException {
@@ -335,6 +351,7 @@ public ExecuteResponse getResponse() {
   @Nullable
   public RemoteActionResult lookupCache(RemoteAction action)
       throws IOException, InterruptedException {
+    checkNotNull(remoteCache, "remoteCache can't be null");
     ActionResult actionResult =
         remoteCache.downloadActionResult(
             action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false);
@@ -350,6 +367,7 @@ public RemoteActionResult lookupCache(RemoteAction action)
   @Nullable
   public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult result)
       throws InterruptedException, IOException, ExecException {
+    checkNotNull(remoteCache, "remoteCache can't be null");
     RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
     boolean downloadOutputs =
         shouldDownloadAllSpawnOutputs(
@@ -384,6 +402,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
   /** Upload outputs of a remote action which was executed locally to remote cache. */
   public void uploadOutputs(RemoteAction action)
       throws InterruptedException, IOException, ExecException {
+    checkNotNull(remoteCache, "remoteCache can't be null");
     Collection<Path> outputFiles =
         action.spawn.getOutputFiles().stream()
             .map((inp) -> execRoot.getRelative(inp.getExecPath()))
@@ -405,7 +424,8 @@ public void uploadOutputs(RemoteAction action)
    */
   public void uploadInputsIfNotPresent(RemoteAction action)
       throws IOException, InterruptedException {
-    Preconditions.checkState(remoteCache instanceof RemoteExecutionCache);
+    checkNotNull(remoteCache, "remoteCache can't be null");
+    checkState(remoteCache instanceof RemoteExecutionCache);
     RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache;
     // Upload the command and all the inputs into the remote cache.
     Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
@@ -424,7 +444,7 @@ public void uploadInputsIfNotPresent(RemoteAction action)
   public RemoteActionResult execute(
       RemoteAction action, boolean acceptCachedResult, OperationObserver observer)
       throws IOException, InterruptedException {
-    Preconditions.checkNotNull(remoteExecutor, "remoteExecutor");
+    checkNotNull(remoteExecutor, "remoteExecutor can't be null");
 
     ExecuteRequest.Builder requestBuilder =
         ExecuteRequest.newBuilder()
@@ -458,6 +478,7 @@ public static class ServerLogs {
   /** Downloads server logs from a remotely executed action if any. */
   public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse resp, Path logDir)
       throws InterruptedException, IOException {
+    checkNotNull(remoteCache, "remoteCache can't be null");
     ServerLogs serverLogs = new ServerLogs();
     serverLogs.directory = logDir.getRelative(action.getActionId());
 

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -269,6 +269,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
 
     if (!enableDiskCache && !enableHttpCache && !enableGrpcCache && !enableRemoteExecution) {
       // Quit if no remote caching or execution was enabled.
+      actionContextProvider =
+          RemoteActionContextProvider.createForPlaceholder(env, retryScheduler, digestUtil);
       return;
     }
 
@@ -459,6 +461,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
           errorMessage += System.lineSeparator() + Throwables.getStackTraceAsString(e);
         }
         env.getReporter().handle(Event.warn(errorMessage));
+        actionContextProvider =
+            RemoteActionContextProvider.createForPlaceholder(env, retryScheduler, digestUtil);
         return;
       } else {
         if (verboseFailures) {
@@ -779,7 +783,7 @@ public void registerSpawnStrategies(
             env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
     registryBuilder.setRemoteLocalFallbackStrategyIdentifier(
         remoteOptions.remoteLocalFallbackStrategy);
-    actionContextProvider.registerRemoteSpawnStrategyIfApplicable(registryBuilder);
+    actionContextProvider.registerRemoteSpawnStrategy(registryBuilder);
   }
 
   @Override
@@ -806,7 +810,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
         Preconditions.checkNotNull(
             env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
     RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
-    if (!remoteOutputsMode.downloadAllOutputs()) {
+    if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) {
       actionInputFetcher =
           new RemoteActionInputFetcher(
               env.getBuildRequestId(),

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java

@@ -27,7 +27,6 @@
 import com.google.devtools.build.lib.actions.SpawnMetrics;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
-import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
@@ -78,8 +77,10 @@ final class RemoteSpawnCache implements SpawnCache {
   @Override
   public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       throws InterruptedException, IOException, ExecException, ForbiddenActionInputException {
-    if (!Spawns.mayBeCached(spawn)
-        || (!Spawns.mayBeCachedRemotely(spawn) && useRemoteCache(options))) {
+    boolean mayBeCached =
+        remoteExecutionService.mayBeCachedRemotely(spawn)
+            || (!useRemoteCache(options) && remoteExecutionService.mayBeCached(spawn));
+    if (!mayBeCached) {
       // returning SpawnCache.NO_RESULT_NO_STORE in case the caching is disabled or in case
       // the remote caching is disabled and the only configured cache is remote.
       return SpawnCache.NO_RESULT_NO_STORE;

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java

@@ -359,7 +359,7 @@ private SpawnResult downloadAndFinalizeSpawnResult(
 
   @Override
   public boolean canExec(Spawn spawn) {
-    return Spawns.mayBeExecutedRemotely(spawn);
+    return remoteExecutionService.mayBeExecutedRemotely(spawn);
   }
 
   @Override

src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java

@@ -430,13 +430,54 @@ public void getCapabilities(
       remoteModule.beforeCommand(env);
 
       assertThat(Thread.interrupted()).isFalse();
-      assertThat(remoteModule.getActionContextProvider()).isNull();
+      RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider();
+      assertThat(actionContextProvider).isNotNull();
+      assertThat(actionContextProvider.getRemoteCache()).isNull();
+      assertThat(actionContextProvider.getRemoteExecutionClient()).isNull();
     } finally {
       cacheServer.shutdownNow();
       cacheServer.awaitTermination();
     }
   }
 
+  @Test
+  public void testLocalFallback_shouldIgnoreInaccessibleGrpcRemoteExecutor() throws Exception {
+    CapabilitiesImplBase executionServerCapabilitiesImpl =
+        new CapabilitiesImplBase() {
+          @Override
+          public void getCapabilities(
+              GetCapabilitiesRequest request, StreamObserver<ServerCapabilities> responseObserver) {
+            responseObserver.onError(new UnsupportedOperationException());
+          }
+        };
+    String executionServerName = "execution-server";
+    Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl);
+    executionServer.start();
+
+    try {
+      RemoteModule remoteModule = new RemoteModule();
+      RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
+      remoteOptions.remoteExecutor = executionServerName;
+      remoteOptions.remoteLocalFallback = true;
+      remoteModule.setChannelFactory(
+          (target, proxy, options, interceptors) ->
+              InProcessChannelBuilder.forName(target).directExecutor().build());
+
+      CommandEnvironment env = createTestCommandEnvironment(remoteOptions);
+
+      remoteModule.beforeCommand(env);
+
+      assertThat(Thread.interrupted()).isFalse();
+      RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider();
+      assertThat(actionContextProvider).isNotNull();
+      assertThat(actionContextProvider.getRemoteCache()).isNull();
+      assertThat(actionContextProvider.getRemoteExecutionClient()).isNull();
+    } finally {
+      executionServer.shutdownNow();
+      executionServer.awaitTermination();
+    }
+  }
+
   @Test
   public void testNetrc_emptyEnv_shouldIgnore() throws Exception {
     Map<String, String> clientEnv = ImmutableMap.of();

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -467,6 +467,54 @@ EOF
       && fail "Expected failure" || true
 }
 
+function test_local_fallback_if_no_remote_executor() {
+  # Test that when manually set --spawn_strategy that includes remote, but remote_executor isn't set, we ignore
+  # the remote strategy rather than reporting an error. See https://github.com/bazelbuild/bazel/issues/13340.
+  mkdir -p gen1
+  cat > gen1/BUILD <<'EOF'
+genrule(
+name = "gen1",
+srcs = [],
+outs = ["out1"],
+cmd = "touch \"$@\"",
+)
+EOF
+
+  bazel build \
+      --spawn_strategy=remote,local \
+      --build_event_text_file=gen1.log \
+      //gen1 >& $TEST_log \
+      || fail "Expected success"
+
+  mv gen1.log $TEST_log
+  expect_log "2 processes: 1 internal, 1 local"
+}
+
+function test_local_fallback_if_remote_executor_unavailable() {
+  # Test that when --remote_local_fallback is set and remote_executor is unavailable when build starts, we fallback to
+  # local strategy. See https://github.com/bazelbuild/bazel/issues/13487.
+  mkdir -p gen1
+  cat > gen1/BUILD <<'EOF'
+genrule(
+name = "gen1",
+srcs = [],
+outs = ["out1"],
+cmd = "touch \"$@\"",
+)
+EOF
+
+  bazel build \
+      --spawn_strategy=remote,local \
+      --remote_executor=grpc://noexist.invalid \
+      --remote_local_fallback \
+      --build_event_text_file=gen1.log \
+      //gen1 >& $TEST_log \
+      || fail "Expected success"
+
+  mv gen1.log $TEST_log
+  expect_log "2 processes: 1 internal, 1 local"
+}
+
 function is_file_uploaded() {
   h=$(shasum -a256 < $1)
   if [ -e "$cas_path/${h:0:64}" ]; then return 0; else return 1; fi