Register a FileValue dep on bzlCompileCache hits in BzlLoadFunction

src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java

@@ -286,6 +286,32 @@ private static void updateFileOptions(FileOptions.Builder builder, TypeOptions t
         .tolerateInvalidTypeExpressions(!needsTypeInfo);
   }
 
+  /**
+   * Requests the {@link FileValue} for the .bzl file denoted by {@code key} so that the requesting
+   * Skyframe node depends on it, without using the result.
+   *
+   * <p>Used by {@code BzlLoadFunction.InliningAndCachingGetter} on a {@code bzlCompileCache} hit:
+   * the cached value may have been computed on behalf of a different {@code BzlLoadValue} node that
+   * shares the same compile key (e.g. the {@code KeyForBuild} and {@code KeyForBzlmod} variants of
+   * the same label), in which case the node consuming the cache hit doesn't yet depend on the file
+   * and would miss invalidation when it changes.
+   *
+   * @return false if the {@link FileValue} is not yet available
+   */
+  static boolean requestFileDepOnCacheHit(BzlCompileValue.Key key, Environment env)
+      throws FailedIOException, InterruptedException {
+    if (key.kind == BzlCompileValue.Kind.EMPTY_PRELUDE) {
+      // Does not correspond to a file.
+      return true;
+    }
+    RootedPath rootedPath = RootedPath.toRootedPath(key.root, key.label.toPathFragment());
+    try {
+      return env.getValueOrThrow(FileValue.key(rootedPath), IOException.class) != null;
+    } catch (IOException e) {
+      throw new FailedIOException(e, Transience.PERSISTENT);
+    }
+  }
+
   /**
    * Replays the syntax errors from a file onto an event handler, adding more context if necessary.
    */

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

@@ -1535,6 +1535,12 @@ 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.
+        if (!BzlCompileFunction.requestFileDepOnCacheHit(key, env)) {
+          return null;
+        }
       }
       return value;
     }

src/test/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -2167,6 +2167,7 @@ java_test(
         "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
         "//src/test/java/com/google/devtools/build/lib/skyframe/util:SkyframeExecutorTestUtils",
         "//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/java/guava:collect",
         "//third_party/java/jsr305_annotations",

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;
@@ -49,7 +51,11 @@
 import com.google.devtools.common.options.OptionsParser;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.InterruptedIOException;
 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 net.starlark.java.syntax.Types;
@@ -1288,9 +1294,86 @@ def provides_str():
         "in call to requires_int(), parameter 'x' got value of type 'str', want 'int'");
   }
 
+  @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 InterruptedIOException 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<BzlLoadValue> 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);
@@ -1301,6 +1384,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 InterruptedIOException();
+        }
+      }
       return super.statIfFound(path, followSymlinks);
     }