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 041d0108761f84..7d52d7c8d2839f 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 @@ -174,6 +174,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(); @@ -183,7 +185,13 @@ void clearPrefix(PathFragment pathPrefix) { switch (node) { case SymlinkNode symlinkNode -> { - // Path prefix not in trie. + // The path prefix passes through a cached symlink. Since the caller is invalidating + // entries inside this symlink, the cached resolution itself may be stale (e.g. the + // symlink has since been replaced by a directory) and must be dropped so that the next + // canonicalization re-reads the on-disk state. + if (parent != null) { + parent.remove(parentSegment); + } return; } case NonSymlinkNode nonSymlinkNode -> { @@ -191,6 +199,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..30fbc284810fac 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) { @@ -361,9 +362,13 @@ public boolean delete(PathFragment path) throws IOException { return false; } - // No action implementations call renameTo concurrently with other filesystem operations, so - // there's no risk of a race condition below. - pathCanonicalizer.clearPrefix(path); + // Invalidate any cached symlink resolution for the unresolved path. If a parent of `path` is + // a symlink that has been (or will be) replaced -- for example, by a concurrent action that + // turns a stale executable symlink into a directory for a same-named subpackage -- our trie + // would otherwise keep returning the stale target. The resolution above may have populated + // such a stale entry, so this must happen *after* the resolution and on the *unresolved* + // path: clearing the resolved path would only invalidate a different subtree of the trie. + pathCanonicalizer.clearPrefix(originalPath); boolean deleted = localFs.getPath(path).delete(); if (isOutput(path)) { diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD index 87945d69f8356d..e220780885eb88 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -382,6 +382,14 @@ sh_test( tags = ["no_windows"], ) +sh_test( + name = "stale_output_symlink_test", + size = "medium", + srcs = ["stale_output_symlink_test.sh"], + data = [":test-deps"], + tags = ["no_windows"], +) + sh_test( name = "starlark_flag_test", size = "medium", diff --git a/src/test/shell/integration/stale_output_symlink_test.sh b/src/test/shell/integration/stale_output_symlink_test.sh new file mode 100755 index 00000000000000..2315067f4f7373 --- /dev/null +++ b/src/test/shell/integration/stale_output_symlink_test.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# +# Copyright 2026 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Regression test for https://github.com/bazelbuild/bazel/issues/29480. +# +# When an output path was previously created as a symlink (e.g. the executable +# of an sh_binary), and a later build needs to use that same path as a +# directory (because a same-named subpackage appeared at that location), +# Bazel must clean up the stale symlink before placing files underneath, and +# the per-action filesystem's cached symlink resolution must be invalidated. + +CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${CURRENT_DIR}/../integration_test_setup.sh" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +function tear_down() { + bazel clean + bazel shutdown + rm -rf tools +} + +function test_symlink_output_replaced_by_subpackage() { + add_rules_shell "MODULE.bazel" + # The bug only manifests when an action filesystem (RemoteActionFileSystem) is in use, which + # happens whenever a disk or remote cache is configured. + add_to_bazelrc "build --disk_cache=$TEST_TMPDIR/disk_cache" + mkdir -p tools || fail "Can't create tools" + cat > tools/wrapper_script.sh << 'EOF' +#!/bin/bash +echo hello +EOF + chmod +x tools/wrapper_script.sh + + cat > tools/BUILD.bazel << 'EOF' +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") + +sh_binary( + name = "bazel_wrapper", + srcs = ["wrapper_script.sh"], +) +EOF + + bazel build //tools:bazel_wrapper >&$TEST_log \ + || fail "Failed first build of //tools:bazel_wrapper" + + # rules_shell creates the executable as a symlink to the source script, + # which is what triggers the stale-state path in subsequent builds. + local exec_path + exec_path=$(bazel info bazel-bin)/tools/bazel_wrapper + [[ -L "$exec_path" ]] \ + || fail "Expected $exec_path to be a symlink, got: $(ls -la "$exec_path" 2>&1)" + + # 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. + rm tools/BUILD.bazel + mkdir -p tools/bazel_wrapper + cat > tools/bazel_wrapper/BUILD.bazel << 'EOF' +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") + +sh_binary( + name = "bazel_wrapper", + srcs = ["//tools:wrapper_script.sh"], +) +EOF + cat > tools/BUILD.bazel << 'EOF' +exports_files(["wrapper_script.sh"]) +EOF + + bazel build //tools/bazel_wrapper:bazel_wrapper >&$TEST_log \ + || fail "Failed second build after moving sh_binary to a subpackage" +} + +run_suite "Stale output symlink tests"