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, 49 insertions, 44 deletions

diff --git a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
index d9ebca5ebc..223b14b37c 100644
--- a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
@@ -185,19 +185,20 @@ public class EagerInvalidatorTest {
       }
     });
     graph = new InMemoryGraphImpl();
-    set("a", "a");
-    set("b", "b");
-    tester.getOrCreate("ab").addDependency("a").addDependency("b")
-        .setComputedValue(CONCATENATE);
+    SkyKey aKey = GraphTester.nonHermeticKey("a");
+    SkyKey bKey = GraphTester.nonHermeticKey("b");
+    tester.set(aKey, new StringValue("a"));
+    tester.set(bKey, new StringValue("b"));
+    tester.getOrCreate("ab").addDependency(aKey).addDependency(bKey).setComputedValue(CONCATENATE);
     assertValueValue("ab", "ab");
 
-    set("a", "c");
-    invalidateWithoutError(receiver, skyKey("a"));
-    assertThat(invalidated).containsExactly(skyKey("a"), skyKey("ab"));
+    tester.set(aKey, new StringValue("c"));
+    invalidateWithoutError(receiver, aKey);
+    assertThat(invalidated).containsExactly(aKey, skyKey("ab"));
     assertValueValue("ab", "cb");
-    set("b", "d");
-    invalidateWithoutError(receiver, skyKey("b"));
-    assertThat(invalidated).containsExactly(skyKey("a"), skyKey("ab"), skyKey("b"));
+    tester.set(bKey, new StringValue("d"));
+    invalidateWithoutError(receiver, bKey);
+    assertThat(invalidated).containsExactly(aKey, skyKey("ab"), bKey);
   }
 
   @Test
@@ -215,15 +216,16 @@ public class EagerInvalidatorTest {
     // Given a graph consisting of two nodes, "a" and "ab" such that "ab" depends on "a",
     // And given "ab" is in error,
     graph = new InMemoryGraphImpl();
-    set("a", "a");
-    tester.getOrCreate("ab").addDependency("a").setHasError(true);
+    SkyKey aKey = GraphTester.nonHermeticKey("a");
+    tester.set(aKey, new StringValue("a"));
+    tester.getOrCreate("ab").addDependency(aKey).setHasError(true);
     eval(false, skyKey("ab"));
 
     // When "a" is invalidated,
-    invalidateWithoutError(receiver, skyKey("a"));
+    invalidateWithoutError(receiver, aKey);
 
     // Then the invalidation receiver is notified of both "a" and "ab"'s invalidations.
-    assertThat(invalidated).containsExactly(skyKey("a"), skyKey("ab"));
+    assertThat(invalidated).containsExactly(aKey, skyKey("ab"));
 
     // Note that this behavior isn't strictly required for correctness. This test is
     // meant to document current behavior and protect against programming error.
@@ -241,20 +243,22 @@ public class EagerInvalidatorTest {
       }
     });
     graph = new InMemoryGraphImpl();
-    invalidateWithoutError(receiver, skyKey("a"));
+    SkyKey aKey = GraphTester.nonHermeticKey("a");
+    invalidateWithoutError(receiver, aKey);
     assertThat(invalidated).isEmpty();
-    set("a", "a");
-    assertValueValue("a", "a");
-    invalidateWithoutError(receiver, skyKey("b"));
+    tester.set(aKey, new StringValue("a"));
+    StringValue value = (StringValue) eval(false, aKey);
+    assertThat(value.getValue()).isEqualTo("a");
+    invalidateWithoutError(receiver, GraphTester.nonHermeticKey("b"));
     assertThat(invalidated).isEmpty();
   }
 
   @Test
   public void invalidatedValuesAreGCedAsExpected() throws Exception {
-    SkyKey key = GraphTester.skyKey("a");
+    SkyKey key = GraphTester.nonHermeticKey("a");
     HeavyValue heavyValue = new HeavyValue();
     WeakReference<HeavyValue> weakRef = new WeakReference<>(heavyValue);
-    tester.set("a", heavyValue);
+    tester.set(key, heavyValue);
 
     graph = new InMemoryGraphImpl();
     eval(false, key);
@@ -278,30 +282,34 @@ public class EagerInvalidatorTest {
     set("a", "a");
     set("b", "b");
     set("c", "c");
-    tester.getOrCreate("ab").addDependency("a").addDependency("b").setComputedValue(CONCATENATE);
+    SkyKey abKey = GraphTester.nonHermeticKey("ab");
+    tester.getOrCreate(abKey).addDependency("a").addDependency("b").setComputedValue(CONCATENATE);
     tester.getOrCreate("bc").addDependency("b").addDependency("c").setComputedValue(CONCATENATE);
-    tester.getOrCreate("ab_c").addDependency("ab").addDependency("c")
+    tester
+        .getOrCreate("ab_c")
+        .addDependency(abKey)
+        .addDependency("c")
         .setComputedValue(CONCATENATE);
     eval(false, skyKey("ab_c"), skyKey("bc"));
 
     assertThat(graph.get(null, Reason.OTHER, skyKey("a")).getReverseDepsForDoneEntry())
-        .containsExactly(skyKey("ab"));
+        .containsExactly(abKey);
     assertThat(graph.get(null, Reason.OTHER, skyKey("b")).getReverseDepsForDoneEntry())
-        .containsExactly(skyKey("ab"), skyKey("bc"));
+        .containsExactly(abKey, skyKey("bc"));
     assertThat(graph.get(null, Reason.OTHER, skyKey("c")).getReverseDepsForDoneEntry())
         .containsExactly(skyKey("ab_c"), skyKey("bc"));
 
-    invalidateWithoutError(new DirtyTrackingProgressReceiver(null), skyKey("ab"));
+    invalidateWithoutError(new DirtyTrackingProgressReceiver(null), abKey);
     eval(false);
 
     // The graph values should be gone.
-    assertThat(isInvalidated(skyKey("ab"))).isTrue();
+    assertThat(isInvalidated(abKey)).isTrue();
     assertThat(isInvalidated(skyKey("abc"))).isTrue();
 
     // The reverse deps to ab and ab_c should have been removed if reverse deps are cleared.
     Set<SkyKey> reverseDeps = new HashSet<>();
     if (reverseDepsPresent()) {
-      reverseDeps.add(skyKey("ab"));
+      reverseDeps.add(abKey);
     }
     assertThat(graph.get(null, Reason.OTHER, skyKey("a")).getReverseDepsForDoneEntry())
         .containsExactlyElementsIn(reverseDeps);
@@ -321,9 +329,9 @@ public class EagerInvalidatorTest {
   public void interruptChild() throws Exception {
     graph = new InMemoryGraphImpl();
     int numValues = 50; // More values than the invalidator has threads.
-    final SkyKey[] family = new SkyKey[numValues];
-    final SkyKey child = GraphTester.skyKey("child");
-    final StringValue childValue = new StringValue("child");
+    SkyKey[] family = new SkyKey[numValues];
+    SkyKey child = GraphTester.nonHermeticKey("child");
+    StringValue childValue = new StringValue("child");
     tester.set(child, childValue);
     family[0] = child;
     for (int i = 1; i < numValues; i++) {
@@ -400,11 +408,12 @@ public class EagerInvalidatorTest {
     SkyKey[] values = new SkyKey[size];
     for (int i = 0; i < size; i++) {
       String iString = Integer.toString(i);
-      SkyKey iKey = GraphTester.toSkyKey(iString);
+      SkyKey iKey = GraphTester.nonHermeticKey(iString);
+      tester.set(iKey, new StringValue(iString));
       set(iString, iString);
       for (int j = 0; j < i; j++) {
         if (random.nextInt(3) == 0) {
-          tester.getOrCreate(iKey).addDependency(Integer.toString(j));
+          tester.getOrCreate(iKey).addDependency(GraphTester.nonHermeticKey(Integer.toString(j)));
         }
       }
       values[i] = iKey;
@@ -488,11 +497,12 @@ public class EagerInvalidatorTest {
 
   protected void setupInvalidatableGraph() throws Exception {
     graph = new InMemoryGraphImpl();
-    set("a", "a");
+    SkyKey aKey = GraphTester.nonHermeticKey("a");
+    tester.set(aKey, new StringValue("a"));
     set("b", "b");
-    tester.getOrCreate("ab").addDependency("a").addDependency("b").setComputedValue(CONCATENATE);
+    tester.getOrCreate("ab").addDependency(aKey).addDependency("b").setComputedValue(CONCATENATE);
     assertValueValue("ab", "ab");
-    set("a", "c");
+    tester.set(aKey, new StringValue("c"));
   }
 
   private static class HeavyValue implements SkyValue {
@@ -549,20 +559,15 @@ public class EagerInvalidatorTest {
           new EvaluationProgressReceiver.NullEvaluationProgressReceiver());
 
       // Dirty the node, and ensure that the tracker is aware of it:
-      Iterable<SkyKey> diff1 = ImmutableList.of(skyKey("a"));
+      ImmutableList<SkyKey> diff = ImmutableList.of(GraphTester.nonHermeticKey("a"));
       InvalidationState state1 = new DirtyingInvalidationState();
       Preconditions.checkNotNull(
               EagerInvalidator.createInvalidatingVisitorIfNeeded(
-                  graph,
-                  diff1,
-                  receiver,
-                  state1,
-                  AbstractQueueVisitor.EXECUTOR_FACTORY))
+                  graph, diff, receiver, state1, AbstractQueueVisitor.EXECUTOR_FACTORY))
           .run();
-      assertThat(receiver.getUnenqueuedDirtyKeys()).containsExactly(skyKey("a"), skyKey("ab"));
+      assertThat(receiver.getUnenqueuedDirtyKeys()).containsExactly(diff.get(0), skyKey("ab"));
 
       // Delete the node, and ensure that the tracker is no longer tracking it:
-      Iterable<SkyKey> diff = ImmutableList.of(skyKey("a"));
       Preconditions.checkNotNull(EagerInvalidator.createDeletingVisitorIfNeeded(graph, diff,
           receiver, state, true)).run();
       assertThat(receiver.getUnenqueuedDirtyKeys()).isEmpty();
@@ -624,7 +629,7 @@ public class EagerInvalidatorTest {
           new EvaluationProgressReceiver.NullEvaluationProgressReceiver());
 
       // Dirty the node, and ensure that the tracker is aware of it:
-      invalidate(graph, receiver, skyKey("a"));
+      invalidate(graph, receiver, GraphTester.nonHermeticKey("a"));
       assertThat(receiver.getUnenqueuedDirtyKeys()).hasSize(2);
     }
   }