Speculative fix for repo contents cache race

fmeum · copybara-github · commit ca1cbfece457 · 2025-09-10T06:46:47.000-07:00
The error message observed in #26713 is consistent with `FileChannel#open` failing because the given path doesn't exist. This could happen if one Bazel process observed that `!entryDir.isDirectory()`, which is true if the path doesn't exist, and proceeded to delete the path while another process had just created the directory and is now opening the channel. Since the entry dir is never expected to be an existing non-directory unless the cache has been corrupted, this logic can be removed. Another possible source of `IOException` during normal operation is on an interrupt (such as the user hitting Ctrl+C). Instead, follow Skyframe best practices by surfacing this as an `InterruptedException` instead of a `FileLockInterruptionException`. Also document that concurrent use on the same path is not supported (it results in an `OverlappingFileLockException` if the lock is already held, regardless of whether that is in shared or exclusive mode) and why the current usages are safe. Fixes #26713 Closes #26914. PiperOrigin-RevId: 805338728 Change-Id: Ie808ebe6113b935180b93c21679d5398aa168802
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
@@ -396,7 +396,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
             Profiler.instance()
                 .profile(ProfilerTask.REPO_CACHE_GC_WAIT, "waiting to acquire repo cache lock")) {
           repositoryCache.getRepoContentsCache().acquireSharedLock();
-        } catch (IOException e) {
+        } catch (IOException | InterruptedException e) {
           throw new AbruptExitException(
               detailedExitCode(
                   "could not acquire lock on repo contents cache", Code.BAD_REPO_CONTENTS_CACHE),
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepoContentsCache.java
@@ -157,13 +157,10 @@ private Path ensureTrashDir() throws IOException {
    */
   public Path moveToCache(
       Path fetchedRepoDir, Path fetchedRepoMarkerFile, String predeclaredInputHash)
-      throws IOException {
+      throws IOException, InterruptedException {
     Preconditions.checkState(path != null);
 
     Path entryDir = path.getRelative(predeclaredInputHash);
-    if (!entryDir.isDirectory()) {
-      entryDir.delete();
-    }
     String counter = getNextCounterInDir(entryDir);
     Path cacheRecordedInputsFile = entryDir.getChild(counter + RECORDED_INPUTS_SUFFIX);
     Path cacheRepoDir = entryDir.getChild(counter);
@@ -188,8 +185,12 @@ public Path moveToCache(
     return cacheRepoDir;
   }
 
-  private static String getNextCounterInDir(Path entryDir) throws IOException {
+  private static String getNextCounterInDir(Path entryDir)
+      throws IOException, InterruptedException {
     Path counterFile = entryDir.getRelative("counter");
+    // This use of FileSystemLock.get is safe since the predeclared input hash is part of entryDir's
+    // path and in particular includes the canonical repository name. This ensures that the same
+    // lock file will not be acquired concurrently by multiple threads, which isn't supported.
     try (var lock = FileSystemLock.get(entryDir.getRelative("lock"), LockMode.EXCLUSIVE)) {
       int c = 0;
       if (counterFile.exists()) {
@@ -205,7 +206,7 @@ private static String getNextCounterInDir(Path entryDir) throws IOException {
     }
   }
 
-  public void acquireSharedLock() throws IOException {
+  public void acquireSharedLock() throws IOException, InterruptedException {
     Preconditions.checkState(path != null);
     Preconditions.checkState(sharedLock == null, "this process already has the shared lock");
     sharedLock = FileSystemLock.get(path.getRelative(LOCK_PATH), LockMode.SHARED);
diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD
@@ -46,6 +46,7 @@ java_library(
     srcs = ["FileSystemLock.java"],
     deps = [
         ":string_encoding",
+        "//src/main/java/com/google/devtools/build/lib/concurrent:thread_safety",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//third_party:guava",
     ],
diff --git a/src/main/java/com/google/devtools/build/lib/util/FileSystemLock.java b/src/main/java/com/google/devtools/build/lib/util/FileSystemLock.java
@@ -13,12 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.util;
 
-
 import com.google.common.annotations.VisibleForTesting;
+import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
+import java.nio.channels.FileLockInterruptionException;
 import java.nio.file.StandardOpenOption;
 
 /**
@@ -71,9 +72,12 @@ private static FileChannel prepareChannel(Path path) throws IOException {
    * Tries to acquires a lock on the given path with the given mode. Throws an exception if the lock
    * is already held by another process.
    *
+   * <p>This method must not be called concurrently from multiple threads with the same path.
+   *
    * @throws LockAlreadyHeldException if the lock is already exclusively held by another process
    * @throws IOException if another error occurred
    */
+  @ThreadSafety.ThreadHostile
   public static FileSystemLock tryGet(Path path, LockMode mode) throws IOException {
     FileChannel channel = prepareChannel(path);
     FileLock lock = channel.tryLock(0, Long.MAX_VALUE, mode == LockMode.SHARED);
@@ -83,10 +87,23 @@ public static FileSystemLock tryGet(Path path, LockMode mode) throws IOException
     return new FileSystemLock(channel, lock);
   }
 
-  /** Acquires a lock on the given path with the given mode. Blocks until the lock is acquired. */
-  public static FileSystemLock get(Path path, LockMode mode) throws IOException {
+  /**
+   * Acquires a lock on the given path with the given mode. Blocks until the lock is acquired.
+   *
+   * <p>This method must not be called concurrently from multiple threads with the same path.
+   */
+  @ThreadSafety.ThreadHostile
+  public static FileSystemLock get(Path path, LockMode mode)
+      throws IOException, InterruptedException {
     FileChannel channel = prepareChannel(path);
-    FileLock lock = channel.lock(0, Long.MAX_VALUE, mode == LockMode.SHARED);
+    FileLock lock;
+    try {
+      lock = channel.lock(0, Long.MAX_VALUE, mode == LockMode.SHARED);
+    } catch (FileLockInterruptionException e) {
+      Thread.interrupted(); // clear interrupt bit
+      channel.close();
+      throw new InterruptedException();
+    }
     return new FileSystemLock(channel, lock);
   }
 
