Don't remove reverse deps until node is known to be changed. This helps avoid mutating the deps of nodes that are still going to be deps after evaluation is finished.

--
MOS_MIGRATED_REVID=103659429
diff --git a/src/main/java/com/google/devtools/build/skyframe/BuildingState.java b/src/main/java/com/google/devtools/build/skyframe/BuildingState.java
index 547549e..41b94ad 100644
--- a/src/main/java/com/google/devtools/build/skyframe/BuildingState.java
+++ b/src/main/java/com/google/devtools/build/skyframe/BuildingState.java
@@ -73,9 +73,10 @@
 
   /**
    * The state of a dirty node. A node is marked dirty in the BuildingState constructor, and goes
-   * into either the state {@link DirtyState#CHECK_DEPENDENCIES} or {@link DirtyState#REBUILDING},
-   * depending on whether the caller specified that the node was itself changed or not. A non-null
-   * {@code dirtyState} indicates that the node {@link #isDirty} in some way.
+   * into either the state {@link DirtyState#CHECK_DEPENDENCIES} or
+   * {@link DirtyState#NEEDS_REBUILDING}, depending on whether the caller specified that the node
+   * was itself changed or not. A non-null {@code dirtyState} indicates that the node
+   * {@link #isDirty} in some way.
    */
   private DirtyState dirtyState = null;
 
@@ -118,41 +119,41 @@
   // node will be removed by the time evaluation starts, so reverse deps to signal can just be
   // reverse deps in the main ValueEntry object.
   private Object reverseDepsToSignal = ImmutableList.of();
-  private List<SkyKey> reverseDepsToRemove = null;
+  private Object reverseDepsDataToConsolidate = null;
   private boolean reverseDepIsSingleObject = false;
 
   private static final ReverseDepsUtil<BuildingState> REVERSE_DEPS_UTIL =
       new ReverseDepsUtil<BuildingState>() {
-    @Override
-    void setReverseDepsObject(BuildingState container, Object object) {
-      container.reverseDepsToSignal = object;
-    }
+        @Override
+        void setReverseDepsObject(BuildingState container, Object object) {
+          container.reverseDepsToSignal = object;
+        }
 
-    @Override
-    void setSingleReverseDep(BuildingState container, boolean singleObject) {
-      container.reverseDepIsSingleObject = singleObject;
-    }
+        @Override
+        void setSingleReverseDep(BuildingState container, boolean singleObject) {
+          container.reverseDepIsSingleObject = singleObject;
+        }
 
-    @Override
-    void setReverseDepsToRemove(BuildingState container, List<SkyKey> object) {
-      container.reverseDepsToRemove = object;
-    }
+        @Override
+        void setDataToConsolidate(BuildingState container, Object dataToConsolidate) {
+          container.reverseDepsDataToConsolidate = dataToConsolidate;
+        }
 
-    @Override
-    Object getReverseDepsObject(BuildingState container) {
-      return container.reverseDepsToSignal;
-    }
+        @Override
+        Object getReverseDepsObject(BuildingState container) {
+          return container.reverseDepsToSignal;
+        }
 
-    @Override
-    boolean isSingleReverseDep(BuildingState container) {
-      return container.reverseDepIsSingleObject;
-    }
+        @Override
+        boolean isSingleReverseDep(BuildingState container) {
+          return container.reverseDepIsSingleObject;
+        }
 
-    @Override
-    List<SkyKey> getReverseDepsToRemove(BuildingState container) {
-      return container.reverseDepsToRemove;
-    }
-  };
+        @Override
+        Object getDataToConsolidate(BuildingState container) {
+          return container.reverseDepsDataToConsolidate;
+        }
+      };
 
   // Below are fields that are used for dirty nodes.
 
@@ -180,12 +181,10 @@
     Preconditions.checkState(isChanged || !this.lastBuildDirectDeps.isEmpty(),
         "%s is being marked dirty, not changed, but has no children that could have dirtied it",
         this);
-    dirtyState = isChanged ? DirtyState.REBUILDING
-        : DirtyState.CHECK_DEPENDENCIES;
-    if (dirtyState == DirtyState.CHECK_DEPENDENCIES) {
-      // We need to iterate through the deps to see if they have changed. Initialize the iterator.
-      dirtyDirectDepIterator = lastBuildDirectDeps.iterator();
-    }
+    dirtyState = isChanged ? DirtyState.NEEDS_REBUILDING : DirtyState.CHECK_DEPENDENCIES;
+    // We need to iterate through the deps to see if they have changed, or to remove them if one
+    // has. Initialize the iterator.
+    dirtyDirectDepIterator = lastBuildDirectDeps.iterator();
   }
 
   static BuildingState newDirtyState(boolean isChanged,
@@ -197,7 +196,7 @@
     Preconditions.checkState(isDirty(), this);
     Preconditions.checkState(!isChanged(), this);
     Preconditions.checkState(!evaluating, this);
-    dirtyState = DirtyState.REBUILDING;
+    dirtyState = DirtyState.NEEDS_REBUILDING;
   }
 
   void forceChanged() {
@@ -234,11 +233,7 @@
    * @see NodeEntry#isChanged()
    */
   boolean isChanged() {
-    return dirtyState == DirtyState.REBUILDING;
-  }
-
-  private boolean rebuilding() {
-    return dirtyState == DirtyState.REBUILDING;
+    return dirtyState == DirtyState.NEEDS_REBUILDING || dirtyState == DirtyState.REBUILDING;
   }
 
   /**
@@ -246,10 +241,14 @@
    * actually like to check that the node has been evaluated, but that is not available in
    * this context.
    */
-  private void checkNotProcessing() {
+  private void checkFinishedBuildingWhenAboutToSetValue() {
     Preconditions.checkState(evaluating, "not started building %s", this);
-    Preconditions.checkState(!isDirty() || dirtyState == DirtyState.VERIFIED_CLEAN
-        || rebuilding(), "not done building %s", this);
+    Preconditions.checkState(
+        !isDirty()
+            || dirtyState == DirtyState.VERIFIED_CLEAN
+            || dirtyState == DirtyState.REBUILDING,
+        "not done building %s",
+        this);
     Preconditions.checkState(isReady(), "not done building %s", this);
   }
 
@@ -269,7 +268,7 @@
    * finished is equal to the number of known children.
    *
    * <p>If the node is dirty and checking its deps for changes, this also updates {@link
-   * #dirtyState} as needed -- {@link DirtyState#REBUILDING} if the child has changed,
+   * #dirtyState} as needed -- {@link DirtyState#NEEDS_REBUILDING} if the child has changed,
    * and {@link DirtyState#VERIFIED_CLEAN} if the child has not changed and this was the
    * last child to be checked (as determined by {@link #dirtyDirectDepIterator}.hasNext() and
    * isReady()).
@@ -278,15 +277,15 @@
    */
   boolean signalDep(boolean childChanged) {
     signaledDeps++;
-    if (isDirty() && !rebuilding()) {
-      // Synchronization isn't needed here because the only caller is ValueEntry, which does it
-      // through the synchronized method signalDep(long).
+    if (isDirty() && !isChanged()) {
+      // Synchronization isn't needed here because the only caller is NodeEntry, which does it
+      // through the synchronized method signalDep(Version).
       if (childChanged) {
-        dirtyState = DirtyState.REBUILDING;
+        dirtyState = DirtyState.NEEDS_REBUILDING;
       } else if (dirtyState == DirtyState.CHECK_DEPENDENCIES
           && isReady()
           && !dirtyDirectDepIterator.hasNext()) {
-        // No other dep already marked this as REBUILDING, no deps outstanding, and this was
+        // No other dep already marked this as NEEDS_REBUILDING, no deps outstanding, and this was
         // the last block of deps to be checked.
         dirtyState = DirtyState.VERIFIED_CLEAN;
       }
@@ -300,7 +299,7 @@
    * was built, in the same order. Should only be used by {@link NodeEntry#setValue}.
    */
   boolean unchangedFromLastBuild(SkyValue newValue) {
-    checkNotProcessing();
+    checkFinishedBuildingWhenAboutToSetValue();
     return getLastBuildValue().equals(newValue) && lastBuildDirectDeps.equals(directDeps);
   }
 
@@ -341,6 +340,22 @@
     return nextDeps;
   }
 
+  Collection<SkyKey> getAllRemainingDirtyDirectDeps() {
+    Preconditions.checkState(isDirty(), this);
+    ImmutableList.Builder<SkyKey> result = ImmutableList.builder();
+    while (dirtyDirectDepIterator.hasNext()) {
+      result.addAll(dirtyDirectDepIterator.next());
+    }
+    return result.build();
+  }
+
+  Collection<SkyKey> markRebuildingAndGetAllRemainingDirtyDirectDeps() {
+    Preconditions.checkState(dirtyState == DirtyState.NEEDS_REBUILDING, this);
+    Collection<SkyKey> result = getAllRemainingDirtyDirectDeps();
+    dirtyState = DirtyState.REBUILDING;
+    return result;
+  }
+
   void addDirectDeps(GroupedListHelper<SkyKey> depsThisRun) {
     directDeps.append(depsThisRun);
   }
@@ -380,7 +395,7 @@
    * @see NodeEntry#addReverseDepAndCheckIfDone(SkyKey)
    */
   void addReverseDepToSignal(SkyKey newReverseDep) {
-    REVERSE_DEPS_UTIL.consolidateReverseDepsRemovals(this);
+    REVERSE_DEPS_UTIL.consolidateData(this);
     REVERSE_DEPS_UTIL.addReverseDeps(this, Collections.singleton(newReverseDep));
   }