Add a random number to action temp dir

Fix for #4035

@laszlocsomor

Closes #4110.

PiperOrigin-RevId: 176346381

3 files changed, 27 insertions, 11 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index c6892e6479..e33c010263 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -47,6 +47,7 @@ import java.util.ArrayList;
 import java.util.EnumMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ThreadLocalRandom;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
@@ -130,15 +131,15 @@ public final class LocalSpawnRunner implements SpawnRunner {
   }
 
   private static Path createActionTemp(Path execRoot) throws IOException {
-    // Use this executor thread's ID as the directory name's suffix.
-    // The ID is safe for the following reasons:
-    // - being a thread ID, it's guaranteed to be unique among other action executor threads
-    // - this thread will only execute one action at a time, so there's no risk of concurrently
-    //   running actions using the same temp directory.
-    // The next action that this thread executes can reuse the temp directory name. The only caveat
-    // is, if {@link #start} fails to delete directory after the action is done, the next
-    // action will see stale files in its temp directory.
-    String idStr = Long.toHexString(Thread.currentThread().getId());
+    String idStr =
+        // Make the name unique among other executor threads.
+        Long.toHexString(Thread.currentThread().getId())
+            + "_"
+            // Make the name unique among other temp directories that this thread has ever created.
+            // On Windows, file and directory deletion is asynchronous, meaning the previous temp
+            // directory name isn't immediately available for the next action that this thread runs.
+            // See https://github.com/bazelbuild/bazel/issues/4035
+            + Long.toHexString(ThreadLocalRandom.current().nextLong());
     Path result = execRoot.getRelative("tmp" + idStr);
     if (!result.exists() && !result.createDirectory()) {
       throw new IOException(String.format("Could not create temp directory '%s'", result));
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 260debfbc1..c33c33cc2f 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -45,6 +46,7 @@ import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.common.options.Options;
@@ -68,6 +70,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatcher;
 
 /**
  * Unit tests for {@link LocalSpawnRunner}.
@@ -514,7 +517,19 @@ public class LocalSpawnRunnerTest {
         .rewriteLocalEnv(
             any(),
             eq(fs.getPath("/execroot")),
-            eq(fs.getPath("/execroot/tmp1")),
+            argThat(
+                new ArgumentMatcher<Path>() {
+                  @Override
+                  public boolean matches(Object arg) {
+                    if (!(arg instanceof Path)) {
+                      return false;
+                    }
+                    return ((Path) arg)
+                        .getPathString()
+                        .matches("^/execroot/tmp[0-9a-fA-F]+_[0-9a-fA-F]+$");
+                  }
+                }
+            ),
             eq("product-name"));
   }
 
diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh
index da50cc12cf..5e70ebb689 100755
--- a/src/test/shell/bazel/bazel_rules_test.sh
+++ b/src/test/shell/bazel/bazel_rules_test.sh
@@ -251,7 +251,7 @@ EOF
     || fail "Failed to build //pkg:test"
   assert_contains "PATH=$PATH_TO_BAZEL_WRAPPER:/bin:/usr/bin:/random/path" \
     bazel-genfiles/pkg/test.out
-  assert_contains "TMPDIR=.*execroot.*tmp[0-9a-fA-F]\+$" \
+  assert_contains "TMPDIR=.*execroot.*tmp[0-9a-fA-F]\+_[0-9a-fA-F]\+$" \
     bazel-genfiles/pkg/test.out
   assert_not_contains "TMPDIR=.*newfancytmpdir" \
     bazel-genfiles/pkg/test.out