Move sandbox_stash to sandboxBase from outputBase

Because the sandbox_base can be set via the configuration flag `--sandbox_base`, it might be that the stashes end up in a different filesystem than the actual sandbox, which makes `reuse_sandbox_directories` inefficient.

Apart from making sure that the stash is under `sandboxBase`, this change refactors the logic about which directories to delete from `sandboxBase` and when. The stash will persist between incremental builds but after a bazel server restart it will be deleted.

This change also refactors the tests since we had three different tests which weren't testing anything different. The change also includes more profiling spans related to reuse_sandbox_directories.

Works towards fixing #20965.

PiperOrigin-RevId: 607023622
Change-Id: I59f9d2066e966dd60dd11f7d82815d0c89f2ce23

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

@@ -20,6 +20,8 @@
 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.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -118,20 +120,28 @@ public void createFileSystem() throws IOException, InterruptedException {
             .filter(p -> p.startsWith(sandboxExecRoot))
             .map(p -> p.relativeTo(sandboxExecRoot))
             .collect(Collectors.toSet());
-    SandboxHelpers.populateInputsAndDirsToCreate(
-        writableSandboxDirs,
-        inputsToCreate,
-        dirsToCreate,
-        Iterables.concat(
-            ImmutableSet.of(), inputs.getFiles().keySet(), inputs.getSymlinks().keySet()),
-        outputs);
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.populateInputsAndDirsToCreate")) {
+      SandboxHelpers.populateInputsAndDirsToCreate(
+          writableSandboxDirs,
+          inputsToCreate,
+          dirsToCreate,
+          Iterables.concat(
+              ImmutableSet.of(), inputs.getFiles().keySet(), inputs.getSymlinks().keySet()),
+          outputs);
+    }
 
-    // Allow subclasses to filter out inputs and dirs that don't need to be created.
-    filterInputsAndDirsToCreate(inputsToCreate, dirsToCreate);
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.filterInputsAndDirsToCreate")) {
+      // Allow subclasses to filter out inputs and dirs that don't need to be created.
+      filterInputsAndDirsToCreate(inputsToCreate, dirsToCreate);
+    }
 
     // Finally create what needs creating.
-    SandboxHelpers.createDirectories(dirsToCreate, sandboxExecRoot, /* strict=*/ true);
-    createInputs(inputsToCreate, inputs);
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.createDirectories")) {
+      SandboxHelpers.createDirectories(dirsToCreate, sandboxExecRoot, /* strict= */ true);
+    }
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.createInputs")) {
+      createInputs(inputsToCreate, inputs);
+    }
   }
 
   protected void filterInputsAndDirsToCreate(

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

@@ -38,6 +38,9 @@
  * finished, this number should be raised to quickly go through any pending deletions.
  */
 public class AsynchronousTreeDeleter implements TreeDeleter {
+
+  public static final String MOVED_TRASH_DIR = "_moved_trash_dir";
+
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   private final AtomicInteger trashCount = new AtomicInteger(0);

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

@@ -75,6 +75,7 @@ java_library(
         ":sandbox_helpers",
         ":sandbox_options",
         "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/util:command",
         "//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit",
         "//src/main/java/com/google/devtools/build/lib/vfs",

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

@@ -111,7 +111,7 @@ public static Path getInaccessibleHelperFile(Path sandboxBase) throws IOExceptio
   public static Path getInaccessibleHelperDir(Path sandboxBase) throws IOException {
     // The order of the permissions settings calls matters, see
     // https://github.com/bazelbuild/bazel/issues/16364
-    Path inaccessibleHelperDir = sandboxBase.getRelative("inaccessibleHelperDir");
+    Path inaccessibleHelperDir = sandboxBase.getRelative(SandboxHelpers.INACCESSIBLE_HELPER_DIR);
     inaccessibleHelperDir.createDirectory();
     inaccessibleHelperDir.setExecutable(false);
     inaccessibleHelperDir.setWritable(false);

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

@@ -73,11 +73,15 @@
  * <p>All sandboxed strategies within a build should share the same instance of this object.
  */
 public final class SandboxHelpers {
+
+  public static final String INACCESSIBLE_HELPER_DIR = "inaccessibleHelperDir";
+
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false);
 
   private static final AtomicInteger tempFileUniquifierForVirtualInputWrites = new AtomicInteger();
+
   /**
    * Moves all given outputs from a root to another.
    *
@@ -148,7 +152,7 @@ public static void cleanExisting(
       Path workDir,
       @Nullable TreeDeleter treeDeleter)
       throws IOException, InterruptedException {
-    Path inaccessibleHelperDir = workDir.getRelative("inaccessibleHelperDir");
+    Path inaccessibleHelperDir = workDir.getRelative(INACCESSIBLE_HELPER_DIR);
     // 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.

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

@@ -15,9 +15,12 @@
 package com.google.devtools.build.lib.sandbox;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 
+import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.eventbus.Subscribe;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -63,6 +66,9 @@
 /** This module provides the Sandbox spawn strategy. */
 public final class SandboxModule extends BlazeModule {
 
+  private static final ImmutableSet<String> SANDBOX_BASE_PERSISTENT_DIRS =
+      ImmutableSet.of(SandboxStash.SANDBOX_STASH_BASE, AsynchronousTreeDeleter.MOVED_TRASH_DIR);
+
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   /** Tracks whether we are issuing the very first build within this Bazel server instance. */
@@ -81,7 +87,7 @@ public final class SandboxModule extends BlazeModule {
    * completion but to avoid wiping the whole sandbox base itself, which could be problematic across
    * builds.
    */
-  private final Set<SpawnRunner> spawnRunners = new HashSet<>();
+  private final Set<SandboxFallbackSpawnRunner> spawnRunners = new HashSet<>();
 
   /**
    * Handler to process expensive tree deletions outside of the critical path.
@@ -193,7 +199,7 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
       throws IOException, InterruptedException {
     SandboxOptions options = checkNotNull(env.getOptions().getOptions(SandboxOptions.class));
     sandboxBase = computeSandboxBase(options, env);
-    Path trashBase = sandboxBase.getRelative("_moved_trash_dir");
+    Path trashBase = sandboxBase.getRelative(AsynchronousTreeDeleter.MOVED_TRASH_DIR);
 
     SandboxHelpers helpers = new SandboxHelpers();
 
@@ -214,15 +220,35 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
         treeDeleter = new AsynchronousTreeDeleter(trashBase);
       }
     }
-    SandboxStash.initialize(env.getWorkspaceName(), env.getOutputBase(), options);
+    SandboxStash.initialize(env.getWorkspaceName(), sandboxBase, options);
 
     // SpawnExecutionPolicy#getId returns unique base directories for each sandboxed action during
     // the life of a Bazel server instance so we don't need to worry about stale directories from
     // previous builds. However, on the very first build of an instance of the server, we must
     // wipe old contents to avoid reusing stale directories.
     if (firstBuild && sandboxBase.exists()) {
-      cmdEnv.getReporter().handle(Event.info("Deleting stale sandbox base " + sandboxBase));
-      sandboxBase.deleteTree();
+      if (trashBase.exists()) {
+        // Delete stale trash from a previous server instance.
+        Path staleTrash = getStaleTrashDir(trashBase);
+        trashBase.renameTo(staleTrash);
+        trashBase.createDirectory();
+        treeDeleter.deleteTree(staleTrash);
+      } else {
+        trashBase.createDirectory();
+      }
+      // We can delete other dirs asynchronously (if the flag is on).
+      for (Path entry : sandboxBase.getDirectoryEntries()) {
+        if (entry.getBaseName().equals(AsynchronousTreeDeleter.MOVED_TRASH_DIR)) {
+          continue;
+        }
+        if (entry.getBaseName().equals(SandboxHelpers.INACCESSIBLE_HELPER_DIR)) {
+          entry.deleteTree();
+        } else if (entry.isDirectory()) {
+          treeDeleter.deleteTree(entry);
+        } else {
+          entry.delete();
+        }
+      }
     }
     firstBuild = false;
     sandboxBase.createDirectoryAndParents();
@@ -247,14 +273,10 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
     // This works on most platforms, but isn't the best choice, so we put it first and let later
     // platform-specific sandboxing strategies become the default.
     if (processWrapperSupported) {
-      SpawnRunner spawnRunner =
+      SandboxFallbackSpawnRunner spawnRunner =
           withFallback(
               cmdEnv,
-              new ProcessWrapperSandboxedSpawnRunner(
-                  helpers,
-                  cmdEnv,
-                  sandboxBase,
-                  treeDeleter));
+              new ProcessWrapperSandboxedSpawnRunner(helpers, cmdEnv, sandboxBase, treeDeleter));
       spawnRunners.add(spawnRunner);
       builder.registerStrategy(
           new ProcessWrapperSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner, executionOptions),
@@ -271,7 +293,7 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
       if (pathToDocker != null && DockerSandboxedSpawnRunner.isSupported(cmdEnv, pathToDocker)) {
         String defaultImage = options.dockerImage;
         boolean useCustomizedImages = options.dockerUseCustomizedImages;
-        SpawnRunner spawnRunner =
+        SandboxFallbackSpawnRunner spawnRunner =
             withFallback(
                 cmdEnv,
                 new DockerSandboxedSpawnRunner(
@@ -298,15 +320,11 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
 
     // This is the preferred sandboxing strategy on Linux.
     if (linuxSandboxSupported) {
-      SpawnRunner spawnRunner =
+      SandboxFallbackSpawnRunner spawnRunner =
           withFallback(
               cmdEnv,
               LinuxSandboxedStrategy.create(
-                  helpers,
-                  cmdEnv,
-                  sandboxBase,
-                  timeoutKillDelay,
-                  treeDeleter));
+                  helpers, cmdEnv, sandboxBase, timeoutKillDelay, treeDeleter));
       spawnRunners.add(spawnRunner);
       builder.registerStrategy(
           new LinuxSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner, executionOptions),
@@ -316,14 +334,9 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
 
     // This is the preferred sandboxing strategy on macOS.
     if (darwinSandboxSupported) {
-      SpawnRunner spawnRunner =
+      SandboxFallbackSpawnRunner spawnRunner =
           withFallback(
-              cmdEnv,
-              new DarwinSandboxedSpawnRunner(
-                  helpers,
-                  cmdEnv,
-                  sandboxBase,
-                  treeDeleter));
+              cmdEnv, new DarwinSandboxedSpawnRunner(helpers, cmdEnv, sandboxBase, treeDeleter));
       spawnRunners.add(spawnRunner);
       builder.registerStrategy(
           new DarwinSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner, executionOptions),
@@ -332,7 +345,7 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
     }
 
     if (windowsSandboxSupported) {
-      SpawnRunner spawnRunner =
+      SandboxFallbackSpawnRunner spawnRunner =
           withFallback(
               cmdEnv,
               new WindowsSandboxedSpawnRunner(
@@ -383,7 +396,7 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
     return null;
   }
 
-  private static SpawnRunner withFallback(
+  private static SandboxFallbackSpawnRunner withFallback(
       CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) {
     SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class);
     return new SandboxFallbackSpawnRunner(
@@ -484,29 +497,33 @@ public void cleanupSandboxBase(Path sandboxBase, TreeDeleter treeDeleter) throws
         fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter);
       }
     }
+
+    public SpawnRunner getSandboxSpawnRunner() {
+      return sandboxSpawnRunner;
+    }
   }
 
   @Subscribe
   public void cleanStarting(@SuppressWarnings("unused") CleanStartingEvent event) {
-    SandboxStash.clean(treeDeleter, env.getOutputBase());
+    if (sandboxBase != null) {
+      SandboxStash.clean(treeDeleter, sandboxBase);
+    }
   }
 
   /**
-   * Best-effort cleanup of the sandbox base assuming all per-spawn contents have been removed.
-   *
-   * <p>When this gets called, the individual trees of each spawn should have been cleaned up but we
-   * may be left with the top-level subdirectories used by each sandboxed spawn runner (e.g. {@code
-   * darwin-sandbox}) and the sandbox base itself. Try to delete those so that a Bazel server
-   * restart doesn't print a spurious {@code Deleting stale sandbox base} message.
+   * If there is anything other than SANDBOX_BASE_PERSISTENT_DIRS in sandboxBase when we hit this
+   * precondition then there is a programming error somewhere (or I made a wrong assumption that
+   * wasn't caught by any of our tests).
    */
-  private static void cleanupSandboxBaseTop(Path sandboxBase) {
+  private static void checkSandboxBaseTopOnlyContainsPersistentDirs(Path sandboxBase) {
     try {
-      // This might be called twice for a given sandbox base, so don't bother recording error
-      // messages if any of the files we try to delete don't exist.
-      for (Path leftover : sandboxBase.getDirectoryEntries()) {
-        leftover.delete();
-      }
-      sandboxBase.delete();
+      Preconditions.checkState(
+          SANDBOX_BASE_PERSISTENT_DIRS.containsAll(
+              sandboxBase.getDirectoryEntries().stream()
+                  .map(Path::getBaseName)
+                  .collect(toImmutableList())),
+          "Found unexpected files in sandbox base. Please report this in"
+              + " https://github.com/bazelbuild/bazel/issues.");
     } catch (IOException e) {
       logger.atWarning().withCause(e).log("Failed to clean up sandbox base %s", sandboxBase);
     }
@@ -533,16 +550,17 @@ public void afterCommand() {
     if (shouldCleanupSandboxBase) {
       try {
         checkNotNull(sandboxBase, "shouldCleanupSandboxBase implies sandboxBase has been set");
-        for (SpawnRunner spawnRunner : spawnRunners) {
+        for (SandboxFallbackSpawnRunner spawnRunner : spawnRunners) {
           spawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter);
+          sandboxBase.getChild(spawnRunner.getSandboxSpawnRunner().getName()).delete();
         }
       } catch (IOException e) {
         env.getReporter()
             .handle(Event.warn("Failed to delete contents of sandbox " + sandboxBase + ": " + e));
       }
       shouldCleanupSandboxBase = false;
 
-      cleanupSandboxBaseTop(sandboxBase);
+      checkSandboxBaseTopOnlyContainsPersistentDirs(sandboxBase);
       // We intentionally keep sandboxBase around, without resetting it to null, in case we have
       // asynchronous deletions going on. In that case, we'd still want to retry this during
       // shutdown.
@@ -565,10 +583,6 @@ private void commonShutdown() {
         treeDeleter = null; // Avoid potential reexecution if we crash.
       }
     }
-
-    if (sandboxBase != null) {
-      cleanupSandboxBaseTop(sandboxBase);
-    }
   }
 
   @Override
@@ -580,4 +594,12 @@ public void blazeShutdown() {
   public void blazeShutdownOnCrash(DetailedExitCode exitCode) {
     commonShutdown();
   }
+
+  private Path getStaleTrashDir(Path trashBase) {
+    int i = 0;
+    while (trashBase.getParentDirectory().getChild("stale-trash-" + i++).exists()) {
+      ;
+    }
+    return trashBase.getParentDirectory().getChild("stale-trash-" + --i);
+  }
 }