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, 32 insertions, 2 deletions

diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
index 9b129df70e..b688880b1a 100644
--- a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
+++ b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
@@ -57,6 +57,7 @@ public class GraphTester {
 
   public GraphTester() {
     functionMap.put(NODE_TYPE, new DelegatingFunction());
+    functionMap.put(FOR_TESTING_NONHERMETIC, new DelegatingFunction());
   }
 
   public TestFunction getOrCreate(String name) {
@@ -170,6 +171,10 @@ public class GraphTester {
     return Key.create(key);
   }
 
+  public static NonHermeticKey nonHermeticKey(String key) {
+    return NonHermeticKey.create(key);
+  }
+
   /** A value in the testing graph that is constructed in the tester. */
   public static class TestFunction {
     // TODO(bazel-team): We could use a multiset here to simulate multi-pass dependency discovery.
@@ -421,11 +426,12 @@ public class GraphTester {
   static class Key extends AbstractSkyKey<String> {
     private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
 
-    @AutoCodec.VisibleForSerialization
-    Key(String arg) {
+    private Key(String arg) {
       super(arg);
     }
 
+    @AutoCodec.VisibleForSerialization
+    @AutoCodec.Instantiator
     static Key create(String arg) {
       return interner.intern(new Key(arg));
     }
@@ -435,4 +441,28 @@ public class GraphTester {
       return SkyFunctionName.FOR_TESTING;
     }
   }
+
+  @AutoCodec.VisibleForSerialization
+  @AutoCodec
+  static class NonHermeticKey extends AbstractSkyKey<String> {
+    private static final Interner<NonHermeticKey> interner = BlazeInterners.newWeakInterner();
+
+    private NonHermeticKey(String arg) {
+      super(arg);
+    }
+
+    @AutoCodec.VisibleForSerialization
+    @AutoCodec.Instantiator
+    static NonHermeticKey create(String arg) {
+      return interner.intern(new NonHermeticKey(arg));
+    }
+
+    @Override
+    public SkyFunctionName functionName() {
+      return FOR_TESTING_NONHERMETIC;
+    }
+  }
+
+  private static final SkyFunctionName FOR_TESTING_NONHERMETIC =
+      SkyFunctionName.createNonHermetic("FOR_TESTING_NONHERMETIC");
 }