Stop injecting WorkspaceStatusAction into the Skyframe graph as a precomputed value. Instead, manually check if the value has changed, and if it has, invalidate its consuming WorkspaceStatusValue node, forcing its re-evaluation, where it will pick up the new value.

This seems more awkward than the original code, but it is more correct in spirit: injecting a precomputed value which can change even while the source state does not is a smell. Long-term, the key for the WorkspaceStatusValue should incorporate a hash of the action, and that hash should be in the configuration, just as other configuration flags are. That isn't possible right now just because we don't have configuration trimming, and we drop all nodes on configuration changes, so putting workspace status options into the configuration would lose change pruning whenever we changed workspace status options.

If/when those problems are fixed, we can extend this change to have WorkspaceStatusFunction continue to request the action out-of-band, but keyed by the hash. Then we can stop invalidating stale nodes.

See also https://github.com/bazelbuild/bazel/issues/3785.

PiperOrigin-RevId: 169947071

12 files changed, 67 insertions, 50 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 9abeb1a561..cd641b1d1c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -326,7 +326,7 @@ public class BuildView {
 
   @VisibleForTesting
   WorkspaceStatusAction getLastWorkspaceBuildInfoActionForTesting() {
-    return skyframeExecutor.getLastWorkspaceStatusActionForTesting();
+    return skyframeExecutor.getLastWorkspaceStatusAction();
   }
 
   @Override
@@ -593,7 +593,7 @@ public class BuildView {
               target.getFirst(), target.getSecond(), aspectConfigurations.get(target)));
     }
 
-    skyframeExecutor.injectWorkspaceStatusData(loadingResult.getWorkspaceName());
+    skyframeExecutor.maybeInvalidateWorkspaceStatusValue(loadingResult.getWorkspaceName());
     SkyframeAnalysisResult skyframeAnalysisResult;
     try {
       skyframeAnalysisResult =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
index 5f7c242428..1b7ccc9472 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
@@ -19,7 +19,6 @@ import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoKey;
 import com.google.devtools.build.lib.packages.RuleVisibility;
@@ -95,9 +94,6 @@ public final class PrecomputedValue implements SkyValue {
   static final Precomputed<Map<String, String>> ACTION_ENV =
       new Precomputed<>(LegacySkyKey.create(SkyFunctions.PRECOMPUTED, "action_env"));
 
-  static final Precomputed<WorkspaceStatusAction> WORKSPACE_STATUS_KEY =
-      new Precomputed<>(LegacySkyKey.create(SkyFunctions.PRECOMPUTED, "workspace_status_action"));
-
   static final Precomputed<ImmutableList<ActionAnalysisMetadata>> COVERAGE_REPORT_KEY =
       new Precomputed<>(LegacySkyKey.create(SkyFunctions.PRECOMPUTED, "coverage_report_actions"));
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 08eea1fe01..17bdeb1481 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -441,7 +441,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         SkyFunctions.BUILD_INFO_COLLECTION,
         new BuildInfoCollectionFunction(
             artifactFactory, buildInfoFactories, removeActionsAfterEvaluation));
-    map.put(SkyFunctions.BUILD_INFO, new WorkspaceStatusFunction(removeActionsAfterEvaluation));
+    map.put(
+        SkyFunctions.BUILD_INFO,
+        new WorkspaceStatusFunction(removeActionsAfterEvaluation, this::makeWorkspaceStatusAction));
     map.put(SkyFunctions.COVERAGE_REPORT, new CoverageReportFunction(removeActionsAfterEvaluation));
     ActionExecutionFunction actionExecutionFunction =
         new ActionExecutionFunction(skyframeActionExecutor, tsgm);
@@ -695,10 +697,31 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     PrecomputedValue.DEFAULTS_PACKAGE_CONTENTS.set(injectable(), defaultsPackageContents);
   }
 
-  public void injectWorkspaceStatusData(String workspaceName) {
-    PrecomputedValue.WORKSPACE_STATUS_KEY.set(injectable(),
-        workspaceStatusActionFactory.createWorkspaceStatusAction(
-            artifactFactory.get(), WorkspaceStatusValue.ARTIFACT_OWNER, buildId, workspaceName));
+  public void maybeInvalidateWorkspaceStatusValue(String workspaceName) {
+    WorkspaceStatusAction newWorkspaceStatusAction = makeWorkspaceStatusAction(workspaceName);
+    WorkspaceStatusAction oldWorkspaceStatusAction = getLastWorkspaceStatusAction();
+    if (oldWorkspaceStatusAction != null
+        && !newWorkspaceStatusAction.equals(oldWorkspaceStatusAction)) {
+      // TODO(janakr): don't invalidate here, just use different keys for different configs. Can't
+      // be done right now because of lack of configuration trimming and fact that everything
+      // depends on workspace status action.
+      invalidate(WorkspaceStatusValue.SKY_KEY::equals);
+    }
+  }
+
+  private WorkspaceStatusAction makeWorkspaceStatusAction(String workspaceName) {
+    return workspaceStatusActionFactory.createWorkspaceStatusAction(
+        artifactFactory.get(), WorkspaceStatusValue.ARTIFACT_OWNER, buildId, workspaceName);
+  }
+
+  @VisibleForTesting
+  @Nullable
+  public WorkspaceStatusAction getLastWorkspaceStatusAction() {
+    WorkspaceStatusValue workspaceStatusValue =
+        (WorkspaceStatusValue) memoizingEvaluator.getExistingValue(WorkspaceStatusValue.SKY_KEY);
+    return workspaceStatusValue == null
+        ? null
+        : (WorkspaceStatusAction) workspaceStatusValue.getAction(0);
   }
 
   public void injectCoverageReportData(ImmutableList<ActionAnalysisMetadata> actions) {
@@ -808,13 +831,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   @VisibleForTesting
-  public WorkspaceStatusAction getLastWorkspaceStatusActionForTesting() {
-    PrecomputedValue value = (PrecomputedValue) buildDriver.getGraphForTesting()
-        .getExistingValueForTesting(PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting());
-    return (WorkspaceStatusAction) value.get();
-  }
-
-  @VisibleForTesting
   public ToolchainContext getToolchainContextForTesting(
       Set<Label> requiredToolchains, BuildConfiguration config, ExtendedEventHandler eventHandler)
       throws ToolchainContextException, InterruptedException {
@@ -1547,11 +1563,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       Label label,
       BuildConfiguration configuration,
       Attribute.Transition transition) {
-    if (memoizingEvaluator.getExistingValueForTesting(
-        PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting()) == null) {
-      injectWorkspaceStatusData(label.getWorkspaceRoot());
-    }
-
     return Iterables.getFirst(
         getConfiguredTargets(
             eventHandler,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
index d665632beb..5bed654a01 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
@@ -13,31 +13,42 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.base.Supplier;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import java.util.function.Supplier;
 
 /** Creates the workspace status artifacts and action. */
 public class WorkspaceStatusFunction implements SkyFunction {
+  interface WorkspaceStatusActionFactory {
+    WorkspaceStatusAction create(String workspaceName);
+  }
+
   private final Supplier<Boolean> removeActionAfterEvaluation;
+  private final WorkspaceStatusActionFactory workspaceStatusActionFactory;
 
-  WorkspaceStatusFunction(Supplier<Boolean> removeActionAfterEvaluation) {
+  WorkspaceStatusFunction(
+      Supplier<Boolean> removeActionAfterEvaluation,
+      WorkspaceStatusActionFactory workspaceStatusActionFactory) {
     this.removeActionAfterEvaluation = Preconditions.checkNotNull(removeActionAfterEvaluation);
+    this.workspaceStatusActionFactory = workspaceStatusActionFactory;
   }
 
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
     Preconditions.checkState(
         WorkspaceStatusValue.SKY_KEY.equals(skyKey), WorkspaceStatusValue.SKY_KEY);
-
-    WorkspaceStatusAction action = PrecomputedValue.WORKSPACE_STATUS_KEY.get(env);
-    if (action == null) {
+    WorkspaceNameValue workspaceNameValue =
+        (WorkspaceNameValue) env.getValue(WorkspaceNameValue.key());
+    if (env.valuesMissing()) {
       return null;
     }
 
+    WorkspaceStatusAction action =
+        workspaceStatusActionFactory.create(workspaceNameValue.getName());
+
     return new WorkspaceStatusValue(
         action.getStableStatus(),
         action.getVolatileStatus(),
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index f0bc14a514..618c4d162b 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -281,7 +281,8 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
   }
 
   @Override
-  @Nullable public SkyValue getExistingValueForTesting(SkyKey key) {
+  @Nullable
+  public SkyValue getExistingValue(SkyKey key) {
     NodeEntry entry = getExistingEntryForTesting(key);
     try {
       return isDone(entry) ? entry.getValue() : null;
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index 2fe92fa521..889c9df7b8 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -86,7 +86,7 @@ public interface MemoizingEvaluator {
    *
    * <p>The returned map may be a live view of the graph.
    */
-  // TODO(bazel-team): Replace all usages of getValues, getDoneValues, getExistingValueForTesting,
+  // TODO(bazel-team): Replace all usages of getValues, getDoneValues, getExistingValue,
   // and getExistingErrorForTesting with usages of WalkableGraph. Changing the getValues usages
   // require some care because getValues gives access to the previous value for changed/dirty nodes.
   Map<SkyKey, SkyValue> getValues();
@@ -108,12 +108,12 @@ public interface MemoizingEvaluator {
   /**
    * Returns a value if and only if an earlier call to {@link #evaluate} created it; null otherwise.
    *
-   * <p>This method should only be used by tests that need to verify the presence of a value in the
-   * graph after an {@link #evaluate} call.
+   * <p>This method should mainly be used by tests that need to verify the presence of a value in
+   * the graph after an {@link #evaluate} call.
    */
   @VisibleForTesting
   @Nullable
-  SkyValue getExistingValueForTesting(SkyKey key);
+  SkyValue getExistingValue(SkyKey key);
 
   /**
    * Returns an error if and only if an earlier call to {@link #evaluate} created it; null
diff --git a/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java b/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
index 988412b866..82b4ecbb75 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
@@ -60,7 +60,7 @@ public class SequentialBuildDriver implements BuildDriver {
   @Nullable
   @Override
   public SkyValue getExistingValueForTesting(SkyKey key) {
-    return memoizingEvaluator.getExistingValueForTesting(key);
+    return memoizingEvaluator.getExistingValue(key);
   }
 
   @Nullable
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index faee80dd01..5935aa6351 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -712,15 +712,14 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   }
 
   /**
-   * Returns the ConfiguredTarget for the specified label, using the
-   * given build configuration. If the label corresponds to a target with a top-level configuration
-   * transition, that transition is applied to the given config in the returned ConfiguredTarget.
+   * Returns the ConfiguredTarget for the specified label, using the given build configuration. If
+   * the label corresponds to a target with a top-level configuration transition, that transition is
+   * applied to the given config in the returned ConfiguredTarget.
    *
-   * <p>If the evaluation of the SkyKey corresponding to the configured target fails, this
-   * method may return null.  In that case, use a debugger to inspect the {@link ErrorInfo}
-   * for the evaluation, which is produced by the
-   * {@link MemoizingEvaluator#getExistingValueForTesting} call in
-   * {@link SkyframeExecutor#getConfiguredTargetForTesting}.  See also b/26382502.
+   * <p>If the evaluation of the SkyKey corresponding to the configured target fails, this method
+   * may return null. In that case, use a debugger to inspect the {@link ErrorInfo} for the
+   * evaluation, which is produced by the {@link MemoizingEvaluator#getExistingValue} call in {@link
+   * SkyframeExecutor#getConfiguredTargetForTesting}. See also b/26382502.
    */
   protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config) {
     return view.getConfiguredTargetForTesting(reporter, BlazeTestUtils.convertLabel(label), config);
@@ -768,7 +767,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
     invalidatePackages();
     // Need to re-initialize the workspace status.
-    getSkyframeExecutor().injectWorkspaceStatusData("test");
+    getSkyframeExecutor().maybeInvalidateWorkspaceStatusValue("test");
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 5e2dba77c8..673fcfb2b8 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -40,7 +40,6 @@ import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
 import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileStatus;
-import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -461,7 +460,7 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
   }
 
   private void setGeneratingActions() {
-    if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
+    if (evaluator.getExistingValue(OWNER_KEY) == null) {
       differencer.inject(
           ImmutableMap.of(
               OWNER_KEY,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index b243828490..22033a3c89 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -227,7 +227,7 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase {
 
     return new Builder() {
       private void setGeneratingActions() {
-        if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
+        if (evaluator.getExistingValue(OWNER_KEY) == null) {
           differencer.inject(
               ImmutableMap.of(
                   OWNER_KEY, new ActionLookupValue(ImmutableList.copyOf(actions), false)));
@@ -253,8 +253,8 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase {
         skyframeActionExecutor.prepareForExecution(
             reporter,
             executor,
-            keepGoing, /*explain=*/
-            false,
+            keepGoing,
+            /*explain=*/ false,
             new ActionCacheChecker(actionCache, null, ALWAYS_EXECUTE_FILTER, null),
             null);
         skyframeActionExecutor.setActionExecutionProgressReportingObjects(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
index b1d8b087ab..6b53a00383 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
@@ -224,7 +224,7 @@ public class TreeArtifactMetadataTest extends ArtifactFunctionTestCase {
   }
 
   private void setGeneratingActions() {
-    if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
+    if (evaluator.getExistingValue(OWNER_KEY) == null) {
       differencer.inject(
           ImmutableMap.of(
               OWNER_KEY,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
index 21ba998d8f..30b0076010 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
@@ -50,7 +50,7 @@ public class SkyframeExecutorTestUtils {
    */
   @Nullable
   public static SkyValue getExistingValue(SkyframeExecutor skyframeExecutor, SkyKey key) {
-    return skyframeExecutor.getEvaluatorForTesting().getExistingValueForTesting(key);
+    return skyframeExecutor.getEvaluatorForTesting().getExistingValue(key);
   }
 
   /**