[9.3.0] Register a FileValue dep on bzlCompileCache hits in BzlLoadFunction (…

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",

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;
     }

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);

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;
     }

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",

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<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);
@@ -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);
     }