Reduce flakiness on Windows for BwoB tests

coeuvre · copybara-github · commit fbec8e2ad1dc · 2023-02-14T09:18:56.000-08:00
`BuildWithoutTheBytesIntegrationTest` has been flaky on Windows. Example error leads to file system errors. ``` 1) incrementalBuild_treeArtifacts_correctlyProducesNewTree(com.google.devtools.build.lib.remote.BuildWithoutTheBytesIntegrationTest) java.io.IOException: C:/b/iahsqj7l/execroot/io_bazel/_tmp/b474bcb9956744b234827924ec01952d/remote.pid_file (Permission denied) at com.google.devtools.build.lib.windows.WindowsFileSystem.delete(WindowsFileSystem.java:60) at com.google.devtools.build.lib.vfs.Path.delete(Path.java:587) at com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase.bestEffortDeleteTreesBelow(BuildIntegrationTestCase.java:387) at com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase.cleanUp(BuildIntegrationTestCase.java:364) ... 26 trimmed Caused by: java.nio.file.AccessDeniedException: C:/b/iahsqj7l/execroot/io_bazel/_tmp/b474bcb9956744b234827924ec01952d/remote.pid_file at com.google.devtools.build.lib.windows.WindowsFileOperations.deletePath(WindowsFileOperations.java:279) at com.google.devtools.build.lib.windows.WindowsFileSystem.delete(WindowsFileSystem.java:56) ... 30 more 2) downloadToplevel_treeArtifacts(com.google.devtools.build.lib.remote.BuildWithoutTheBytesIntegrationTest) java.io.IOException: C:/b/iahsqj7l/execroot/io_bazel/_tmp/b474bcb9956744b234827924ec01952d/remote.work_path already exists at com.google.devtools.build.lib.remote.util.IntegrationTestUtils.ensureMkdir(IntegrationTestUtils.java:116) at com.google.devtools.build.lib.remote.util.IntegrationTestUtils.startWorker(IntegrationTestUtils.java:87) at com.google.devtools.build.lib.remote.util.IntegrationTestUtils.startWorker(IntegrationTestUtils.java:77) at com.google.devtools.build.lib.remote.BuildWithoutTheBytesIntegrationTest.setupOptions(BuildWithoutTheBytesIntegrationTest.java:49) at com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase.createRuntimeWrapper(BuildIntegrationTestCase.java:317) at com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase.createFilesAndMocks(BuildIntegrationTestCase.java:234) ``` This PR fixes 1) by not using pid but checking port status directly to wait for the remote worker to start up and 2) by cleaning up resources used by remote worker after each test case. Closes bazelbuild#17479. PiperOrigin-RevId: 509549826 Change-Id: Icef2be46d5f2a6025cad0cefcf766ae16b4552d8
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -149,6 +149,7 @@ java_test(
     size = "large",
     srcs = ["BuildWithoutTheBytesIntegrationTest.java"],
     shard_count = 5,
+    tags = ["requires-network"],
     runtime_deps = [
         "//third_party/grpc-java:grpc-jar",
     ],
@@ -173,6 +174,7 @@ java_test(
 java_test(
     name = "DiskCacheIntegrationTest",
     srcs = ["DiskCacheIntegrationTest.java"],
+    tags = ["requires-network"],
     runtime_deps = [
         "//third_party/grpc-java:grpc-jar",
     ],
diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java b/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java
@@ -25,8 +25,14 @@
 import java.io.File;
 import java.io.IOException;
 import java.net.DatagramSocket;
+import java.net.InetSocketAddress;
 import java.net.ServerSocket;
 import java.net.SocketException;
+import java.nio.channels.SocketChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Comparator;
 import java.util.Random;
 
 /** Integration test utilities. */
@@ -73,6 +79,30 @@ public static int pickUnusedRandomPort() throws IOException, InterruptedExceptio
     throw new IOException("Failed to find available port");
   }
 
+  private static void waitForPortOpen(Subprocess process, int port)
+      throws IOException, InterruptedException {
+    var addr = new InetSocketAddress("localhost", port);
+    var timeout = new IOException("Timed out when waiting for port to open");
+    for (var i = 0; i < 20; ++i) {
+      if (!process.isAlive()) {
+        var message = new String(process.getErrorStream().readAllBytes(), UTF_8);
+        throw new IOException("Failed to start worker: " + message);
+      }
+
+      try {
+        try (var socketChannel = SocketChannel.open()) {
+          socketChannel.configureBlocking(/* block= */ true);
+          socketChannel.connect(addr);
+        }
+        return;
+      } catch (IOException e) {
+        timeout.addSuppressed(e);
+        Thread.sleep(1000);
+      }
+    }
+    throw timeout;
+  }
+
   public static WorkerInstance startWorker() throws IOException, InterruptedException {
     return startWorker(/* useHttp= */ false);
   }
@@ -82,7 +112,6 @@ public static WorkerInstance startWorker(boolean useHttp)
     PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
     PathFragment workPath = testTmpDir.getRelative("remote.work_path");
     PathFragment casPath = testTmpDir.getRelative("remote.cas_path");
-    PathFragment pidPath = testTmpDir.getRelative("remote.pid_file");
     int workerPort = pickUnusedRandomPort();
     ensureMkdir(workPath);
     ensureMkdir(casPath);
@@ -94,20 +123,10 @@ public static WorkerInstance startWorker(boolean useHttp)
                     workerPath,
                     "--work_path=" + workPath.getSafePathString(),
                     "--cas_path=" + casPath.getSafePathString(),
-                    (useHttp ? "--http_listen_port=" : "--listen_port=") + workerPort,
-                    "--pid_file=" + pidPath))
+                    (useHttp ? "--http_listen_port=" : "--listen_port=") + workerPort))
             .start();
-
-    File pidFile = new File(pidPath.getSafePathString());
-    while (!pidFile.exists()) {
-      if (!workerProcess.isAlive()) {
-        String message = new String(workerProcess.getErrorStream().readAllBytes(), UTF_8);
-        throw new IOException("Failed to start worker: " + message);
-      }
-      Thread.sleep(1);
-    }
-
-    return new WorkerInstance(workerProcess, workerPort, workPath, casPath, pidPath);
+    waitForPortOpen(workerProcess, workerPort);
+    return new WorkerInstance(workerProcess, workerPort, workPath, casPath);
   }
 
   private static void ensureMkdir(PathFragment path) throws IOException {
@@ -125,23 +144,25 @@ public static class WorkerInstance {
     private final int port;
     private final PathFragment workPath;
     private final PathFragment casPath;
-    private final PathFragment pidPath;
 
     private WorkerInstance(
-        Subprocess process,
-        int port,
-        PathFragment workPath,
-        PathFragment casPath,
-        PathFragment pidPath) {
+        Subprocess process, int port, PathFragment workPath, PathFragment casPath) {
       this.process = process;
       this.port = port;
       this.workPath = workPath;
       this.casPath = casPath;
-      this.pidPath = pidPath;
     }
 
-    public void stop() {
+    public void stop() throws IOException {
       process.destroyAndWait();
+      deleteDir(workPath);
+      deleteDir(casPath);
+    }
+
+    private static void deleteDir(PathFragment path) throws IOException {
+      try (var stream = Files.walk(Paths.get(path.getSafePathString()))) {
+        stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+      }
     }
 
     public int getPort() {
@@ -155,9 +176,5 @@ public PathFragment getWorkPath() {
     public PathFragment getCasPath() {
       return casPath;
     }
-
-    public PathFragment getPidPath() {
-      return pidPath;
-    }
   }
 }
