Address comment

Author: fmeum

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

@@ -23,7 +23,6 @@ java_library(
     deps = [
         "//src/main/java/com/google/devtools/build/lib/server:idle_task",
         "//src/main/java/com/google/devtools/build/lib/util:file_system_lock",
-        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//third_party:guava",
         "//third_party:jsr305",

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

@@ -21,9 +21,7 @@
 import com.google.common.hash.HashFunction;
 import com.google.common.hash.Hasher;
 import com.google.common.io.BaseEncoding;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
-import com.google.devtools.build.lib.vfs.FileAccessException;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
@@ -243,19 +241,7 @@ private void storeCacheValue(
     Path tmpName = cacheEntry.getRelative(TMP_PREFIX + UUID.randomUUID());
     cacheEntry.createDirectoryAndParents();
     fileWriter.writeTo(tmpName);
-    try {
-      tmpName.renameTo(cacheValue);
-    } catch (FileAccessException e) {
-      // On Windows, atomically replacing a file that is currently opened (e.g. due to a concurrent
-      // get on the cache) results in renameTo throwing this exception, which wraps an
-      // AccessDeniedException. This case is benign since if the target path already exists, we know
-      // that another thread won the race to place the file in the cache. As the exception is rather
-      // generic and could result from other failure types, we rethrow the exception if the cache
-      // entry hasn't been created.
-      if (OS.getCurrent() != OS.WINDOWS || !cacheValue.exists()) {
-        throw e;
-      }
-    }
+    FileSystemUtils.renameToleratingConcurrentCreation(tmpName, cacheValue);
 
     if (!Strings.isNullOrEmpty(canonicalId)) {
       String idHash = keyType.newHasher().putBytes(canonicalId.getBytes(UTF_8)).hash().toString();

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

@@ -24,7 +24,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/util:digest_utils",
         "//src/main/java/com/google/devtools/build/lib/server:idle_task",
         "//src/main/java/com/google/devtools/build/lib/util:file_system_lock",
-        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//third_party:flogger",
         "//third_party:guava",

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

@@ -35,8 +35,7 @@
 import com.google.devtools.build.lib.remote.util.DigestOutputStream;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.Utils;
-import com.google.devtools.build.lib.util.OS;
-import com.google.devtools.build.lib.vfs.FileAccessException;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.ExtensionRegistryLite;
@@ -144,20 +143,7 @@ public void captureFile(Path src, Digest digest, Store store) throws IOException
     }
 
     target.getParentDirectory().createDirectoryAndParents();
-    try {
-      src.renameTo(target);
-    } catch (FileAccessException e) {
-      // On Windows, atomically replacing a file that is currently opened (e.g. due to a
-      // concurrent get on the cache) results in renameTo throwing this exception, which wraps an
-      // AccessDeniedException. This case is benign since if the target path already exists, we
-      // know that another thread won the race to place the file in the cache. As the exception is
-      // rather generic and could result from other failure types, we rethrow the exception if the
-      // cache entry hasn't been created.
-      if (OS.getCurrent() != OS.WINDOWS || !target.exists()) {
-        throw e;
-      }
-      src.delete();
-    }
+    FileSystemUtils.renameToleratingConcurrentCreation(src, target);
   }
 
   private ListenableFuture<Void> download(Digest digest, OutputStream out, Store store) {
@@ -348,20 +334,7 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept
         }
       }
       path.getParentDirectory().createDirectoryAndParents();
-      try {
-        temp.renameTo(path);
-      } catch (FileAccessException e) {
-        // On Windows, atomically replacing a file that is currently opened (e.g. due to a
-        // concurrent get on the cache) results in renameTo throwing this exception, which wraps an
-        // AccessDeniedException. This case is benign since if the target path already exists, we
-        // know that another thread won the race to place the file in the cache. As the exception is
-        // rather generic and could result from other failure types, we rethrow the exception if the
-        // cache entry hasn't been created.
-        if (OS.getCurrent() != OS.WINDOWS || !path.exists()) {
-          throw e;
-        }
-        temp.delete();
-      }
+      FileSystemUtils.renameToleratingConcurrentCreation(temp, path);
     } catch (IOException e) {
       try {
         temp.delete();

src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java

@@ -25,6 +25,7 @@
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ConditionallyThreadSafe;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.StringEncoding;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -489,6 +490,40 @@ public static MoveResult moveFile(Path from, Path to) throws IOException {
     }
   }
 
+  /**
+   * Atomically renames a source file to a target file, tolerating the case where another thread has
+   * concurrently created the target file (e.g. because it is known to have the same content in a
+   * CAS-like structure).
+   *
+   * <p>This handles a Windows-specific edge case: when the target file is being read by another
+   * process (e.g., during a concurrent cache lookup), the rename operation fails with a {@link
+   * FileAccessException}. If the target file already exists when this happens, it means another
+   * thread won the race to create it, so we can safely delete the source file.
+   *
+   * <p>The parent directories of the target file must already exist.
+   *
+   * @param source the file to rename
+   * @param target the destination path
+   */
+  @ThreadSafe
+  public static void renameToleratingConcurrentCreation(Path source, Path target)
+      throws IOException {
+    try {
+      source.renameTo(target);
+    } catch (FileAccessException e) {
+      // On Windows, atomically replacing a file that is currently opened (e.g. due to a concurrent
+      // get on the cache) results in renameTo throwing this exception, which wraps an
+      // AccessDeniedException. This case is benign since if the target path already exists, we know
+      // that another thread won the race to place the file in the cache. As the exception is rather
+      // generic and could result from other failure types, we rethrow the exception if the cache
+      // entry hasn't been created.
+      if (OS.getCurrent() != OS.WINDOWS || !target.exists()) {
+        throw e;
+      }
+      source.delete();
+    }
+  }
+
   /* Directory tree operations. */
 
   /**