Fix crashes on interrupt due to concurrent sandboxfs requests.
When we receive an interrupt, we may still have an ongoing request to
sandboxfs to map a sandbox. In that case, attempting to create a
request to destroy the sandbox will make us crash due to the
sanity-checks we have in RealSandboxfs02Process (and the request will
fail anyway if it reaches sandboxfs).
Prevent this by tracking whether the sandbox needs cleanup and only
issuing the deletion requests if the creation succeeded.
Tested: I've tried to write a test for this but it's very difficult.
To test this condition, we need to send an interrupt while we have a
build with a sandbox creation request in-flight. But to catch the
latter, we need the requests to be very big. I've simulated this
behavior by adding a sleep in RealSandboxfs02Process#createSandbox
and killing Bazel during a build. Would reproduce the crashes
instantly before the fix and haven't been able to after this change.
RELNOTES: None.
PiperOrigin-RevId: 294974301
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
index af31555..6dbe31b 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
@@ -93,6 +93,9 @@
*/
private final String sandboxName;
+ /** Flag to track whether the sandbox needs to be unmapped. */
+ private boolean sandboxIsMapped;
+
@Nullable private final Path statisticsPath;
/**
@@ -143,6 +146,7 @@
int id = lastId.getAndIncrement();
this.sandboxName = "" + id;
+ this.sandboxIsMapped = false;
this.execRoot = process.getMountPoint().getRelative(this.sandboxName);
this.statisticsPath = statisticsPath;
}
@@ -183,6 +187,7 @@
}
process.createSandbox(sandboxName, mappings);
+ sandboxIsMapped = true;
}
@Override
@@ -195,13 +200,19 @@
@Override
public void delete() {
- try {
- process.destroySandbox(sandboxName);
- } catch (IOException e) {
- // We use independent subdirectories for each action, so a failure to unmap one, while
- // annoying, is not a big deal. The sandboxfs instance will be unmounted anyway after
- // the build, which will cause these to go away anyway.
- log.warning("Cannot unmap " + sandboxName + ": " + e);
+ // We can only ask sandboxfs to unmap a sandbox if we successfully finished creating it.
+ // Otherwise, the request may fail, or we may fail our own sanity-checks that validate the
+ // lifecycle of the sandboxes.
+ if (sandboxIsMapped) {
+ try {
+ process.destroySandbox(sandboxName);
+ } catch (IOException e) {
+ // We use independent subdirectories for each action, so a failure to unmap one, while
+ // annoying, is not a big deal. The sandboxfs instance will be unmounted anyway after
+ // the build, which will cause these to go away anyway.
+ log.warning("Cannot unmap " + sandboxName + ": " + e);
+ }
+ sandboxIsMapped = false;
}
try {
