Implement RemoteActionFileSystem#statIfFound correctly when the path cannot be canonicalized (because one of the components is a non-directory or a dangling symlink).

tjgq · copybara-github · commit b78d73fab1f0 · 2024-03-21T09:37:08.000-07:00
This improves the error message for a tree artifact containing a dangling symlink, which regressed in 4247c20 (see #15454 (comment)). PiperOrigin-RevId: 617870632 Change-Id: I6847084a52b1e4bb7d8a9384ad6cd5d015dddf1b
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -623,13 +623,17 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) {
   private FileStatus statInternal(PathFragment path, FollowMode followMode, StatSources statSources)
       throws IOException {
     // Canonicalize the path.
-    if (followMode == FollowMode.FOLLOW_ALL) {
-      path = resolveSymbolicLinks(path).asFragment();
-    } else if (followMode == FollowMode.FOLLOW_PARENT) {
-      PathFragment parent = path.getParentDirectory();
-      if (parent != null) {
-        path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName());
+    try {
+      if (followMode == FollowMode.FOLLOW_ALL) {
+        path = resolveSymbolicLinks(path).asFragment();
+      } else if (followMode == FollowMode.FOLLOW_PARENT) {
+        PathFragment parent = path.getParentDirectory();
+        if (parent != null) {
+          path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName());
+        }
       }
+    } catch (FileNotFoundException e) {
+      return null;
     }
 
     // Since the path has been canonicalized, the operations below never need to follow symlinks.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -1484,11 +1484,12 @@ private static void reportOutputTreeArtifactErrors(
       Action action, Artifact output, Reporter reporter, IOException e) {
     String errorMessage;
     if (e instanceof FileNotFoundException) {
-      errorMessage = String.format("TreeArtifact %s was not created", output.prettyPrint());
+      errorMessage = String.format("output tree artifact %s was not created", output.prettyPrint());
     } else {
       errorMessage =
           String.format(
-              "Error while validating output TreeArtifact %s : %s", output, e.getMessage());
+              "error while validating output tree artifact %s: %s",
+              output.prettyPrint(), e.getMessage());
     }
 
     reporter.handle(Event.error(action.getOwner().getLocation(), errorMessage));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -559,9 +559,7 @@ private void visit(
 
           if (statFollow == null) {
             throw new IOException(
-                String.format(
-                    "Child %s of tree artifact %s is a dangling symbolic link",
-                    parentRelativePath, parentDir));
+                String.format("child %s is a dangling symbolic link", parentRelativePath));
           }
 
           if (statFollow.isFile() && !statFollow.isSpecialFile()) {
@@ -575,9 +573,7 @@ private void visit(
 
         if (type == Dirent.Type.UNKNOWN) {
           throw new IOException(
-              String.format(
-                  "Child %s of tree artifact %s has an unsupported type",
-                  parentRelativePath, parentDir));
+              String.format("child %s has an unsupported type", parentRelativePath));
         }
 
         visitor.visit(parentRelativePath, type, traversedSymlink);
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
@@ -451,11 +451,43 @@ public void statAndExists_followSymlinks(
   @Test
   public void statAndExists_notFound() throws Exception {
     RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
-    Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
-    PathFragment path = artifact.getPath().asFragment();
+    PathFragment path = getOutputPath("does_not_exist");
+
+    assertThat(actionFs.exists(path)).isFalse();
+
+    assertThat(actionFs.statIfFound(path, /* followSymlinks= */ true)).isNull();
+
+    assertThrows(
+        FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
+  }
+
+  @Test
+  public void statAndExists_isNotDirectory() throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+    PathFragment nonDirPath = getOutputPath("non_dir");
+    PathFragment path = nonDirPath.getChild("file");
+
+    writeLocalFile(actionFs, nonDirPath, "content");
 
     assertThat(actionFs.exists(path)).isFalse();
 
+    assertThat(actionFs.statIfFound(path, /* followSymlinks= */ true)).isNull();
+
+    assertThrows(
+        FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
+  }
+
+  @Test
+  public void statAndExists_danglingSymlink_notFound() throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+    PathFragment path = getOutputPath("sym");
+
+    actionFs.getPath(path).createSymbolicLink(PathFragment.create("/does_not_exist"));
+
+    assertThat(actionFs.exists(path)).isFalse();
+
+    assertThat(actionFs.statIfFound(path, /* followSymlinks= */ true)).isNull();
+
     assertThrows(
         FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
   }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
@@ -259,6 +259,20 @@ public void createsTreeArtifactValueFromFilesystem() throws Exception {
     assertThat(chmodCalls).isEmpty();
   }
 
+  @Test
+  public void withDanglingSymlinkInTreeArtifactFailsWithException() throws Exception {
+    SpecialArtifact treeArtifact =
+        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "foo/bar");
+    TreeFileArtifact child = TreeFileArtifact.createTreeOutput(treeArtifact, "child");
+    treeArtifact.getPath().createDirectoryAndParents();
+    child.getPath().createSymbolicLink(PathFragment.create("/does_not_exist"));
+
+    ActionOutputMetadataStore store = createStore(/* outputs= */ ImmutableSet.of(treeArtifact));
+
+    IOException e = assertThrows(IOException.class, () -> store.getOutputMetadata(treeArtifact));
+    assertThat(e).hasMessageThat().contains("dangling symbolic link");
+  }
+
   @Test
   public void resettingOutputs() throws Exception {
     PathFragment path = PathFragment.create("foo/bar");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -505,9 +505,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
 
     ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
     assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage())
-        .contains(
-            "Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
+    assertThat(errors.get(0).getMessage()).contains("child links/link is a dangling symbolic link");
     assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
   }
 
@@ -555,9 +553,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
 
     ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
     assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage())
-        .contains(
-            "Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
+    assertThat(errors.get(0).getMessage()).contains("child links/link is a dangling symbolic link");
     assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
   }
 
@@ -607,9 +603,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
 
     ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
     assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage())
-        .contains(
-            "Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
+    assertThat(errors.get(0).getMessage()).contains("child links/link is a dangling symbolic link");
     assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
@@ -418,9 +418,7 @@ public void visitTree_throwsOnDanglingSymlink() throws Exception {
         assertThrows(
             IOException.class,
             () -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
-    assertThat(e)
-        .hasMessageThat()
-        .contains("Child symlink of tree artifact /tree is a dangling symbolic link");
+    assertThat(e).hasMessageThat().contains("child symlink is a dangling symbolic link");
   }
 
   @Test
@@ -455,9 +453,7 @@ public Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)
         assertThrows(
             IOException.class,
             () -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
-    assertThat(e)
-        .hasMessageThat()
-        .contains("Child unknown of tree artifact /tree has an unsupported type");
+    assertThat(e).hasMessageThat().contains("child unknown has an unsupported type");
   }
 
   @Test
@@ -522,9 +518,7 @@ public long getSize() {
         assertThrows(
             IOException.class,
             () -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
-    assertThat(e)
-        .hasMessageThat()
-        .contains("Child sym of tree artifact /tree has an unsupported type");
+    assertThat(e).hasMessageThat().contains("child sym has an unsupported type");
   }
 
   @Test
