diff --git a/src/main/java/com/google/devtools/build/lib/remote/PathCanonicalizer.java b/src/main/java/com/google/devtools/build/lib/remote/PathCanonicalizer.java index 3ffb92ff3df60a..b2271d4314902b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/PathCanonicalizer.java +++ b/src/main/java/com/google/devtools/build/lib/remote/PathCanonicalizer.java @@ -175,6 +175,8 @@ PathFragment resolveSymbolicLinks(PathFragment path) throws IOException { /** Removes cached information for a path prefix. */ void clearPrefix(PathFragment pathPrefix) { Node node = getRootNode(pathPrefix); + NonSymlinkNode parent = null; + String parentSegment = null; Iterator segments = pathPrefix.segments().iterator(); boolean hasNext = segments.hasNext(); @@ -184,7 +186,10 @@ void clearPrefix(PathFragment pathPrefix) { switch (node) { case SymlinkNode symlinkNode -> { - // Path prefix not in trie. + // Invalidate all intermediate symlinks. + if (parent != null) { + parent.remove(parentSegment); + } return; } case NonSymlinkNode nonSymlinkNode -> { @@ -192,6 +197,8 @@ void clearPrefix(PathFragment pathPrefix) { // Found the path prefix. nonSymlinkNode.remove(segment); } else { + parent = nonSymlinkNode; + parentSegment = segment; node = nonSymlinkNode.get(segment); } } 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 index 7d04eaffcd0be4..ac6fca2cfc0744 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -354,6 +354,7 @@ private PathFragment resolveSymbolicLinksForParent(PathFragment path) throws IOE @Override public boolean delete(PathFragment path) throws IOException { + PathFragment originalPath = path; try { path = resolveSymbolicLinksForParent(path); } catch (FileNotFoundException ignored) { @@ -363,7 +364,7 @@ public boolean delete(PathFragment path) throws IOException { // No action implementations call renameTo concurrently with other filesystem operations, so // there's no risk of a race condition below. - pathCanonicalizer.clearPrefix(path); + pathCanonicalizer.clearPrefix(originalPath); boolean deleted = localFs.getPath(path).delete(); if (isOutput(path)) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/PathCanonicalizerTest.java b/src/test/java/com/google/devtools/build/lib/remote/PathCanonicalizerTest.java index 30965c91d4e60b..8e1a5b4c798d6d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/PathCanonicalizerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/PathCanonicalizerTest.java @@ -224,6 +224,19 @@ public void testClearUnknownPathDescendingFromNonSymlink() throws Exception { assertSuccess("/a/b", "/a/b"); } + @Test + public void testClearPathDescendingThroughSymlinkInvalidatesIt() throws Exception { + createSymlink("/a/b", "/d"); + createNonSymlink("/d/c"); + assertSuccess("/a/b/c", "/d/c"); + + fs.getPath(pathFragment("/a/b")).delete(); + createNonSymlink("/a/b/c"); + canonicalizer.clearPrefix(pathFragment("/a/b/c")); + + assertSuccess("/a/b/c", "/a/b/c"); + } + @Test public void testSymlinkSelfLoop() throws Exception { createSymlink("/a/b", "/a/b"); diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh index b3e08dad1e64f3..a806859db08aa9 100755 --- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh +++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh @@ -2664,4 +2664,53 @@ EOF bazel run "${FLAGS[@]}" //:foo >& $TEST_log || fail "Failed to run //:foo" } +function test_symlink_output_replaced_by_subpackage() { + # Regression test for https://github.com/bazelbuild/bazel/issues/29480. + add_rules_shell "MODULE.bazel" + mkdir -p tools + cat > tools/wrapper_script.sh <<'EOF' +#!/bin/bash +echo hello +EOF + chmod +x tools/wrapper_script.sh + + cat > tools/BUILD <<'EOF' +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") + +sh_binary( + name = "bazel_wrapper", + srcs = ["wrapper_script.sh"], +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + //tools:bazel_wrapper >& $TEST_log \ + || fail "Failed first build of //tools:bazel_wrapper" + + [[ -L bazel-bin/tools/bazel_wrapper ]] \ + || fail "Expected bazel-bin/tools/bazel_wrapper to be a symlink" + + # Move the sh_binary into a same-named subpackage. The output that was + # previously a symlinked file at .../tools/bazel_wrapper must now become a + # directory at .../tools/bazel_wrapper/ which contains the new outputs. + mkdir -p tools/bazel_wrapper + cat > tools/bazel_wrapper/BUILD <<'EOF' +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") + +sh_binary( + name = "bazel_wrapper", + srcs = ["//tools:wrapper_script.sh"], +) +EOF + cat > tools/BUILD <<'EOF' +exports_files(["wrapper_script.sh"]) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + //tools/bazel_wrapper:bazel_wrapper >& $TEST_log \ + || fail "Failed second build after moving sh_binary to a subpackage" +} + run_suite "Build without the Bytes tests"