Roll forward of 656a0ba with test (unknown commit) and fix.

Big round of sandbox fixes / performance improvements.

- The number of stat() syscalls in the SymlinkedSandboxedSpawn was way too high. Do less, feel better.

- When using --experimental_sandbox_base, ensure that symlinks in the path are resolved. Before this, you had to check whether on your system /dev/shm is a symlink to /run/shm and then use that instead. Now it no longer matters, as symlinks are resolved.

- Remove an unnecessary directory creation from each sandboxed invocation. Turns out that the "tmpdir" that we created was no longer used after some changes to Bazel's TMPDIR handling.

- Use simpler sandbox paths, by using the unique ID for each Spawn provided by SpawnExecutionPolicy instead of a randomly generated temp folder name. This also saves a round-trip from our VFS to NIO and back. Clean up the sandbox base before each build to ensure that the unique IDs are actually unique. ;)

- Use Java 8's Process#isAlive to check whether a process is alive instead of trying to get the exitcode and catching an exception.

Closes #4913.

PiperOrigin-RevId: 193031017

src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.runtime;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.ExecutorInitException;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -231,7 +232,8 @@ public BuildOptions getDefaultBuildOptions(BlazeRuntime runtime) {
    * @param request the build request
    * @param builder the builder to add action context providers and consumers to
    */
-  public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {}
+  public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder)
+      throws ExecutorInitException {}
 
   /**
    * Called after each command.

src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java

@@ -59,8 +59,6 @@ private static void logStats(String message, CacheStats stats) {
 
   @Override
   public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {
-    super.executorInit(env, request, builder);
-
     ExecutionOptions options = request.getOptions(ExecutionOptions.class);
     if (lastKnownCacheSize == null
         || options.cacheSizeForComputedFileDigests != lastKnownCacheSize) {

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

@@ -50,13 +50,11 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
   private static final String SANDBOX_DEBUG_SUGGESTION =
       "\n\nUse --sandbox_debug to see verbose messages from the sandbox";
 
-  private final Path sandboxBase;
   private final SandboxOptions sandboxOptions;
   private final boolean verboseFailures;
   private final ImmutableSet<Path> inaccessiblePaths;
 
-  public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv, Path sandboxBase) {
-    this.sandboxBase = sandboxBase;
+  public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv) {
     this.sandboxOptions = cmdEnv.getOptions().getOptions(SandboxOptions.class);
     this.verboseFailures = cmdEnv.getOptions().getOptions(ExecutionOptions.class).verboseFailures;
     this.inaccessiblePaths =
@@ -88,7 +86,6 @@ protected SpawnResult runSpawn(
       SandboxedSpawn sandbox,
       SpawnExecutionPolicy policy,
       Path execRoot,
-      Path tmpDir,
       Duration timeout,
       Path statisticsPath)
       throws IOException, InterruptedException {
@@ -97,8 +94,7 @@ protected SpawnResult runSpawn(
       OutErr outErr = policy.getFileOutErr();
       policy.prefetchInputs();
 
-      SpawnResult result =
-          run(originalSpawn, sandbox, outErr, timeout, execRoot, tmpDir, statisticsPath);
+      SpawnResult result = run(originalSpawn, sandbox, outErr, timeout, execRoot, statisticsPath);
 
       policy.lockOutputFiles();
       try {
@@ -121,7 +117,6 @@ private final SpawnResult run(
       OutErr outErr,
       Duration timeout,
       Path execRoot,
-      Path tmpDir,
       Path statisticsPath)
       throws IOException, InterruptedException {
     Command cmd = new Command(
@@ -145,9 +140,6 @@ private final SpawnResult run(
     long startTime = System.currentTimeMillis();
     CommandResult commandResult;
     try {
-      if (!tmpDir.exists() && !tmpDir.createDirectory()) {
-        throw new IOException(String.format("Could not create temp directory '%s'", tmpDir));
-      }
       commandResult = cmd.execute(outErr.getOutputStream(), outErr.getErrorStream());
       if (Thread.currentThread().isInterrupted()) {
         throw new InterruptedException();
@@ -213,17 +205,6 @@ private boolean wasTimeout(Duration timeout, Duration wallTime) {
     return !timeout.isZero() && wallTime.compareTo(timeout) > 0;
   }
 
-  /**
-   * Returns a temporary directory that should be used as the sandbox directory for a single action.
-   */
-  protected Path getSandboxRoot() throws IOException {
-    return sandboxBase.getRelative(
-        java.nio.file.Files.createTempDirectory(
-                java.nio.file.Paths.get(sandboxBase.getPathString()), "")
-            .getFileName()
-            .toString());
-  }
-
   /**
    * Gets the list of directories that the spawn will assume to be writable.
    *

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

@@ -21,7 +21,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.io.ByteStreams;
-import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
@@ -96,6 +95,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
   private final Path execRoot;
   private final boolean allowNetwork;
   private final Path processWrapper;
+  private final Path sandboxBase;
   private final Duration timeoutKillDelay;
   private final @Nullable SandboxfsProcess sandboxfsProcess;
 
@@ -123,13 +123,14 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
       Duration timeoutKillDelay,
       @Nullable SandboxfsProcess sandboxfsProcess)
       throws IOException {
-    super(cmdEnv, sandboxBase);
+    super(cmdEnv);
     this.execRoot = cmdEnv.getExecRoot();
     this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions());
     this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem());
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
     this.localEnvProvider =
         new XcodeLocalEnvProvider(cmdEnv.getRuntime().getProductName(), cmdEnv.getClientEnv());
+    this.sandboxBase = sandboxBase;
     this.timeoutKillDelay = timeoutKillDelay;
     this.sandboxfsProcess = sandboxfsProcess;
   }
@@ -193,21 +194,19 @@ private static String getConfStr(String confVar) throws IOException {
 
   @Override
   protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
-      throws ExecException, IOException, InterruptedException {
+      throws IOException, InterruptedException {
     // Each invocation of "exec" gets its own sandbox.
-    Path sandboxPath = getSandboxRoot();
-    Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+    Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId()));
+    sandboxPath.createDirectory();
 
-    // Each sandboxed action runs in its own directory so we don't need to make the temp directory's
-    // name unique (like we have to with standalone execution strategy).
-    //
-    // Note that, for sandboxfs-based executions, this temp directory lives outside of the sandboxfs
-    // instance. This is perfectly fine (because sandbox-exec controls accesses to this directory)
-    // and is actually desirable for performance reasons.
-    Path tmpDir = sandboxPath.getRelative("tmp");
+    // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like
+    // the normal execroot does.
+    Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+    sandboxExecRoot.getParentDirectory().createDirectory();
+    sandboxExecRoot.createDirectory();
 
     Map<String, String> environment =
-        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString());
+        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
 
     final HashSet<Path> writableDirs = new HashSet<>(alwaysWritableDirs);
     ImmutableSet<Path> extraWritableDirs = getWritableDirs(sandboxExecRoot, environment);
@@ -288,7 +287,7 @@ public void createFileSystem() throws IOException {
             }
           };
     }
-    return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath);
+    return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath);
   }
 
   private void writeConfig(

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

@@ -33,6 +33,6 @@ final class DarwinSandboxedStrategy extends AbstractSpawnStrategy {
 
   @Override
   public String toString() {
-    return "sandboxed";
+    return "darwin-sandbox";
   }
 }

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

@@ -77,6 +77,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
   private final Path execRoot;
   private final boolean allowNetwork;
   private final Path linuxSandbox;
+  private final Path sandboxBase;
   private final Path inaccessibleHelperFile;
   private final Path inaccessibleHelperDir;
   private final LocalEnvProvider localEnvProvider;
@@ -101,12 +102,13 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
       Path inaccessibleHelperDir,
       Duration timeoutKillDelay,
       @Nullable SandboxfsProcess sandboxfsProcess) {
-    super(cmdEnv, sandboxBase);
+    super(cmdEnv);
     this.fileSystem = cmdEnv.getRuntime().getFileSystem();
     this.blazeDirs = cmdEnv.getDirectories();
     this.execRoot = cmdEnv.getExecRoot();
     this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions());
     this.linuxSandbox = LinuxSandboxUtil.getLinuxSandbox(cmdEnv);
+    this.sandboxBase = sandboxBase;
     this.inaccessibleHelperFile = inaccessibleHelperFile;
     this.inaccessibleHelperDir = inaccessibleHelperDir;
     this.timeoutKillDelay = timeoutKillDelay;
@@ -117,21 +119,18 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
   @Override
   protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
       throws IOException, ExecException, InterruptedException {
-    // Each invocation of "exec" gets its own sandbox.
-    Path sandboxPath = getSandboxRoot();
-    Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
-    sandboxExecRoot.createDirectoryAndParents();
+    // Each invocation of "exec" gets its own sandbox base, execroot and temporary directory.
+    Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId()));
+    sandboxPath.createDirectory();
 
-    // Each sandboxed action runs in its own directory so we don't need to make the temp directory's
-    // name unique (like we have to with standalone execution strategy).
-    //
-    // Note that, for sandboxfs-based executions, this temp directory lives outside of the sandboxfs
-    // instance. This is perfectly fine (because linux-sandbox controls accesses to this directory)
-    // and is actually desirable for performance reasons.
-    Path tmpDir = sandboxPath.getRelative("tmp");
+    // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like
+    // the normal execroot does.
+    Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+    sandboxExecRoot.getParentDirectory().createDirectory();
+    sandboxExecRoot.createDirectory();
 
     Map<String, String> environment =
-        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString());
+        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
 
     ImmutableSet<Path> writableDirs = getWritableDirs(sandboxExecRoot, environment);
     ImmutableSet<PathFragment> outputs = SandboxHelpers.getOutputFiles(spawn);
@@ -144,12 +143,13 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
             .setBindMounts(getReadOnlyBindMounts(blazeDirs, sandboxExecRoot))
             .setUseFakeHostname(getSandboxOptions().sandboxFakeHostname)
             .setCreateNetworkNamespace(!(allowNetwork || Spawns.requiresNetwork(spawn)))
-            .setUseDebugMode(getSandboxOptions().sandboxDebug);
+            .setUseDebugMode(getSandboxOptions().sandboxDebug)
+            .setKillDelay(timeoutKillDelay);
 
     if (!timeout.isZero()) {
       commandLineBuilder.setTimeout(timeout);
     }
-    commandLineBuilder.setKillDelay(timeoutKillDelay);
+
     if (spawn.getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_FAKEROOT)) {
       commandLineBuilder.setUseFakeRoot(true);
     } else if (getSandboxOptions().sandboxFakeUsername) {
@@ -185,7 +185,7 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
               writableDirs);
     }
 
-    return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath);
+    return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath);
   }
 
   @Override

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

@@ -38,7 +38,7 @@ public final class LinuxSandboxedStrategy extends AbstractSpawnStrategy {
 
   @Override
   public String toString() {
-    return "sandboxed";
+    return "linux-sandbox";
   }
 
   /**

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

@@ -35,8 +35,9 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
     return OS.isPosixCompatible() && ProcessWrapperUtil.isSupported(cmdEnv);
   }
 
-  private final Path execRoot;
   private final Path processWrapper;
+  private final Path execRoot;
+  private final Path sandboxBase;
   private final LocalEnvProvider localEnvProvider;
   private final Duration timeoutKillDelay;
 
@@ -50,29 +51,32 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
    */
   ProcessWrapperSandboxedSpawnRunner(
       CommandEnvironment cmdEnv, Path sandboxBase, String productName, Duration timeoutKillDelay) {
-    super(cmdEnv, sandboxBase);
-    this.execRoot = cmdEnv.getExecRoot();
-    this.timeoutKillDelay = timeoutKillDelay;
+    super(cmdEnv);
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
+    this.execRoot = cmdEnv.getExecRoot();
     this.localEnvProvider =
         OS.getCurrent() == OS.DARWIN
             ? new XcodeLocalEnvProvider(productName, cmdEnv.getClientEnv())
             : new PosixLocalEnvProvider(cmdEnv.getClientEnv());
+    this.sandboxBase = sandboxBase;
+    this.timeoutKillDelay = timeoutKillDelay;
   }
 
   @Override
   protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
       throws ExecException, IOException, InterruptedException {
     // Each invocation of "exec" gets its own sandbox.
-    Path sandboxPath = getSandboxRoot();
-    Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+    Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId()));
+    sandboxPath.createDirectory();
 
-    // Each sandboxed action runs in its own execroot, so we don't need to make the temp directory's
-    // name unique (like we have to with standalone execution strategy).
-    Path tmpDir = sandboxExecRoot.getRelative("tmp");
+    // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like
+    // the normal execroot does.
+    Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+    sandboxExecRoot.getParentDirectory().createDirectory();
+    sandboxExecRoot.createDirectory();
 
     Map<String, String> environment =
-        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString());
+        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
 
     Duration timeout = policy.getTimeout();
     ProcessWrapperUtil.CommandLineBuilder commandLineBuilder =
@@ -97,7 +101,7 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
             SandboxHelpers.getOutputFiles(spawn),
             getWritableDirs(sandboxExecRoot, environment));
 
-    return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath);
+    return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath);
   }
 
   @Override

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

@@ -33,6 +33,6 @@ final class ProcessWrapperSandboxedStrategy extends AbstractSpawnStrategy {
 
   @Override
   public String toString() {
-    return "sandboxed";
+    return "processwrapper-sandbox";
   }
 }