Move use of legacy sandbox -> local fallback to only be used after all strategies have been tried, and improve messages around it.

RELNOTES: None.
PiperOrigin-RevId: 371873129

Author: larsrc-google

src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java

@@ -50,9 +50,23 @@ default SpawnContinuation beginExecution(
     }
   }
 
-  /** Returns whether this SpawnActionContext supports executing the given Spawn. */
+  /**
+   * Returns whether this SpawnActionContext supports executing the given Spawn. This does not allow
+   * using the legacy fallback to local execution controlled by the {@code
+   * --incompatible_legacy_local_fallback} flag.
+   */
   boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry);
 
+  /**
+   * Returns true if this SpawnActionContext supports executing the given Spawn through a legacy
+   * fallback system. This will only be used if no SpawnActionContexts were able to execute it by
+   * normal means.
+   */
+  default boolean canExecWithLegacyFallback(
+      Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) {
+    return false;
+  }
+
   /**
    * Performs any actions conditional on this strategy not only being registered but triggered as
    * used because its identifier was requested and it was not overridden.

src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java

@@ -445,7 +445,8 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
     for (SandboxedSpawnStrategy strategy :
         dynamicStrategyRegistry.getDynamicSpawnActionContexts(
             spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
-      if (strategy.canExec(spawn, actionContextRegistry)) {
+      if (strategy.canExec(spawn, actionContextRegistry)
+          || strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) {
         return true;
       }
     }
@@ -513,7 +514,8 @@ private static ImmutableList<SpawnResult> runSpawnLocally(
     for (SandboxedSpawnStrategy strategy :
         dynamicStrategyRegistry.getDynamicSpawnActionContexts(
             spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
-      if (strategy.canExec(spawn, actionExecutionContext)) {
+      if (strategy.canExec(spawn, actionExecutionContext)
+          || strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) {
         return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns);
       }
     }

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -96,6 +96,12 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
     return spawnRunner.canExec(spawn);
   }
 
+  @Override
+  public boolean canExecWithLegacyFallback(
+      Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) {
+    return spawnRunner.canExecWithLegacyFallback(spawn);
+  }
+
   @Override
   public ImmutableList<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {

src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java

@@ -269,6 +269,11 @@ SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
   /** Returns whether this SpawnRunner supports executing the given Spawn. */
   boolean canExec(Spawn spawn);
 
+  /** Returns whether this SpawnRunner supports executing the given Spawn using legacy fallbacks. */
+  default boolean canExecWithLegacyFallback(Spawn spawn) {
+    return false;
+  }
+
   /** Returns whether this SpawnRunner handles caching of actions internally. */
   boolean handlesCaching();
 

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java

@@ -88,26 +88,37 @@ public List<? extends SpawnStrategy> resolve(
             .getContext(SpawnStrategyRegistry.class)
             .getStrategies(spawn, actionExecutionContext.getEventHandler());
 
-    strategies =
+    List<? extends SpawnStrategy> execableStrategies =
         strategies.stream()
             .filter(spawnActionContext -> spawnActionContext.canExec(spawn, actionExecutionContext))
             .collect(Collectors.toList());
 
-    if (strategies.isEmpty()) {
-      String message =
-          String.format(
-              "No usable spawn strategy found for spawn with mnemonic %s.  Your"
-                  + " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably too"
-                  + " strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for"
-                  + " migration advice",
-              spawn.getMnemonic());
-      throw new UserExecException(
-          FailureDetail.newBuilder()
-              .setMessage(message)
-              .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND))
-              .build());
+    if (execableStrategies.isEmpty()) {
+      // Legacy implicit fallbacks should be a last-ditch option after all other strategies are
+      // found non-executable.
+      List<? extends SpawnStrategy> fallbackStrategies =
+          strategies.stream()
+              .filter(
+                  spawnActionContext ->
+                      spawnActionContext.canExecWithLegacyFallback(spawn, actionExecutionContext))
+              .collect(Collectors.toList());
+
+      if (fallbackStrategies.isEmpty()) {
+        String message =
+            String.format(
+                "No usable spawn strategy found for spawn with mnemonic %s.  Your --spawn_strategy,"
+                    + " --genrule_strategy and/or --strategy flags are probably too strict. Visit"
+                    + " https://github.com/bazelbuild/bazel/issues/7480 for migration advice",
+                spawn.getMnemonic());
+        throw new UserExecException(
+            FailureDetail.newBuilder()
+                .setMessage(message)
+                .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND))
+                .build());
+      }
+      return fallbackStrategies;
     }
 
-    return strategies;
+    return execableStrategies;
   }
 }

src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java

@@ -429,12 +429,11 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
   private static SpawnRunner withFallback(
       CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) {
     SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class);
-    if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) {
-      return new SandboxFallbackSpawnRunner(
-          sandboxSpawnRunner, createFallbackRunner(env), env.getReporter());
-    } else {
-      return sandboxSpawnRunner;
-    }
+    return new SandboxFallbackSpawnRunner(
+        sandboxSpawnRunner,
+        createFallbackRunner(env),
+        env.getReporter(),
+        sandboxOptions != null && sandboxOptions.legacyLocalFallback);
   }
 
   private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
@@ -451,19 +450,29 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
         RunfilesTreeUpdater.INSTANCE);
   }
 
+  /**
+   * A SpawnRunner that does sandboxing if possible, but might fall back to local execution if
+   * ----incompatible_legacy_local_fallback is true and no other strategy has been usable. This is a
+   * legacy functionality from before the strategies system was added, and can deceive the user into
+   * thinking a build is hermetic when it isn't really. TODO(b/178356138): Flip flag to default to
+   * false and then later remove this code entirely.
+   */
   private static final class SandboxFallbackSpawnRunner implements SpawnRunner {
     private final SpawnRunner sandboxSpawnRunner;
     private final SpawnRunner fallbackSpawnRunner;
     private final ExtendedEventHandler reporter;
     private static final AtomicBoolean warningEmitted = new AtomicBoolean();
+    private final boolean fallbackAllowed;
 
     SandboxFallbackSpawnRunner(
         SpawnRunner sandboxSpawnRunner,
         SpawnRunner fallbackSpawnRunner,
-        ExtendedEventHandler reporter) {
+        ExtendedEventHandler reporter,
+        boolean fallbackAllowed) {
       this.sandboxSpawnRunner = sandboxSpawnRunner;
       this.fallbackSpawnRunner = fallbackSpawnRunner;
       this.reporter = reporter;
+      this.fallbackAllowed = fallbackAllowed;
     }
 
     @Override
@@ -479,12 +488,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
       if (sandboxSpawnRunner.canExec(spawn)) {
         spawnResult = sandboxSpawnRunner.exec(spawn, context);
       } else {
-        if (warningEmitted.compareAndSet(false, true)) {
-          reporter.handle(
-              Event.warn(
-                  "Use of implicit local fallback will go away soon, please"
-                      + " set a fallback strategy instead. See --legacy_local_fallback option."));
-        }
         spawnResult = fallbackSpawnRunner.exec(spawn, context);
       }
       reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant));
@@ -493,7 +496,38 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
 
     @Override
     public boolean canExec(Spawn spawn) {
-      return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn);
+      return sandboxSpawnRunner.canExec(spawn);
+    }
+
+    @Override
+    public boolean canExecWithLegacyFallback(Spawn spawn) {
+      boolean canExec = !sandboxSpawnRunner.canExec(spawn) && fallbackSpawnRunner.canExec(spawn);
+      if (canExec) {
+        // We give a single warning to use strategies instead, whether or not we allow the fallback
+        // to happen. This allows people to switch early, but also explains why the build fails
+        // once we flip the flag. Unfortunately, we can't easily tell if the flag was explicitly
+        // set, if we could we should omit the warnings in that case.
+        if (warningEmitted.compareAndSet(false, true)) {
+          if (fallbackAllowed) {
+            reporter.handle(
+                Event.warn(
+                    String.format(
+                        "%s uses implicit fallback from sandbox to local, which is deprecated"
+                            + " because it is not hermetic. Prefer setting an explicit list of"
+                            + " strategies.",
+                        spawn.getMnemonic())));
+          } else {
+            reporter.handle(
+                Event.warn(
+                    String.format(
+                        "Implicit fallback from sandbox to local is deprecated. Prefer setting an"
+                            + " explicit list of strategies for %s, or for now pass"
+                            + " --incompatible_legacy_local_fallback.",
+                        spawn.getMnemonic())));
+          }
+        }
+      }
+      return canExec && fallbackAllowed;
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java

@@ -80,12 +80,11 @@ public String getTypeDescription() {
   }
 
   @Option(
-    name = "ignore_unsupported_sandboxing",
-    defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help = "Do not print a warning when sandboxed execution is not supported on this system."
-  )
+      name = "ignore_unsupported_sandboxing",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help = "Do not print a warning when sandboxed execution is not supported on this system.")
   public boolean ignoreUnsupportedSandboxing;
 
   @Option(
@@ -115,21 +114,19 @@ public String getTypeDescription() {
   public String sandboxBase;
 
   @Option(
-    name = "sandbox_fake_hostname",
-    defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help = "Change the current hostname to 'localhost' for sandboxed actions."
-  )
+      name = "sandbox_fake_hostname",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help = "Change the current hostname to 'localhost' for sandboxed actions.")
   public boolean sandboxFakeHostname;
 
   @Option(
-    name = "sandbox_fake_username",
-    defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help = "Change the current username to 'nobody' for sandboxed actions."
-  )
+      name = "sandbox_fake_username",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help = "Change the current username to 'nobody' for sandboxed actions.")
   public boolean sandboxFakeUsername;
 
   @Option(
@@ -187,14 +184,13 @@ public String getTypeDescription() {
   public TriState useSandboxfs;
 
   @Option(
-    name = "experimental_sandboxfs_path",
-    defaultValue = "sandboxfs",
-    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help =
-        "Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a "
-            + "bare name, use the first binary of that name found in the PATH."
-  )
+      name = "experimental_sandboxfs_path",
+      defaultValue = "sandboxfs",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a "
+              + "bare name, use the first binary of that name found in the PATH.")
   public String sandboxfsPath;
 
   @Option(
@@ -247,50 +243,48 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
   }
 
   @Option(
-    name = "experimental_collect_local_sandbox_action_metrics",
-    defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-    effectTags = {OptionEffectTag.EXECUTION},
-    help =
-        "When enabled, execution statistics (such as user and system time) are recorded for "
-            + "locally executed actions which use sandboxing"
-  )
+      name = "experimental_collect_local_sandbox_action_metrics",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {OptionEffectTag.EXECUTION},
+      help =
+          "When enabled, execution statistics (such as user and system time) are recorded for "
+              + "locally executed actions which use sandboxing")
   public boolean collectLocalSandboxExecutionStatistics;
 
   @Option(
-    name = "experimental_enable_docker_sandbox",
-    defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-    effectTags = {OptionEffectTag.EXECUTION},
-    help = "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.")
+      name = "experimental_enable_docker_sandbox",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.EXECUTION},
+      help =
+          "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.")
   public boolean enableDockerSandbox;
 
   @Option(
-    name = "experimental_docker_image",
-    defaultValue = "",
-    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-    effectTags = {OptionEffectTag.EXECUTION},
-    help =
-        "Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute "
-            + "a sandboxed action when using the docker strategy and the action itself doesn't "
-            + "already have a container-image attribute in its remote_execution_properties in the "
-            + "platform description. The value of this flag is passed verbatim to 'docker run', so "
-            + "it supports the same syntax and mechanisms as Docker itself."
-  )
+      name = "experimental_docker_image",
+      defaultValue = "",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.EXECUTION},
+      help =
+          "Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute a"
+              + " sandboxed action when using the docker strategy and the action itself doesn't"
+              + " already have a container-image attribute in its remote_execution_properties in"
+              + " the platform description. The value of this flag is passed verbatim to 'docker"
+              + " run', so it supports the same syntax and mechanisms as Docker itself.")
   public String dockerImage;
 
   @Option(
-    name = "experimental_docker_use_customized_images",
-    defaultValue = "true",
-    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-    effectTags = {OptionEffectTag.EXECUTION},
-    help =
-        "If enabled, injects the uid and gid of the current user into the Docker image before "
-            + "using it. This is required if your build / tests depend on the user having a name "
-            + "and home directory inside the container. This is on by default, but you can disable "
-            + "it in case the automatic image customization feature doesn't work in your case or "
-            + "you know that you don't need it."
-  )
+      name = "experimental_docker_use_customized_images",
+      defaultValue = "true",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.EXECUTION},
+      help =
+          "If enabled, injects the uid and gid of the current user into the Docker image before"
+              + " using it. This is required if your build / tests depend on the user having a name"
+              + " and home directory inside the container. This is on by default, but you can"
+              + " disable it in case the automatic image customization feature doesn't work in your"
+              + " case or you know that you don't need it.")
   public boolean dockerUseCustomizedImages;
 
   @Option(
@@ -359,8 +353,9 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
       },
       help =
           "If set to true, enables the legacy implicit fallback from sandboxed to local strategy."
-              + " This flag will eventually default to false and then become a no-op. You should"
-              + " use --strategy or --spawn_strategy to configure fallbacks instead.")
+              + " This flag will eventually default to false and then become a no-op. Use"
+              + " --strategy, --spawn_strategy, or --dynamic_local_strategy to configure fallbacks"
+              + " instead.")
   public boolean legacyLocalFallback;
 
   /** Converter for the number of threads used for asynchronous tree deletion. */