Clean up sandboxed workers' directories asynchronously

The setup code now moves the unneeded directory in the workdir out of the way
then deletes it asynchronously. This is faster than waiting for the whole tree
deletion to complete before a worker can be reused.

This cuts down the time spent on workerExecRoot.createFileSystem when building //devtools/buildrank/frontend from an average of 5 seconds to an average of 3 seconds.

RELNOTES:none
PiperOrigin-RevId: 601043337
Change-Id: I47252060fe1a1ea9c9bcfd34fd2987d030f333de

src/main/java/com/google/devtools/build/lib/sandbox/AsynchronousTreeDeleter.java

@@ -37,7 +37,7 @@
  * precious resources that could otherwise be used for the build itself. But when the build is
  * finished, this number should be raised to quickly go through any pending deletions.
  */
-class AsynchronousTreeDeleter implements TreeDeleter {
+public class AsynchronousTreeDeleter implements TreeDeleter {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   private final AtomicInteger trashCount = new AtomicInteger(0);
@@ -48,7 +48,7 @@ class AsynchronousTreeDeleter implements TreeDeleter {
   private final Path trashBase;
 
   /** Constructs a new asynchronous tree deleter backed by just one thread. */
-  AsynchronousTreeDeleter(Path trashBase) {
+  public AsynchronousTreeDeleter(Path trashBase) {
     logger.atInfo().log("Starting async tree deletion pool with 1 thread");
 
     ThreadFactory threadFactory =
@@ -82,6 +82,9 @@ void setThreads(int threads) {
 
   @Override
   public void deleteTree(Path path) throws IOException {
+    if (!trashBase.exists()) {
+      trashBase.createDirectory();
+    }
     Path trashPath = trashBase.getRelative(Integer.toString(trashCount.getAndIncrement()));
     path.renameTo(trashPath);
     checkNotNull(service, "Cannot call deleteTree after shutdown")

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -148,6 +148,16 @@ public static void cleanExisting(
       Path workDir,
       @Nullable TreeDeleter treeDeleter)
       throws IOException, InterruptedException {
+    Path inaccessibleHelperDir = workDir.getRelative("inaccessibleHelperDir");
+    // Setting the permissions is necessary when we are using an asynchronous tree deleter in order
+    // to move the directory first. This is not necessary for a synchronous tree deleter because the
+    // permissions are only needed in the parent directory in that case.
+    if (inaccessibleHelperDir.exists()) {
+      inaccessibleHelperDir.setExecutable(true);
+      inaccessibleHelperDir.setWritable(true);
+      inaccessibleHelperDir.setReadable(true);
+    }
+
     // To avoid excessive scanning of dirsToCreate for prefix dirs, we prepopulate this set of
     // prefixes.
     Set<PathFragment> prefixDirs = new HashSet<>();

src/main/java/com/google/devtools/build/lib/worker/BUILD

@@ -102,11 +102,13 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/exec:execution_options",
         "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
+        "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/runtime/commands/events",
         "//src/main/java/com/google/devtools/build/lib/sandbox:linux_sandbox_util",
         "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers",
         "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_options",
+        "//src/main/java/com/google/devtools/build/lib/sandbox:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
@@ -185,6 +187,7 @@ java_library(
         ":worker_events",
         ":worker_key",
         "//src/main/java/com/google/devtools/build/lib/events",
+        "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:apache_commons_pool2",
@@ -267,6 +270,7 @@ java_library(
         ":worker_key",
         ":worker_protocol",
         "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/sandbox:linux_sandbox",
         "//src/main/java/com/google/devtools/build/lib/sandbox:linux_sandbox_command_line_builder",
         "//src/main/java/com/google/devtools/build/lib/sandbox:linux_sandbox_util",
@@ -314,6 +318,7 @@ java_library(
     name = "worker_exec_root",
     srcs = ["WorkerExecRoot.java"],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers",
         "//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/worker/SandboxedWorker.java

@@ -22,6 +22,7 @@
 import com.google.common.collect.Maps;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.UserExecException;
+import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.sandbox.CgroupsInfo;
 import com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder;
 import com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.BindMount;
@@ -99,13 +100,15 @@ public static WorkerSandboxOptions create(
 
   private Path inaccessibleHelperDir;
   private Path inaccessibleHelperFile;
+  private final TreeDeleter treeDeleter;
 
   SandboxedWorker(
       WorkerKey workerKey,
       int workerId,
       Path workDir,
       Path logFile,
-      @Nullable WorkerSandboxOptions hardenedSandboxOptions) {
+      @Nullable WorkerSandboxOptions hardenedSandboxOptions,
+      TreeDeleter treeDeleter) {
     super(workerKey, workerId, workDir, logFile);
     this.workerExecRoot =
         new WorkerExecRoot(
@@ -114,6 +117,7 @@ public static WorkerSandboxOptions create(
                 ? ImmutableList.of(PathFragment.create("../" + TMP_DIR_MOUNT_NAME))
                 : ImmutableList.of());
     this.hardenedSandboxOptions = hardenedSandboxOptions;
+    this.treeDeleter = treeDeleter;
   }
 
   @Override
@@ -211,7 +215,7 @@ protected Subprocess createProcess() throws IOException, UserExecException {
   public void prepareExecution(
       SandboxInputs inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
       throws IOException, InterruptedException, UserExecException {
-    workerExecRoot.createFileSystem(workerFiles, inputFiles, outputs);
+    workerExecRoot.createFileSystem(workerFiles, inputFiles, outputs, treeDeleter);
 
     super.prepareExecution(inputFiles, outputs, workerFiles);
   }

src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java

@@ -15,6 +15,7 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
@@ -45,7 +46,10 @@ public WorkerExecRoot(Path workDir, List<PathFragment> extraDirs) {
   }
 
   public void createFileSystem(
-      Set<PathFragment> workerFiles, SandboxInputs inputs, SandboxOutputs outputs)
+      Set<PathFragment> workerFiles,
+      SandboxInputs inputs,
+      SandboxOutputs outputs,
+      TreeDeleter treeDeleter)
       throws IOException, InterruptedException {
     workDir.createDirectoryAndParents();
 
@@ -66,7 +70,7 @@ public void createFileSystem(
     // We're traversing from workDir's parent directory because external repositories can now be
     // symlinked as siblings of workDir when --experimental_sibling_repository_layout is in effect.
     SandboxHelpers.cleanExisting(
-        workDir.getParentDirectory(), inputs, inputsToCreate, dirsToCreate, workDir);
+        workDir.getParentDirectory(), inputs, inputsToCreate, dirsToCreate, workDir, treeDeleter);
 
     // Finally, create anything that is still missing. This is non-strict only for historical
     // reasons,

src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java

@@ -18,6 +18,7 @@
 import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.worker.SandboxedWorker.WorkerSandboxOptions;
@@ -46,6 +47,7 @@ public class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worke
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   private final Path workerBaseDir;
+  private final TreeDeleter treeDeleter;
   private Reporter reporter;
   private EventBus eventBus;
 
@@ -56,12 +58,16 @@ public class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worke
   @Nullable private final WorkerSandboxOptions hardenedSandboxOptions;
 
   public WorkerFactory(Path workerBaseDir) {
-    this(workerBaseDir, null);
+    this(workerBaseDir, null, null);
   }
 
-  public WorkerFactory(Path workerBaseDir, @Nullable WorkerSandboxOptions hardenedSandboxOptions) {
+  public WorkerFactory(
+      Path workerBaseDir,
+      @Nullable WorkerSandboxOptions hardenedSandboxOptions,
+      @Nullable TreeDeleter treeDeleter) {
     this.workerBaseDir = workerBaseDir;
     this.hardenedSandboxOptions = hardenedSandboxOptions;
+    this.treeDeleter = treeDeleter;
   }
 
   public void setReporter(Reporter reporter) {
@@ -90,7 +96,9 @@ public Worker create(WorkerKey key) throws IOException {
         worker = new SandboxedWorkerProxy(key, workerId, logFile, workerMultiplexer, workDir);
       } else {
         Path workDir = getSandboxedWorkerPath(key, workerId);
-        worker = new SandboxedWorker(key, workerId, workDir, logFile, hardenedSandboxOptions);
+        worker =
+            new SandboxedWorker(
+                key, workerId, workDir, logFile, hardenedSandboxOptions, treeDeleter);
       }
     } else if (key.isMultiplex()) {
       WorkerMultiplexer workerMultiplexer = WorkerMultiplexerManager.getInstance(key, logFile);

src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java

@@ -25,12 +25,14 @@
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
 import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
+import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.BlazeWorkspace;
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent;
+import com.google.devtools.build.lib.sandbox.AsynchronousTreeDeleter;
 import com.google.devtools.build.lib.sandbox.LinuxSandboxUtil;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers;
 import com.google.devtools.build.lib.sandbox.SandboxOptions;
@@ -46,6 +48,7 @@ public class WorkerModule extends BlazeModule {
   private CommandEnvironment env;
 
   private WorkerFactory workerFactory;
+  private TreeDeleter treeDeleter;
   @VisibleForTesting WorkerPoolImpl workerPool;
   @Nullable private WorkerLifecycleManager workerLifecycleManager;
 
@@ -108,7 +111,12 @@ public void buildStarting(BuildStartingEvent event) {
     } else {
       workerSandboxOptions = null;
     }
-    WorkerFactory newWorkerFactory = new WorkerFactory(workerDir, workerSandboxOptions);
+    Path trashBase = workerDir.getRelative("_moved_trash_dir");
+    if (treeDeleter == null) {
+      treeDeleter = new AsynchronousTreeDeleter(trashBase);
+    }
+    WorkerFactory newWorkerFactory =
+        new WorkerFactory(workerDir, workerSandboxOptions, treeDeleter);
     if (!newWorkerFactory.equals(workerFactory)) {
       if (workerDir.exists()) {
         try {

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

@@ -98,6 +98,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/metrics:ps_info_collector",
         "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers",
+        "//src/main/java/com/google/devtools/build/lib/sandbox:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
         "//src/main/java/com/google/devtools/build/lib/util:resource_converter",
         "//src/main/java/com/google/devtools/build/lib/vfs",

src/test/java/com/google/devtools/build/lib/worker/WorkerExecRootTest.java

@@ -16,6 +16,7 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.sandbox.SynchronousTreeDeleter;
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -64,7 +65,8 @@ public void cleanFileSystem() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     // Pretend to do some work inside the execRoot.
     workDir.getRelative("tempdir").createDirectory();
@@ -77,7 +79,8 @@ public void cleanFileSystem() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     assertThat(workDir.getRelative("worker.sh").exists()).isTrue();
     assertThat(
@@ -107,7 +110,8 @@ public void createsAndCleansInputSymlinks() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     assertThat(workDir.getRelative("dir/input_symlink_1").readSymbolicLink())
         .isEqualTo(PathFragment.create("new_content"));
@@ -131,7 +135,8 @@ public void createsOutputDirs() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     assertThat(workDir.getRelative("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_sourcegenfiles").exists())
         .isTrue();
@@ -161,7 +166,8 @@ public void workspaceFilesAreNotDeleted() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     assertThat(workDir.getRelative("needed_file").readSymbolicLink())
         .isEqualTo(neededWorkspaceFile.asFragment());
@@ -189,7 +195,8 @@ public void recreatesEmptyFiles() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     assertThat(
             FileSystemUtils.readContent(workDir.getRelative("some_file"), Charset.defaultCharset()))
@@ -222,7 +229,8 @@ public void createsAndDeletesSiblingExternalRepoFiles() throws Exception {
     workerExecRoot.createFileSystem(
         sandboxHelper.getWorkerFiles(),
         sandboxHelper.getSandboxInputs(),
-        sandboxHelper.getSandboxOutputs());
+        sandboxHelper.getSandboxOutputs(),
+        new SynchronousTreeDeleter());
 
     assertThat(workDir.getRelative("../foo/bar/input1").readSymbolicLink())
         .isEqualTo(input1.asFragment());