[9.0.0] Fix repo contents cache FileValue staleness

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -208,10 +208,8 @@ public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builde
   @Override
   public void workspaceInit(
       BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) {
-    // TODO(b/27143724): Remove this guard when Google-internal flavor no longer uses repositories.
-    if ("bazel".equals(runtime.getProductName())) {
-      builder.allowExternalRepositories(true);
-    }
+    builder.allowExternalRepositories(true);
+    builder.setRepoContentsCachePathSupplier(repositoryCache.getRepoContentsCache()::getPath);
 
     repositoryFetchFunction =
         new RepositoryFetchFunction(

src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFetchFunction.java

@@ -21,6 +21,7 @@
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.FileStateValue;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.bazel.bzlmod.NonRegistryOverride;
@@ -297,16 +298,40 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                       e),
                   Transience.TRANSIENT);
             }
-            // Don't forget to register a FileValue on the cache repo dir, so that we know to
-            // refetch
-            // if the cache entry gets GC'd from under us.
+            // Don't forget to register a FileStateValue on the cache repo dir, so that we know to
+            // refetch if the cache entry gets GC'd from under us or the entire cache is deleted.
+            //
+            // Note that registering a FileValue dependency instead would lead to subtly incorrect
+            // behavior when the repo contents cache directory is deleted between builds:
+            // 1. We register a FileValue dependency on the cache entry.
+            // 2. Before the next build, the repo contents cache directory is deleted.
+            // 3. On the next build, FileSystemValueChecker invalidates the underlying
+            //    FileStateValue, which in turn results in the FileValue and the current
+            //    RepositoryDirectoryValue being marked as dirty.
+            // 4. Skyframe visits the dirty nodes bottom up to check for actual changes. In
+            //    particular, it reevaluates FileFunction before RepositoryFetchFunction and thus
+            //    the FileValue of the repo contents cache directory is locked in as non-existent
+            //    before RepositoryFetchFunction can recreate it.
+            // 5. Any other SkyFunction that depends on the FileValue of a file in the repo (e.g.
+            //    PackageFunction) will report that file as missing since the resolved path has a
+            //    parent that is non-existent.
+            // By using FileStateValue directly, which benefits from special logic built into
+            // DirtinessCheckerUtils that recognizes the repo contents cache directories with
+            // non-UUID names and prevents locking in their value during dirtiness checking, we
+            // avoid 4. and thus the incorrect missing file errors in 5.
             if (env.getValue(
-                    FileValue.key(
+                    FileStateValue.key(
                         RootedPath.toRootedPath(
                             Root.absoluteRoot(cachedRepoDir.getFileSystem()), cachedRepoDir)))
                 == null) {
               return null;
             }
+            // This is never reached: the repo dir in the repo contents cache is created under a new
+            // UUID-named directory and thus the FileStateValue above will always be missing from
+            // Skyframe. After the restart, the repo will either encounter the just created cache
+            // entry as a candidate or will create a new one if it got GC'd in the meantime.
+            throw new IllegalStateException(
+                "FileStateValue unexpectedly present for " + cachedRepoDir);
           }
           if (remoteRepoContentsCache != null) {
             remoteRepoContentsCache.addToCache(

src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecRegistry;
 import com.google.devtools.build.lib.skyframe.serialization.analysis.RemoteAnalysisCachingServicesSupplier;
 import com.google.devtools.build.lib.util.AbruptExitException;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.SingleFileSystemSyscallCache;
 import com.google.devtools.build.lib.vfs.SyscallCache;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -62,6 +63,7 @@ public final class WorkspaceBuilder {
   private SyscallCache syscallCache;
 
   private boolean allowExternalRepositories = true;
+  private Supplier<Path> repoContentsCachePathSupplier = () -> null;
   @Nullable private Supplier<ObjectCodecRegistry> analysisCodecRegistrySupplier = null;
 
   @Nullable
@@ -123,6 +125,7 @@ BlazeWorkspace build(
             skyFunctions.buildOrThrow(),
             singleFsSyscallCache,
             allowExternalRepositories,
+            repoContentsCachePathSupplier,
             skyKeyStateReceiver == null
                 ? SkyframeExecutor.SkyKeyStateReceiver.NULL_INSTANCE
                 : skyKeyStateReceiver,
@@ -223,6 +226,13 @@ public WorkspaceBuilder allowExternalRepositories(boolean allowExternalRepositor
     return this;
   }
 
+  @CanIgnoreReturnValue
+  public WorkspaceBuilder setRepoContentsCachePathSupplier(
+      Supplier<Path> repoContentsCachePathSupplier) {
+    this.repoContentsCachePathSupplier = repoContentsCachePathSupplier;
+    return this;
+  }
+
   @CanIgnoreReturnValue
   public WorkspaceBuilder setSkyKeyStateReceiver(
       SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver) {

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

@@ -167,7 +167,7 @@ Map<RepositoryName, RootedPath> getDirtyExternalRepos() {
     private static boolean isCacheableType(FileType fileType) {
       return switch (fileType) {
         case INTERNAL, EXTERNAL_OTHER, BUNDLED -> true;
-        case EXTERNAL_REPO, OUTPUT -> false;
+        case EXTERNAL_REPO, OUTPUT, REPO_CONTENTS_CACHE_DIRS -> false;
       };
     }
   }

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

@@ -40,6 +40,7 @@
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
 /** Common utilities for dealing with paths outside the package roots. */
@@ -50,6 +51,7 @@ public class ExternalFilesHelper {
   private final ExternalFileAction externalFileAction;
   private final BlazeDirectories directories;
   private final int maxNumExternalFilesToLog;
+  private final Supplier<Path> repoContentsCachePathSupplier;
   private final AtomicInteger numExternalFilesLogged = new AtomicInteger(0);
   private static final int MAX_EXTERNAL_FILES_TO_TRACK = 2500;
 
@@ -66,31 +68,52 @@ private ExternalFilesHelper(
       AtomicReference<PathPackageLocator> pkgLocator,
       ExternalFileAction externalFileAction,
       BlazeDirectories directories,
+      Supplier<Path> repoContentsCachePathSupplier,
       int maxNumExternalFilesToLog) {
     this.pkgLocator = pkgLocator;
     this.externalFileAction = externalFileAction;
     this.directories = directories;
+    this.repoContentsCachePathSupplier = repoContentsCachePathSupplier;
     this.maxNumExternalFilesToLog = maxNumExternalFilesToLog;
   }
 
   public static ExternalFilesHelper create(
       AtomicReference<PathPackageLocator> pkgLocator,
       ExternalFileAction externalFileAction,
-      BlazeDirectories directories) {
+      BlazeDirectories directories,
+      Supplier<Path> repoContentsCachePathSupplier) {
     return TestType.isInTest()
-        ? createForTesting(pkgLocator, externalFileAction, directories)
+        ? createForTesting(
+            pkgLocator, externalFileAction, directories, repoContentsCachePathSupplier)
         : new ExternalFilesHelper(
-            pkgLocator, externalFileAction, directories, /* maxNumExternalFilesToLog= */ 100);
+            pkgLocator,
+            externalFileAction,
+            directories,
+            repoContentsCachePathSupplier,
+            /* maxNumExternalFilesToLog= */ 100);
   }
 
   public static ExternalFilesHelper createForTesting(
       AtomicReference<PathPackageLocator> pkgLocator,
       ExternalFileAction externalFileAction,
       BlazeDirectories directories) {
+    return createForTesting(
+        pkgLocator,
+        externalFileAction,
+        directories,
+        /* repoContentsCachePathSupplier= */ () -> null);
+  }
+
+  public static ExternalFilesHelper createForTesting(
+      AtomicReference<PathPackageLocator> pkgLocator,
+      ExternalFileAction externalFileAction,
+      BlazeDirectories directories,
+      Supplier<Path> repoContentsCachePathSupplier) {
     return new ExternalFilesHelper(
         pkgLocator,
         externalFileAction,
         directories,
+        repoContentsCachePathSupplier,
         // These log lines are mostly spam during unit and integration tests.
         /* maxNumExternalFilesToLog= */ 0);
   }
@@ -155,11 +178,26 @@ public enum FileType {
      */
     EXTERNAL_REPO,
 
+    /**
+     * The directory containing the repo contents cache entries as well as direct children
+     * corresponding to individual predeclared input hashes. These directories are created by Bazel
+     * but may be deleted when users delete the entire repo contents cache.
+     *
+     * <p>These files' handling differs from EXTERNAL_REPO as they are never modified after they are
+     * created and don't live under the external directory, as well as from EXTERNAL_OTHER as they
+     * can be recreated by Bazel after diff detection.
+     *
+     * <p>The contents of these directories are considered EXTERNAL_OTHER as they carry UUID names
+     * and are thus never reused.
+     */
+    REPO_CONTENTS_CACHE_DIRS,
+
     /**
      * None of the above. We encounter these paths when outputs, source files or external repos
      * symlink to files outside aforementioned Bazel-managed directories. For example, C compilation
      * by the host compiler may depend on /usr/bin/gcc. Bazel makes a best-effort attempt to detect
-     * changes in such files.
+     * changes in such files. This type also includes files in repo contents cache entries, which
+     * are created by Bazel but never modified after creation.
      */
     EXTERNAL_OTHER,
   }
@@ -207,7 +245,11 @@ void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) {
 
   ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
     return new ExternalFilesHelper(
-        pkgLocator, externalFileAction, directories, maxNumExternalFilesToLog);
+        pkgLocator,
+        externalFileAction,
+        directories,
+        repoContentsCachePathSupplier,
+        maxNumExternalFilesToLog);
   }
 
   public FileType getAndNoteFileType(RootedPath rootedPath) {
@@ -240,6 +282,12 @@ private FileType detectFileType(RootedPath rootedPath) {
     if (packageLocator.getPathEntries().contains(rootedPath.getRoot())) {
       return FileType.INTERNAL;
     }
+    var repoContentsCachePath = repoContentsCachePathSupplier.get();
+    if (repoContentsCachePath != null
+        && rootedPath.asPath().startsWith(repoContentsCachePath)
+        && !rootedPath.asPath().relativeTo(repoContentsCachePath).isMultiSegment()) {
+      return FileType.REPO_CONTENTS_CACHE_DIRS;
+    }
     // The outputBase may be null if we're not actually running a build.
     Path outputBase = packageLocator.getOutputBase();
     if (outputBase == null) {
@@ -270,6 +318,7 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment
     switch (fileType) {
       case BUNDLED:
       case INTERNAL:
+      case REPO_CONTENTS_CACHE_DIRS:
         break;
       case EXTERNAL_OTHER:
         if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) {
@@ -294,14 +343,12 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment
 
   /**
    * For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding repo so
-   * that if the repo definition changes, the File/DirectoryStateValue will be re-evaluated.
+   * that if the repo is refetched, the {File,DirectoryListing}StateValue's of files underneath will
+   * be re-evaluated.
    *
-   * <p>Note that: - We don't add a dependency on the parent directory at the package root boundary,
-   * so the only transitive dependencies from files inside the package roots to external files are
-   * through symlinks. So the upwards transitive closure of external files is small. - The only way
-   * other than external repositories for external source files to get into the skyframe graph in
-   * the first place is through symlinks outside the package roots, which we neither want to
-   * encourage nor optimize for since it is not common. So the set of external files is small.
+   * <p>Note that we don't add a dependency on the parent directory at the package root boundary, so
+   * the only transitive dependencies from files inside the package roots to external files are
+   * through symlinks. So the upwards transitive closure of external files is small.
    */
   private void addExternalFilesDependencies(RootedPath rootedPath, Environment env)
       throws InterruptedException {

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

@@ -79,6 +79,7 @@
 import com.google.devtools.build.lib.vfs.FileStateKey;
 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.SyscallCache;
 import com.google.devtools.build.skyframe.DelegatingGraphInconsistencyReceiver;
 import com.google.devtools.build.skyframe.EmittedEventState;
@@ -115,6 +116,7 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
+import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
 /**
@@ -168,6 +170,7 @@ protected SequencedSkyframeExecutor(
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
       ImmutableList<BuildFileName> buildFilesByPriority,
       boolean allowExternalRepositories,
+      Supplier<Path> repoContentsCachePathSupplier,
       ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
       ActionOnFilesystemErrorCodeLoadingBzlFile actionOnFilesystemErrorCodeLoadingBzlFile,
       boolean shouldUseRepoDotBazel,
@@ -200,6 +203,7 @@ protected SequencedSkyframeExecutor(
         workspaceInfoFromDiffReceiver,
         new SequencedRecordingDifferencer(),
         allowExternalRepositories,
+        repoContentsCachePathSupplier,
         globUnderSingleDep,
         diffCheckNotificationOptions);
   }
@@ -828,6 +832,7 @@ public static final class Builder {
     private WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver =
         (ignored1, ignored2) -> {};
     private boolean allowExternalRepositories = false;
+    private Supplier<Path> repoContentsCachePathSupplier = () -> null;
     private Consumer<SkyframeExecutor> skyframeExecutorConsumerOnInit = skyframeExecutor -> {};
     private SkyFunction ignoredSubdirectoriesFunction;
     private BugReporter bugReporter = BugReporter.defaultInstance();
@@ -866,6 +871,7 @@ public SequencedSkyframeExecutor build() {
               crossRepositoryLabelViolationStrategy,
               buildFilesByPriority,
               allowExternalRepositories,
+              repoContentsCachePathSupplier,
               actionOnIOExceptionReadingBuildFile,
               actionOnFilesystemErrorCodeLoadingBzlFile,
               shouldUseRepoDotBazel,
@@ -946,6 +952,12 @@ public Builder allowExternalRepositories(boolean allowExternalRepositories) {
       return this;
     }
 
+    @CanIgnoreReturnValue
+    public Builder setRepoContentsCachePathSupplier(Supplier<Path> repoContentsCachePathSupplier) {
+      this.repoContentsCachePathSupplier = repoContentsCachePathSupplier;
+      return this;
+    }
+
     @CanIgnoreReturnValue
     public Builder setCrossRepositoryLabelViolationStrategy(
         CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) {

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

@@ -20,9 +20,11 @@
 import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.SyscallCache;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
+import java.util.function.Supplier;
 
 /** A factory of SkyframeExecutors that returns SequencedSkyframeExecutor. */
 public final class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory {
@@ -38,6 +40,7 @@ public SkyframeExecutor create(
       ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
       SyscallCache syscallCache,
       boolean allowExternalRepositories,
+      Supplier<Path> repoContentsCachePathSupplier,
       SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver,
       BugReporter bugReporter) {
     return BazelSkyframeExecutorConstants.newBazelSkyframeExecutorBuilder()
@@ -50,6 +53,7 @@ public SkyframeExecutor create(
         .setExtraSkyFunctions(extraSkyFunctions)
         .setSyscallCache(syscallCache)
         .allowExternalRepositories(allowExternalRepositories)
+        .setRepoContentsCachePathSupplier(repoContentsCachePathSupplier)
         .setSkyKeyStateReceiver(skyKeyStateReceiver)
         .setBugReporter(bugReporter)
         .build();

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

@@ -688,6 +688,7 @@ protected SkyframeExecutor(
       @Nullable WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver,
       @Nullable RecordingDifferencer recordingDiffer,
       boolean allowExternalRepositories,
+      Supplier<Path> repoContentsCachePathSupplier,
       boolean globUnderSingleDep,
       Optional<DiffCheckNotificationOptions> diffCheckNotificationOptions) {
     // Strictly speaking, these arguments are not required for initialization, but all current
@@ -733,7 +734,8 @@ protected SkyframeExecutor(
     this.skyframeBuildView =
         new SkyframeBuildView(artifactFactory, this, ruleClassProvider, actionKeyContext);
     this.externalFilesHelper =
-        ExternalFilesHelper.create(pkgLocator, externalFileAction, directories);
+        ExternalFilesHelper.create(
+            pkgLocator, externalFileAction, directories, repoContentsCachePathSupplier);
     this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy;
     this.buildFilesByPriority = buildFilesByPriority;
     this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile;

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

@@ -21,9 +21,11 @@
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.SyscallCache;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
+import java.util.function.Supplier;
 
 /** A factory that creates instances of SkyframeExecutor. */
 public interface SkyframeExecutorFactory {
@@ -50,6 +52,7 @@ SkyframeExecutor create(
       ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
       SyscallCache syscallCache,
       boolean allowExternalRepositories,
+      Supplier<Path> repoContentsCachePathSupplier,
       SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver,
       BugReporter bugReporter)
       throws AbruptExitException;