Fix blatant logic bug in FileFunction that causes incorrect FileValue…

…s on incremental evaluations.

For a path like "<prefix>/b", we should be using FileStateValue(r/b), where "r" is the fully resolved path of "<prefix>", when we construct the FileValue. This is important in the case where "<prefix>" has a different resolved path (due to ancestor directory symlinks). This was the intent of the FileFunction + FileStateValue algorithm, but the code's structure hid the bug. While I'm here, also slightly improve this.

The bug is: if "r/b" changes (e.g. goes from a directory to a symlink, or goes from a symlink pointing to t1 to a symlink pointing to t2), then the Skyframe's diff will correctly contain FileStateValue(r/b). But if FileFunction(<prefix>/b) uses FileStateValue(<prefix>/b), then FileValue(<prefix>/b) will be wrong. Note that FileFunction#resolveFromAncestors is already aptly declaring the dep on FileValue(r/b) -- the bug is that we're not using it when constructing the FileValue!

Fwiw, this bug is most easily exploited when multiple levels of directory symlinks are involved. That's a fairly obscure setup, so I'm not super surprised it took us this long to notice this bug.

RELNOTES: None
PiperOrigin-RevId: 232803261

src/main/java/com/google/devtools/build/lib/actions/FileValue.java

@@ -150,22 +150,26 @@ public SkyFunctionName functionName() {
    * not be used for symlink cycles.
    */
   public static FileValue value(
-      RootedPath rootedPath,
-      FileStateValue fileStateValue,
+      RootedPath originalRootedPath,
+      FileStateValue fileStateValueFromAncestors,
       RootedPath realRootedPath,
       FileStateValue realFileStateValue) {
-    if (rootedPath.equals(realRootedPath)) {
-      Preconditions.checkState(fileStateValue.getType() != FileStateType.SYMLINK,
-          "rootedPath: %s, fileStateValue: %s, realRootedPath: %s, realFileStateValue: %s",
-          rootedPath, fileStateValue, realRootedPath, realFileStateValue);
-      return new RegularFileValue(rootedPath, fileStateValue);
+    if (originalRootedPath.equals(realRootedPath)) {
+      Preconditions.checkState(
+          fileStateValueFromAncestors.getType() != FileStateType.SYMLINK,
+          "originalRootedPath: %s, fileStateValue: %s, realRootedPath: %s, "
+              + "fileStateValueFromAncestors: %s",
+          originalRootedPath,
+          fileStateValueFromAncestors,
+          realRootedPath,
+          realFileStateValue);
+      return new RegularFileValue(originalRootedPath, fileStateValueFromAncestors);
     } else {
-      if (fileStateValue.getType() == FileStateType.SYMLINK) {
-        return new SymlinkFileValue(realRootedPath, realFileStateValue,
-            fileStateValue.getSymlinkTarget());
+      if (fileStateValueFromAncestors.getType() == FileStateType.SYMLINK) {
+        return new SymlinkFileValue(
+            realRootedPath, realFileStateValue, fileStateValueFromAncestors.getSymlinkTarget());
       } else {
-        return new DifferentRealPathFileValue(
-            realRootedPath, realFileStateValue);
+        return new DifferentRealPathFileValue(realRootedPath, realFileStateValue);
       }
     }
   }

src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java

@@ -71,45 +71,42 @@ public FileFunction(AtomicReference<PathPackageLocator> pkgLocator) {
   public FileValue compute(SkyKey skyKey, Environment env)
       throws FileFunctionException, InterruptedException {
     RootedPath rootedPath = (RootedPath) skyKey.argument();
-    RootedPath realRootedPath = null;
-    FileStateValue realFileStateValue = null;
     PathFragment relativePath = rootedPath.getRootRelativePath();
 
-    // Resolve ancestor symlinks, but only if the current file is not the filesystem root (has no
-    // parent) or a package path root (treated opaquely and handled by skyframe's DiffAwareness
-    // interface). Note that this is the first thing we do - if an ancestor is part of a
-    // symlink cycle, we want to detect that quickly as it gives a more informative error message
-    // than we'd get doing bogus filesystem operations.
-    if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) {
+    // Fully resolve the path of the parent directory, but only if the current file is not the
+    // filesystem root (has no parent) or a package path root (treated opaquely and handled by
+    // skyframe's DiffAwareness interface).
+    //
+    // This entails resolving ancestor symlinks fully. Note that this is the first thing we do - if
+    // an ancestor is part of a symlink cycle, we want to detect that quickly as it gives a more
+    // informative error message than we'd get doing bogus filesystem operations.
+    RootedPath rootedPathFromAncestors;
+    FileStateValue fileStateValueFromAncestors;
+    if (relativePath.equals(PathFragment.EMPTY_FRAGMENT)) {
+      FileStateValue fileStateValue = (FileStateValue) env.getValue(FileStateValue.key(rootedPath));
+      if (fileStateValue == null) {
+        return null;
+      }
+      rootedPathFromAncestors = rootedPath;
+      fileStateValueFromAncestors = fileStateValue;
+    } else {
       Pair<RootedPath, FileStateValue> resolvedState = resolveFromAncestors(rootedPath, env);
       if (resolvedState == null) {
         return null;
       }
-      realRootedPath = resolvedState.getFirst();
-      realFileStateValue = resolvedState.getSecond();
-      if (realFileStateValue.getType() == FileStateType.NONEXISTENT) {
+      rootedPathFromAncestors = resolvedState.getFirst();
+      fileStateValueFromAncestors = resolvedState.getSecond();
+      if (fileStateValueFromAncestors.getType() == FileStateType.NONEXISTENT) {
         return FileValue.value(
             rootedPath,
             FileStateValue.NONEXISTENT_FILE_STATE_NODE,
-            realRootedPath,
-            realFileStateValue);
-      }
-    }
-
-    FileStateValue fileStateValue;
-    if (rootedPath.equals(realRootedPath)) {
-      fileStateValue = Preconditions.checkNotNull(realFileStateValue, rootedPath);
-    } else {
-      fileStateValue = (FileStateValue) env.getValue(FileStateValue.key(rootedPath));
-      if (fileStateValue == null) {
-        return null;
+            rootedPathFromAncestors,
+            fileStateValueFromAncestors);
       }
     }
 
-    if (realFileStateValue == null) {
-      realRootedPath = rootedPath;
-      realFileStateValue = fileStateValue;
-    }
+    RootedPath realRootedPath = rootedPathFromAncestors;
+    FileStateValue realFileStateValue = fileStateValueFromAncestors;
 
     ArrayList<RootedPath> symlinkChain = new ArrayList<>();
     TreeSet<Path> orderedSeenPaths = Sets.newTreeSet();
@@ -124,7 +121,9 @@ public FileValue compute(SkyKey skyKey, Environment env)
       realRootedPath = resolvedState.getFirst();
       realFileStateValue = resolvedState.getSecond();
     }
-    return FileValue.value(rootedPath, fileStateValue, realRootedPath, realFileStateValue);
+
+    return FileValue.value(
+        rootedPath, fileStateValueFromAncestors, realRootedPath, realFileStateValue);
   }
 
   /**

src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java

@@ -199,17 +199,17 @@ private SequentialBuildDriver makeDriver(ExternalFileAction externalFileAction)
   }
 
   private FileValue valueForPath(Path path) throws InterruptedException {
-    return valueForPathHelper(pkgRoot, path);
+    return valueForPathHelper(pkgRoot, path, makeDriver());
   }
 
   private FileValue valueForPathOutsidePkgRoot(Path path) throws InterruptedException {
-    return valueForPathHelper(Root.absoluteRoot(fs), path);
+    return valueForPathHelper(Root.absoluteRoot(fs), path, makeDriver());
   }
 
-  private FileValue valueForPathHelper(Root root, Path path) throws InterruptedException {
+  private FileValue valueForPathHelper(Root root, Path path, SequentialBuildDriver driver)
+      throws InterruptedException {
     PathFragment pathFragment = root.relativize(path);
     RootedPath rootedPath = RootedPath.toRootedPath(root, pathFragment);
-    SequentialBuildDriver driver = makeDriver();
     SkyKey key = FileValue.key(rootedPath);
     EvaluationResult<FileValue> result = driver.evaluate(ImmutableList.of(key), EVALUATION_OPTIONS);
     assertThat(result.hasError()).isFalse();
@@ -1261,6 +1261,35 @@ public void testInjectionOverIOException() throws Exception {
     assertThat(result.get(fooKey).exists()).isTrue();
   }
 
+  @Test
+  public void testMultipleLevelsOfDirectorySymlinks_Clean() throws Exception {
+    symlink("a/b/c", "../c");
+    Path abcd = path("a/b/c/d");
+    symlink("a/c/d", "../d");
+    assertThat(valueForPath(abcd).isSymlink()).isTrue();
+  }
+
+  @Test
+  public void testMultipleLevelsOfDirectorySymlinks_Incremental() throws Exception {
+    SequentialBuildDriver driver = makeDriver();
+
+    symlink("a/b/c", "../c");
+    Path acd = directory("a/c/d");
+    Path abcd = path("a/b/c/d");
+
+    FileValue abcdFileValue = valueForPathHelper(pkgRoot, abcd, driver);
+    assertThat(abcdFileValue.isDirectory()).isTrue();
+    assertThat(abcdFileValue.isSymlink()).isFalse();
+
+    acd.delete();
+    symlink("a/c/d", "../d");
+    differencer.invalidate(ImmutableList.of(fileStateSkyKey("a/c/d")));
+
+    abcdFileValue = valueForPathHelper(pkgRoot, abcd, driver);
+
+    assertThat(abcdFileValue.isSymlink()).isTrue();
+  }
+
   private void checkRealPath(String pathString) throws Exception {
     Path realPath = pkgRoot.getRelative(pathString).resolveSymbolicLinks();
     assertRealPath(pathString, pkgRoot.relativize(realPath).toString());