[8.8.0] Fix repo contents cache FileValue staleness

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

@@ -249,6 +249,7 @@ public void workspaceInit(
           SkyframeExecutorRepositoryHelpersHolder.create(
               new RepositoryDirectoryDirtinessChecker()));
     }
+    builder.setRepoContentsCachePathSupplier(repositoryCache.getRepoContentsCache()::getPath);
 
     // Create the repository function everything flows through.
     repositoryDelegatorFunction =

src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java

@@ -269,8 +269,9 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) throws
     }
 
     // If an action output is stale, Skyframe will delete it prior to action execution. However,
-    // this doesn't apply to spawn outputs that aren't action outputs. To avoid incorrectly reusing
-    // one such stale output, check for its up-to-dateness here.
+    // this doesn't apply to spawn outputs that aren't action outputs, or to files in external repos
+    // that are remote repo contents cache hits. To avoid incorrectly reusing one such stale file,
+    // check for its up-to-dateness here.
     if (stat.getSize() != metadata.getSize()) {
       return true;
     }

src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java

@@ -26,7 +26,7 @@
 import build.bazel.remote.execution.v2.Tree;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionInputPrefetcher;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -68,6 +68,7 @@
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
+import java.util.function.Consumer;
 import javax.annotation.Nullable;
 
 /**
@@ -79,16 +80,6 @@
  */
 public final class RemoteExternalOverlayFileSystem extends FileSystem {
 
-  @Override
-  public boolean isFilePathCaseSensitive() {
-    return nativeFs.isFilePathCaseSensitive();
-  }
-
-  @Override
-  public FileSystem getHostFileSystem() {
-    return nativeFs.getHostFileSystem();
-  }
-
   private final PathFragment externalDirectory;
   private final int externalDirectorySegmentCount;
   private final FileSystem nativeFs;
@@ -189,31 +180,54 @@ public void afterCommand() {
     this.evaluator = null;
   }
 
-  public void injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile)
-      throws IOException {
+  /**
+   * Injects the given remote contents, possibly prefetching some files, and returns true on
+   * success.
+   */
+  public boolean injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile)
+      throws IOException, InterruptedException {
     var childMap =
         remoteContents.getChildrenList().stream()
             .collect(
                 toImmutableMap(cache.digestUtil::compute, directory -> directory, (a, b) -> a));
+    var repoDir = externalDirectory.getChild(repo.getName());
+    var filesToPrefetch = new ArrayList<PathFragment>();
     injectRecursively(
-        externalFs, externalDirectory.getChild(repo.getName()), remoteContents.getRoot(), childMap);
+        externalFs, repoDir, remoteContents.getRoot(), childMap, filesToPrefetch::add);
+    try {
+      // TODO: This prefetches a large number of small files. Investigate whether BatchReadBlobs
+      // would be more efficient.
+      prefetch(filesToPrefetch);
+    } catch (BulkTransferException e) {
+      if (e.allCausedByCacheNotFoundException()) {
+        // The cache has lost the .bzl files, which should be treated just like a cache miss.
+        externalFs.deleteTree(repoDir);
+        return false;
+      }
+      throw e;
+    }
     // Create the repo directory on disk so that readdir reflects the overlaid state of the external
     // directory.
     nativeFs.createDirectoryAndParents(externalDirectory.getChild(repo.getName()));
     // Keep the marker file contents in memory so that it can be written out when the repo is
     // materialized. This doubles as a presence marker for the in-memory repo contents.
     markerFileContents.put(repo.getName(), markerFile);
+    return true;
   }
 
   private static void injectRecursively(
       RemoteExternalFileSystem fs,
       PathFragment path,
       Directory dir,
-      ImmutableMap<Digest, Directory> childMap)
+      ImmutableMap<Digest, Directory> childMap,
+      Consumer<PathFragment> filesToPrefetch)
       throws IOException {
     fs.createDirectoryAndParents(path);
     for (var file : dir.getFilesList()) {
       var filePath = path.getRelative(unicodeToInternal(file.getName()));
+      if (shouldPrefetch(filePath)) {
+        filesToPrefetch.accept(filePath);
+      }
       fs.injectFile(
           filePath,
           FileArtifactValue.RemoteFileArtifactValue.create(
@@ -238,7 +252,7 @@ private static void injectRecursively(
             "Directory %s with digest %s not found in tree"
                 .formatted(subdirPath, subdirNode.getDigest().getHash()));
       }
-      injectRecursively(fs, subdirPath, subdir, childMap);
+      injectRecursively(fs, subdirPath, subdir, childMap, filesToPrefetch);
     }
   }
 
@@ -276,22 +290,15 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter)
     var remoteRepo = externalFs.getPath(repoPath);
     var walkResult = walk(remoteRepo);
     for (var directory : walkResult.directories()) {
-      nativeFs.getPath(directory.asFragment()).createDirectory();
+      nativeFs.getPath(directory).createDirectory();
     }
-    var unused =
-        getFromFuture(
-            inputPrefetcher.prefetchFiles(
-                /* action= */ null,
-                Iterables.transform(
-                    walkResult.files(), path -> ActionInputHelper.fromPath(path.asFragment())),
-                actionInput -> externalFs.getMetadata(actionInput.getExecPath()),
-                ActionInputPrefetcher.Priority.CRITICAL,
-                ActionInputPrefetcher.Reason.INPUTS));
+    prefetch(walkResult.files());
     // Create symlinks last as some platforms don't allow creating a symlink to a non-existent
     // target. A symlink may have already been created as an input to an action.
     for (var remoteSymlink : walkResult.symlinks()) {
-      var nativeSymlink = nativeFs.getPath(remoteSymlink.asFragment());
-      FileSystemUtils.ensureSymbolicLink(nativeSymlink, remoteSymlink.readSymbolicLink());
+      var nativeSymlink = nativeFs.getPath(remoteSymlink);
+      FileSystemUtils.ensureSymbolicLink(
+          nativeSymlink, externalFs.getPath(remoteSymlink).readSymbolicLink());
     }
 
     // After the repo has been copied, atomically materialize the marker file. This ensures that the
@@ -304,7 +311,19 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter)
     markerFileSibling.renameTo(markerFile);
   }
 
-  private record WalkResult(List<Path> files, List<Path> symlinks, List<Path> directories) {}
+  private void prefetch(List<PathFragment> paths) throws IOException, InterruptedException {
+    var unused =
+        getFromFuture(
+            inputPrefetcher.prefetchFiles(
+                /* action= */ null,
+                Lists.transform(paths, ActionInputHelper::fromPath),
+                actionInput -> externalFs.getMetadata(actionInput.getExecPath()),
+                ActionInputPrefetcher.Priority.CRITICAL,
+                ActionInputPrefetcher.Reason.INPUTS));
+  }
+
+  private record WalkResult(
+      List<PathFragment> files, List<PathFragment> symlinks, List<PathFragment> directories) {}
 
   private static WalkResult walk(Path root) throws IOException {
     var result = new WalkResult(new ArrayList<>(), new ArrayList<>(), new ArrayList<>());
@@ -316,17 +335,37 @@ private static void walk(Path root, WalkResult result) throws IOException {
     for (var dirent : root.readdir(Symlinks.NOFOLLOW)) {
       var fromChild = root.getChild(dirent.getName());
       switch (dirent.getType()) {
-        case FILE -> result.files.add(fromChild);
-        case SYMLINK -> result.symlinks.add(fromChild);
+        case FILE -> result.files.add(fromChild.asFragment());
+        case SYMLINK -> result.symlinks.add(fromChild.asFragment());
         case DIRECTORY -> {
-          result.directories.add(fromChild);
+          result.directories.add(fromChild.asFragment());
           walk(fromChild, result);
         }
         default -> throw new IOException("Unsupported file type: " + dirent);
       }
     }
   }
 
+  /** Whether the file with the given path should be materialized eagerly when injecting a repo. */
+  private static boolean shouldPrefetch(PathFragment path) {
+    // .bzl files are typically small and the loads between them can form complex DAGs that can only
+    // be discovered layer by layer, so prefetching is worthwhile to reduce the number of sequential
+    // cache requests.
+    // The REPO.bazel file, if present, is a dependency of any package and will thus have to be
+    // fetched anyway.
+    return path.getFileExtension().equals("bzl") || path.getBaseName().equals("REPO.bazel");
+  }
+
+  @Override
+  public FileSystem getHostFileSystem() {
+    return nativeFs.getHostFileSystem();
+  }
+
+  @Override
+  public boolean isFilePathCaseSensitive() {
+    return nativeFs.isFilePathCaseSensitive();
+  }
+
   // Always mirror tree deletions to the underlying native file system to support bazel clean and
   // repository refetching.
 
@@ -603,6 +642,9 @@ private FileArtifactValue getMetadata(PathFragment path) throws IOException {
 
     @Override
     public synchronized InputStream getInputStream(PathFragment path) throws IOException {
+      if (shouldPrefetch(path)) {
+        return nativeFs.getInputStream(path);
+      }
       var relativePath = path.relativeTo(externalDirectory);
       var info =
           (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true);

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -768,7 +768,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
             invocationId,
             remoteOptions.remoteInstanceName,
             remoteOptions.remoteAcceptCached,
-            remoteOptions.remoteUploadLocalResults));
+            remoteOptions.remoteUploadLocalResults,
+            verboseFailures));
     if (env.getDirectories().getOutputBase().getFileSystem()
         instanceof RemoteExternalOverlayFileSystem remoteFs) {
       remoteFs.beforeCommand(

src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java

@@ -24,6 +24,7 @@
 import build.bazel.remote.execution.v2.Directory;
 import build.bazel.remote.execution.v2.Tree;
 import com.google.common.base.Splitter;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.Futures;
@@ -95,6 +96,7 @@ public final class RemoteRepoContentsCacheImpl implements RemoteRepoContentsCach
   private final String commandId;
   private final boolean acceptCached;
   private final boolean uploadLocalResults;
+  private final boolean verboseFailures;
   private final DigestUtil digestUtil;
   private final Action baseAction;
 
@@ -103,12 +105,14 @@ public RemoteRepoContentsCacheImpl(
       String buildRequestId,
       String commandId,
       boolean acceptCached,
-      boolean uploadLocalResults) {
+      boolean uploadLocalResults,
+      boolean verboseFailures) {
     this.buildRequestId = buildRequestId;
     this.commandId = commandId;
     this.cache = cache;
     this.acceptCached = acceptCached;
     this.uploadLocalResults = uploadLocalResults;
+    this.verboseFailures = verboseFailures;
     this.digestUtil = cache.digestUtil;
     this.baseAction =
         Action.newBuilder()
@@ -151,7 +155,7 @@ public void addToCache(
       reporter.handle(
           Event.warn(
               "Failed to read marker file repo %s, skipping: %s"
-                  .formatted(repoName, e.getMessage())));
+                  .formatted(repoName, maybeGetStackTrace(e))));
     }
     var action = buildAction(predeclaredInputHash);
     var actionKey = new ActionKey(digestUtil.compute(action));
@@ -178,7 +182,7 @@ public void addToCache(
       reporter.handle(
           Event.warn(
               "Failed to upload repo contents to remote cache for repo %s: %s"
-                  .formatted(repoName, e.getMessage())));
+                  .formatted(repoName, maybeGetStackTrace(e))));
     }
   }
 
@@ -189,6 +193,22 @@ public boolean lookupCache(
       String predeclaredInputHash,
       ExtendedEventHandler reporter)
       throws IOException, InterruptedException {
+    try {
+      return doLookupCache(repoName, repoDir, predeclaredInputHash, reporter);
+    } catch (IOException e) {
+      throw new IOException(
+          "Failed to look up repo %s in the remote repo contents cache: %s"
+              .formatted(repoName, maybeGetStackTrace(e)),
+          e);
+    }
+  }
+
+  private boolean doLookupCache(
+      RepositoryName repoName,
+      Path repoDir,
+      String predeclaredInputHash,
+      ExtendedEventHandler reporter)
+      throws IOException, InterruptedException {
     if (!(repoDir.getFileSystem() instanceof RemoteExternalOverlayFileSystem remoteFs)) {
       return false;
     }
@@ -246,7 +266,8 @@ public boolean lookupCache(
       markerFileContent = new String(markerFileContentFuture.get(), StandardCharsets.ISO_8859_1);
       repoDirectoryContent = repoDirectoryContentFuture.get();
     } catch (ExecutionException e) {
-      throw new IllegalStateException("waitForBulkTransfer should have thrown", e);
+      throw new IllegalStateException(
+          "waitForBulkTransfer should have thrown: " + maybeGetStackTrace(e));
     }
     var markerFileLines =
         Splitter.on('\n')
@@ -270,8 +291,7 @@ public boolean lookupCache(
       return false;
     }
 
-    remoteFs.injectRemoteRepo(repoName, repoDirectoryContent, markerFileContent);
-    return true;
+    return remoteFs.injectRemoteRepo(repoName, repoDirectoryContent, markerFileContent);
   }
 
   private RemoteActionExecutionContext buildContext(RepositoryName repoName) {
@@ -295,6 +315,10 @@ private Action buildAction(String predeclaredInputHash) {
         .build();
   }
 
+  private String maybeGetStackTrace(Exception e) {
+    return verboseFailures ? Throwables.getStackTraceAsString(e) : e.getMessage();
+  }
+
   private record RepoRemotePathResolver(Path fetchedRepoMarkerFile, Path fetchedRepoDir)
       implements RemotePathResolver {
 

src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java

@@ -30,6 +30,7 @@ class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFacto
   private final String remoteInstanceName;
   private final boolean acceptCached;
   private final boolean uploadLocalResults;
+  private final boolean verboseFailures;
 
   RepositoryRemoteHelpersFactoryImpl(
       CombinedCache cache,
@@ -38,14 +39,16 @@ class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFacto
       String commandId,
       String remoteInstanceName,
       boolean acceptCached,
-      boolean uploadLocalResults) {
+      boolean uploadLocalResults,
+      boolean verboseFailures) {
     this.cache = cache;
     this.remoteExecutor = remoteExecutor;
     this.buildRequestId = buildRequestId;
     this.commandId = commandId;
     this.remoteInstanceName = remoteInstanceName;
     this.acceptCached = acceptCached;
     this.uploadLocalResults = uploadLocalResults;
+    this.verboseFailures = verboseFailures;
   }
 
   @Nullable
@@ -68,6 +71,6 @@ public RepositoryRemoteExecutor createExecutor() {
   @Override
   public RemoteRepoContentsCache createRepoContentsCache() {
     return new RemoteRepoContentsCacheImpl(
-        cache, buildRequestId, commandId, acceptCached, uploadLocalResults);
+        cache, buildRequestId, commandId, acceptCached, uploadLocalResults, verboseFailures);
   }
 }