Set the version of a computed node to the max of its child versions rather than the graph version when that is feasible.

  * It's not feasible when the computation accesses outside state, i.e. is non-hermetic, so see below.
  * It's also more complicated (and not worth the trouble) when the computation is taking place just for the error status.

Have SkyFunctionName declare whether the function it corresponds to is hermetic or non-hermetic. Only non-hermetically-generated SkyValues can be directly marked changed, and non-hermetic SkyFunctions have their values saved at the graph version, not the max of the child versions. All SkyFunctions are hermetic except for the ones that can be explicitly dirtied.

A marked-hermetic SkyFunction that has a transient error due to filesystem access can be re-evaluated and get the correct version: if it throws an IOException at version 1 and then, when re-evaluated at version 2 with unchanged dependencies, has a value, the version will be version 1.

All Skyframe unit tests that were doing non-hermetic things to nodes need to declare that those nodes are non-hermetic. I tried to make the minimal set of changes there, so that we had good incidental coverage of hermetic+non-hermetic nodes. Also did some drive-by clean-ups around that code.

Artifacts are a weird case, since they're doing untracked filesystem access (for source directories). Using max(child versions) for them gives rise to the following correctness bug: 1. do a build at v1 that creates a FileStateValue for dir/ at v1. Then at v2, add a file to dir/ and do a build that consumes dir/ as a source artifact. Now the artifact for dir/ will (incorrectly) have v1. Then at v1, do that build again. We'll consume the "artifact from the future". However, this can only have an effect when using the local action cache, since the incorrect value of the artifact (the mtime) is only consumed by the action cache. Bazel is already broken in this way (incremental builds don't invalidate directories), so this change doesn't make things worse.

PiperOrigin-RevId: 204210719

1 files changed, 5 insertions, 2 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
index a82d1158df..af658f32d5 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
@@ -114,7 +114,8 @@ public abstract class AbstractExceptionalParallelEvaluator<E extends Exception>
       DirtyTrackingProgressReceiver progressReceiver,
       GraphInconsistencyReceiver graphInconsistencyReceiver,
       ForkJoinPool forkJoinPool,
-      CycleDetector cycleDetector) {
+      CycleDetector cycleDetector,
+      EvaluationVersionBehavior evaluationVersionBehavior) {
     super(
         graph,
         graphVersion,
@@ -127,7 +128,8 @@ public abstract class AbstractExceptionalParallelEvaluator<E extends Exception>
         progressReceiver,
         graphInconsistencyReceiver,
         forkJoinPool,
-        cycleDetector);
+        cycleDetector,
+        evaluationVersionBehavior);
   }
 
   private void informProgressReceiverThatValueIsDone(SkyKey key, NodeEntry entry)
@@ -469,6 +471,7 @@ public abstract class AbstractExceptionalParallelEvaluator<E extends Exception>
             maybeMarkRebuilding(parentEntry);
             // Fall through to REBUILDING.
           case REBUILDING:
+          case FORCED_REBUILDING:
             break;
           default:
             throw new AssertionError(parent + " not in valid dirty state: " + parentEntry);