From 5b95d9162b56b51c8e8f66258981ddf3c5d96765 Mon Sep 17 00:00:00 2001
From: larsrc <larsrc@google.com>
Date: Wed, 12 May 2021 05:14:42 -0700
Subject: [PATCH] Check the result of Future.cancel() when cancelling the other
 branch of dynamic execution.

Under --experimental_local_lockfree_output, the local branch may succeed and set the future before cancelling the remote branch. If the remote branch succeeded after that but before the local listener got going, it would falsely think the local was cancelled, which would occasionally lead to strange errors.

RELNOTES: None.
PiperOrigin-RevId: 373347494
---
 .../build/lib/actions/DynamicStrategyRegistry.java   |  4 ++++
 .../build/lib/dynamic/DynamicSpawnStrategy.java      | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/actions/DynamicStrategyRegistry.java b/src/main/java/com/google/devtools/build/lib/actions/DynamicStrategyRegistry.java
index 18e063c2f4e601..3c9c633a1c3c58 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/DynamicStrategyRegistry.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/DynamicStrategyRegistry.java
@@ -34,6 +34,10 @@ enum DynamicMode {
     public String toString() {
       return name;
     }
+
+    public DynamicMode other() {
+      return this == REMOTE ? LOCAL : REMOTE;
+    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java
index bb2317a2649e55..40ba2a50ab7b74 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java
@@ -173,7 +173,17 @@ private static void stopBranch(
                           spawn.getMnemonic(), strategyThatCancelled.get())));
         }
 
-        branchToCancel.cancel(true);
+        if (!branchToCancel.cancel(true)) {
+          // This can happen if the other branch is local under local_lockfree and has returned
+          // its result but not yet cancelled this branch, or if the other branch was already
+          // cancelled for other reasons.
+          if (!branchToCancel.isCancelled()) {
+            throw new DynamicInterruptedException(
+                String.format(
+                    "Execution of %s strategy stopped because %s strategy could not be cancelled",
+                    cancellingStrategy, cancellingStrategy.other()));
+          }
+        }
         branchDone.acquire();
       } else {
         throw new DynamicInterruptedException(