Fix rare crash in dynamic execution where both branches got cancelled.

Races in `stopBranch` caused the losing branch to throw `DynamicInterruptedException`, which could then get to that branch's listener before the winning branch actually cancelled.

RELNOTES: None.
PiperOrigin-RevId: 383126912

Author: larsrc-google

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

@@ -268,10 +268,7 @@ private static ImmutableList<SpawnResult> waitBranches(
     ImmutableList<SpawnResult> remoteResult = waitBranch(remoteBranch);
 
     if (remoteResult != null && localResult != null) {
-      throw new AssertionError(
-          String.format(
-              "Neither branch of %s cancelled the other one.",
-              spawn.getResourceOwner().getPrimaryOutput().prettyPrint()));
+      throw new AssertionError("Neither branch cancelled the other one.");
     } else if (localResult != null) {
       return localResult;
     } else if (remoteResult != null) {
@@ -507,6 +504,23 @@ ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
                               DynamicSpawnStrategy.this.options,
                               actionExecutionContext,
                               spawn));
+                } catch (DynamicInterruptedException e) {
+                  // This exception can be thrown due to races in stopBranch(), in which case
+                  // the branch that lost the race may not have been cancelled yet. Cancel it here
+                  // to prevent the listener from cross-cancelling.
+                  localBranch.cancel(true);
+                  throw e;
+                } catch (
+                    @SuppressWarnings("InterruptedExceptionSwallowed")
+                    Throwable e) {
+                  if (options.debugSpawnScheduler) {
+                    logger.atInfo().log(
+                        "Local branch of %s failed with %s: '%s'",
+                        spawn.getResourceOwner(),
+                        e.getClass().getSimpleName(),
+                        e.getMessage());
+                  }
+                  throw e;
                 } finally {
                   localDone.release();
                 }
@@ -554,6 +568,23 @@ public ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
                                   spawn));
                   delayLocalExecution.set(true);
                   return spawnResults;
+                } catch (DynamicInterruptedException e) {
+                  // This exception can be thrown due to races in stopBranch(), in which case
+                  // the branch that lost the race may not have been cancelled yet. Cancel it here
+                  // to prevent the listener from cross-cancelling.
+                  remoteBranch.cancel(true);
+                  throw e;
+                } catch (
+                    @SuppressWarnings("InterruptedExceptionSwallowed")
+                    Throwable e) {
+                  if (options.debugSpawnScheduler) {
+                    logger.atInfo().log(
+                        "Remote branch of %s failed with %s: '%s'",
+                        spawn.getResourceOwner(),
+                        e.getClass().getSimpleName(),
+                        e.getMessage());
+                  }
+                  throw e;
                 } finally {
                   remoteDone.release();
                 }

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

@@ -18,8 +18,11 @@ java_test(
     srcs = ["DynamicSpawnStrategyUnitTest.java"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/dynamic",
         "//src/main/java/com/google/devtools/build/lib/exec:execution_policy",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/protobuf:failure_details_java_proto",
         "//src/test/java/com/google/devtools/build/lib/exec/util",
         "//src/test/java/com/google/devtools/build/lib/testutil",

src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java

@@ -16,6 +16,7 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
 import static junit.framework.TestCase.fail;
 import static org.junit.Assert.assertThrows;
 
@@ -571,6 +572,7 @@ public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalFailsLater() throw
 
   @Test
   public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() throws Exception {
+    assume().that(legacyBehavior).isFalse();
     MockSpawnStrategy localStrategy = new MockSpawnStrategy("MockLocalSpawnStrategy");
 
     MockSpawnStrategy remoteStrategy =
@@ -592,6 +594,7 @@ public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() thro
 
   @Test
   public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalRunsNothing() throws Exception {
+    assume().that(legacyBehavior).isFalse();
     MockSpawnStrategy localStrategy =
         new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false);
 
@@ -723,6 +726,7 @@ public void actionFailsIfLocalAndRemoteFail() throws Exception {
 
   @Test
   public void actionFailsIfLocalAndRemoteRunNothing() throws Exception {
+    assume().that(legacyBehavior).isFalse();
     MockSpawnStrategy localStrategy =
         new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false);
 

src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java

@@ -28,8 +28,10 @@
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.util.concurrent.Futures;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.DynamicStrategyRegistry;
 import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy;
 import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy.StopConcurrentSpawns;
@@ -41,8 +43,12 @@
 import com.google.devtools.build.lib.exec.util.SpawnBuilder;
 import com.google.devtools.build.lib.server.FailureDetails.Execution;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.testutil.TestFileOutErr;
 import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
 import java.util.Optional;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -85,8 +91,15 @@ private static void verifyNoInteractions(Object... objs) {
 
   @Mock private Function<Spawn, Optional<Spawn>> mockGetPostProcessingSpawn;
 
+  private Scratch scratch;
+  private Path execDir;
+  private ArtifactRoot rootDir;
+
   @Before
-  public void initMocks() {
+  public void initMocks() throws IOException {
+    scratch = new Scratch();
+    execDir = scratch.dir("/base/exec");
+    rootDir = ArtifactRoot.asDerivedRoot(execDir, "root");
     MockitoAnnotations.initMocks(this);
     // Mockito can't see that we want the function to return Optional.empty() instead
     // of null on apply by default (thanks generic type erasure). Set that up ourselves.
@@ -270,21 +283,6 @@ public void exec_runAnywhereSpawn_runsLocalPostProcessingSpawn() throws Exceptio
     assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT, SUCCESSFUL_SPAWN_RESULT);
   }
 
-  @Test
-  public void waitBranches_givesDebugOutputOnWeirdCases() throws Exception {
-    Spawn spawn =
-        new SpawnBuilder()
-            .withOwnerPrimaryOutput(new SourceArtifact(rootDir, PathFragment.create("/foo"), null))
-            .build();
-    AssertionError error =
-        assertThrows(
-            AssertionError.class,
-            () ->
-                DynamicSpawnStrategy.waitBranches(
-                    Futures.immediateFuture(null), Futures.immediateFuture(null), spawn));
-    assertThat(error).hasMessageThat().contains("Neither branch of /foo completed.");
-  }
-
   @Test
   public void exec_runAnywhereSpawn_localCantExec_runsRemote() throws Exception {
     Spawn spawn = new SpawnBuilder().build();