From 7b1050b6a942e494677156d63ac04e1f82852b6e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 14 Sep 2026 12:48:37 -0700 Subject: [PATCH] Register a FileValue dep on bzlCompileCache hits in BzlLoadFunction (#30902) The KeyForBuild and KeyForBzlmod variants of a BzlLoadValue key for the same .bzl file share a single BzlCompileValue.Key, and thus a single entry in the bzlCompileCache used when BzlCompileFunction is inlined. When a BzlLoadValue node got a cache hit for an entry that was computed on behalf of a node of the other key variant, it never requested the FileValue for the .bzl file, so it was missing the Skyframe edge that would invalidate it when the file changes and kept serving stale file contents on subsequent builds. Fix this by (re-)requesting the FileValue for the .bzl file on a cache hit. Fixes https://github.com/bazelbuild/bazel/issues/30900 No - [x] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). RELNOTES: None Closes #30902 COPYBARA_INTEGRATE_REVIEW=https://github.com/bazelbuild/bazel/pull/30902 from fmeum:claude/bazel-issue-30900-gtza50 bb2313790823279a783990f3c1bdb247a2af1a7d PiperOrigin-RevId: 981304041 Change-Id: I16c31a54ad4996d2f94f7061b1b72bb0a6eb9db1 --- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../lib/skyframe/BzlCompileFunction.java | 2 +- .../build/lib/skyframe/BzlCompileValue.java | 11 +++ .../build/lib/skyframe/BzlLoadFunction.java | 14 +++ .../google/devtools/build/lib/skyframe/BUILD | 1 + .../lib/skyframe/BzlLoadFunctionTest.java | 92 +++++++++++++++++++ 6 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 5a71b5a2fa12cf..d31e9135322c9a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -849,6 +849,7 @@ java_library( name = "bzl_compile_value", srcs = ["BzlCompileValue.java"], deps = [ + ":filesystem_keys", ":sky_functions", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java index f07ba453d4c638..7fbe91d29ea559 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java @@ -293,7 +293,7 @@ private static void addSyntaxErrorsToListener( static final class FailedIOException extends Exception { private final Transience transience; - private FailedIOException(IOException cause, Transience transience) { + FailedIOException(IOException cause, Transience transience) { super(cause.getMessage(), cause); this.transience = transience; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java index 3ae4acabf485cc..baf8e825181f23 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.vfs.Root; +import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.NotComparableSkyValue; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -207,6 +208,16 @@ public Label getLabel() { return label; } + /** Returns the dep to register for the underlying .bzl file (if any). */ + @Nullable + public FileKey getBzlFileKey() { + if (label == null) { + // Implies kind == Kind.EMPTY_PRELUDE. + return null; + } + return FileKey.create(RootedPath.toRootedPath(root, label.toPathFragment())); + } + @Override public int hashCode() { return Objects.hash(Key.class, root, label, kind); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index e561cb20f6ec57..695fcd3be4b135 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -62,6 +62,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.io.IOException; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; @@ -1499,6 +1500,19 @@ public BzlCompileValue getBzlCompileValue(BzlCompileValue.Key key, Environment e if (value != null) { bzlCompileCache.put(key, value); } + } else { + // The cache hit may have been populated on behalf of a different BzlLoadValue node with + // the same compile key; make sure this node depends on the .bzl file too. + var bzlFileKey = key.getBzlFileKey(); + if (bzlFileKey != null) { + try { + if (env.getValueOrThrow(bzlFileKey, IOException.class) == null) { + return null; + } + } catch (IOException e) { + throw new BzlCompileFunction.FailedIOException(e, Transience.PERSISTENT); + } + } } return value; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index fb14784a473b01..db91c75041458e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2117,6 +2117,7 @@ java_test( "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util", "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", + "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//src/test/java/com/google/devtools/build/skyframe:testutil", "//third_party:guava", "//third_party:jsr305", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java index 4d169b6b175ff5..bc359d1eae0e32 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java @@ -34,10 +34,12 @@ import com.google.devtools.build.lib.runtime.QuiescingExecutorsImpl; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -50,6 +52,9 @@ import java.io.IOException; import java.io.InputStream; import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkInt; import org.junit.Before; @@ -1157,9 +1162,86 @@ public void testErrorStatingBzlFileInFileStateFunctionIsPersistent() throws Exce assertThatEvaluationResult(result).hasErrorEntryForKeyThat(key).isNotTransient(); } + @Test + public void bzlCompileCacheHitFromNodeWithOtherKeyKind_registersFileDep() throws Exception { + // Regression test for https://github.com/bazelbuild/bazel/issues/30900: the KeyForBuild and + // KeyForBzlmod variants of the same .bzl share a single entry in BzlLoadFunction's + // bzlCompileCache. A node that gets a cache hit for an entry computed on behalf of the other + // variant must still register a dependency on the .bzl's FileValue; otherwise it isn't + // invalidated when the file changes and keeps serving stale contents. + CustomInMemoryFs fs = (CustomInMemoryFs) fileSystem; + scratch.file("pkg/BUILD"); + scratch.file( + "pkg/foo.bzl", + """ + load(":bar.bzl", "x") + + y = x + """); + Path barBzl = scratch.file("pkg/bar.bzl", "x = 1"); + + // Evaluate the KeyForBuild node on another thread and interrupt the evaluation while it is + // blocked statting bar.bzl. At that point foo.bzl has already been compiled and cached in the + // bzlCompileCache (a .bzl is compiled before its load() deps are requested), and the interrupt + // strands the entry there since it is only released when the owning BzlLoadValue node + // completes. This simulates a .bzl file compiled on behalf of one BzlLoadValue node while a + // node for the other key variant of the same file is in flight. + SkyKey keyForBuild = key("//pkg:foo.bzl"); + fs.pathToBlockOnStat = barBzl; + AtomicBoolean evaluationInterrupted = new AtomicBoolean(false); + Thread evalThread = + new Thread( + () -> { + try { + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), keyForBuild, /* keepGoing= */ false, reporter); + } catch (InterruptedException e) { + evaluationInterrupted.set(true); + } + }); + evalThread.start(); + assertThat(fs.blockedStatReached.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) + .isTrue(); + evalThread.interrupt(); + evalThread.join(); + assertThat(evaluationInterrupted.get()).isTrue(); + fs.pathToBlockOnStat = null; + fs.blockedStatMayProceed.countDown(); + + // The interrupted evaluation may have committed an error for bar.bzl's FileStateValue (the + // blocked stat throws IOException when the evaluation shuts down). Invalidate it so + // that the next evaluation stats the file afresh. + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(PathFragment.create("pkg/bar.bzl")).build(), + Root.fromPath(rootDirectory)); + + // The KeyForBzlmod node gets a bzlCompileCache hit for the entry compiled on behalf of the + // KeyForBuild node above. + SkyKey keyForBzlmod = BzlLoadValue.keyForBzlmod(Label.parseCanonical("//pkg:foo.bzl")); + EvaluationResult result = get(keyForBzlmod); + assertThat(result.get(keyForBzlmod).getModule().getGlobals()) + .containsEntry("y", StarlarkInt.of(1)); + + // Change foo.bzl. The KeyForBzlmod node must pick up the new file contents. + scratch.overwriteFile("pkg/foo.bzl", "y = 2"); + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(PathFragment.create("pkg/foo.bzl")).build(), + Root.fromPath(rootDirectory)); + result = get(keyForBzlmod); + assertThat(result.get(keyForBzlmod).getModule().getGlobals()) + .containsEntry("y", StarlarkInt.of(2)); + } + private static class CustomInMemoryFs extends InMemoryFileSystem { @Nullable private Path badPathForStat; @Nullable private Path badPathForRead; + @Nullable private volatile Path pathToBlockOnStat; + private final CountDownLatch blockedStatReached = new CountDownLatch(1); + private final CountDownLatch blockedStatMayProceed = new CountDownLatch(1); CustomInMemoryFs() { super(DigestHashFunction.SHA256); @@ -1170,6 +1252,16 @@ public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws if (badPathForStat != null && badPathForStat.asFragment().equals(path)) { throw new IOException("bad"); } + Path blockedPath = pathToBlockOnStat; + if (blockedPath != null && blockedPath.asFragment().equals(path)) { + blockedStatReached.countDown(); + try { + blockedStatMayProceed.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("interrupted"); + } + } return super.statIfFound(path, followSymlinks); }