From 3ca4fd45c68a5e74f668c2ae212842b0e6036b18 Mon Sep 17 00:00:00 2001
From: oquenchil <23365806+oquenchil@users.noreply.github.com>
Date: Tue, 27 Feb 2024 19:56:17 +0100
Subject: [PATCH] [7.1.0] Fix stale trash dir not cleaned up on worker creation
 (#21510)

Cherrypicks: e482e8b

The bug in Bazel is that ${output_base}/blaze-workers/_moved_trash_dir/
doesn't
get wiped between blaze restarts. The directory to move to for
asynchronous
deletion is an incrementing AtomicInteger but after a restart it will be
zero
again. If there were previous directories there, we might get an error
trying
to replace a non-empty directory while renaming.
---
 .../lib/sandbox/AsynchronousTreeDeleter.java  |  4 +++
 .../google/devtools/build/lib/worker/BUILD    |  3 +-
 .../build/lib/worker/WorkerFactory.java       | 10 +++++--
 .../build/lib/worker/WorkerModule.java        | 30 +++++++++++++++++--
 .../build/lib/worker/WorkerModuleTest.java    | 27 +++++++++++++++++
 5 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AsynchronousTreeDeleter.java b/src/main/java/com/google/devtools/build/lib/sandbox/AsynchronousTreeDeleter.java
index 70aa26583f5a16..ce809f3ad6c9d4 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/AsynchronousTreeDeleter.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/AsynchronousTreeDeleter.java
@@ -110,4 +110,8 @@ public void shutdown() {
       service = null;
     }
   }
+
+  public Path getTrashBase() {
+    return trashBase;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD
index 9f0d8dc6b42835..03a625a5cc160c 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD
@@ -102,7 +102,6 @@ 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",
@@ -187,7 +186,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/sandbox: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",
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
index 6e1f6b33e594de..a47fbd834546c5 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
@@ -18,7 +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.sandbox.AsynchronousTreeDeleter;
 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;
@@ -47,7 +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 final AsynchronousTreeDeleter treeDeleter;
   private Reporter reporter;
   private EventBus eventBus;
 
@@ -64,7 +64,7 @@ public WorkerFactory(Path workerBaseDir) {
   public WorkerFactory(
       Path workerBaseDir,
       @Nullable WorkerSandboxOptions hardenedSandboxOptions,
-      @Nullable TreeDeleter treeDeleter) {
+      @Nullable AsynchronousTreeDeleter treeDeleter) {
     this.workerBaseDir = workerBaseDir;
     this.hardenedSandboxOptions = hardenedSandboxOptions;
     this.treeDeleter = treeDeleter;
@@ -84,6 +84,10 @@ public Worker create(WorkerKey key) throws IOException {
     String workTypeName = key.getWorkerTypeName();
     if (!workerBaseDir.isDirectory()) {
       workerBaseDir.createDirectoryAndParents();
+      Path deleterTrashBase = treeDeleter == null ? null : treeDeleter.getTrashBase();
+      if (deleterTrashBase != null) {
+        deleterTrashBase.createDirectory();
+      }
     }
     Path logFile =
         workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log");
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
index 23a25ced83e395..5defc2f9457c50 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
@@ -25,7 +25,6 @@
 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;
@@ -45,10 +44,12 @@
 
 /** A module that adds the WorkerActionContextProvider to the available action context providers. */
 public class WorkerModule extends BlazeModule {
+
+  private static final String STALE_TRASH = "_stale_trash";
   private CommandEnvironment env;
 
   private WorkerFactory workerFactory;
-  private TreeDeleter treeDeleter;
+  private AsynchronousTreeDeleter treeDeleter;
   @VisibleForTesting WorkerPoolImpl workerPool;
   @Nullable private WorkerLifecycleManager workerLifecycleManager;
 
@@ -114,6 +115,9 @@ public void buildStarting(BuildStartingEvent event) {
     Path trashBase = workerDir.getRelative(AsynchronousTreeDeleter.MOVED_TRASH_DIR);
     if (treeDeleter == null) {
       treeDeleter = new AsynchronousTreeDeleter(trashBase);
+      if (trashBase.exists()) {
+        removeStaleTrash(workerDir, trashBase);
+      }
     }
     WorkerFactory newWorkerFactory =
         new WorkerFactory(workerDir, workerSandboxOptions, treeDeleter);
@@ -184,6 +188,28 @@ public void buildStarting(BuildStartingEvent event) {
     workerPool.clearDoomedWorkers();
   }
 
+  private void removeStaleTrash(Path workerDir, Path trashBase) {
+    try {
+      // The AsynchronousTreeDeleter relies on a counter for naming directories that will be
+      // moved out of the way before being deleted asynchronously.
+      // If there is trash on disk from a previous bazel server instance, the dirs will have
+      // names not synced with the counter, therefore we may run the risk of moving a directory
+      // in this server instance to a path of an existing directory. To solve this we rename
+      // the trash directory that was on disk, create a new empty trash directory and delete
+      // the old trash via the AsynchronousTreeDeleter. Before deletion the stale trash will be
+      // moved to a directory named `0` under MOVED_TRASH_DIR.
+      Path staleTrash = trashBase.getParentDirectory().getChild(STALE_TRASH);
+      trashBase.renameTo(staleTrash);
+      trashBase.createDirectory();
+      treeDeleter.deleteTree(staleTrash);
+    } catch (IOException e) {
+      env.getReporter()
+          .handle(
+              Event.error(
+                  String.format("Could not trash dir in '%s': %s", workerDir, e.getMessage())));
+    }
+  }
+
   @Override
   public void registerSpawnStrategies(
       SpawnStrategyRegistry.Builder registryBuilder, CommandEnvironment env) {
diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java
index 9c3ac4f6d0aafc..53b0db07aa171f 100644
--- a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java
@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.BlazeWorkspace;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.sandbox.AsynchronousTreeDeleter;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -232,6 +233,32 @@ public void buildStarting_survivesNoWorkerDir() throws Exception {
     assertThrows(IOException.class, () -> module.workerPool.borrowObject(key));
   }
 
+  @Test
+  public void buildStarting_cleansStaleTrashDirCleanedOnFirstBuild() throws Exception {
+    WorkerModule module = new WorkerModule();
+    WorkerOptions options = WorkerOptions.DEFAULTS;
+
+    when(request.getOptions(WorkerOptions.class)).thenReturn(options);
+    setupEnvironment("/outputRoot");
+
+    module.beforeCommand(env);
+    Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers");
+    Path trashBase = workerDir.getChild(AsynchronousTreeDeleter.MOVED_TRASH_DIR);
+    trashBase.createDirectoryAndParents();
+
+    Path staleTrash = trashBase.getChild("random-trash");
+
+    staleTrash.createDirectoryAndParents();
+    module.buildStarting(BuildStartingEvent.create(env, request));
+    // Trash is cleaned upon first build.
+    assertThat(staleTrash.exists()).isFalse();
+
+    staleTrash.createDirectoryAndParents();
+    module.buildStarting(BuildStartingEvent.create(env, request));
+    // Trash is not cleaned upon subsequent builds.
+    assertThat(staleTrash.exists()).isTrue();
+  }
+
   private void setupEnvironment(String rootDir) throws IOException, AbruptExitException {
     storedEventHandler = new StoredEventHandler();
     Path root = fs.getPath(rootDir);