Tolerate injected nodes having deps.

We don't check explicitly that these are the only two ways, but this can happen if the error transience node is a dep of a node that's being injected, or if an injected node is an "external" file that needs to depend on an external package.

The first possibility can happen if there was an IOException reading the node on the previous build.

We handle the situation by just dirtying the node, not injecting it. Actual evaluation can handle the re-stat.

PiperOrigin-RevId: 162622092

3 files changed, 57 insertions, 22 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index 1750263b43..e8b1f634ce 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -205,11 +205,19 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
       if (prevEntry != null && prevEntry.isDone()) {
         try {
           Iterable<SkyKey> directDeps = prevEntry.getDirectDeps();
-          Preconditions.checkState(
-              Iterables.isEmpty(directDeps), "existing entry for %s has deps: %s", key, directDeps);
-          if (newValue.equals(prevEntry.getValue())
-              && !valuesToDirty.contains(key)
-              && !valuesToDelete.contains(key)) {
+          if (Iterables.isEmpty(directDeps)) {
+            if (newValue.equals(prevEntry.getValue())
+                && !valuesToDirty.contains(key)
+                && !valuesToDelete.contains(key)) {
+              it.remove();
+            }
+          } else {
+            // Rare situation of an injected dep that depends on another node. Usually the dep is
+            // the error transience node. When working with external repositories, it can also be an
+            // external workspace file. Don't bother injecting it, just invalidate it.
+            // We'll wastefully evaluate the node freshly during evaluation, but this happens very
+            // rarely.
+            valuesToDirty.add(key);
             it.remove();
           }
         } catch (InterruptedException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index 50b98a6967..4c349f831f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -44,19 +44,21 @@ import com.google.devtools.build.lib.testutil.ManualClock;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.util.BlazeClock;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystem;
-import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.build.lib.vfs.util.FileSystems;
+import com.google.devtools.build.skyframe.BuildDriver;
 import com.google.devtools.build.skyframe.ErrorInfo;
+import com.google.devtools.build.skyframe.ErrorInfoSubject;
 import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
@@ -1272,6 +1274,41 @@ public class FileFunctionTest {
     assertThat(valueForPath(child).exists()).isFalse();
   }
 
+  @Test
+  public void testInjectionOverIOException() throws Exception {
+    Path foo = file("foo");
+    SkyKey fooKey = skyKey("foo");
+    fs.stubStatError(foo, new IOException("bork"));
+    BuildDriver driver = makeDriver();
+    EvaluationResult<FileValue> result =
+        driver.evaluate(
+            ImmutableList.of(fooKey),
+            /*keepGoing=*/ true,
+            /*numThreads=*/ 1,
+            NullEventHandler.INSTANCE);
+    ErrorInfoSubject errorInfoSubject = assertThatEvaluationResult(result)
+        .hasErrorEntryForKeyThat(fooKey);
+    errorInfoSubject.isTransient();
+    errorInfoSubject
+        .hasExceptionThat()
+        .hasMessageThat()
+        .isEqualTo("bork");
+    fs.stubbedStatErrors.remove(foo);
+    differencer.inject(
+        fileStateSkyKey("foo"),
+        FileStateValue.create(
+            RootedPath.toRootedPath(pkgRoot, foo),
+            new TimestampGranularityMonitor(BlazeClock.instance())));
+    result =
+        driver.evaluate(
+            ImmutableList.of(fooKey),
+            /*keepGoing=*/ true,
+            /*numThreads=*/ 1,
+            NullEventHandler.INSTANCE);
+    assertThatEvaluationResult(result).hasNoError();
+    assertThat(result.get(fooKey).exists()).isTrue();
+  }
+
   private void checkRealPath(String pathString) throws Exception {
     Path realPath = pkgRoot.getRelative(pathString).resolveSymbolicLinks();
     assertRealPath(pathString, realPath.relativeTo(pkgRoot).toString());
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index 7e21d9ee18..df97dc6b06 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -3773,14 +3773,11 @@ public class MemoizingEvaluatorTest {
     tester.getOrCreate(key).addDependency("other").setComputedValue(COPY);
     assertThat(tester.evalAndGet("value")).isEqualTo(prevVal);
     tester.differencer.inject(ImmutableMap.of(key, val));
-    try {
-      tester.evalAndGet("value");
-      fail("injection over value with deps should have failed");
-    } catch (IllegalStateException e) {
-      assertThat(e).hasMessage(
-          "existing entry for " + NODE_TYPE.getName() + ":value has deps: "
-              + "[" + NODE_TYPE.getName() + ":other]");
-    }
+    StringValue depVal = new StringValue("newfoo");
+    tester.getOrCreate("other").setConstantValue(depVal);
+    tester.differencer.invalidate(ImmutableList.of(GraphTester.toSkyKey("other")));
+    // Injected value is ignored for value with deps.
+    assertThat(tester.evalAndGet("value")).isEqualTo(depVal);
   }
 
   @Test
@@ -3792,14 +3789,7 @@ public class MemoizingEvaluatorTest {
     tester.getOrCreate(key).addDependency("other").setComputedValue(COPY);
     assertThat(tester.evalAndGet("value")).isEqualTo(val);
     tester.differencer.inject(ImmutableMap.of(key, val));
-    try {
-      tester.evalAndGet("value");
-      fail("injection over value with deps should have failed");
-    } catch (IllegalStateException e) {
-      assertThat(e).hasMessage(
-          "existing entry for " + NODE_TYPE.getName() + ":value has deps: "
-              + "[" + NODE_TYPE.getName() + ":other]");
-    }
+    assertThat(tester.evalAndGet("value")).isEqualTo(val);
   }
 
   @Test