Prevent "Deleting stale sandbox" messages when things go right.
Make Bazel clean up the sandbox base's top directory after the end of each
build and during server shutdown to prevent this directory from staying
behind.
This directory is only supposed to stay behind when --sandbox_debug is
used, and the startup message is intended to tell the user that a heavy
operation is about to happen -- but we should not print it unnecessarily.
This was the original intent when this message was added but for some
reason I missed this obvious case. Added a test now.
Fixes #8525.
RELNOTES: None.
PiperOrigin-RevId: 315247251
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
index cb96021..16ce5ba 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
@@ -20,6 +20,7 @@
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.Subscribe;
+import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
@@ -66,6 +67,8 @@
*/
public final class SandboxModule extends BlazeModule {
+ private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+
/** Tracks whether we are issuing the very first build within this Bazel server instance. */
private static boolean firstBuild = true;
@@ -524,6 +527,27 @@
unmountSandboxfs();
}
+ /**
+ * Best-effort cleanup of the sandbox base assuming all per-spawn contents have been removed.
+ *
+ * <p>When this gets called, the individual trees of each spawn should have been cleaned up but we
+ * may be left with the top-level subdirectories used by each sandboxed spawn runner (e.g. {@code
+ * darwin-sandbox}) and the sandbox base itself. Try to delete those so that a Bazel server
+ * restart doesn't print a spurious {@code Deleting stale sandbox base} message.
+ */
+ private static void cleanupSandboxBaseTop(Path sandboxBase) {
+ try {
+ // This might be called twice for a given sandbox base, so don't bother recording error
+ // messages if any of the files we try to delete don't exist.
+ for (Path leftover : sandboxBase.getDirectoryEntries()) {
+ leftover.delete();
+ }
+ sandboxBase.delete();
+ } catch (IOException e) {
+ logger.atWarning().withCause(e).log("Failed to clean up sandbox base %s", sandboxBase);
+ }
+ }
+
@Override
public void afterCommand() {
checkNotNull(env, "env not initialized; was beforeCommand called?");
@@ -555,7 +579,11 @@
sandboxfsProcess == null,
"sandboxfs instance should have been shut down at this "
+ "point; were the buildComplete/buildInterrupted events sent?");
- sandboxBase = null;
+
+ cleanupSandboxBaseTop(sandboxBase);
+ // We intentionally keep sandboxBase around, without resetting it to null, in case we have
+ // asynchronous deletions going on. In that case, we'd still want to retry this during
+ // shutdown.
}
spawnRunners.clear();
@@ -577,6 +605,10 @@
treeDeleter = null; // Avoid potential reexecution if we crash.
}
}
+
+ if (sandboxBase != null) {
+ cleanupSandboxBaseTop(sandboxBase);
+ }
}
@Override
diff --git a/src/test/shell/integration/sandboxing_test.sh b/src/test/shell/integration/sandboxing_test.sh
index 433df86..3b106c9 100755
--- a/src/test/shell/integration/sandboxing_test.sh
+++ b/src/test/shell/integration/sandboxing_test.sh
@@ -41,7 +41,8 @@
local output_base="$(bazel info output_base)"
do_build() {
- bazel build --genrule_strategy=sandboxed "${extra_args[@]}" //pkg
+ bazel build --genrule_strategy=sandboxed --sandbox_debug \
+ "${extra_args[@]}" //pkg
}
do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
@@ -127,7 +128,7 @@
local output_base="$(bazel info output_base)"
do_build() {
- bazel build --genrule_strategy=sandboxed //pkg
+ bazel build --genrule_strategy=sandboxed --sandbox_debug //pkg
}
do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
@@ -145,6 +146,32 @@
do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
}
+function test_sandbox_base_top_is_removed() {
+ mkdir pkg
+ cat >pkg/BUILD <<EOF
+genrule(name = "pkg", outs = ["pkg.out"], cmd = "echo >\$@")
+EOF
+
+ local output_base="$(bazel info output_base)"
+
+ do_build() {
+ bazel build --genrule_strategy=sandboxed //pkg
+ }
+
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ find "${output_base}" >>"${TEST_log}" 2>&1 || true
+
+ local sandbox_base="${output_base}/sandbox"
+ [[ ! -d "${sandbox_base}" ]] \
+ || fail "${sandbox_base} left behind unnecessarily"
+
+ # Restart Bazel and check we don't print spurious "Deleting stale sandbox"
+ # warnings.
+ bazel shutdown
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ expect_not_log "Deleting stale sandbox"
+}
+
function test_sandbox_old_contents_not_reused_in_consecutive_builds() {
mkdir pkg
cat >pkg/BUILD <<EOF
