Only allow worker async finishing when sandboxed.
All workers under dynamic execution are sandboxed, but on user interrupt non-sandboxed workers might keep working even after another invocation has begun, which could cause issues. Instead, we kill off interrupted workers that are neither sandboxed nor under
dynamic execution.
RELNOTES: None.
PiperOrigin-RevId: 373141238
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 bf11c68..9798e96 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
@@ -33,6 +33,11 @@
}
@Override
+ public boolean isSandboxed() {
+ return true;
+ }
+
+ @Override
public void prepareExecution(
SandboxInputs inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
throws IOException {
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 3edce2c..3412dc3 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
@@ -93,6 +93,11 @@
}
@Override
+ public boolean isSandboxed() {
+ return false;
+ }
+
+ @Override
public void prepareExecution(
SandboxInputs inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
throws IOException {
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 defc5be..ce2447e 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 @@
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 f05b8fe..1b3d25a 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 @@
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<Worker> p) throws Exception {
+ public void destroyObject(WorkerKey key, PooledObject<Worker> 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 7a508f8..0b93239 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
@@ -55,6 +55,11 @@
}
@Override
+ public boolean isSandboxed() {
+ return false;
+ }
+
+ @Override
void setReporter(EventHandler reporter) {
// We might have created this multiplexer after setting the reporter for existing multiplexers
workerMultiplexer.setReporter(reporter);
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 2682aab..ca6a3b0 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 @@
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 8991d27..fcdfea1 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 @@
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 @@
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(
@@ -230,6 +233,53 @@
}
@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<WorkRequest> 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 {
WorkerOptions workerOptions = new WorkerOptions();
