Add --experimental_cancel_concurrent_tests=on_failed

The existing `--experimental_cancel_concurrent_tests` flag can be used to let Bazel cancel concurrent runs of the same test as soon as one run passes with `--runs_per_test_detects_flakes`. This change turns the boolean-valued into an enum-valued flag that also accepts the value `on_failed` to instead cancel on the first failed run. This can be used to avoid reruns of flaky tests when the intention is to only detect flakes, not their probability of being flaky.

Also fix the quirk that `--experimental_cancel_concurrent_tests` has no effect with default Bazel settings since the `exclusive` test strategy is registered last and thus always wins. Perhaps surprisingly, this is the only effect of that incorrect registration order - all other `exclusive` logic is explicitly gated behind the value of `--test_strategy`.

RELNOTES[NEW]: The `--experimental_cancel_concurrent_tests` option now accepts the values `on_passed`, `on_failed` and `never` and cancels concurrent test runs on the first matching result. If enabled, it's now effective by default and no longer requires `--test_strategy=standalone` to be passed explicitly.

Closes bazelbuild#26630.

PiperOrigin-RevId: 789203343
Change-Id: I5e7838fa3562a1137987efbf2235d22b122e3e51

Author: fmeum

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.analysis.actions.LazyWriteNestedSetOfTupleAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions.CancelConcurrentTests;
 import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
 import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams.CoverageParams;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -403,9 +404,10 @@ private TestParams createTestAction()
         Artifact undeclaredOutputsDir =
             ruleContext.getPackageRelativeTreeArtifact(dir.getRelative("test.outputs"), root);
 
-        boolean cancelConcurrentTests =
+        CancelConcurrentTests cancelConcurrentTests =
             testConfiguration.runsPerTestDetectsFlakes()
-                && testConfiguration.cancelConcurrentTests();
+                ? testConfiguration.cancelConcurrentTests()
+                : CancelConcurrentTests.NEVER;
 
         boolean splitCoveragePostProcessing = testConfiguration.splitCoveragePostProcessing();
         TestRunnerAction testRunnerAction =

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java

@@ -28,18 +28,17 @@
 import java.util.List;
 import javax.annotation.Nullable;
 
-/**
- * A context for the execution of test actions ({@link TestRunnerAction}).
- */
+/** A context for the execution of test actions ({@link TestRunnerAction}). */
 public interface TestActionContext extends ActionContext {
 
   /**
    * A group of attempts for a single test shard, ran either sequentially or in parallel.
    *
-   * <p>When one attempt succeeds, threads running the other attempts get an {@link
-   * InterruptedException} and {@link #cancelled()} will in the future return true. When a thread
-   * joins an attempt group that is already cancelled, {@link InterruptedException} will be thrown
-   * on the call to {@link #register()}.
+   * <p>When one attempt matches the result specified by {@link
+   * com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions.CancelConcurrentTests},
+   * threads running the other attempts get an {@link InterruptedException} and {@link #cancelled()}
+   * will in the future return true. When a thread joins an attempt group that is already cancelled,
+   * {@link InterruptedException} will be thrown on the call to {@link #register()}.
    */
   interface AttemptGroup {
 
@@ -53,7 +52,9 @@ interface AttemptGroup {
     /** Unregisters a thread from the attempt group. */
     void unregister();
 
-    /** Signal that the attempt run by this thread has succeeded and cancel all the others. */
+    /**
+     * Signal that the attempt run by this thread has the desired result and cancel all the others.
+     */
     void cancelOthers();
 
     /** Whether the attempt group has been cancelled. */

src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java

@@ -28,12 +28,14 @@
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.analysis.test.CoverageConfiguration.CoverageOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions.CancelConcurrentTests;
 import com.google.devtools.build.lib.analysis.test.TestShardingStrategy.ShardingStrategyConverter;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.TestSize;
 import com.google.devtools.build.lib.packages.TestTimeout;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.RegexFilter;
+import com.google.devtools.common.options.BoolOrEnumConverter;
 import com.google.devtools.common.options.Converters;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDefinition;
@@ -269,16 +271,32 @@ public static class TestOptions extends FragmentOptions {
                 + "run/attempt fails gets a FLAKY status.")
     public boolean runsPerTestDetectsFlakes;
 
+    /** When to cancel concurrently running tests. */
+    public enum CancelConcurrentTests {
+      NEVER,
+      ON_FAILED,
+      ON_PASSED;
+
+      /** Converts to {@link CancelConcurrentTests}. */
+      static class Converter extends BoolOrEnumConverter<CancelConcurrentTests> {
+        public Converter() {
+          super(CancelConcurrentTests.class, "when to cancel concurrent tests", ON_PASSED, NEVER);
+        }
+      }
+    }
+
     @Option(
         name = "experimental_cancel_concurrent_tests",
-        defaultValue = "false",
+        defaultValue = "never",
+        converter = CancelConcurrentTests.Converter.class,
         documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
         effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOADING_AND_ANALYSIS},
         metadataTags = {OptionMetadataTag.EXPERIMENTAL},
         help =
-            "If true, then Blaze will cancel concurrently running tests on the first successful "
-                + "run. This is only useful in combination with --runs_per_test_detects_flakes.")
-    public boolean cancelConcurrentTests;
+            "If 'on_failed' or 'on_passed, then Blaze will cancel concurrently running tests on "
+                + "the first run with that result. This is only useful in combination with "
+                + "--runs_per_test_detects_flakes.")
+    public CancelConcurrentTests cancelConcurrentTests;
 
     @Option(
         name = "coverage_support",
@@ -446,7 +464,7 @@ public boolean runsPerTestDetectsFlakes() {
     return options.runsPerTestDetectsFlakes;
   }
 
-  public boolean cancelConcurrentTests() {
+  public CancelConcurrentTests cancelConcurrentTests() {
     return options.cancelConcurrentTests;
   }
 

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -55,6 +55,7 @@
 import com.google.devtools.build.lib.analysis.test.TestActionContext.TestAttemptResult;
 import com.google.devtools.build.lib.analysis.test.TestActionContext.TestAttemptResult.Result;
 import com.google.devtools.build.lib.analysis.test.TestActionContext.TestRunnerSpawn;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions.CancelConcurrentTests;
 import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -163,7 +164,7 @@ public class TestRunnerAction extends AbstractAction
    */
   private final Collection<String> requiredClientEnvVariables;
 
-  private final boolean cancelConcurrentTestsOnSuccess;
+  private final CancelConcurrentTests cancelConcurrentTests;
 
   private final boolean splitCoveragePostProcessing;
   private final NestedSetBuilder<Artifact> lcovMergerFilesToRun;
@@ -210,7 +211,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       BuildConfigurationValue configuration,
       String workspaceName,
       @Nullable PathFragment shExecutable,
-      boolean cancelConcurrentTestsOnSuccess,
+      CancelConcurrentTests cancelConcurrentTests,
       boolean splitCoveragePostProcessing,
       NestedSetBuilder<Artifact> lcovMergerFilesToRun,
       @Nullable Artifact lcovMergerRunfilesTree,
@@ -268,7 +269,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
             configuration.getActionEnvironment().getInheritedEnv(),
             configuration.getTestActionEnvironment().getInheritedEnv(),
             this.extraTestEnv.getInheritedEnv());
-    this.cancelConcurrentTestsOnSuccess = cancelConcurrentTestsOnSuccess;
+    this.cancelConcurrentTests = cancelConcurrentTests;
     this.splitCoveragePostProcessing = splitCoveragePostProcessing;
     this.lcovMergerFilesToRun = lcovMergerFilesToRun;
     this.lcovMergerRunfilesTree = lcovMergerRunfilesTree;
@@ -1011,15 +1012,22 @@ public ActionResult execute(
       try {
         testRunnerSpawn = testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
         attemptGroup =
-            cancelConcurrentTestsOnSuccess
+            cancelConcurrentTests != CancelConcurrentTests.NEVER
                 ? testActionContext.getAttemptGroup(getOwner(), shardNum)
                 : AttemptGroup.NOOP;
+        var cancelOnResult =
+            switch (cancelConcurrentTests) {
+              case NEVER -> null;
+              case ON_FAILED -> Result.FAILED_CAN_RETRY;
+              case ON_PASSED -> Result.PASSED;
+            };
         try {
           attemptGroup.register();
           var result =
               executeAllAttempts(
                   testRunnerSpawn,
                   testActionContext.isTestKeepGoing(),
+                  cancelOnResult,
                   attemptGroup,
                   spawnResults,
                   failedAttempts);
@@ -1198,6 +1206,7 @@ public Path getCoverageDataPath() {
   public ActionResult executeAllAttempts(
       TestRunnerSpawn testRunnerSpawn,
       boolean keepGoing,
+      @Nullable Result cancelOnResult,
       final AttemptGroup attemptGroup,
       List<SpawnResult> spawnResults,
       List<ProcessedAttemptResult> failedAttempts)
@@ -1212,9 +1221,10 @@ public ActionResult executeAllAttempts(
 
       spawnResults.addAll(result.spawnResults());
       TestAttemptResult.Result testResult = result.result();
-      if (testResult == TestAttemptResult.Result.PASSED) {
+      if (testResult == cancelOnResult) {
         attemptGroup.cancelOthers();
-      } else {
+      }
+      if (testResult != TestAttemptResult.Result.PASSED) {
         TestRunnerSpawnAndMaxAttempts nextRunnerAndAttempts =
             computeNextRunnerAndMaxAttempts(
                 testResult,

src/main/java/com/google/devtools/build/lib/rules/test/ExclusiveTestStrategy.java

@@ -31,9 +31,13 @@
  * <p>This strategy should be registered with a command line identifier of 'exclusive' which will
  * trigger behavior in SkyframeExecutor to schedule test execution sequentially after non-test
  * actions. This ensures streamed test output is not polluted by other action output.
+ *
+ * <p>Note: It's expected that this strategy is largely identical to the one it wraps. Most of the
+ * behavior specific to the 'exclusive' strategy is enabled based on the value of the <code>
+ * --test_strategy</code> flag, not instance methods of this class.
  */
 public class ExclusiveTestStrategy implements TestActionContext {
-  private TestActionContext parent;
+  private final TestActionContext parent;
 
   public ExclusiveTestStrategy(TestActionContext parent) {
     this.parent = parent;

src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java

@@ -62,9 +62,10 @@ public void registerActionContexts(
         TestStrategy.getTmpRoot(env.getWorkspace(), env.getExecRoot(), executionOptions);
     TestActionContext testStrategy =
         new StandaloneTestStrategy(executionOptions, testSummaryOptions, testTmpRoot);
-    registryBuilder.register(TestActionContext.class, testStrategy, "standalone");
+    // Keep the standalone test strategy last so that it is the default one.
     registryBuilder.register(
         TestActionContext.class, new ExclusiveTestStrategy(testStrategy), "exclusive");
+    registryBuilder.register(TestActionContext.class, testStrategy, "standalone");
     registryBuilder.register(FileWriteActionContext.class, new FileWriteStrategy(), "local");
     registryBuilder.register(
         TemplateExpansionContext.class, new LocalTemplateExpansionStrategy(), "local");

src/test/java/com/google/devtools/build/lib/exec/BUILD

@@ -37,6 +37,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/config:build_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/config:build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis/config:core_options",