Fold killDelay into ProcessWrapper.

The kill delay setting comes from the --local_termination_grace_seconds
option (it's not specific to individual process-wrapper invocations) and
we want every process-wrapper invocation to respect it.

In the past, we were propagating this value through various layers so
that we could reach the places where command lines were built... but we
can now take advantage of the ProcessWrapper object to pass this
information around.

Part of #10245.

RELNOTES: None.
PiperOrigin-RevId: 311112402

src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java

@@ -328,8 +328,7 @@ private SpawnResult start() throws InterruptedException, IOException {
           ProcessWrapper.CommandLineBuilder commandLineBuilder =
               processWrapper
                   .commandLineBuilder(spawn.getArguments())
-                  .setTimeout(context.getTimeout())
-                  .setKillDelay(Duration.ofSeconds(localExecutionOptions.localSigkillGraceSeconds));
+                  .setTimeout(context.getTimeout());
           if (localExecutionOptions.collectLocalExecutionStatistics) {
             statisticsPath = tmpDir.getRelative("stats.out");
             commandLineBuilder.setStatisticsPath(statisticsPath);

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.runtime;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.OsUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -32,10 +33,14 @@ public final class ProcessWrapper {
   /** Path to the process-wrapper binary to use. */
   private final Path binPath;
 
+  /** Grace delay between asking a process to stop and forcibly killing it, or null for none. */
+  @Nullable private final Duration killDelay;
+
   /** Creates a new process-wrapper instance from explicit values. */
   @VisibleForTesting
-  public ProcessWrapper(Path binPath) {
+  public ProcessWrapper(Path binPath, @Nullable Duration killDelay) {
     this.binPath = binPath;
+    this.killDelay = killDelay;
   }
 
   /**
@@ -46,17 +51,20 @@ public ProcessWrapper(Path binPath) {
    */
   @Nullable
   public static ProcessWrapper fromCommandEnvironment(CommandEnvironment cmdEnv) {
+    LocalExecutionOptions options = cmdEnv.getOptions().getOptions(LocalExecutionOptions.class);
+    Duration killDelay = options == null ? null : options.getLocalSigkillGraceSeconds();
+
     Path path = cmdEnv.getBlazeWorkspace().getBinTools().getEmbeddedPath(BIN_BASENAME);
     if (OS.isPosixCompatible() && path != null && path.exists()) {
-      return new ProcessWrapper(path);
+      return new ProcessWrapper(path, killDelay);
     } else {
       return null;
     }
   }
 
   /** Returns a new {@link CommandLineBuilder} for the process-wrapper tool. */
   public CommandLineBuilder commandLineBuilder(List<String> commandArguments) {
-    return new CommandLineBuilder(binPath.getPathString(), commandArguments);
+    return new CommandLineBuilder(binPath.getPathString(), commandArguments, killDelay);
   }
 
   /**
@@ -66,15 +74,18 @@ public CommandLineBuilder commandLineBuilder(List<String> commandArguments) {
   public static class CommandLineBuilder {
     private final String processWrapperPath;
     private final List<String> commandArguments;
+    @Nullable private final Duration killDelay;
+
     private Path stdoutPath;
     private Path stderrPath;
     private Duration timeout;
-    private Duration killDelay;
     private Path statisticsPath;
 
-    private CommandLineBuilder(String processWrapperPath, List<String> commandArguments) {
+    private CommandLineBuilder(
+        String processWrapperPath, List<String> commandArguments, @Nullable Duration killDelay) {
       this.processWrapperPath = processWrapperPath;
       this.commandArguments = commandArguments;
+      this.killDelay = killDelay;
     }
 
     /** Sets the path to use for redirecting stdout, if any. */
@@ -95,15 +106,6 @@ public CommandLineBuilder setTimeout(Duration timeout) {
       return this;
     }
 
-    /**
-     * Sets the kill delay for commands run using the process-wrapper tool that exceed their
-     * timeout.
-     */
-    public CommandLineBuilder setKillDelay(Duration killDelay) {
-      this.killDelay = killDelay;
-      return this;
-    }
-
     /** Sets the path for writing execution statistics (e.g. resource usage). */
     public CommandLineBuilder setStatisticsPath(Path statisticsPath) {
       this.statisticsPath = statisticsPath;

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

@@ -104,7 +104,6 @@ private static boolean computeIsSupported() {
   private final boolean allowNetwork;
   private final ProcessWrapper processWrapper;
   private final Path sandboxBase;
-  private final Duration timeoutKillDelay;
   @Nullable private final SandboxfsProcess sandboxfsProcess;
   private final boolean sandboxfsMapSymlinkTargets;
   private final TreeDeleter treeDeleter;
@@ -124,7 +123,6 @@ private static boolean computeIsSupported() {
    * @param helpers common tools and state across all spawns during sandboxed execution
    * @param cmdEnv the command environment to use
    * @param sandboxBase path to the sandbox base directory
-   * @param timeoutKillDelay additional grace period before killing timing out commands
    * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in
    *     which case the runner uses a symlinked sandbox
    * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true
@@ -133,7 +131,6 @@ private static boolean computeIsSupported() {
       SandboxHelpers helpers,
       CommandEnvironment cmdEnv,
       Path sandboxBase,
-      Duration timeoutKillDelay,
       @Nullable SandboxfsProcess sandboxfsProcess,
       boolean sandboxfsMapSymlinkTargets,
       TreeDeleter treeDeleter)
@@ -146,7 +143,6 @@ private static boolean computeIsSupported() {
     this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv);
     this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
     this.sandboxBase = sandboxBase;
-    this.timeoutKillDelay = timeoutKillDelay;
     this.sandboxfsProcess = sandboxfsProcess;
     this.sandboxfsMapSymlinkTargets = sandboxfsMapSymlinkTargets;
     this.treeDeleter = treeDeleter;
@@ -249,8 +245,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     ProcessWrapper.CommandLineBuilder processWrapperCommandLineBuilder =
         processWrapper.commandLineBuilder(spawn.getArguments()).setTimeout(timeout);
 
-    processWrapperCommandLineBuilder.setKillDelay(timeoutKillDelay);
-
     final Path statisticsPath;
     if (getSandboxOptions().collectLocalSandboxExecutionStatistics) {
       statisticsPath = sandboxPath.getRelative("stats.out");

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

@@ -33,7 +33,6 @@ final class DockerCommandLineBuilder {
   private Path sandboxExecRoot;
   private Map<String, String> environmentVariables;
   private Duration timeout;
-  private Duration killDelay;
   private boolean createNetworkNamespace;
   private UUID uuid;
   private int uid;
@@ -78,11 +77,6 @@ public DockerCommandLineBuilder setTimeout(Duration timeout) {
     return this;
   }
 
-  public DockerCommandLineBuilder setKillDelay(Duration killDelay) {
-    this.killDelay = killDelay;
-    return this;
-  }
-
   public DockerCommandLineBuilder setCreateNetworkNamespace(boolean createNetworkNamespace) {
     this.createNetworkNamespace = createNetworkNamespace;
     return this;
@@ -179,9 +173,6 @@ public List<String> build() {
     if (timeout != null) {
       processWrapperCmdLine.setTimeout(timeout);
     }
-    if (killDelay != null) {
-      processWrapperCmdLine.setKillDelay(killDelay);
-    }
     return processWrapperCmdLine.build();
   }
 }

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

@@ -144,7 +144,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
   private final Path sandboxBase;
   private final String defaultImage;
   private final LocalEnvProvider localEnvProvider;
-  private final Duration timeoutKillDelay;
   private final String commandId;
   private final Reporter reporter;
   private final boolean useCustomizedImages;
@@ -162,7 +161,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
    * @param dockerClient path to the `docker` executable
    * @param sandboxBase path to the sandbox base directory
    * @param defaultImage the Docker image to use if the platform doesn't specify one
-   * @param timeoutKillDelay an additional grace period before killing timing out commands
    * @param useCustomizedImages whether to use customized images for execution
    * @param treeDeleter scheduler for tree deletions
    */
@@ -172,7 +170,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
       Path dockerClient,
       Path sandboxBase,
       String defaultImage,
-      Duration timeoutKillDelay,
       boolean useCustomizedImages,
       TreeDeleter treeDeleter) {
     super(cmdEnv);
@@ -184,7 +181,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
     this.sandboxBase = sandboxBase;
     this.defaultImage = defaultImage;
     this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
-    this.timeoutKillDelay = timeoutKillDelay;
     this.commandId = cmdEnv.getCommandId().toString();
     this.reporter = cmdEnv.getReporter();
     this.useCustomizedImages = useCustomizedImages;
@@ -258,7 +254,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
         .setAdditionalMounts(getSandboxOptions().sandboxAdditionalMounts)
         .setPrivileged(getSandboxOptions().dockerPrivileged)
         .setEnvironmentVariables(environment)
-        .setKillDelay(timeoutKillDelay)
         .setCreateNetworkNamespace(
             !(allowNetwork
                 || Spawns.requiresNetwork(spawn, getSandboxOptions().defaultSandboxAllowNetwork)))

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

@@ -42,7 +42,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
   private final Path execRoot;
   private final Path sandboxBase;
   private final LocalEnvProvider localEnvProvider;
-  private final Duration timeoutKillDelay;
   @Nullable private final SandboxfsProcess sandboxfsProcess;
   private final boolean sandboxfsMapSymlinkTargets;
   private final TreeDeleter treeDeleter;
@@ -53,7 +52,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
    * @param helpers common tools and state across all spawns during sandboxed execution
    * @param cmdEnv the command environment to use
    * @param sandboxBase path to the sandbox base directory
-   * @param timeoutKillDelay additional grace period before killing timing out commands
    * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in
    *     which case the runner uses a symlinked sandbox
    * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true
@@ -62,7 +60,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
       SandboxHelpers helpers,
       CommandEnvironment cmdEnv,
       Path sandboxBase,
-      Duration timeoutKillDelay,
       @Nullable SandboxfsProcess sandboxfsProcess,
       boolean sandboxfsMapSymlinkTargets,
       TreeDeleter treeDeleter) {
@@ -72,7 +69,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
     this.execRoot = cmdEnv.getExecRoot();
     this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
     this.sandboxBase = sandboxBase;
-    this.timeoutKillDelay = timeoutKillDelay;
     this.sandboxfsProcess = sandboxfsProcess;
     this.sandboxfsMapSymlinkTargets = sandboxfsMapSymlinkTargets;
     this.treeDeleter = treeDeleter;
@@ -103,8 +99,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     ProcessWrapper.CommandLineBuilder commandLineBuilder =
         processWrapper.commandLineBuilder(spawn.getArguments()).setTimeout(timeout);
 
-    commandLineBuilder.setKillDelay(timeoutKillDelay);
-
     Path statisticsPath = null;
     if (getSandboxOptions().collectLocalSandboxExecutionStatistics) {
       statisticsPath = sandboxPath.getRelative("stats.out");

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

@@ -289,7 +289,6 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
                   helpers,
                   cmdEnv,
                   sandboxBase,
-                  timeoutKillDelay,
                   sandboxfsProcess,
                   options.sandboxfsMapSymlinkTargets,
                   treeDeleter));
@@ -318,7 +317,6 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
                     pathToDocker,
                     sandboxBase,
                     defaultImage,
-                    timeoutKillDelay,
                     useCustomizedImages,
                     treeDeleter));
         spawnRunners.add(spawnRunner);
@@ -360,7 +358,6 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil
                   helpers,
                   cmdEnv,
                   sandboxBase,
-                  timeoutKillDelay,
                   sandboxfsProcess,
                   options.sandboxfsMapSymlinkTargets,
                   treeDeleter));

src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java

@@ -312,8 +312,9 @@ private FileSystem setupEnvironmentForFakeExecution() {
     return new InMemoryFileSystem();
   }
 
-  private static ProcessWrapper makeProcessWrapper(FileSystem fs) {
-    return new ProcessWrapper(fs.getPath("/process-wrapper"));
+  private static ProcessWrapper makeProcessWrapper(FileSystem fs, LocalExecutionOptions options) {
+    return new ProcessWrapper(
+        fs.getPath("/process-wrapper"), options.getLocalSigkillGraceSeconds());
   }
 
   /**
@@ -348,7 +349,7 @@ public void vanillaZeroExit() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -399,7 +400,7 @@ public void testParamFiles() throws Exception {
             execRoot,
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
     ParamFileActionInput paramFileActionInput =
         new ParamFileActionInput(
@@ -494,7 +495,7 @@ public void nonZeroExit() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
@@ -510,7 +511,6 @@ public void nonZeroExit() throws Exception {
     assertThat(captor.getValue().getArgv())
         .containsExactlyElementsIn(
             ImmutableList.of(
-                // process-wrapper timeout grace_time stdout stderr
                 "/process-wrapper", "--timeout=0", "--kill_delay=15", "/bin/echo", "Hi!"));
     assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
     assertThat(captor.getValue().getStdout()).isEqualTo(StreamAction.REDIRECT);
@@ -536,7 +536,7 @@ public void processStartupThrows() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     assertThat(fs.getPath("/out").createDirectory()).isTrue();
@@ -570,7 +570,7 @@ public void disallowLocalExecution() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
@@ -619,7 +619,7 @@ public void waitFor() throws InterruptedException {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -729,7 +729,7 @@ public void checkPrefetchCalled() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -754,7 +754,7 @@ public void checkNoPrefetchCalled() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             LocalSpawnRunnerTest::keepLocalEnvUnchanged);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -783,7 +783,7 @@ public void checkLocalEnvProviderCalled() throws Exception {
             fs.getPath("/execroot"),
             options,
             resourceManager,
-            makeProcessWrapper(fs),
+            makeProcessWrapper(fs, options),
             localEnvProvider);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -910,7 +910,7 @@ public void hasExecutionStatistics_whenOptionIsEnabled() throws Exception {
             resourceManager,
             LocalSpawnRunnerTest::keepLocalEnvUnchanged,
             binTools,
-            new ProcessWrapper(processWrapperPath),
+            new ProcessWrapper(processWrapperPath, /*killDelay=*/ Duration.ZERO),
             Mockito.mock(RunfilesTreeUpdater.class));
 
     Spawn spawn =
@@ -974,7 +974,7 @@ public void hasNoExecutionStatistics_whenOptionIsDisabled() throws Exception {
             resourceManager,
             LocalSpawnRunnerTest::keepLocalEnvUnchanged,
             binTools,
-            new ProcessWrapper(processWrapperPath),
+            new ProcessWrapper(processWrapperPath, /*killDelay=*/ Duration.ZERO),
             Mockito.mock(RunfilesTreeUpdater.class));
 
     Spawn spawn =