Add reset deps to the set returned by `NodeEntry#getAllDirectDepsForI…

…ncompleteNode`.

When an incomplete node is deleted from the graph, this method is used to determine the corresponding reverse dep edges to remove. If a node is reset but does not complete, it must report the reset deps here to keep the graph in a consistent state.

PiperOrigin-RevId: 563200630
Change-Id: Ic02dbb71c0ad24ac784fa04cf4503f9bdc66eb9a

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

@@ -377,6 +377,7 @@ public final synchronized ImmutableSet<SkyKey> getAllDirectDepsForIncompleteNode
     return ImmutableSet.<SkyKey>builder()
         .addAll(getTemporaryDirectDeps().getAllElementsAsIterable())
         .addAll(dirtyBuildingState.getAllRemainingDirtyDirectDeps(/* preservePosition= */ false))
+        .addAll(getResetDirectDeps())
         .build();
   }
 

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

@@ -452,12 +452,18 @@ default Version getMaxTransitiveSourceVersion() {
 
   /**
    * Returns all deps of a node that has not yet finished evaluating. In other words, if a node has
-   * a reverse dep on this node, its key will be in the returned set here. If this node was freshly
-   * created, this is just any elements that were added using one of the methods to add temporary
-   * direct deps (so it is the same as {@link #getTemporaryDirectDeps}). If this node is marked
-   * dirty, this includes all the elements that would have been returned by successive calls to
-   * {@link #getNextDirtyDirectDeps} (or, equivalently, one call to {@link
-   * #getAllRemainingDirtyDirectDeps}).
+   * a reverse dep on this node, its key will be in the returned set here.
+   *
+   * <p>The returned set is the union of:
+   *
+   * <ul>
+   *   <li>This node's {@linkplain #getTemporaryDirectDeps temporary direct deps}.
+   *   <li>Deps from a previous evaluation, if this this node was {@linkplain #markDirty marked
+   *       dirty} (all the elements that would have been returned by successive calls to {@link
+   *       #getNextDirtyDirectDeps} or, equivalently, one call to {@link
+   *       #getAllRemainingDirtyDirectDeps}).
+   *   <li>This node's {@linkplain #getResetDirectDeps reset direct deps}.
+   * </ul>
    *
    * <p>This method should only be called when this node is about to be deleted after an aborted
    * evaluation. After such an evaluation, any nodes that did not finish evaluating are deleted, as
@@ -564,7 +570,6 @@ default Version getMaxTransitiveSourceVersion() {
    * <p>If this node was reset multiple times since it was last done, must return deps requested
    * prior to <em>any</em> of those restarts, not just the most recent one.
    */
-  // TODO(b/228090759): These need to be added to getAllDirectDepsForIncompleteNode.
   @ThreadSafe
   ImmutableSet<SkyKey> getResetDirectDeps();
 

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

@@ -687,6 +687,20 @@ public void getAllDirectDepsForIncompleteNode_incrementalBuild() throws Exceptio
         .containsExactly(oldDep, oldAndNewDep, newDep);
   }
 
+  @Test
+  public void getAllDirectDepsForIncompleteNode_afterReset() throws Exception {
+    InMemoryNodeEntry entry = createEntry();
+    entry.addReverseDepAndCheckIfDone(null);
+    entry.markRebuilding();
+
+    SkyKey dep = key("dep");
+    entry.addSingletonTemporaryDirectDep(dep);
+    entry.signalDep(initialVersion, dep);
+    entry.resetForRestartFromScratch();
+
+    assertThat(entry.getAllDirectDepsForIncompleteNode()).containsExactly(dep);
+  }
+
   @Test
   public void resetOnDirtyNode(@TestParameter boolean valueChanges) throws Exception {
     InMemoryNodeEntry entry = createEntry();

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

@@ -62,6 +62,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -2404,8 +2405,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
     var result = tester.eval(/* keepGoing= */ false, top);
 
     assertThatEvaluationResult(result).hasEntryThat(top).isEqualTo(new StringValue("topVal"));
-    assertThat(inconsistencyReceiver.inconsistencies)
-        .containsExactly(InconsistencyData.resetRequested(top));
+    assertThat(inconsistencyReceiver).containsExactly(InconsistencyData.resetRequested(top));
 
     if (!incrementalitySupported()) {
       return; // Skip assertions on dependency edges when they aren't kept.
@@ -2469,8 +2469,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
     var result = tester.eval(/* keepGoing= */ false, top);
 
     assertThatEvaluationResult(result).hasEntryThat(top).isEqualTo(new StringValue("topVal"));
-    assertThat(inconsistencyReceiver.inconsistencies)
-        .containsExactly(InconsistencyData.resetRequested(top));
+    assertThat(inconsistencyReceiver).containsExactly(InconsistencyData.resetRequested(top));
 
     if (!incrementalitySupported()) {
       return; // Skip assertions on dependency edges when they aren't kept.
@@ -2532,8 +2531,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
     var result = tester.eval(/* keepGoing= */ false, top);
 
     assertThatEvaluationResult(result).hasEntryThat(top).isEqualTo(new StringValue("topVal"));
-    assertThat(inconsistencyReceiver.inconsistencies)
-        .containsExactly(InconsistencyData.resetRequested(top));
+    assertThat(inconsistencyReceiver).containsExactly(InconsistencyData.resetRequested(top));
 
     if (!incrementalitySupported()) {
       return; // Skip assertions on dependency edges when they aren't kept.
@@ -2544,7 +2542,65 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
     assertThatEvaluationResult(result).hasReverseDepsInGraphThat(flakyDep).isEmpty();
   }
 
-  private static final class RecordingInconsistencyReceiver implements GraphInconsistencyReceiver {
+  /**
+   * Tests that reset nodes are properly handled during invalidation after an aborted evaluation.
+   *
+   * <p>Invalidation deletes any nodes that are incomplete from the prior evaluation (in this case
+   * {@code top}). It should also remove the corresponding reverse dep edge from {@code dep} even
+   * though {@code top} does not have {@code dep} as a temporary direct dep when the evaluation is
+   * aborted.
+   *
+   * <p>An aborted evaluation can happen in practice when there is an error on a {@code
+   * --nokeep_going} build or if the user hits ctrl+c.
+   */
+  @Test
+  public void restartSelfOnly_evaluationAborted() throws Exception {
+    assume().that(restartSupported()).isTrue();
+    assume().that(incrementalitySupported()).isTrue();
+
+    var inconsistencyReceiver = new RecordingInconsistencyReceiver();
+    tester.setGraphInconsistencyReceiver(inconsistencyReceiver);
+    tester.initialize();
+
+    SkyKey top = skyKey("top");
+    SkyKey dep = skyKey("dep");
+
+    tester
+        .getOrCreate(top)
+        .setBuilder(
+            new SkyFunction() {
+              private boolean restarted = false;
+
+              @Nullable
+              @Override
+              public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+                if (restarted) {
+                  throw new InterruptedException("Evaluation aborted");
+                }
+                var depValue = env.getValue(dep);
+                if (depValue == null) {
+                  return null;
+                }
+                restarted = true;
+                return Restart.of(Restart.newRewindGraphFor(top));
+              }
+            });
+    tester.getOrCreate(dep).setConstantValue(new StringValue("depVal"));
+
+    assertThrows(InterruptedException.class, () -> tester.eval(/* keepGoing= */ false, top));
+    assertThat(inconsistencyReceiver).containsExactly(InconsistencyData.resetRequested(top));
+    inconsistencyReceiver.clear();
+
+    var result = tester.eval(/* keepGoing= */ false, dep);
+
+    assertThatEvaluationResult(result).hasEntryThat(dep).isEqualTo(new StringValue("depVal"));
+    assertThat(inconsistencyReceiver).isEmpty();
+    assertThatEvaluationResult(result).hasEntryThat(top).isNull();
+    assertThatEvaluationResult(result).hasReverseDepsInGraphThat(dep).isEmpty();
+  }
+
+  private static final class RecordingInconsistencyReceiver
+      implements GraphInconsistencyReceiver, Iterable<InconsistencyData> {
     private final List<InconsistencyData> inconsistencies = new ArrayList<>();
 
     @Override
@@ -2557,6 +2613,15 @@ public synchronized void noteInconsistencyAndMaybeThrow(
     public boolean restartPermitted() {
       return true;
     }
+
+    @Override
+    public Iterator<InconsistencyData> iterator() {
+      return inconsistencies.iterator();
+    }
+
+    void clear() {
+      inconsistencies.clear();
+    }
   }
 
   /**