Fix cycles when checking the local repo contents cache

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

@@ -90,7 +90,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

@@ -383,12 +383,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/BUILD

@@ -42,10 +42,10 @@ java_library(
 java_library(
     name = "repository_fetch_function",
     srcs = [
-        "DigestWriter.java",
         "RepositoryFetchFunction.java",
     ],
     deps = [
+        ":digest_writer",
         ":exception",
         ":repo_definition",
         ":repo_definition_value",
@@ -83,6 +83,32 @@ java_library(
     ],
 )
 
+java_library(
+    name = "digest_writer",
+    srcs = [
+        "DigestWriter.java",
+    ],
+    deps = [
+        ":exception",
+        ":repo_definition",
+        ":utils",
+        "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
+        "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:serialization",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
+        "//src/main/java/com/google/devtools/build/lib/repository:repository_events",
+        "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:repo_environment_function",
+        "//src/main/java/com/google/devtools/build/lib/util",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//src/main/java/net/starlark/java/eval",
+        "//third_party:guava",
+        "//third_party:jsr305",
+    ],
+)
+
 java_library(
     name = "repo_definition",
     srcs = [
@@ -144,6 +170,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//src/main/java/net/starlark/java/eval",
+        "//third_party:guava",
         "//third_party:jsr305",
     ],
 )

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

@@ -18,6 +18,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
+import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.bazel.bzlmod.GsonTypeAdapterUtil;
@@ -38,7 +39,7 @@
 import net.starlark.java.eval.StarlarkSemantics;
 
 /** Handles writing and reading of repo marker files. */
-class DigestWriter {
+public class DigestWriter {
 
   // The marker file version is inject in the rule key digest so the rule key is always different
   // when we decide to update the format.
@@ -91,53 +92,53 @@ void writeMarkerFile(List<RepoRecordedInput.WithValue> recordedInputValues)
     }
   }
 
-  sealed interface RepoDirectoryState {
-    record UpToDate() implements RepoDirectoryState {}
-
-    record OutOfDate(String reason) implements RepoDirectoryState {}
-  }
-
-  RepoDirectoryState areRepositoryAndMarkerFileConsistent(Environment env)
+  Optional<String> areRepositoryAndMarkerFileConsistent(Environment env)
       throws InterruptedException, RepositoryFunctionException {
     return areRepositoryAndMarkerFileConsistent(env, markerPath);
   }
 
   /**
-   * Checks if the state of the repository in the file system is consistent with the rule in the
-   * WORKSPACE file.
-   *
-   * <p>Returns null if a Skyframe status is needed.
+   * Checks if the state of the repo in the filesystem is consistent with its current definition.
+   * Returns {@link Optional#empty()} if they are consistent; otherwise, returns a description of
+   * why they are not.
    *
-   * <p>We check the repository root for existence here, but we can't depend on the FileValue,
-   * because it's possible that we eventually create that directory in which case the FileValue and
-   * the state of the file system would be inconsistent.
+   * <p>This method treats a missing Skyframe dependency as if the repo is not up to date. The
+   * caller is responsible for checking {@code env.valuesMissing()}.
    */
-  @Nullable
-  RepoDirectoryState areRepositoryAndMarkerFileConsistent(Environment env, Path markerPath)
+  Optional<String> areRepositoryAndMarkerFileConsistent(Environment env, Path markerPath)
       throws RepositoryFunctionException, InterruptedException {
     if (!markerPath.exists()) {
-      return new RepoDirectoryState.OutOfDate("repo hasn't been fetched yet");
+      return Optional.of("repo hasn't been fetched yet");
     }
 
     try {
       String content = FileSystemUtils.readContent(markerPath, ISO_8859_1);
-      var recordedInputValues =
+      Optional<ImmutableList<RepoRecordedInput.WithValue>> recordedInputValues =
           readMarkerFile(content, Preconditions.checkNotNull(predeclaredInputHash));
-      Optional<String> outdatedReason =
-          RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputValues);
-      if (env.valuesMissing()) {
-        return null;
+      if (recordedInputValues.isEmpty()) {
+        return Optional.of("Bazel version, flags, repo rule definition or attributes changed");
       }
-      if (outdatedReason.isPresent()) {
-        return new RepoDirectoryState.OutOfDate(outdatedReason.get());
+      // Check inputs in batches to prevent Skyframe cycles caused by outdated dependencies.
+      for (ImmutableList<RepoRecordedInput.WithValue> batch :
+          RepoRecordedInput.WithValue.splitIntoBatches(recordedInputValues.get())) {
+        Optional<String> outdatedReason =
+            RepoRecordedInput.isAnyValueOutdated(env, directories, batch);
+        if (outdatedReason.isPresent()) {
+          return outdatedReason;
+        }
       }
-      return new RepoDirectoryState.UpToDate();
+      return Optional.empty();
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }
   }
 
-  private static ImmutableList<RepoRecordedInput.WithValue> readMarkerFile(
+  /**
+   * Returns a list of recorded inputs with their values parsed from the given marker file if the
+   * predeclared input hash matches, or {@code Optional.empty()} if the hash doesn't match or any
+   * error occurs during parsing.
+   */
+  public static Optional<ImmutableList<RepoRecordedInput.WithValue>> readMarkerFile(
       String content, String predeclaredInputHash) {
     Iterable<String> lines = Splitter.on('\n').split(content);
 
@@ -151,26 +152,22 @@ private static ImmutableList<RepoRecordedInput.WithValue> readMarkerFile(
         if (!line.equals(predeclaredInputHash)) {
           // Break early, need to reload anyway. This also detects marker file version changes
           // so that unknown formats are not parsed.
-          return ImmutableList.of(
-              new RepoRecordedInput.WithValue(
-                  new NeverUpToDateRepoRecordedInput(
-                      "Bazel version, flags, repo rule definition or attributes changed"),
-                  ""));
+          return Optional.empty();
         }
         firstLineVerified = true;
       } else {
         var inputAndValue = RepoRecordedInput.WithValue.parse(line);
         if (inputAndValue.isEmpty()) {
           // On parse failure, just forget everything else and mark the whole input out of date.
-          return PARSE_FAILURE;
+          return Optional.empty();
         }
         recordedInputValues.add(inputAndValue.get());
       }
     }
     if (!firstLineVerified) {
-      return PARSE_FAILURE;
+      return Optional.empty();
     }
-    return recordedInputValues.build();
+    return Optional.of(recordedInputValues.build());
   }
 
   @Nullable