Fix bug in lazy removal of reverse deps -- there was one remaining case where we weren't checking to see if a reverse dep already existed when we declared a reverse dep.

--
MOS_MIGRATED_REVID=122581019

3 files changed, 27 insertions, 1 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
index 0fafa861a3..7b38324110 100644
--- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -152,6 +152,9 @@ public interface NodeEntry extends ThinNodeEntry {
    *
    * <p>If the parameter is {@code null}, then no reverse dependency is added, but we still check
    * if the node needs to be scheduled.
+   *
+   * <p>If {@code reverseDep} is a rebuilding dirty entry that was already a reverse dep of this
+   * entry, then {@link #checkIfDoneForDirtyReverseDep} must be called instead.
    */
   @ThreadSafe
   DependencyState addReverseDepAndCheckIfDone(@Nullable SkyKey reverseDep);
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 926a4c6ff1..b88ef42b61 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -1059,7 +1059,12 @@ public final class ParallelEvaluator implements Evaluator {
         Preconditions.checkState(
             newDirectDeps.contains(childErrorKey), "%s %s %s", state, childErrorKey, newDirectDeps);
         state.addTemporaryDirectDeps(GroupedListHelper.create(ImmutableList.of(childErrorKey)));
-        DependencyState childErrorState = childErrorEntry.addReverseDepAndCheckIfDone(skyKey);
+        DependencyState childErrorState;
+        if (oldDeps.contains(childErrorKey)) {
+          childErrorState = childErrorEntry.checkIfDoneForDirtyReverseDep(skyKey);
+        } else {
+          childErrorState = childErrorEntry.addReverseDepAndCheckIfDone(skyKey);
+        }
         Preconditions.checkState(
             childErrorState == DependencyState.DONE,
             "skyKey: %s, state: %s childErrorKey: %s",
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 c4f0047f3b..08b0f26c6b 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -596,6 +596,24 @@ public class MemoizingEvaluatorTest {
   }
 
   @Test
+  public void noKeepGoingErrorAfterKeepGoingError() throws Exception {
+    SkyKey topKey = GraphTester.skyKey("top");
+    SkyKey errorKey = GraphTester.skyKey("error");
+    tester.getOrCreate(errorKey).setHasError(true);
+    tester.getOrCreate(topKey).addDependency(errorKey).setComputedValue(CONCATENATE);
+    EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/ true, topKey);
+    assertThatEvaluationResult(result)
+        .hasErrorEntryForKeyThat(topKey)
+        .rootCauseOfExceptionIs(errorKey);
+    tester.getOrCreate(topKey, /*markAsModified=*/ true);
+    tester.invalidate();
+    result = tester.eval(/*keepGoing=*/ false, topKey);
+    assertThatEvaluationResult(result)
+        .hasErrorEntryForKeyThat(topKey)
+        .rootCauseOfExceptionIs(errorKey);
+  }
+
+  @Test
   public void transientErrorValueInvalidation() throws Exception {
     // Verify that invalidating errors causes all transient error values to be rerun.
     tester.getOrCreate("error-value").setHasTransientError(true).setProgress(