Inform EvaluationProgressReceiver of nodes that are built in error.

This allows the pending action counter to be decremented correctly when in --keep_going mode.

As part of this, there's a small refactor in ParallelEvaluator that also fixes a potential bug, that nodes in error were signaling their parents with the graph version as opposed to their actual version. Because errors never compare equal (ErrorInfo doesn't override equality), this isn't an issue in practice. But it could be in the future.

--
MOS_MIGRATED_REVID=88395500

4 files changed, 48 insertions, 53 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
index c96c178d7f..b58e57c1dd 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
@@ -258,13 +258,14 @@ public class SkyframeBuilder implements Builder {
     @Override
     public void evaluated(SkyKey skyKey, SkyValue node, EvaluationState state) {
       SkyFunctionName type = skyKey.functionName();
-      if (type == SkyFunctions.TARGET_COMPLETION) {
+      if (type == SkyFunctions.TARGET_COMPLETION && node != null) {
         TargetCompletionValue val = (TargetCompletionValue) node;
         ConfiguredTarget target = val.getConfiguredTarget();
         builtTargets.add(target);
         eventBus.post(TargetCompleteEvent.createSuccessful(target));
       } else if (type == SkyFunctions.ACTION_EXECUTION) {
-        // Remember all completed actions, regardless of having been cached or really executed.
+        // Remember all completed actions, even those in error, regardless of having been cached or
+        // really executed.
         actionCompleted((Action) skyKey.argument());
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 857c23187f..1221f0bd92 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -495,7 +495,7 @@ public final class SkyframeBuildView {
 
     @Override
     public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
-      if (skyKey.functionName() == SkyFunctions.CONFIGURED_TARGET) {
+      if (skyKey.functionName() == SkyFunctions.CONFIGURED_TARGET && value != null) {
         if (state == EvaluationState.BUILT) {
           evaluatedConfiguredTargets.add(skyKey);
           // During multithreaded operation, this is only set to true, so no concurrency issues.
diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java
index 7928878e98..aeef2e4404 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java
@@ -15,6 +15,8 @@ package com.google.devtools.build.skyframe;
 
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 
+import javax.annotation.Nullable;
+
 /**
  * Receiver to inform callers which values have been invalidated. Values may be invalidated and then
  * re-validated if they have been found not to be changed.
@@ -66,12 +68,12 @@ public interface EvaluationProgressReceiver {
   void enqueueing(SkyKey skyKey);
 
   /**
-   * Notifies that {@code value} has been evaluated.
+   * Notifies that the node for {@code skyKey} has been evaluated.
    *
-   * <p>{@code state} indicates the new state of the value.
+   * <p>{@code state} indicates the new state of the node.
    *
-   * <p>This method is not called if the value builder threw an error when building this value.
+   * <p>If the value builder threw an error when building this node, then {@code value} is null.
    */
   @ThreadSafety.ThreadSafe
-  void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state);
+  void evaluated(SkyKey skyKey, @Nullable SkyValue value, EvaluationState state);
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index a99105847b..55c1ab59c0 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -555,43 +555,38 @@ public final class ParallelEvaluator implements Evaluator {
       // during a --keep_going build.
 
       NestedSet<TaggedEvents> events = buildEvents(/*missingChildren=*/false);
+      Version valueVersion;
+      SkyValue valueWithMetadata;
       if (value == null) {
         Preconditions.checkNotNull(errorInfo, "%s %s", skyKey, primaryEntry);
-        // We could consider using max(childVersions) here instead of graphVersion. When full
-        // versioning is implemented, this would allow evaluation at a version between
-        // max(childVersions) and graphVersion to re-use this result.
-        Set<SkyKey> reverseDeps = primaryEntry.setValue(
-            ValueWithMetadata.error(errorInfo, events), graphVersion);
-        signalValuesAndEnqueueIfReady(enqueueParents ? visitor : null, reverseDeps, graphVersion);
+        valueWithMetadata = ValueWithMetadata.error(errorInfo, events);
       } else {
         // We must be enqueueing parents if we have a value.
         Preconditions.checkState(enqueueParents, "%s %s", skyKey, primaryEntry);
-        Set<SkyKey> reverseDeps;
-        Version valueVersion;
-        // If this entry is dirty, setValue may not actually change it, if it determines that
-        // the data being written now is the same as the data already present in the entry.
-        // We could consider using max(childVersions) here instead of graphVersion. When full
-        // versioning is implemented, this would allow evaluation at a version between
-        // max(childVersions) and graphVersion to re-use this result.
-        reverseDeps = primaryEntry.setValue(
-            ValueWithMetadata.normal(value, errorInfo, events), graphVersion);
-        // Note that if this update didn't actually change the value entry, this version may not
-        // be the graph version.
-        valueVersion = primaryEntry.getVersion();
-        Preconditions.checkState(valueVersion.atMost(graphVersion),
-            "%s should be at most %s in the version partial ordering",
-            valueVersion, graphVersion);
-        if (progressReceiver != null) {
-          // Tell the receiver that this value was built. If valueVersion.equals(graphVersion), it
-          // was evaluated this run, and so was changed. Otherwise, it is less than graphVersion,
-          // by the Preconditions check above, and was not actually changed this run -- when it was
-          // written above, its version stayed below this update's version, so its value remains the
-          // same as before.
-          progressReceiver.evaluated(skyKey, value,
-              valueVersion.equals(graphVersion) ? EvaluationState.BUILT : EvaluationState.CLEAN);
-        }
-        signalValuesAndEnqueueIfReady(visitor, reverseDeps, valueVersion);
-      }
+        valueWithMetadata = ValueWithMetadata.normal(value, errorInfo, events);
+      }
+      // If this entry is dirty, setValue may not actually change it, if it determines that
+      // the data being written now is the same as the data already present in the entry.
+      // We could consider using max(childVersions) here instead of graphVersion. When full
+      // versioning is implemented, this would allow evaluation at a version between
+      // max(childVersions) and graphVersion to re-use this result.
+      Set<SkyKey> reverseDeps = primaryEntry.setValue(valueWithMetadata, graphVersion);
+      // Note that if this update didn't actually change the value entry, this version may not
+      // be the graph version.
+      valueVersion = primaryEntry.getVersion();
+      Preconditions.checkState(valueVersion.atMost(graphVersion),
+          "%s should be at most %s in the version partial ordering",
+          valueVersion, graphVersion);
+      if (progressReceiver != null) {
+        // Tell the receiver that this value was built. If valueVersion.equals(graphVersion), it
+        // was evaluated this run, and so was changed. Otherwise, it is less than graphVersion,
+        // by the Preconditions check above, and was not actually changed this run -- when it was
+        // written above, its version stayed below this update's version, so its value remains the
+        // same as before.
+        progressReceiver.evaluated(skyKey, value,
+            valueVersion.equals(graphVersion) ? EvaluationState.BUILT : EvaluationState.CLEAN);
+      }
+      signalValuesAndEnqueueIfReady(enqueueParents ? visitor : null, reverseDeps, valueVersion);
 
       visitor.notifyDone(skyKey);
       replayingNestedSetEventVisitor.visit(events);
@@ -815,7 +810,7 @@ public final class ParallelEvaluator implements Evaluator {
             visitor.notifyDone(skyKey);
             Set<SkyKey> reverseDeps = state.markClean();
             SkyValue value = state.getValue();
-            if (progressReceiver != null && value != null) {
+            if (progressReceiver != null) {
               // Tell the receiver that the value was not actually changed this run.
               progressReceiver.evaluated(skyKey, value, EvaluationState.CLEAN);
             }
@@ -1054,19 +1049,16 @@ public final class ParallelEvaluator implements Evaluator {
       NodeEntry entry = graph.get(key);
       Preconditions.checkState(entry.isDone(), entry);
       SkyValue value = entry.getValue();
-      if (value != null) {
-        Version valueVersion = entry.getVersion();
-        Preconditions.checkState(valueVersion.atMost(graphVersion),
-            "%s should be at most %s in the version partial ordering", valueVersion, graphVersion);
-        // Nodes with errors will have no value. Don't inform the receiver in that case.
-        // For most nodes we do not inform the progress receiver if they were already done
-        // when we retrieve them, but top-level nodes are presumably of more interest.
-        // If valueVersion is not equal to graphVersion, it must be less than it (by the
-        // Preconditions check above), and so the node is clean.
-        progressReceiver.evaluated(key, value, valueVersion.equals(graphVersion)
-            ? EvaluationState.BUILT
-            : EvaluationState.CLEAN);
-      }
+      Version valueVersion = entry.getVersion();
+      Preconditions.checkState(valueVersion.atMost(graphVersion),
+          "%s should be at most %s in the version partial ordering", valueVersion, graphVersion);
+      // For most nodes we do not inform the progress receiver if they were already done when we
+      // retrieve them, but top-level nodes are presumably of more interest.
+      // If valueVersion is not equal to graphVersion, it must be less than it (by the
+      // Preconditions check above), and so the node is clean.
+      progressReceiver.evaluated(key, value, valueVersion.equals(graphVersion)
+          ? EvaluationState.BUILT
+          : EvaluationState.CLEAN);
     }
   }