[8.8.0] Support dynamic inputs with the remote repo contents cache

src/main/java/com/google/devtools/build/lib/BUILD

@@ -288,6 +288,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
     ],
 )
 

src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java

@@ -17,6 +17,7 @@
 
 import com.google.common.util.concurrent.ListenableFuture;
 import java.io.IOException;
+import javax.annotation.Nullable;
 
 /** Prefetches files to local disk. */
 public interface ActionInputPrefetcher {
@@ -34,7 +35,7 @@ public interface MetadataSupplier {
       new ActionInputPrefetcher() {
         @Override
         public ListenableFuture<Void> prefetchFiles(
-            ActionExecutionMetadata action,
+            @Nullable ActionExecutionMetadata action,
             Iterable<? extends ActionInput> inputs,
             MetadataSupplier metadataSupplier,
             Priority priority,
@@ -90,7 +91,7 @@ enum Reason {
    * @return future success if prefetch is finished or {@link IOException}.
    */
   ListenableFuture<Void> prefetchFiles(
-      ActionExecutionMetadata action,
+      @Nullable ActionExecutionMetadata action,
       Iterable<? extends ActionInput> inputs,
       MetadataSupplier metadataSupplier,
       Priority priority,

src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java

@@ -141,6 +141,7 @@ public Path getWorkspace() {
   }
 
   /** Returns working directory of the server. */
+  @Nullable
   public Path getWorkingDirectory() {
     return workspace;
   }

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

@@ -720,10 +720,18 @@ private String getAbsolutePath(String path, CommandEnvironment env) {
    */
   @Nullable
   private Path toPath(PathFragment path, CommandEnvironment env) {
-    if (path.isEmpty() || env.getBlazeWorkspace().getWorkspace() == null) {
+    if (path.isEmpty() || env.getDirectories().getWorkspace() == null) {
       return null;
     }
-    return env.getBlazeWorkspace().getWorkspace().getRelative(path);
+    // It is important to use getWorkspace() here, not getWorkingDirectory(). Both Paths have the
+    // same underlying PathFragment, but may differ in their FileSystem if the remote repo contents
+    // cache is in use. getWorkspace() uses the same FileSystem as everything other than the
+    // workspace directory, while getWorkingDirectory() uses the workspace directory's FileSystem.
+    // Even though the users of the returned Path may end up writing to it, they are not expected to
+    // update source files within the workspace. Thus, the correct FileSystem is the one from
+    // getWorkspace(), which e.g. allows moves from the external directory under the output base to
+    // the local repo contents cache without crossing FileSystems.
+    return env.getDirectories().getWorkspace().getRelative(path);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java

@@ -91,7 +91,7 @@ protected ModuleExtensionContext(
     this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency;
     // Record inputs to the extension that are known prior to evaluation.
     RepoRecordedInput.EnvVar.wrap(staticEnvVars)
-        .forEach((input, value) -> recordInput(input, value.orElse(null)));
+        .forEach((input, value) -> recordInputWithValue(input, value.orElse(null)));
     repoMappingRecorder.record(staticRepoMappingEntries);
   }
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java

@@ -395,12 +395,18 @@ private static Optional<String> didRecordedInputsChange(
       BlazeDirectories directories,
       List<RepoRecordedInput.WithValue> recordedInputs)
       throws InterruptedException, NeedsSkyframeRestartException {
-    Optional<String> outdated =
-        RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputs);
-    if (env.valuesMissing()) {
-      throw new NeedsSkyframeRestartException();
+    // Check inputs in batches to prevent Skyframe cycles caused by outdated dependencies.
+    for (ImmutableList<RepoRecordedInput.WithValue> batch :
+        RepoRecordedInput.WithValue.splitIntoBatches(recordedInputs)) {
+      Optional<String> outdated = RepoRecordedInput.isAnyValueOutdated(env, directories, batch);
+      if (env.valuesMissing()) {
+        throw new NeedsSkyframeRestartException();
+      }
+      if (outdated.isPresent()) {
+        return outdated;
+      }
     }
-    return outdated;
+    return Optional.empty();
   }
 
   private SingleExtensionValue createSingleExtensionValue(

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

@@ -154,9 +154,9 @@ private Path ensureTrashDir() throws IOException {
   /**
    * Moves a freshly fetched repo into the contents cache.
    *
-   * @return the repo dir in the contents cache.
+   * @return the new cache entry
    */
-  public Path moveToCache(
+  public CandidateRepo moveToCache(
       Path fetchedRepoDir, Path fetchedRepoMarkerFile, String predeclaredInputHash)
       throws IOException {
     Preconditions.checkState(path != null);
@@ -183,7 +183,7 @@ public Path moveToCache(
     // Set up a symlink at the original fetched repo dir path.
     fetchedRepoDir.deleteTree();
     FileSystemUtils.ensureSymbolicLink(fetchedRepoDir, cacheRepoDir);
-    return cacheRepoDir;
+    return new CandidateRepo(cacheRecordedInputsFile, cacheRepoDir);
   }
 
   public void acquireSharedLock() throws IOException, InterruptedException {

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

@@ -23,13 +23,10 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.flogger.GoogleLogger;
 import com.google.common.util.concurrent.Futures;
-import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent;
 import com.google.devtools.build.lib.bazel.repository.DecompressorDescriptor;
@@ -55,6 +52,7 @@
 import com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem;
 import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException;
 import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.MaybeValue;
 import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.RepoCacheFriendlyPath;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
@@ -71,6 +69,7 @@
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.ForOverride;
 import java.io.File;
 import java.io.IOException;
@@ -212,7 +211,7 @@ protected StarlarkBaseExternalContext(
     // apparent repo names to canonical repo names. See #20721 for why this is necessary.
     this.repoMappingRecorder =
         (fromRepo, apparentRepoName, canonicalRepoName) ->
-            recordInput(
+            recordInputWithValue(
                 new RepoRecordedInput.RecordedRepoMapping(fromRepo, apparentRepoName),
                 canonicalRepoName.isVisible() ? canonicalRepoName.getName() : null);
   }
@@ -251,7 +250,7 @@ public void storeRepoMappingRecorderInThread(StarlarkThread thread) {
     repoMappingRecorder.storeInThread(thread);
   }
 
-  protected void recordInput(RepoRecordedInput input, @Nullable String value) {
+  protected void recordInputWithValue(RepoRecordedInput input, @Nullable String value) {
     if (recordedInputs.containsKey(input) && !Objects.equals(recordedInputs.get(input), value)) {
       throw new IllegalStateException(
           "Conflicting values recorded for input %s: '%s' vs. '%s'"
@@ -260,6 +259,23 @@ protected void recordInput(RepoRecordedInput input, @Nullable String value) {
     recordedInputs.put(input, value);
   }
 
+  @CanIgnoreReturnValue
+  @Nullable
+  protected String getValueAndRecordInput(RepoRecordedInput input)
+      throws InterruptedException, NeedsSkyframeRestartException, IOException {
+    var maybeValue = input.getValue(env, directories);
+    if (env.valuesMissing()) {
+      throw new NeedsSkyframeRestartException();
+    }
+    return switch (maybeValue) {
+      case MaybeValue.Invalid(String reason) -> throw new IOException(reason);
+      case MaybeValue.Valid(String value) -> {
+        recordInputWithValue(input, value);
+        yield value;
+      }
+    };
+  }
+
   private boolean cancelPendingAsyncTasks() {
     boolean hadPendingItems = false;
     for (AsyncTask task : asyncTasks) {
@@ -1489,18 +1505,12 @@ private static <T> T nullIfNone(Object object, Class<T> type) {
   @Nullable
   public String getEnvironmentValue(String name, Object defaultValue)
       throws InterruptedException, NeedsSkyframeRestartException {
-    // Must look up via Skyframe, rather than solely copy from `this.repoEnvVariables`, in order to
-    // establish a SkyKey dependency relationship.
-    var nameAndValue = RepositoryFunction.getEnvVarValues(env, ImmutableSet.of(name));
-    if (nameAndValue == null) {
-      return null;
+    try {
+      String value = getValueAndRecordInput(new RepoRecordedInput.EnvVar(name));
+      return value != null ? value : nullIfNone(defaultValue, String.class);
+    } catch (IOException e) {
+      throw new IllegalStateException("getting EnvVar never throws IOException", e);
     }
-    // However, to account for --repo_env we take the value from `this.repoEnvVariables`.
-    // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 .
-    var entry = Iterables.getOnlyElement(RepoRecordedInput.EnvVar.wrap(nameAndValue).entrySet());
-    String envVarValue = repoEnvVariables.get(name);
-    recordInput(entry.getKey(), envVarValue);
-    return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class);
   }
 
   @StarlarkMethod(
@@ -1661,17 +1671,8 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch)
     if (repoCacheFriendlyPath == null) {
       return;
     }
-    var recordedInput = new RepoRecordedInput.File(repoCacheFriendlyPath);
-    var skyKey = recordedInput.getSkyKey(directories);
     try {
-      FileValue fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class);
-      if (fileValue == null) {
-        throw new NeedsSkyframeRestartException();
-      }
-
-      recordInput(
-          recordedInput,
-          RepoRecordedInput.File.fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue));
+      getValueAndRecordInput(new RepoRecordedInput.File(repoCacheFriendlyPath));
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }
@@ -1683,12 +1684,8 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch)
     if (repoCacheFriendlyPath == null) {
       return;
     }
-    var recordedInput = new RepoRecordedInput.Dirents(repoCacheFriendlyPath);
-    if (env.getValue(recordedInput.getSkyKey(directories)) == null) {
-      throw new NeedsSkyframeRestartException();
-    }
     try {
-      recordInput(recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path));
+      getValueAndRecordInput(new RepoRecordedInput.Dirents(repoCacheFriendlyPath));
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }

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

@@ -42,7 +42,6 @@
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.runtime.ProcessWrapper;
 import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
-import com.google.devtools.build.lib.skyframe.DirectoryTreeDigestValue;
 import com.google.devtools.build.lib.util.StringUtilities;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -587,15 +586,7 @@ public void watchTree(Object path)
       return;
     }
     try {
-      var recordedInput = new RepoRecordedInput.DirTree(repoCacheFriendlyPath);
-      DirectoryTreeDigestValue digestValue =
-          (DirectoryTreeDigestValue)
-              env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class);
-      if (digestValue == null) {
-        throw new NeedsSkyframeRestartException();
-      }
-
-      recordInput(recordedInput, digestValue.hexDigest());
+      getValueAndRecordInput(new RepoRecordedInput.DirTree(repoCacheFriendlyPath));
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }

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

@@ -329,7 +329,7 @@ protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOExc
    */
   @Override
   public ListenableFuture<Void> prefetchFiles(
-      ActionExecutionMetadata action,
+      @Nullable ActionExecutionMetadata action,
       Iterable<? extends ActionInput> inputs,
       MetadataSupplier metadataSupplier,
       Priority priority,

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -119,6 +119,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/util",
         "//src/main/java/com/google/devtools/build/lib/remote/util:digest_utils",
         "//src/main/java/com/google/devtools/build/lib/remote/zstd",
+        "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",

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

@@ -215,6 +215,15 @@ public CachedActionResult downloadActionResult(
       boolean inlineOutErr,
       Set<String> inlineOutputFiles)
       throws IOException, InterruptedException {
+    return getFromFuture(
+        downloadActionResultAsync(context, actionKey, inlineOutErr, inlineOutputFiles));
+  }
+
+  public ListenableFuture<CachedActionResult> downloadActionResultAsync(
+      RemoteActionExecutionContext context,
+      ActionKey actionKey,
+      boolean inlineOutErr,
+      Set<String> inlineOutputFiles) {
     var spawnExecutionContext = context.getSpawnExecutionContext();
 
     ListenableFuture<CachedActionResult> future = immediateFuture(null);
@@ -258,7 +267,7 @@ public CachedActionResult downloadActionResult(
               directExecutor());
     }
 
-    return getFromFuture(future);
+    return future;
   }
 
   private ListenableFuture<ActionResult> downloadActionResultFromRemote(

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

@@ -186,11 +186,13 @@ public void afterCommand() {
    */
   public boolean injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile)
       throws IOException, InterruptedException {
+    var repoDir = externalDirectory.getChild(repo.getName());
+    deleteTree(repoDir);
+    var unused = delete(externalDirectory.getChild(repo.getMarkerFileName()));
     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, repoDir, remoteContents.getRoot(), childMap, filesToPrefetch::add);
@@ -208,7 +210,7 @@ public boolean injectRemoteRepo(RepositoryName repo, Tree remoteContents, String
     }
     // Create the repo directory on disk so that readdir reflects the overlaid state of the external
     // directory.
-    nativeFs.createDirectoryAndParents(externalDirectory.getChild(repo.getName()));
+    nativeFs.createDirectoryAndParents(repoDir);
     // 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);

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

@@ -762,6 +762,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
 
     repositoryRemoteHelpersFactoryDelegate.init(
         new RepositoryRemoteHelpersFactoryImpl(
+            env.getDirectories(),
             actionContextProvider.getCombinedCache(),
             actionContextProvider.getRemoteExecutionClient(),
             buildRequestId,

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

@@ -190,24 +190,21 @@ private void addRunfiles(ProviderCollection buildTarget) {
     }
     var runfiles = runfilesSupport.getRunfiles();
     for (Artifact runfile : runfiles.getArtifacts().toList()) {
-      if (runfile.isSourceArtifact()) {
-        continue;
+      if (mayBeRemote(runfile)) {
+        addOutputToDownload(runfile);
       }
-      addOutputToDownload(runfile);
     }
     for (var symlink : runfiles.getSymlinks().toList()) {
       var artifact = symlink.getArtifact();
-      if (artifact.isSourceArtifact()) {
-        continue;
+      if (mayBeRemote(artifact)) {
+        addOutputToDownload(artifact);
       }
-      addOutputToDownload(artifact);
     }
     for (var symlink : runfiles.getRootSymlinks().toList()) {
       var artifact = symlink.getArtifact();
-      if (artifact.isSourceArtifact()) {
-        continue;
+      if (mayBeRemote(artifact)) {
+        addOutputToDownload(artifact);
       }
-      addOutputToDownload(artifact);
     }
   }
 
@@ -292,6 +289,19 @@ private boolean matchesPattern(PathFragment execPath) {
     return false;
   }
 
+  /**
+   * Returns whether this {@link ActionInput} could conceivably be only available remotely.
+   *
+   * <p>Use this as a quick check to avoid unnecessary extra work for artifacts that are definitely
+   * local.
+   */
+  public static boolean mayBeRemote(ActionInput actionInput) {
+    return !(actionInput instanceof Artifact artifact
+        && artifact.isSourceArtifact()
+        // Source artifacts in the main repo don't need to be fetched.
+        && (artifact.getOwner() == null || artifact.getOwner().getRepository().isMain()));
+  }
+
   /** Returns whether this {@link ActionInput} should be downloaded. */
   @Override
   public boolean shouldDownloadOutput(ActionInput output, RemoteFileArtifactValue metadata) {