[8.3.1] Fix hang with force fetching + repo contents cache (#26412)

With the repo contents cache enabled, `bazel fetch --force` (and the
now-removed `bazel sync`) would cause Bazel to hang forever. The reason
is that writing into the repo contents cache causes a Skyframe restart
very late into `RepositoryDelegatorFunction`, and the force-fetch bit
causes the restarted function to consider it a cache miss, writing into
the repo contents cache again, causing an infinite loop.

This PR changes it so that a restarted `RepositoryDelegatorFunction`
will remember whether it had _just_ finished a fetch before; if so, it
will use the cache, even if force-fetch is on.

Fixes #26389.

Closes #26409.

PiperOrigin-RevId: 776306095
Change-Id: I52ad7aeb6581a16268d26a8b142041dc0e748768

Commit
bf67251

---------

Co-authored-by: Xdng Yng <[email protected]>

Author: bazel-io

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

@@ -121,10 +121,16 @@ public ImmutableList<CandidateRepo> getCandidateRepos(String predeclaredInputHas
     try {
       return entryDir.getDirectoryEntries().stream()
           .filter(path -> path.getBaseName().endsWith(RECORDED_INPUTS_SUFFIX))
-          // We're just sorting for consistency here. (Note that "10.recorded_inputs" would sort
-          // before "1.recorded_inputs".) If necessary, we could use some sort of heuristics here
-          // to speed up the subsequent up-to-date-ness checking.
-          .sorted(Comparator.comparing(Path::getBaseName))
+          // Prefer newer cache entries over older ones. They're more likely to be up-to-date; plus,
+          // if a repo is force-fetched, we want to use the new repo instead of always being stuck
+          // with the old one.
+          // To "prefer newer cache entries", we sort the entry file names by length DESC and then
+          // lexicographically DESC. This approximates sorting by converting to int and then DESC,
+          // but is defensive against non-numerically named entries.
+          .sorted(
+              Comparator.comparing((Path path) -> path.getBaseName().length())
+                  .thenComparing(Path::getBaseName)
+                  .reversed())
           .map(CandidateRepo::fromRecordedInputsFile)
           .collect(toImmutableList());
     } catch (IOException e) {

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

@@ -127,6 +127,11 @@ private static class State extends WorkerSkyKeyComputeState<FetchResult> {
     @Nullable FetchResult result;
   }
 
+  @Override
+  public boolean wasJustFetched(Environment env) {
+    return env.getState(State::new).result != null;
+  }
+
   private record FetchArgs(
       Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) {
     FetchArgs toWorkerArgs(Environment env) {
@@ -139,19 +144,21 @@ FetchArgs toWorkerArgs(Environment env) {
   public FetchResult fetch(
       Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key)
       throws RepositoryFunctionException, InterruptedException {
+    var state = env.getState(State::new);
+    if (state.result != null) {
+      // Escape early if we've already finished fetching once. This can happen if
+      // RepositoryDelegatorFunction triggers a Skyframe restart _after_
+      // StarlarkRepositoryFunction#fetch is finished.
+      return state.result;
+    }
     var args = new FetchArgs(rule, outputDirectory, directories, env, key);
     if (!useWorkers) {
-      return fetchInternal(args);
+      state.result = fetchInternal(args);
+      return state.result;
     }
     // See below (the `catch CancellationException` clause) for why there's a `while` loop here.
     while (true) {
-      var state = env.getState(State::new);
-      if (state.result != null) {
-        // Escape early if we've already finished fetching once. This can happen if
-        // RepositoryDelegatorFunction triggers a Skyframe restart _after_
-        // StarlarkRepositoryFunction#fetch is finished.
-        return state.result;
-      }
+      state = env.getState(State::new);
       try {
         state.result =
             state.startOrContinueWork(

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java

@@ -450,6 +450,14 @@ private RepositoryFunction getHandler(Rule rule) {
   /* Determines whether we should use the cached repositories */
   private boolean shouldUseCachedRepos(Environment env, RepositoryFunction handler, Rule rule)
       throws InterruptedException {
+    if (handler.wasJustFetched(env)) {
+      // If this SkyFunction has finished fetching once, then we should always use the cached
+      // result. This means that we _very_ recently (as in, in the same command invocation) fetched
+      // this repo (possibly with --force or --configure), and are only here again due to a Skyframe
+      // restart very late into RepositoryDelegatorFunction.
+      return true;
+    }
+
     boolean forceFetchEnabled = !FORCE_FETCH.get(env).isEmpty();
     boolean forceFetchConfigureEnabled =
         handler.isConfigure(rule) && !FORCE_FETCH_CONFIGURE.get(env).isEmpty();

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java

@@ -218,6 +218,15 @@ protected static void ensureNativeRepoRuleEnabled(Rule rule, Environment env, St
         Transience.PERSISTENT);
   }
 
+  /**
+   * We sometimes get to the point before fetching a repo when we have <em>just</em> fetched it,
+   * particularly because of Skyframe restarts very late into RepositoryDelegatorFunction. In that
+   * case, this method can be used to report that fact.
+   */
+  public boolean wasJustFetched(Environment env) {
+    return false;
+  }
+
   /**
    * Verify the data provided by the marker file to check if a refetch is needed. Returns an empty
    * Optional if the data is up to date and no refetch is needed and an Optional with a

src/test/py/bazel/bzlmod/bazel_fetch_test.py

@@ -16,6 +16,7 @@
 
 import os
 import tempfile
+
 from absl.testing import absltest
 from src.test.py.bazel import test_base
 from src.test.py.bazel.bzlmod.test_utils import BazelRegistry
@@ -305,6 +306,61 @@ def testForceFetch(self):
     _, _, stderr = self.RunBazel(['fetch', '--repo=@hello', '--force'])
     self.assertIn('No more Orange Juice!', ''.join(stderr))
 
+  def testForceFetchWithRepoCache(self):
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            'ext = use_extension("extension.bzl", "ext")',
+            'use_repo(ext, "hello")',
+        ],
+    )
+    self.ScratchFile('BUILD')
+    self.ScratchFile('name.txt', ['foo'])
+    file_path = self.Path('name.txt').replace('\\', '\\\\')
+    self.ScratchFile(
+        'extension.bzl',
+        [
+            'def impl(ctx):',
+            '    file_content = ctx.read("' + file_path + '", watch="no")',
+            '    print("name is " + file_content)',
+            '    ctx.file("BUILD",',
+            '             "filegroup(name=\'" + file_content.strip() + "\')")',
+            '    return ctx.repo_metadata(reproducible=True)',
+            'repo_rule = repository_rule(implementation=impl)',
+            '',
+            'def _ext_impl(ctx):',
+            '    repo_rule(name="hello")',
+            'ext = module_extension(implementation=_ext_impl)',
+        ],
+    )
+
+    _, _, stderr = self.RunBazel(['fetch', '--repo=@hello'])
+    self.assertIn('name is foo', ''.join(stderr))
+    self.RunBazel(['build', '@hello//:foo'])
+
+    # Change file content and run WITHOUT force, assert no fetching!
+    self.ScratchFile('name.txt', ['bar'])
+    _, _, stderr = self.RunBazel(['fetch', '--repo=@hello'])
+    self.assertNotIn('name is bar', ''.join(stderr))
+    self.RunBazel(['build', '@hello//:foo'])
+
+    # Run again WITH --force and assert fetching
+    _, _, stderr = self.RunBazel(['fetch', '--repo=@hello', '--force'])
+    self.assertIn('name is bar', ''.join(stderr))
+    self.RunBazel(['build', '@hello//:bar'])
+
+    # Clean expunge. Assert the cache entry with "bar" is selected (despite
+    # "foo" also still existing in the cache).
+    self.RunBazel(['clean', '--expunge'])
+    self.ScratchFile('name.txt', ['quux'])
+    _, _, stderr = self.RunBazel(['build', '@hello//:bar'])
+    self.assertNotIn('name is ', ''.join(stderr))
+
+  def testForceFetchWithRepoCacheNoRepoWorkers(self):
+    with open(self.Path('.bazelrc'), 'a') as f:
+      f.write('common --experimental_worker_for_repo_fetching=off\n')
+    self.testForceFetchWithRepoCache()
+
   def testFetchTarget(self):
     self.main_registry.createCcModule('aaa', '1.0').createCcModule(
         'bbb', '1.0', {'aaa': '1.0'}