[Skymeld] Don't use SkyKeyComputeState to manage conflict checking in…

… BuildDriverFunction.

When all analysis work in the build is finished, we clear the various intermediate states and shut down the executors for conflict checking. We don't expect any further conflict checking beyond this point, and until now have been using SkyKeyComputeState for that. This is a poor fit because SkyKeyComputeState doesn't guarantee that the same instance is returned when calling #compute on the same SkyKey and should only be used as a performance optimization. A bug may then occur: some conflict checking tasks may be sent to the executors still after they were shut down and resulted in a RejectedExecutionException.

This CL fixes that issue by using a dedicated Set in BuildDriverFunction that tracks whether a BuildDriverKey has been checked for conflicts or not. This set is reset after each build for consistency.

PiperOrigin-RevId: 519718108
Change-Id: I1cc8d883815514f3a8ba5f37f365ebabca9215d3

src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java

@@ -25,6 +25,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionLookupKey;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
@@ -72,6 +73,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
@@ -83,6 +85,17 @@ public class BuildDriverFunction implements SkyFunction {
   private final Supplier<RuleContextConstraintSemantics> ruleContextConstraintSemantics;
   private final Supplier<RegexFilter> extraActionFilterSupplier;
 
+  // A set of BuildDriverKeys that have been checked for conflicts.
+  // This gets cleared after each build.
+  // We can't use SkyKeyComputeState here since it doesn't guarantee that the same state for
+  // a previously requested SkyKey is retrieved. This could cause a correctness issue:
+  // - we clear the conflict checking states and shut down the Executors after all the analysis
+  //   work is done in the build
+  // - If the SkyKeyComputeState for this BuildDriverKey was cleared, an evaluation of this key
+  //   would attempt again to check for conflicts => we redo the work, or a race condition with the
+  //   shutting down of the Executors could lead to a RejectedExecutionException.
+  private Set<BuildDriverKey> checkedForConflicts = Sets.newConcurrentHashSet();
+
   BuildDriverFunction(
       TransitiveActionLookupValuesHelper transitiveActionLookupValuesHelper,
       Supplier<IncrementalArtifactConflictFinder> incrementalArtifactConflictFinder,
@@ -95,7 +108,6 @@ public class BuildDriverFunction implements SkyFunction {
   }
 
   private static class State implements SkyKeyComputeState {
-    private ImmutableMap<ActionAnalysisMetadata, ConflictException> actionConflicts;
     // It's only necessary to do this check once.
     private boolean checkedForCompatibility = false;
     private boolean checkedForPlatformCompatibility = false;
@@ -129,28 +141,22 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       return null;
     }
 
-    // This code path should not be run during error bubbling for several reasons:
-    // 1. Correctness: to check for action conflicts, we need access to the transitive
-    //    ConfiguredTargets, which will be null after AnalysisPhaseCompleteEvent in
-    //    --discard_analysis_cache mode.
-    // 2. Performance: this method is CPU intensive, and it does not offer anything while error
-    //    bubbling.
-    if (!env.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors()) {
-      // Unconditionally check for action conflicts.
-      // TODO(b/214371092): Only check when necessary.
-      try (SilentCloseable c =
-          Profiler.instance().profile("BuildDriverFunction.checkActionConflicts")) {
-        if (state.actionConflicts == null) {
-          state.actionConflicts =
-              checkActionConflicts(actionLookupKey, buildDriverKey.strictActionConflictCheck());
-        }
-        if (!state.actionConflicts.isEmpty()) {
+    // Unconditionally check for action conflicts.
+    // TODO(b/214371092): Only check when necessary.
+    try (SilentCloseable c =
+        Profiler.instance().profile("BuildDriverFunction.checkActionConflicts")) {
+      // We only check for action conflict once per BuildDriverKey.
+      if (checkedForConflicts.add(buildDriverKey)) {
+        ImmutableMap<ActionAnalysisMetadata, ConflictException> actionConflicts =
+            checkActionConflicts(actionLookupKey, buildDriverKey.strictActionConflictCheck());
+        if (!actionConflicts.isEmpty()) {
           throw new BuildDriverFunctionException(
               new TopLevelConflictException(
                   "Action conflict(s) detected while analyzing top-level target "
                       + actionLookupKey.getLabel(),
-                  state.actionConflicts));
+                  actionConflicts));
         }
+
       }
     }
 
@@ -246,6 +252,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     return new BuildDriverValue(topLevelSkyValue, /*skipped=*/ false);
   }
 
+  public void resetActionConflictCheckingStatus() {
+    checkedForConflicts = Sets.newConcurrentHashSet();
+  }
+
   private static void postTopLevelTargetAnalyzedEvent(
       Environment env,
       ConfiguredTargetValue configuredTargetValue,

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java

@@ -636,6 +636,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
           // We unconditionally reset the states here instead of in #analysisFinishedCallback since
           // in case of --nokeep_going & analysis error, the analysis phase is never finished.
           skyframeExecutor.resetIncrementalArtifactConflictFindingStates();
+          skyframeExecutor.resetBuildDriverFunction();
 
           Set<Artifact> additionalArtifacts = new HashSet<>();
 

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -335,6 +335,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur
       new AtomicReference<>();
   protected final SkyframeActionExecutor skyframeActionExecutor;
   private ActionExecutionFunction actionExecutionFunction;
+  private BuildDriverFunction buildDriverFunction;
   private GlobFunction globFunction;
   protected SkyframeProgressReceiver progressReceiver;
   private CyclesReporter cyclesReporter = null;
@@ -660,8 +661,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions() {
     map.put(
         SkyFunctions.ARTIFACT_NESTED_SET,
         ArtifactNestedSetFunction.createInstance(valueBasedChangePruningEnabled()));
-    map.put(
-        SkyFunctions.BUILD_DRIVER,
+    BuildDriverFunction buildDriverFunction =
         new BuildDriverFunction(
             new TransitiveActionLookupValuesHelper() {
               @Override
@@ -676,7 +676,10 @@ public void registerConflictFreeKeys(ImmutableSet<SkyKey> keys) {
             },
             this::getIncrementalArtifactConflictFinder,
             this::getRuleContextConstraintSemantics,
-            this::getExtraActionFilter));
+            this::getExtraActionFilter);
+    map.put(SkyFunctions.BUILD_DRIVER, buildDriverFunction);
+    this.buildDriverFunction = buildDriverFunction;
+
     map.putAll(extraSkyFunctions);
     return ImmutableMap.copyOf(map);
   }
@@ -2398,6 +2401,10 @@ void resetActionConflictsStoredInSkyframe() {
         SkyFunctionName.functionIs(SkyFunctions.ACTION_LOOKUP_CONFLICT_FINDING));
   }
 
+  public void resetBuildDriverFunction() {
+    buildDriverFunction.resetActionConflictCheckingStatus();
+  }
+
   /** Resets the incremental artifact conflict finder to ensure incremental correctness. */
   public void resetIncrementalArtifactConflictFindingStates() throws InterruptedException {
     incrementalArtifactConflictFinder.shutdown();

src/main/java/com/google/devtools/build/skyframe/SkyFunction.java

@@ -318,10 +318,6 @@ default void injectVersionForNonHermeticFunction(Version version) {}
      * <p>Such a SkyFunction cannot unconditionally return a value, since in --nokeep_going mode it
      * may be called upon to transform a lower-level exception. This method can tell it whether to
      * transform a dependency's exception or ignore it and return a value as usual.
-     *
-     * <p>An exception is with {@link
-     * com.google.devtools.build.lib.skyframe.BuildDriverFunction#checkActionConflicts}. See the
-     * documentation at the method for more details.
      */
     boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors();