Reapply "Fix disk cache failures on concurrent read-write access on Windows"

This reverts commit ac743a4.

Author: fmeum

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

@@ -23,6 +23,7 @@ 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,6 +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;
@@ -251,7 +252,7 @@ private void storeCacheValue(
       // 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 (!cacheValue.exists()) {
+      if (OS.getCurrent() != OS.WINDOWS || !cacheValue.exists()) {
         throw e;
       }
     }

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

@@ -24,6 +24,7 @@ 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,6 +35,8 @@
 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.Path;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.ExtensionRegistryLite;
@@ -138,7 +140,20 @@ public void captureFile(Path src, Digest digest, Store store) throws IOException
     }
 
     target.getParentDirectory().createDirectoryAndParents();
-    src.renameTo(target);
+    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();
+    }
   }
 
   private ListenableFuture<Void> download(Digest digest, OutputStream out, Store store) {
@@ -327,7 +342,20 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept
         }
       }
       path.getParentDirectory().createDirectoryAndParents();
-      temp.renameTo(path);
+      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();
+      }
     } catch (IOException e) {
       try {
         temp.delete();