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

src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java

@@ -258,13 +258,14 @@ public void enqueueing(SkyKey skyKey) {
     @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());
       }
     }

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

@@ -495,7 +495,7 @@ public void enqueueing(SkyKey skyKey) {}
 
     @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.

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

@@ -15,6 +15,8 @@
 
 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 @@ enum InvalidationState {
   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);
 }

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

@@ -555,43 +555,38 @@ void commit(boolean enqueueParents) {
       // 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 void run() {
             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 @@ private void informProgressReceiverThatValueIsDone(SkyKey key) {
       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);
     }
   }
 

src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java

@@ -486,11 +486,11 @@ private void setupDiamondDependency() {
         .setComputedValue(COPY);
   }
 
-  // Regression test: ParallelEvaluator notifies ValueProgressReceiver of already-built top-level
-  // values in error: we built "top" and "mid" as top-level targets; "mid" contains an error. We
-  // make sure "mid" is built as a dependency of "top" before enqueuing mid as a top-level target
-  // (by using a latch), so that the top-level enqueuing finds that mid has already been built. The
-  // progress receiver should not be notified of any value having been evaluated.
+  // ParallelEvaluator notifies ValueProgressReceiver of already-built top-level values in error: we
+  // built "top" and "mid" as top-level targets; "mid" contains an error. We make sure "mid" is
+  // built as a dependency of "top" before enqueuing mid as a top-level target (by using a latch),
+  // so that the top-level enqueuing finds that mid has already been built. The progress receiver
+  // should be notified that mid has been built.
   @Test
   public void alreadyAnalyzedBadTarget() throws Exception {
     final SkyKey mid = GraphTester.toSkyKey("mid");
@@ -523,24 +523,24 @@ public void accept(SkyKey key, EventType type, Order order, Object context) {
     tester.eval(/*keepGoing=*/false, top, mid);
     assertEquals(0L, valueSet.getCount());
     trackingAwaiter.assertNoErrors();
-    assertThat(tester.invalidationReceiver.evaluated).isEmpty();
+    assertThat(tester.invalidationReceiver.evaluated).containsExactly(mid);
   }
 
   @Test
-  public void receiverNotToldOfVerifiedValueDependingOnCycle() throws Exception {
+  public void receiverToldOfVerifiedValueDependingOnCycle() throws Exception {
     SkyKey leaf = GraphTester.toSkyKey("leaf");
     SkyKey cycle = GraphTester.toSkyKey("cycle");
     SkyKey top = GraphTester.toSkyKey("top");
     tester.set(leaf, new StringValue("leaf"));
     tester.getOrCreate(cycle).addDependency(cycle);
     tester.getOrCreate(top).addDependency(leaf).addDependency(cycle);
     tester.eval(/*keepGoing=*/true, top);
-    assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf).inOrder();
+    assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf, top, cycle);
     tester.invalidationReceiver.clear();
     tester.getOrCreate(leaf, /*markAsModified=*/true);
     tester.invalidate();
     tester.eval(/*keepGoing=*/true, top);
-    assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf).inOrder();
+    assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf, top);
   }
 
   @Test

src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java

@@ -51,11 +51,9 @@ public void enqueueing(SkyKey skyKey) {
   @Override
   public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
     evaluated.add(skyKey);
-    switch (state) {
-      default:
-        dirty.remove(value);
-        deleted.remove(value);
-        break;
+    if (value != null) {
+      dirty.remove(value);
+      deleted.remove(value);
     }
   }