Acquire an exclusive lock on the disk cache while running a garbage c…

…ollection.

PiperOrigin-RevId: 679173101
Change-Id: I3a095a420e7933c4f4073c2f8d19e3b9b13fda5c

src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java

@@ -154,6 +154,14 @@ public CollectionPolicy getPolicy() {
    * @throws InterruptedException if the thread was interrupted
    */
   public CollectionStats run() throws IOException, InterruptedException {
+    // Acquire an exclusive lock to prevent two Bazel processes from simultaneously running
+    // garbage collection, which can waste resources and lead to incorrect results.
+    try (var lock = DiskCacheLock.getExclusive(root.getRelative("gc/lock"))) {
+      return runUnderLock();
+    }
+  }
+
+  private CollectionStats runUnderLock() throws IOException, InterruptedException {
     EntryScanner scanner = new EntryScanner();
     EntryDeleter deleter = new EntryDeleter();
 

src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java

@@ -15,6 +15,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static org.junit.Assert.assertThrows;
 
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.remote.disk.DiskCacheGarbageCollector.CollectionStats;
@@ -174,6 +175,17 @@ public void ignoresTmpAndGcSubdirectories() throws Exception {
     assertFilesExist("gc/foo", "tmp/foo");
   }
 
+  @Test
+  public void failsWhenLockIsAlreadyHeld() throws Exception {
+    try (var externalLock = ExternalLock.getShared(rootDir.getRelative("gc/lock"))) {
+      Exception e =
+          assertThrows(
+              Exception.class, () -> runGarbageCollector(Optional.of(1L), Optional.empty()));
+      assertThat(e).isInstanceOf(IOException.class);
+      assertThat(e).hasMessageThat().contains("failed to acquire exclusive disk cache lock");
+    }
+  }
+
   private void assertFilesExist(String... relativePaths) throws IOException {
     for (String relativePath : relativePaths) {
       Path path = rootDir.getRelative(relativePath);
@@ -193,7 +205,8 @@ private void assertFilesDoNotExist(String... relativePaths) throws IOException {
   }
 
   private CollectionStats runGarbageCollector(
-      Optional<Long> maxSizeBytes, Optional<Duration> maxAge) throws Exception {
+      Optional<Long> maxSizeBytes, Optional<Duration> maxAge)
+      throws IOException, InterruptedException {
     var gc =
         new DiskCacheGarbageCollector(
             rootDir,

src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheLockTest.java

@@ -16,13 +16,8 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.shell.Subprocess;
-import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.testutil.TestUtils;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.runfiles.Runfiles;
 import java.io.IOException;
 import org.junit.Before;
 import org.junit.Test;
@@ -86,47 +81,4 @@ public void getExclusive_whenLockedForExclusiveUse_fails() throws Exception {
       assertThat(e).hasMessageThat().contains("failed to acquire exclusive disk cache lock");
     }
   }
-
-  /**
-   * Runs an external process that holds a shared or exclusive lock on a file.
-   *
-   * <p>This is needed because the JVM does not allow overlapping locks.
-   */
-  private static class ExternalLock implements AutoCloseable {
-    private static final String HELPER_PATH =
-        "io_bazel/src/test/java/com/google/devtools/build/lib/remote/disk/external_lock_helper"
-            + (OS.getCurrent() == OS.WINDOWS ? ".exe" : "");
-
-    private final Subprocess subprocess;
-
-    static ExternalLock getShared(Path lockPath) throws IOException {
-      return new ExternalLock(lockPath, true);
-    }
-
-    static ExternalLock getExclusive(Path lockPath) throws IOException {
-      return new ExternalLock(lockPath, false);
-    }
-
-    ExternalLock(Path lockPath, boolean shared) throws IOException {
-      String binaryPath = Runfiles.preload().withSourceRepository("").rlocation(HELPER_PATH);
-      this.subprocess =
-          new SubprocessBuilder()
-              .setArgv(
-                  ImmutableList.of(
-                      binaryPath, lockPath.getPathString(), shared ? "shared" : "exclusive"))
-              .start();
-      // Wait for child to report that the lock has been acquired.
-      // We could read the entire stdout/stderr here to obtain additional debugging information,
-      // but for some reason that hangs forever on Windows, even if we close them on the child side.
-      if (subprocess.getInputStream().read() != '!') {
-        throw new IOException("external helper process failed");
-      }
-    }
-
-    @Override
-    public void close() throws IOException {
-      // Wait for process to exit and release the lock.
-      subprocess.destroyAndWait();
-    }
-  }
 }

src/test/java/com/google/devtools/build/lib/remote/disk/ExternalLock.java

@@ -0,0 +1,65 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.remote.disk;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.shell.Subprocess;
+import com.google.devtools.build.lib.shell.SubprocessBuilder;
+import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.runfiles.Runfiles;
+import java.io.IOException;
+
+/**
+ * Runs an external process that holds a shared or exclusive lock on a file.
+ *
+ * <p>This is needed for testing because the JVM does not allow overlapping locks.
+ */
+public class ExternalLock implements AutoCloseable {
+  private static final String HELPER_PATH =
+      "io_bazel/src/test/java/com/google/devtools/build/lib/remote/disk/external_lock_helper"
+          + (OS.getCurrent() == OS.WINDOWS ? ".exe" : "");
+
+    private final Subprocess subprocess;
+
+    static ExternalLock getShared(Path lockPath) throws IOException {
+      return new ExternalLock(lockPath, true);
+    }
+
+    static ExternalLock getExclusive(Path lockPath) throws IOException {
+      return new ExternalLock(lockPath, false);
+    }
+
+    ExternalLock(Path lockPath, boolean shared) throws IOException {
+      String binaryPath = Runfiles.preload().withSourceRepository("").rlocation(HELPER_PATH);
+      this.subprocess =
+          new SubprocessBuilder()
+              .setArgv(
+                  ImmutableList.of(
+                      binaryPath, lockPath.getPathString(), shared ? "shared" : "exclusive"))
+              .start();
+      // Wait for child to report that the lock has been acquired.
+      // We could read the entire stdout/stderr here to obtain additional debugging information,
+      // but for some reason that hangs forever on Windows, even if we close them on the child side.
+      if (subprocess.getInputStream().read() != '!') {
+        throw new IOException("external helper process failed");
+      }
+    }
+
+    @Override
+    public void close() throws IOException {
+    // Wait for process to exit and release the lock.
+    subprocess.destroyAndWait();
+  }
+}