[8.4.0] Fix runfiles directory staleness issues caused by symlinking the output manifest (#26868)
`SymlinkTreeAction` is only executed for its side effect of updating the
runfiles tree, but its up-to-dateness is tracked solely by the digest of
its output, the manifest under the runfiles directory. If this manifest
is a symlink to the input manifest and a previous build modified the
runfiles directory while not updating the action cache entry for
`SymlinkTreeAction`, this would cause Bazel to consider the action
up-to-date even though the runfiles directory is stale.
The fix consists of two parts:
* Always copy the manifest instead of symlinking it, matching the
behavior of the former `--noexperimental_inprocess_symlink_creation`
flag. This fixes staleness issues when only using versions of Bazel with
this fix.
* Detect stale state left behind by older versions of Bazel by skipping
action cache hits if the output manifest is a symlink. Since this
situation is no longer created by fixed versions of Bazel, we could
consider removing this logic in the future.
Fixes #26818
Closes #26837.
PiperOrigin-RevId: 801798862
Change-Id: I02c6d1b353981249a98df224efc4f46c5e4866d7
(cherry picked from commit 585e16c701a6eef9a0b96cce31cae56893f04278)
Also includes:
* a3eb3516b26ddaa2aa81d15a3cbfb6876a07d359
Fixes #26824
---------
Co-authored-by: Googler <tjgq@google.com>
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
index 7f89438..c2d25a6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
@@ -27,6 +27,7 @@
import com.google.devtools.build.lib.actions.ActionResult;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactExpander;
+import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
@@ -42,7 +43,7 @@
* trees.
*/
@Immutable
-public final class SymlinkTreeAction extends AbstractAction {
+public final class SymlinkTreeAction extends AbstractAction implements NotifyOnActionCacheHit {
private static final String GUID = "7a16371c-cd4a-494d-b622-963cd89f5212";
@@ -235,4 +236,14 @@
public boolean mayInsensitivelyPropagateInputs() {
return true;
}
+
+ @Override
+ public boolean actionCacheHit(NotifyOnActionCacheHit.ActionCachedContext context) {
+ // Before 8.4.0, Bazel created the output manifest as a symlink to the input manifest, which
+ // makes it infeasible to reliably detect stale runfiles directories, which are only created as
+ // a side effect of this action and not tracked in the action cache.
+ // https://github.com/bazelbuild/bazel/issues/26818
+ // TODO: Consider removing this when Bazel 7 is no longer supported.
+ return !context.getPathResolver().toPath(getPrimaryOutput()).isSymbolicLink();
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
index 38ac043..5a0979b 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
@@ -22,9 +22,9 @@
import com.google.devtools.build.lib.actions.RunfilesTree;
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
-import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.build.lib.vfs.DigestUtils;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Symlinks;
@@ -127,19 +127,19 @@
// Avoid rebuilding the runfiles directory if the manifest in it matches the input manifest,
// implying the symlinks exist and are already up to date. If the output manifest is a
// symbolic link, it is likely a symbolic link to the input manifest, so we cannot trust it as
- // an up-to-date check.
- // On Windows, where symlinks may be silently replaced by copies, a previous run in SKIP mode
- // could have resulted in an output manifest that is an identical copy of the input manifest,
- // which we must not treat as up to date, but we also don't want to unnecessarily rebuild the
- // runfiles directory all the time. Instead, check for the presence of the first runfile in
- // the manifest. If it is present, we can be certain that the previous mode wasn't SKIP.
+ // an up-to-date check. Ignoring external interference, this situation can only arise if an
+ // older version of Bazel created the runfiles tree - before 8.4.0, Bazel created a symlink.
+ // A previous run in SKIP mode could have resulted in an output manifest that is an
+ // identical copy of the input manifest, which we must not treat as up to date. Since we also
+ // don't want to unnecessarily rebuild the runfiles directory all the time, instead check for
+ // the presence of the first runfile in the manifest. If it is present, we can be certain that
+ // the previous mode wasn't SKIP.
if (tree.getSymlinksMode() != SKIP
&& !outputManifest.isSymbolicLink()
&& Arrays.equals(
DigestUtils.getDigestWithManualFallback(outputManifest, xattrProvider),
DigestUtils.getDigestWithManualFallback(inputManifest, xattrProvider))
- && (OS.getCurrent() != OS.WINDOWS
- || isRunfilesDirectoryPopulated(runfilesDir, outputManifest))) {
+ && isRunfilesDirectoryPopulated(runfilesDir, outputManifest)) {
return;
}
} catch (IOException e) {
@@ -164,7 +164,8 @@
case EXTERNAL -> helper.createSymlinksUsingCommand(binTools, env, outErr);
case INTERNAL -> {
helper.createRunfilesSymlinksDirectly(tree.getMapping());
- outputManifest.createSymbolicLink(inputManifest);
+ // See SymlinkTreeStrategy#createOutput for why we copy instead of hardlink or symlink.
+ FileSystemUtils.copyFile(inputManifest, outputManifest);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
index cb4ee3f..53dd654 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -165,12 +165,12 @@
}
/**
- * Ensures that the runfiles directory is empty except for the symlinked MANIFEST and the
- * workspace subdirectory. This is the expected state with --noenable_runfiles.
+ * Ensures that the runfiles directory is empty except for the copied MANIFEST and the workspace
+ * subdirectory. This is the expected state with --noenable_runfiles.
*/
public void clearRunfilesDirectory() throws ExecException {
deleteRunfilesDirectory();
- linkManifest();
+ copyManifest();
try {
createWorkspaceSubdirectory();
} catch (IOException e) {
@@ -187,15 +187,15 @@
}
}
- /** Links the output manifest to the input manifest. */
- private void linkManifest() throws ExecException {
- // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest.
+ /** Copies the output manifest to the input manifest. */
+ private void copyManifest() throws ExecException {
+ // Pretend we created the runfiles tree by copying the output manifest to the input manifest.
try {
symlinkTreeRoot.createDirectoryAndParents();
outputManifest.delete();
- outputManifest.createSymbolicLink(inputManifest);
+ FileSystemUtils.copyFile(inputManifest, outputManifest);
} catch (IOException e) {
- throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION);
+ throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_COPY_IO_EXCEPTION);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
index 219b6dc..13967d2 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
@@ -34,6 +34,7 @@
import com.google.devtools.build.lib.server.FailureDetails.Execution;
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.OutputService;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -151,13 +152,16 @@
SymlinkTreeAction action, ActionExecutionContext actionExecutionContext, Path inputManifest)
throws EnvironmentalExecException {
Path outputManifest = actionExecutionContext.getInputPath(action.getOutputManifest());
- // Link output manifest on success. We avoid a file copy as these manifests may be
- // large. Note that this step has to come last because the OutputService may delete any
- // pre-existing symlink tree before creating a new one.
+ // Copy output manifest on success. We can't use a symlink or hardlink here because
+ // SymlinkTreeAction is executed for its side effect (the symlink tree creation) and could
+ // erroneously be considered up-to-date if the input manifest changed back to a different state
+ // that had previously been recorded in the action cache.
+ // Note that this step has to come last because the OutputService may delete any pre-existing
+ // symlink tree before creating a new one.
try {
- outputManifest.createSymbolicLink(inputManifest);
+ FileSystemUtils.copyFile(inputManifest, outputManifest);
} catch (IOException e) {
- throw createLinkFailureException(outputManifest, e);
+ throw createCopyFailureException(outputManifest, e);
}
}
@@ -178,14 +182,15 @@
workspaceName);
}
- private static EnvironmentalExecException createLinkFailureException(
+ private static EnvironmentalExecException createCopyFailureException(
Path outputManifest, IOException e) {
return new EnvironmentalExecException(
e,
FailureDetail.newBuilder()
- .setMessage("Failed to link output manifest '" + outputManifest.getPathString() + "'")
+ .setMessage(
+ "Failed to copy output manifest '%s'".formatted(outputManifest.getPathString()))
.setExecution(
- Execution.newBuilder().setCode(Code.SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION))
+ Execution.newBuilder().setCode(Code.SYMLINK_TREE_MANIFEST_COPY_IO_EXCEPTION))
.build());
}
}
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 aeb2745..853dea4 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
@@ -723,12 +723,17 @@
@Override
public long getLastModifiedTime() {
- return m.getModifiedTime();
+ try {
+ return m.getModifiedTime();
+ } catch (UnsupportedOperationException e) {
+ // Not every FileArtifactValue supports getModifiedTime.
+ return 0;
+ }
}
@Override
public long getLastChangeTime() {
- return m.getModifiedTime();
+ return getLastModifiedTime();
}
@Override
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto
index fa8d524..4c9bd6f 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -468,8 +468,7 @@
TEST_OUT_ERR_IO_EXCEPTION = 14 [(metadata) = { exit_code: 36 }];
SYMLINK_TREE_MANIFEST_COPY_IO_EXCEPTION = 15
[(metadata) = { exit_code: 36 }];
- SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION = 16
- [(metadata) = { exit_code: 36 }];
+ reserved 16; // was SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION
SYMLINK_TREE_CREATION_IO_EXCEPTION = 17 [(metadata) = { exit_code: 36 }];
SYMLINK_TREE_CREATION_COMMAND_EXCEPTION = 18
[(metadata) = { exit_code: 36 }];
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
index 2a55962..ed7ea2c 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
@@ -74,6 +74,8 @@
when(outputService.canCreateSymlinkTree()).thenReturn(true);
Artifact inputManifest = getBinArtifactWithNoOwner("dir/manifest.in");
+ inputManifest.getPath().getParentDirectory().createDirectoryAndParents();
+ FileSystemUtils.createEmptyFile(inputManifest.getPath());
Artifact outputManifest = getBinArtifactWithNoOwner("dir.runfiles/MANIFEST");
Artifact runfile = getBinArtifactWithNoOwner("dir/runfile");
doAnswer(
@@ -137,6 +139,8 @@
when(outputService.canCreateSymlinkTree()).thenReturn(false);
Artifact inputManifest = getBinArtifactWithNoOwner("dir/manifest.in");
+ inputManifest.getPath().getParentDirectory().createDirectoryAndParents();
+ FileSystemUtils.createEmptyFile(inputManifest.getPath());
Artifact outputManifest = getBinArtifactWithNoOwner("dir.runfiles/MANIFEST");
Artifact runfile = getBinArtifactWithNoOwner("dir/runfile");
diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh
index 936151b..8459b9a 100755
--- a/src/test/shell/bazel/cc_integration_test.sh
+++ b/src/test/shell/bazel/cc_integration_test.sh
@@ -1004,7 +1004,7 @@
# Null build.
bazel build --experimental_sibling_repository_layout //baz:binary &> "$TEST_log" \
|| fail "expected build success"
- expect_log "INFO: 1 process: 1 internal"
+ expect_log "INFO: 1 process: 1 action cache hit, 1 internal"
}
function test_execroot_sibling_layout_header_scanning_in_external_subpackage() {
diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh
index 2601ed0..f545f03 100755
--- a/src/test/shell/integration/runfiles_test.sh
+++ b/src/test/shell/integration/runfiles_test.sh
@@ -840,4 +840,140 @@
//pkg:gen >&$TEST_log || fail "build failed"
}
+function setup_runfile_manifest_only_change() {
+ mkdir -p pkg
+ cat > pkg/constants.bzl <<'EOF'
+SYMLINK_PATH = "old_name"
+EOF
+ cat > pkg/defs.bzl <<'EOF'
+load(":constants.bzl", "SYMLINK_PATH")
+
+def _my_script_impl(ctx):
+ out = ctx.actions.declare_file(ctx.label.name)
+ print(SYMLINK_PATH)
+ ctx.actions.write(
+ out,
+ """#!/usr/bin/env sh
+set -eu
+[ -f {} ] || exit 1
+""".format(SYMLINK_PATH),
+ is_executable = True,
+ )
+ runfiles = ctx.runfiles(
+ symlinks = {
+ SYMLINK_PATH: out,
+ },
+ )
+ return [DefaultInfo(executable = out, runfiles = runfiles)]
+
+my_script = rule(
+ implementation = _my_script_impl,
+ executable = True,
+)
+EOF
+ cat > pkg/BUILD <<'EOF'
+load(":defs.bzl", "my_script")
+
+my_script(
+ name = "foo",
+)
+EOF
+}
+
+function test_runfile_manifest_only_change_not_stale_no_action_cache_entry() {
+ if "$is_windows"; then
+ # Can't run shell scripts directly.
+ return
+ fi
+
+ setup_runfile_manifest_only_change
+
+ bazel run //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "first run failed"
+
+ # Modify the symlink path only while not overwriting the existing action cache
+ # entry for the SymlinkTreeAction.
+ inplace-sed 's/old_name/new_name/' pkg/constants.bzl
+ bazel run //pkg:foo --nouse_action_cache --nobuild_runfile_links \
+ $EXTRA_BUILD_FLAGS >&$TEST_log || fail "second run failed"
+
+ # Revert the symlink path.
+ inplace-sed 's/new_name/old_name/' pkg/constants.bzl
+ bazel run //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "third run failed"
+}
+
+function test_runfile_manifest_only_change_not_stale_no_action_cache_entry_nobuild_runfile_links() {
+ if "$is_windows"; then
+ # Can't run shell scripts directly.
+ return
+ fi
+
+ setup_runfile_manifest_only_change
+
+ bazel run //pkg:foo --nobuild_runfile_links $EXTRA_BUILD_FLAGS >&$TEST_log \
+ || fail "first run failed"
+
+ # Modify the symlink path only while not overwriting the existing action cache
+ # entry for the SymlinkTreeAction.
+ inplace-sed 's/old_name/new_name/' pkg/constants.bzl
+ bazel run //pkg:foo --nouse_action_cache --nobuild_runfile_links \
+ $EXTRA_BUILD_FLAGS >&$TEST_log || fail "second run failed"
+
+ # Revert the symlink path.
+ inplace-sed 's/new_name/old_name/' pkg/constants.bzl
+ bazel run //pkg:foo --nobuild_runfile_links $EXTRA_BUILD_FLAGS >&$TEST_log \
+ || fail "third run failed"
+}
+
+function test_runfile_manifest_only_change_not_stale_legacy_bazel_version() {
+ if "$is_windows"; then
+ # Can't run shell scripts directly.
+ return
+ fi
+
+ setup_runfile_manifest_only_change
+
+ bazel run //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "first run failed"
+
+ # Simulate the effect of an old version of Bazel that doesn't override the
+ # action cache entry (because it stores the action cache in a different path)
+ # and also creates the output manifest as a symlink instead of a copy.
+ inplace-sed 's/old_name/new_name/' bazel-bin/pkg/foo${EXT}.runfiles_manifest
+ rm bazel-bin/pkg/foo${EXT}.runfiles/MANIFEST
+ ln -s "$(pwd)/bazel-bin/pkg/foo${EXT}.runfiles_manifest" bazel-bin/pkg/foo${EXT}.runfiles/MANIFEST
+ local -r old_path="$(find bazel-bin/pkg/foo${EXT}.runfiles -name old_name)"
+ local -r new_path="${old_path/old_name/new_name}"
+ mv "$old_path" "$new_path"
+
+ # Revert the symlink path.
+ inplace-sed 's/new_name/old_name/' pkg/constants.bzl
+ bazel run //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "second run failed"
+}
+
+function test_runfile_manifest_only_change_not_stale_legacy_bazel_version_nobuild_runfile_links() {
+ if "$is_windows"; then
+ # Can't run shell scripts directly.
+ return
+ fi
+
+ setup_runfile_manifest_only_change
+
+ bazel run //pkg:foo --nobuild_runfile_links $EXTRA_BUILD_FLAGS >&$TEST_log \
+ || fail "first run failed"
+
+ # Simulate the effect of an old version of Bazel that doesn't override the
+ # action cache entry (because it stores the action cache in a different path)
+ # and also creates the output manifest as a symlink instead of a copy.
+ inplace-sed 's/old_name/new_name/' bazel-bin/pkg/foo${EXT}.runfiles_manifest
+ rm bazel-bin/pkg/foo${EXT}.runfiles/MANIFEST
+ ln -s "$(pwd)/bazel-bin/pkg/foo${EXT}.runfiles_manifest" bazel-bin/pkg/foo${EXT}.runfiles/MANIFEST
+ local -r old_path="$(find bazel-bin/pkg/foo${EXT}.runfiles -name old_name)"
+ local -r new_path="${old_path/old_name/new_name}"
+ mv "$old_path" "$new_path"
+
+ # Revert the symlink path.
+ inplace-sed 's/new_name/old_name/' pkg/constants.bzl
+ bazel run //pkg:foo --nobuild_runfile_links $EXTRA_BUILD_FLAGS >&$TEST_log \
+ || fail "second run failed"
+}
+
run_suite "runfiles"
