diff --git a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java index bf11c681a6a6ef..9798e9616b292c 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java @@ -32,6 +32,11 @@ final class SandboxedWorker extends SingleplexWorker { workerExecRoot = new WorkerExecRoot(workDir); } + @Override + public boolean isSandboxed() { + return true; + } + @Override public void prepareExecution( SandboxInputs inputFiles, SandboxOutputs outputs, Set workerFiles) diff --git a/src/main/java/com/google/devtools/build/lib/worker/SingleplexWorker.java b/src/main/java/com/google/devtools/build/lib/worker/SingleplexWorker.java index 3edce2c3227b6f..3412dc33013b09 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/SingleplexWorker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/SingleplexWorker.java @@ -92,6 +92,11 @@ Subprocess createProcess() throws IOException { return processBuilder.start(); } + @Override + public boolean isSandboxed() { + return false; + } + @Override public void prepareExecution( SandboxInputs inputFiles, SandboxOutputs outputs, Set workerFiles) diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java index defc5be2764dfa..ce2447e720c287 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java @@ -66,6 +66,9 @@ SortedMap getWorkerFilesWithHashes() { return workerKey.getWorkerFilesWithHashes(); } + /** Returns true if this worker is sandboxed. */ + public abstract boolean isSandboxed(); + /** * Sets the reporter this {@code Worker} should report anomalous events to, or clears it. We * expect the reporter to be cleared at end of build. 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 f05b8fe31c6296..1b3d25ab3767cf 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 @@ -99,11 +99,9 @@ public PooledObject wrap(Worker worker) { return new DefaultPooledObject<>(worker); } - /** - * When a worker process is discarded, destroy its process, too. - */ + /** When a worker process is discarded, destroy its process, too. */ @Override - public void destroyObject(WorkerKey key, PooledObject p) throws Exception { + public void destroyObject(WorkerKey key, PooledObject p) { if (workerOptions.workerVerbose) { int workerId = p.getObject().getWorkerId(); reporter.handle( diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerProxy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerProxy.java index 7a508f829854c3..0b932393b87055 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerProxy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerProxy.java @@ -54,6 +54,11 @@ final class WorkerProxy extends Worker { Runtime.getRuntime().addShutdownHook(shutdownHook); } + @Override + public boolean isSandboxed() { + return false; + } + @Override void setReporter(EventHandler reporter) { // We might have created this multiplexer after setting the reporter for existing multiplexers diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 2682aab81c563c..ca6a3b0d497ae2 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -457,12 +457,27 @@ WorkResponse execInWorker( try { response = worker.getResponse(request.getRequestId()); } catch (InterruptedException e) { - finishWorkAsync( - key, - worker, - request, - workerOptions.workerCancellation && Spawns.supportsWorkerCancellation(spawn)); - worker = null; + if (worker.isSandboxed()) { + // Sandboxed workers can safely finish their work async. + finishWorkAsync( + key, + worker, + request, + workerOptions.workerCancellation && Spawns.supportsWorkerCancellation(spawn)); + worker = null; + } else if (!key.isSpeculative()) { + // Non-sandboxed workers interrupted outside of dynamic execution can only mean that + // the user interrupted the build, and we don't want to delay finishing. Instead we + // kill the worker. + // Technically, workers are always sandboxed under dynamic execution, at least for now. + try { + workers.invalidateObject(key, worker); + } catch (IOException e1) { + // Nothing useful we can do here, in fact it may not be possible to get here. + } finally { + worker = null; + } + } throw e; } catch (IOException e) { restoreInterrupt(e); diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index 8991d27760758d..fcdfea1654e8b1 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -179,8 +179,10 @@ public void testExecInWorker_sendsCancelMessageOnInterrupt() throws ExecException, InterruptedException, IOException { WorkerOptions workerOptions = new WorkerOptions(); workerOptions.workerCancellation = true; + workerOptions.workerSandboxing = true; when(spawn.getExecutionInfo()) .thenReturn(ImmutableMap.of(ExecutionRequirements.SUPPORTS_WORKER_CANCELLATION, "1")); + when(worker.isSandboxed()).thenReturn(true); WorkerSpawnRunner runner = new WorkerSpawnRunner( new SandboxHelpers(false), @@ -196,6 +198,7 @@ public void testExecInWorker_sendsCancelMessageOnInterrupt() WorkerKey key = createWorkerKey(fs, "mnem", false); Path logFile = fs.getPath("/worker.log"); Semaphore secondResponseRequested = new Semaphore(0); + // Fake that the getting the regular response gets interrupted and we then answer the cancel. when(worker.getResponse(anyInt())) .thenThrow(new InterruptedException()) .thenAnswer( @@ -229,6 +232,53 @@ public void testExecInWorker_sendsCancelMessageOnInterrupt() .isEqualTo(WorkRequest.newBuilder().setRequestId(0).setCancel(true).build()); } + @Test + public void testExecInWorker_unsandboxedDiesOnInterrupt() + throws InterruptedException, IOException { + WorkerOptions workerOptions = new WorkerOptions(); + workerOptions.workerCancellation = true; + workerOptions.workerSandboxing = false; + when(spawn.getExecutionInfo()) + .thenReturn(ImmutableMap.of(ExecutionRequirements.SUPPORTS_WORKER_CANCELLATION, "1")); + WorkerSpawnRunner runner = + new WorkerSpawnRunner( + new SandboxHelpers(false), + fs.getPath("/execRoot"), + createWorkerPool(), + /* multiplex */ false, + reporter, + localEnvProvider, + /* binTools */ null, + resourceManager, + /* runfilesTreeUpdater=*/ null, + workerOptions); + WorkerKey key = createWorkerKey(fs, "mnem", false); + Path logFile = fs.getPath("/worker.log"); + when(worker.getResponse(anyInt())).thenThrow(new InterruptedException()); + + // Since this worker is not sandboxed, it will just get killed on interrupt. + assertThrows( + InterruptedException.class, + () -> + runner.execInWorker( + spawn, + key, + context, + new SandboxInputs(ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of()), + SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), + ImmutableList.of(), + inputFileCache, + spawnMetrics)); + + assertThat(logFile.exists()).isFalse(); + verify(context, times(1)).report(ProgressStatus.EXECUTING, "worker"); + ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(WorkRequest.class); + verify(worker, times(1)).putRequest(argumentCaptor.capture()); + assertThat(argumentCaptor.getAllValues().get(0)) + .isEqualTo(WorkRequest.newBuilder().setRequestId(0).build()); + verify(worker, times(1)).destroy(); + } + @Test public void testExecInWorker_noMultiplexWithDynamic() throws ExecException, InterruptedException, IOException {