Prevent new evaluations from starting if a done child's error is discovered. Also delete some code that's been dead for a while, now that we eagerly shut down evaluation when we come across a child in error during a fail-fast evaluation.

--
MOS_MIGRATED_REVID=115272603

2 files changed, 56 insertions, 25 deletions

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 ff885d8dbe..ce92df94e4 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -1037,28 +1037,26 @@ public final class ParallelEvaluator implements Evaluator {
         SkyKey childErrorKey = env.getDepErrorKey();
         NodeEntry childErrorEntry = Preconditions.checkNotNull(graph.get(childErrorKey),
             "skyKey: %s, state: %s childErrorKey: %s", skyKey, state, childErrorKey);
-        if (!state.getTemporaryDirectDeps().contains(childErrorKey)) {
-          // This means the cached error was freshly requested (e.g. the parent has never been
-          // built before).
-          Preconditions.checkState(newDirectDeps.contains(childErrorKey), "%s %s %s", state,
-              childErrorKey, newDirectDeps);
-          state.addTemporaryDirectDeps(GroupedListHelper.create(ImmutableList.of(childErrorKey)));
-          DependencyState childErrorState = childErrorEntry.addReverseDepAndCheckIfDone(skyKey);
-          Preconditions.checkState(childErrorState == DependencyState.DONE,
-              "skyKey: %s, state: %s childErrorKey: %s", skyKey, state, childErrorKey,
-              childErrorEntry);
-        } else {
-          // This means the cached error was previously requested, and was then subsequently (after
-          // a restart) requested along with another sibling dep. This can happen on an incremental
-          // eval call when the parent is dirty and the child error is in a separate dependency
-          // group from the sibling dep.
-          Preconditions.checkState(!newDirectDeps.contains(childErrorKey), "%s %s %s", state,
-              childErrorKey, newDirectDeps);
-          Preconditions.checkState(childErrorEntry.isDone(),
-              "skyKey: %s, state: %s childErrorKey: %s", skyKey, state, childErrorKey,
-              childErrorEntry);
-        }
+        Preconditions.checkState(
+            !state.getTemporaryDirectDeps().contains(childErrorKey),
+            "Done error was already know: %s %s %s %s",
+            skyKey,
+            state,
+            childErrorKey,
+            childErrorEntry);
+        Preconditions.checkState(
+            newDirectDeps.contains(childErrorKey), "%s %s %s", state, childErrorKey, newDirectDeps);
+        state.addTemporaryDirectDeps(GroupedListHelper.create(ImmutableList.of(childErrorKey)));
+        DependencyState childErrorState = childErrorEntry.addReverseDepAndCheckIfDone(skyKey);
+        Preconditions.checkState(
+            childErrorState == DependencyState.DONE,
+            "skyKey: %s, state: %s childErrorKey: %s",
+            skyKey,
+            state,
+            childErrorKey,
+            childErrorEntry);
         ErrorInfo childErrorInfo = Preconditions.checkNotNull(childErrorEntry.getErrorInfo());
+        visitor.preventNewEvaluations();
         throw SchedulerException.ofError(childErrorInfo, childErrorKey);
       }
 
@@ -1078,10 +1076,16 @@ public final class ParallelEvaluator implements Evaluator {
         // TODO(bazel-team): This means a bug in the SkyFunction. What to do?
         Preconditions.checkState(!env.childErrorInfos.isEmpty(),
             "Evaluation of SkyKey failed and no dependencies were requested: %s %s", skyKey, state);
-        env.commit(/*enqueueParents=*/keepGoing);
-        if (!keepGoing) {
-          throw SchedulerException.ofError(state.getErrorInfo(), skyKey);
-        }
+        Preconditions.checkState(
+            keepGoing,
+            "nokeep_going evaluation should have failed on first child error: %s %s %s",
+            skyKey,
+            state,
+            env.childErrorInfos);
+        // If the child error was catastrophic, committing this parent to the graph is not
+        // necessary, but since we don't do error bubbling in catastrophes, it doesn't violate any
+        // invariants either.
+        env.commit(/*enqueueParents=*/ true);
         return;
       }
 
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index 94c9b819c3..2fcd1a99c0 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -220,6 +220,33 @@ public class MemoizingEvaluatorTest {
   }
 
   @Test
+  public void cachedErrorShutsDownThreadpool() throws Exception {
+    // When a node throws an error on the first build,
+    SkyKey cachedErrorKey = GraphTester.skyKey("error");
+    tester.getOrCreate(cachedErrorKey).setHasError(true);
+    assertThat(tester.evalAndGetError(cachedErrorKey)).isNotNull();
+    // And on the second build, it is requested as a dep,
+    SkyKey topKey = GraphTester.skyKey("top");
+    tester.getOrCreate(topKey).addDependency(cachedErrorKey).setComputedValue(CONCATENATE);
+    // And another node throws an error, but waits to throw until the child error is thrown,
+    SkyKey newErrorKey = GraphTester.skyKey("newError");
+    tester
+        .getOrCreate(newErrorKey)
+        .setBuilder(
+            new ChainedFunction.Builder()
+                .setWaitForException(true)
+                .setWaitToFinish(new CountDownLatch(0))
+                .setValue(null)
+                .build());
+    // Then when evaluation happens,
+    EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/ false, newErrorKey, topKey);
+    // The result has an error,
+    assertThatEvaluationResult(result).hasError();
+    // But the new error is not persisted to the graph, since the child error shut down evaluation.
+    assertThatEvaluationResult(result).hasErrorEntryForKeyThat(newErrorKey).isNull();
+  }
+
+  @Test
   public void crashAfterInterruptCrashes() throws Exception {
     SkyKey failKey = GraphTester.skyKey("fail");
     SkyKey badInterruptkey = GraphTester.skyKey("bad-interrupt");