[8.2.1] Fix atomic writes of VirtualActionInputs on Windows (#25857)

Moves to files that are concurrently open for reading fail on Windows,
which makes it necessary to ignore this particular mode of failure when
atomically writing a `VirtualActionInput`.

Fixes #25800

Closes #25805.

PiperOrigin-RevId: 746475729
Change-Id: I6830bc6cd8d5d32242b8823f0d5fc4587223c7b5 
(cherry picked from commit b44530d)

Closes #25851

Author: fmeum

src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java

@@ -17,13 +17,16 @@
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.util.StreamWriter;
+import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.vfs.FileAccessException;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.protobuf.ByteString;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.util.Arrays;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -49,9 +52,8 @@ public abstract class VirtualActionInput implements ActionInput, StreamWriter {
    * by the local and remote branches.
    *
    * <p>This implementation works by first creating a temporary file with a unique name and then
-   * renaming it into place, relying on the atomicity of {@link FileSystem#renameTo} (which is
-   * guaranteed for Unix filesystems, but possibly not for Windows). Subclasses may provide a more
-   * efficient implementation.
+   * renaming it into place, relying on the atomicity of {@link FileSystem#renameTo}. Subclasses may
+   * provide a more efficient implementation.
    *
    * @param execRoot the path that this input should be written inside, typically the execroot
    * @return digest of written virtual input
@@ -80,7 +82,15 @@ protected byte[] atomicallyWriteTo(Path outputPath) throws IOException {
     tmpPath.delete();
     try {
       byte[] digest = writeTo(tmpPath);
-      tmpPath.renameTo(outputPath);
+      try {
+        tmpPath.renameTo(outputPath);
+      } catch (FileAccessException e) {
+        // Moves fail on Windows if the target is accessed concurrently.
+        if (OS.getCurrent() == OS.WINDOWS && Arrays.equals(outputPath.getDigest(), digest)) {
+          return digest;
+        }
+        throw e;
+      }
       tmpPath = null; // Avoid unnecessary deletion attempt.
       return digest;
     } finally {

src/test/java/com/google/devtools/build/lib/actions/BUILD

@@ -67,11 +67,14 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils:depsutils",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
         "//src/main/java/com/google/devtools/build/lib/testing/common:directory_listing_helper",
+        "//src/main/java/com/google/devtools/build/lib/unix",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
+        "//src/main/java/com/google/devtools/build/lib/windows",
         "//src/main/java/com/google/devtools/build/lib/worker",
         "//src/main/java/com/google/devtools/build/lib/worker:worker_key",
         "//src/main/java/com/google/devtools/build/lib/worker:worker_process_status",

src/test/java/com/google/devtools/build/lib/actions/cache/VirtualActionInputTest.java

@@ -0,0 +1,105 @@
+// Copyright 2014 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.actions.cache;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.vfs.DigestHashFunction.SHA256;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.unix.UnixFileSystem;
+import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.JavaIoFileSystem;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.build.lib.windows.WindowsFileSystem;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+
+@RunWith(TestParameterInjector.class)
+public class VirtualActionInputTest {
+  @Rule public TemporaryFolder tempFolder = new TemporaryFolder();
+
+  public enum FileSystemType {
+    IN_MEMORY,
+    JAVA,
+    NATIVE;
+
+    FileSystem getFileSystem() {
+      return switch (this) {
+        case IN_MEMORY -> new InMemoryFileSystem(SHA256);
+        case JAVA -> new JavaIoFileSystem(SHA256);
+        case NATIVE ->
+            OS.getCurrent() == OS.WINDOWS
+                ? new WindowsFileSystem(SHA256, /* createSymbolicLinks= */ false)
+                : new UnixFileSystem(SHA256, "hash");
+      };
+    }
+  }
+
+  @Test
+  public void testAtomicallyWriteRelativeTo(@TestParameter FileSystemType fileSystemType)
+      throws Exception {
+    FileSystem fs = fileSystemType.getFileSystem();
+    Path execRoot = fs.getPath(tempFolder.getRoot().getPath());
+
+    Path outputFile = execRoot.getRelative("some/file");
+    VirtualActionInput input =
+        ActionsTestUtil.createVirtualActionInput(
+            outputFile.relativeTo(execRoot).getPathString(), "hello");
+
+    var digest = input.atomicallyWriteRelativeTo(execRoot);
+
+    assertThat(outputFile.getParentDirectory().readdir(Symlinks.NOFOLLOW))
+        .containsExactly(new Dirent("file", Dirent.Type.FILE));
+    assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("hello");
+    assertThat(outputFile.isExecutable()).isTrue();
+    assertThat(digest).isEqualTo(SHA256.getHashFunction().hashString("hello", UTF_8).asBytes());
+  }
+
+  @Test
+  public void testAtomicallyWriteRelativeTo_concurrentRead(
+      @TestParameter FileSystemType fileSystemType) throws Exception {
+    FileSystem fs = fileSystemType.getFileSystem();
+    Path execRoot = fs.getPath(tempFolder.getRoot().getPath());
+
+    Path outputFile = execRoot.getRelative("some/file");
+    VirtualActionInput input =
+        ActionsTestUtil.createVirtualActionInput(
+            outputFile.relativeTo(execRoot).getPathString(), "hello");
+
+    input.atomicallyWriteRelativeTo(execRoot);
+    byte[] digest;
+    byte[] bytes;
+    try (var in = outputFile.getInputStream()) {
+      digest = input.atomicallyWriteRelativeTo(execRoot);
+      bytes = in.readAllBytes();
+    }
+
+    assertThat(outputFile.getParentDirectory().readdir(Symlinks.NOFOLLOW))
+        .containsExactly(new Dirent("file", Dirent.Type.FILE));
+    assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("hello");
+    assertThat(outputFile.isExecutable()).isTrue();
+    assertThat(digest).isEqualTo(SHA256.getHashFunction().hashString("hello", UTF_8).asBytes());
+    assertThat(bytes).isEqualTo("hello".getBytes(UTF_8));
+  }
+}

src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java

@@ -216,19 +216,6 @@ public void atomicallyWriteVirtualInput_writesBinToolsFile() throws Exception {
     assertThat(outputFile.isExecutable()).isTrue();
   }
 
-  @Test
-  public void atomicallyWriteVirtualInput_writesArbitraryVirtualInput() throws Exception {
-    VirtualActionInput input = ActionsTestUtil.createVirtualActionInput("file", "hello");
-
-    input.atomicallyWriteRelativeTo(scratch.resolve("/outputs"));
-
-    assertThat(scratch.resolve("/outputs").readdir(Symlinks.NOFOLLOW))
-        .containsExactly(new Dirent("file", Dirent.Type.FILE));
-    Path outputFile = scratch.resolve("/outputs/file");
-    assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("hello");
-    assertThat(outputFile.isExecutable()).isTrue();
-  }
-
   @Test
   public void cleanExisting_updatesDirs() throws IOException, InterruptedException {
     Path inputTxt = scratch.getFileSystem().getPath(PathFragment.create("/hello.txt"));