Also use an AsynchronousTreeDeleter for multiplex workers.

I don't see a reason for singleplex and multiplex workers to differ in this regard. This makes it possible to clean up logic that previously needed to handle a null TreeDeleter.

PiperOrigin-RevId: 662499579
Change-Id: I7138daad36c0f9f2db72588bfcc2947344c00a25

Author: tjgq

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

@@ -129,23 +129,13 @@ public static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path tar
    * directories or files that are either not needed {@code inputs} or doesn't have the right
    * content or symlink target path are removed.
    */
-  public static void cleanExisting(
-      Path root,
-      SandboxInputs inputs,
-      Set<PathFragment> inputsToCreate,
-      Set<PathFragment> dirsToCreate,
-      Path workDir)
-      throws IOException, InterruptedException {
-    cleanExisting(root, inputs, inputsToCreate, dirsToCreate, workDir, /* treeDeleter= */ null);
-  }
-
   public static void cleanExisting(
       Path root,
       SandboxInputs inputs,
       Set<PathFragment> inputsToCreate,
       Set<PathFragment> dirsToCreate,
       Path workDir,
-      @Nullable TreeDeleter treeDeleter)
+      TreeDeleter treeDeleter)
       throws IOException, InterruptedException {
     cleanExisting(
         root,
@@ -163,7 +153,7 @@ public static void cleanExisting(
       Set<PathFragment> inputsToCreate,
       Set<PathFragment> dirsToCreate,
       Path workDir,
-      @Nullable TreeDeleter treeDeleter,
+      TreeDeleter treeDeleter,
       @Nullable SandboxContents sandboxContents)
       throws IOException, InterruptedException {
     Path inaccessibleHelperDir = workDir.getRelative(INACCESSIBLE_HELPER_DIR);
@@ -216,7 +206,7 @@ private static void cleanRecursivelyWithInMemoryContents(
       Set<PathFragment> dirsToCreate,
       Path workDir,
       Set<PathFragment> prefixDirs,
-      @Nullable TreeDeleter treeDeleter,
+      TreeDeleter treeDeleter,
       SandboxContents stashContents)
       throws IOException, InterruptedException {
     Path execroot = workDir.getParentDirectory();
@@ -254,12 +244,7 @@ private static void cleanRecursivelyWithInMemoryContents(
             dirent.getValue());
         dirsToCreate.remove(pathRelativeToWorkDir);
       } else {
-        if (treeDeleter == null) {
-          // TODO(bazel-team): Use async tree deleter for workers too
-          absPath.deleteTree();
-        } else {
-          treeDeleter.deleteTree(absPath);
-        }
+        treeDeleter.deleteTree(absPath);
       }
     }
   }

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

@@ -94,7 +94,7 @@ public final class SandboxModule extends BlazeModule {
   private final Set<SandboxFallbackSpawnRunner> spawnRunners = new HashSet<>();
 
   /**
-   * Handler to process expensive tree deletions outside of the critical path.
+   * Handler to process expensive tree deletions, potentially outside of the critical path.
    *
    * <p>Sandboxing creates one separate tree for each action, and this tree is used to run the
    * action commands in. These trees are disjoint for all actions and have unique identifiers.

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

@@ -331,6 +331,7 @@ java_library(
         ":worker_protocol",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//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/sandbox:cgroups_info",
         "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers",
         "//src/main/java/com/google/devtools/build/lib/shell",

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

@@ -18,6 +18,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.flogger.GoogleLogger;
+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;
@@ -42,12 +43,15 @@ public class SandboxedWorkerProxy extends WorkerProxy {
 
   private final PathFragment sandboxName;
 
+  private final TreeDeleter treeDeleter;
+
   SandboxedWorkerProxy(
       WorkerKey workerKey,
       int workerId,
       Path logFile,
       WorkerMultiplexer workerMultiplexer,
-      Path workDir) {
+      Path workDir,
+      TreeDeleter treeDeleter) {
     super(workerKey, workerId, logFile, workerMultiplexer, workDir);
     sandboxName =
         PathFragment.create(
@@ -57,6 +61,7 @@ public class SandboxedWorkerProxy extends WorkerProxy {
                     Integer.toString(workerId),
                     workerKey.getExecRoot().getBaseName()));
     sandboxDir = this.workDir.getRelative(sandboxName);
+    this.treeDeleter = treeDeleter;
   }
 
   @Override
@@ -68,7 +73,7 @@ public boolean isSandboxed() {
   public void prepareExecution(
       SandboxInputs inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
       throws IOException, InterruptedException {
-    workerMultiplexer.createSandboxedProcess(workDir, workerFiles, inputFiles);
+    workerMultiplexer.createSandboxedProcess(workDir, workerFiles, inputFiles, treeDeleter);
 
     sandboxDir.createDirectoryAndParents();
     LinkedHashSet<PathFragment> dirsToCreate = new LinkedHashSet<>();
@@ -81,7 +86,12 @@ public void prepareExecution(
         Iterables.concat(inputFiles.getFiles().keySet(), inputFiles.getSymlinks().keySet()),
         outputs);
     SandboxHelpers.cleanExisting(
-        sandboxDir.getParentDirectory(), inputFiles, inputsToCreate, dirsToCreate, sandboxDir);
+        sandboxDir.getParentDirectory(),
+        inputFiles,
+        inputsToCreate,
+        dirsToCreate,
+        sandboxDir,
+        treeDeleter);
     // Finally, create anything that is still missing. This is non-strict only for historical
     // reasons, we haven't seen what would break if we make it strict.
     SandboxHelpers.createDirectories(dirsToCreate, sandboxDir, /* strict=*/ false);

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

@@ -100,7 +100,9 @@ public Worker create(WorkerKey key) throws IOException {
       if (key.isMultiplex()) {
         WorkerMultiplexer workerMultiplexer = WorkerMultiplexerManager.getInstance(key, logFile);
         Path workDir = getSandboxedWorkerPath(key);
-        worker = new SandboxedWorkerProxy(key, workerId, logFile, workerMultiplexer, workDir);
+        worker =
+            new SandboxedWorkerProxy(
+                key, workerId, logFile, workerMultiplexer, workDir, treeDeleter);
       } else {
         Path workDir = getSandboxedWorkerPath(key, workerId);
         worker =

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

@@ -20,6 +20,7 @@
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+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;
@@ -151,7 +152,10 @@ public WorkerProcessStatus getStatus() {
    * sets up the sandbox root dir with the required worker files.
    */
   public synchronized void createSandboxedProcess(
-      Path workDir, Set<PathFragment> workerFiles, SandboxInputs inputFiles)
+      Path workDir,
+      Set<PathFragment> workerFiles,
+      SandboxInputs inputFiles,
+      TreeDeleter treeDeleter)
       throws IOException, InterruptedException {
     // TODO: Make blaze clean remove the workdir.
     if (this.process == null) {
@@ -167,7 +171,12 @@ public synchronized void createSandboxedProcess(
           workerFiles,
           SandboxOutputs.getEmptyInstance());
       SandboxHelpers.cleanExisting(
-          workDir.getParentDirectory(), inputFiles, inputsToCreate, dirsToCreate, workDir);
+          workDir.getParentDirectory(),
+          inputFiles,
+          inputsToCreate,
+          dirsToCreate,
+          workDir,
+          treeDeleter);
       SandboxHelpers.createDirectories(dirsToCreate, workDir, /* strict=*/ false);
       WorkerExecRoot.createInputs(inputsToCreate, inputFiles.limitedCopy(workerFiles), workDir);
       createProcess(workDir);

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

@@ -32,6 +32,7 @@
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.exec.BinTools;
+import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.exec.util.SpawnBuilder;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
@@ -67,6 +68,8 @@
 @RunWith(JUnit4.class)
 public class SandboxHelpersTest {
 
+  private final TreeDeleter treeDeleter = new SynchronousTreeDeleter();
+
   private final Scratch scratch = new Scratch();
   private Path execRoot;
   @Nullable private ExecutorService executorToCleanup;
@@ -281,7 +284,8 @@ public void cleanExisting_updatesDirs() throws IOException, InterruptedException
             ImmutableMap.of(input1, inputTxt, input2, inputTxt, input3, inputTxt, input4, inputTxt),
             ImmutableMap.of(),
             ImmutableMap.of());
-    SandboxHelpers.cleanExisting(rootDir, inputs2, inputsToCreate, dirsToCreate, execRoot);
+    SandboxHelpers.cleanExisting(
+        rootDir, inputs2, inputsToCreate, dirsToCreate, execRoot, treeDeleter);
     assertThat(dirsToCreate).containsExactly(inputDir2, inputDir3, outputDir);
     assertThat(execRoot.getRelative("existing/directory/with").exists()).isTrue();
     assertThat(execRoot.getRelative("partial").exists()).isTrue();