Fix blatant bug with Skyframe globbing where we incorrectly allow dangling symlinks to match a glob pattern.

This bug has/had two consequences:

(1) Change pruning will incorrectly cut off changes to GlobValues that ought to now match more files (say, if a dangling symlink comes into existence), causing a package to be incorrectly incrementally not re-loaded.

(2) After a recent change to PackageFunction where we use a fancy hybrid globbing approach, we use skyframe globbing on incremental package loading. So if a re-loaded package has the same glob pattern but this glob pattern incorrectly matches a dangling symlink, the re-loaded package will incorrectly have a target for the dangling symlink path.

--
MOS_MIGRATED_REVID=115274842

3 files changed, 69 insertions, 0 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
index 50b7c294c7..e1c877f455 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
@@ -146,6 +146,9 @@ public final class GlobFunction implements SkyFunction {
                 "readdir and stat disagree about whether " + symlinkRootedPath.asPath()
                     + " is a symlink."), Transience.TRANSIENT);
           }
+          if (!symlinkFileValue.exists()) {
+            continue;
+          }
           isDirectory = symlinkFileValue.isDirectory();
         }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
index 32dffabba7..0246f864e2 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
@@ -676,6 +676,15 @@ public abstract class GlobFunctionTest {
     assertThat(errorInfo.getException().getMessage()).contains(expectedMessage);
   }
 
+  @Test
+  public void testSymlinks() throws Exception {
+    FileSystemUtils.createDirectoryAndParents(pkgPath.getRelative("symlinks"));
+    FileSystemUtils.ensureSymbolicLink(pkgPath.getRelative("symlinks/dangling.txt"), "nope");
+    FileSystemUtils.createEmptyFile(pkgPath.getRelative("symlinks/yup"));
+    FileSystemUtils.ensureSymbolicLink(pkgPath.getRelative("symlinks/existing.txt"), "yup");
+    assertGlobMatches("symlinks/*.txt", "symlinks/existing.txt");
+  }
+
   private class CustomInMemoryFs extends InMemoryFileSystem {
 
     private Map<Path, FileStatus> stubbedStats = Maps.newHashMap();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
index 5a5d7cc913..8cd8003f26 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
+import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Preprocessor;
 import com.google.devtools.build.lib.packages.util.SubincludePreprocessor;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
@@ -513,6 +514,62 @@ public class PackageFunctionTest extends BuildViewTestCase {
     assertTrue(result.get(skyKey).getPackage().containsErrors());
   }
 
+  // Regression test for the two ugly consequences of a bug where GlobFunction incorrectly matched
+  // dangling symlinks.
+  @Test
+  public void testIncrementalSkyframeHybridGlobbingOnDanglingSymlink() throws Exception {
+    Path packageDirPath = scratch.file("foo/BUILD",
+        "exports_files(glob(['*.txt']))").getParentDirectory();
+    scratch.file("foo/existing.txt");
+    FileSystemUtils.ensureSymbolicLink(packageDirPath.getChild("dangling.txt"), "nope");
+
+    getSkyframeExecutor().preparePackageLoading(
+        new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)),
+        ConstantRuleVisibility.PUBLIC, true,
+        7, "", UUID.randomUUID());
+
+    SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("foo"));
+    PackageValue value = validPackage(skyKey);
+    assertFalse(value.getPackage().containsErrors());
+    assertThat(value.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt");
+    try {
+      value.getPackage().getTarget("dangling.txt");
+      fail();
+    } catch (NoSuchTargetException expected) {
+    }
+
+    scratch.overwriteFile("foo/BUILD",
+        "exports_files(glob(['*.txt'])),",
+        "#some-irrelevant-comment");
+
+    getSkyframeExecutor().invalidateFilesUnderPathForTesting(reporter,
+        ModifiedFileSet.builder().modify(new PathFragment("foo/BUILD")).build(), rootDirectory);
+
+    value = validPackage(skyKey);
+    assertFalse(value.getPackage().containsErrors());
+    assertThat(value.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt");
+    try {
+      value.getPackage().getTarget("dangling.txt");
+      fail();
+    } catch (NoSuchTargetException expected) {
+      // One consequence of the bug was that dangling symlinks were matched by globs evaluated by
+      // Skyframe globbing, meaning there would incorrectly be corresponding targets in packages
+      // that had skyframe cache hits during skyframe hybrid globbing.
+    }
+
+    scratch.file("foo/nope");
+    getSkyframeExecutor().invalidateFilesUnderPathForTesting(reporter,
+        ModifiedFileSet.builder().modify(new PathFragment("foo/nope")).build(), rootDirectory);
+
+    PackageValue newValue = validPackage(skyKey);
+    assertFalse(newValue.getPackage().containsErrors());
+    assertThat(newValue.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt");
+    // Another consequence of the bug is that change pruning would incorrectly cut off changes that
+    // caused a dangling symlink potentially matched by a glob to come into existence.
+    assertThat(newValue.getPackage().getTarget("dangling.txt").getName()).isEqualTo("dangling.txt");
+    assertThat(newValue.getPackage()).isNotSameAs(value.getPackage());
+  }
+
   private static class CustomInMemoryFs extends InMemoryFileSystem {
     private abstract static class FileStatusOrException {
       abstract FileStatus get() throws IOException;