[7.1.0] Canonicalize the parent path in RemoteActionFileSystem#delete. (

#21282)

FileSystem#delete is documented as not following symlinks, but that
refers to the last component only; we are still required to canonicalize
the parent path, possibly taking into account symlinks that straddle
underlying sources.

PiperOrigin-RevId: 605583053
Change-Id: Iaae6b433dd631de459ef0ecaf09736c79c710ba0

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -291,8 +291,24 @@ public String getFileSystemType(PathFragment path) {
     return "remoteActionFS";
   }
 
+  // Like resolveSymbolicLinks(), except that only the parent path is canonicalized.
+  private PathFragment resolveSymbolicLinksForParent(PathFragment path) throws IOException {
+    PathFragment parentPath = path.getParentDirectory();
+    if (parentPath != null) {
+      return resolveSymbolicLinks(parentPath).asFragment().getChild(path.getBaseName());
+    }
+    return path;
+  }
+
   @Override
   protected boolean delete(PathFragment path) throws IOException {
+    try {
+      path = resolveSymbolicLinksForParent(path);
+    } catch (FileNotFoundException ignored) {
+      // Failure to delete a nonexistent path is not an error.
+      return false;
+    }
+
     boolean deleted = localFs.getPath(path).delete();
     if (isOutput(path)) {
       deleted = remoteOutputTree.getPath(path).delete() || deleted;
@@ -462,10 +478,7 @@ protected void chmod(PathFragment path, int mode) throws IOException {
 
   @Override
   protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
-    PathFragment parentPath = path.getParentDirectory();
-    if (parentPath != null) {
-      path = resolveSymbolicLinks(parentPath).asFragment().getChild(path.getBaseName());
-    }
+    path = resolveSymbolicLinksForParent(path);
 
     if (path.startsWith(execRoot)) {
       var execPath = path.relativeTo(execRoot);
@@ -497,10 +510,7 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
   @Override
   protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
       throws IOException {
-    PathFragment parentPath = linkPath.getParentDirectory();
-    if (parentPath != null) {
-      linkPath = resolveSymbolicLinks(parentPath).asFragment().getChild(linkPath.getBaseName());
-    }
+    linkPath = resolveSymbolicLinksForParent(linkPath);
 
     if (isOutput(linkPath)) {
       remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment);

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java

@@ -460,6 +460,49 @@ public void statAndExists_notFound() throws Exception {
         FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
   }
 
+  @Test
+  public void delete_deleteSymlink() throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+
+    PathFragment linkPath = getOutputPath("link");
+    PathFragment targetPath = getOutputPath("target");
+    actionFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());
+    writeLocalFile(actionFs, targetPath, "content");
+
+    assertThat(actionFs.delete(linkPath)).isTrue();
+    assertThat(actionFs.exists(linkPath, /* followSymlinks= */ false)).isFalse();
+    assertThat(actionFs.exists(targetPath, /* followSymlinks= */ false)).isTrue();
+  }
+
+  @Test
+  public void delete_followSymlinks(
+      @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
+      throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+    FileSystem fromFs = from.getFilesystem(actionFs);
+    FileSystem toFs = to.getFilesystem(actionFs);
+
+    PathFragment dirLinkPath = getOutputPath("dirLink");
+    PathFragment dirTargetPath = getOutputPath("dirTarget");
+    fromFs
+        .getPath(dirLinkPath)
+        .createSymbolicLink(execRoot.getRelative(dirTargetPath).asFragment());
+    actionFs.getPath(dirTargetPath).createDirectoryAndParents();
+
+    PathFragment naivePath = dirLinkPath.getChild("file");
+    PathFragment canonicalPath = dirTargetPath.getChild("file");
+
+    if (toFs.equals(actionFs.getLocalFileSystem())) {
+      writeLocalFile(actionFs, canonicalPath, "content");
+    } else {
+      injectRemoteFile(actionFs, canonicalPath, "content");
+    }
+
+    assertThat(actionFs.delete(naivePath)).isTrue();
+    assertThat(actionFs.exists(naivePath, /* followSymlinks= */ false)).isFalse();
+    assertThat(actionFs.exists(canonicalPath, /* followSymlinks= */ false)).isFalse();
+  }
+
   @Test
   public void setLastModifiedTime_forRemoteOutputTree() throws Exception {
     RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();