diff --git a/bazel/rules/testing/bwrap/src/lib.rs b/bazel/rules/testing/bwrap/src/lib.rs index d3ab8672ee..429a26ca0a 100644 --- a/bazel/rules/testing/bwrap/src/lib.rs +++ b/bazel/rules/testing/bwrap/src/lib.rs @@ -11,9 +11,11 @@ use std::os::fd::AsRawFd; use std::os::unix::fs::PermissionsExt; use std::os::unix::net::UnixStream; use std::os::unix::process::CommandExt; +use std::os::unix::process::ExitStatusExt; use std::path::Path; use std::path::PathBuf; use std::process::Command as StdCommand; +use std::process::ExitStatus; use std::process::Stdio; use std::time::Duration; use std::time::Instant; @@ -61,6 +63,12 @@ struct BwrapRuntimePaths { bwrap: PathBuf, } +#[derive(Clone, Copy)] +enum LauncherExitPolicy { + RequireSuccess, + AllowTeardownKill, +} + impl BwrapTestCommand { /// Creates an isolated bwrap command for `executable`. pub fn new(executable: impl Into) -> Self { @@ -337,14 +345,16 @@ impl BwrapProcesses { .sandbox_init .start_kill() .context("kill bwrap PID namespace init"); - let (mut check_exit_status, status_check_result) = match child_status { - Ok(Some(_)) => (true, Ok(())), - Ok(None) => (false, Ok(())), - Err(error) => (false, Err(error).context("check bwrap process status")), + let (exit_policy, status_check_result) = match child_status { + Ok(Some(_)) => (LauncherExitPolicy::RequireSuccess, Ok(())), + Ok(None) => (LauncherExitPolicy::AllowTeardownKill, Ok(())), + Err(error) => ( + LauncherExitPolicy::RequireSuccess, + Err(error).context("check bwrap process status"), + ), }; let mut fallback_started = false; let early_fallback_result = if sandbox_kill_result.is_err() { - check_exit_status = false; fallback_started = true; self.start_kill_launcher() } else { @@ -356,7 +366,6 @@ impl BwrapProcesses { .await .context("wait for bwrap PID namespace init"); let late_fallback_result = if sandbox_wait_result.is_err() && !fallback_started { - check_exit_status = false; self.start_kill_launcher() } else { Ok(()) @@ -373,7 +382,6 @@ impl BwrapProcesses { launcher_timeout_result = Err(anyhow::anyhow!( "timed out waiting for bwrap launcher after {BWRAP_CLEANUP_TIMEOUT:?}" )); - check_exit_status = false; timeout_kill_result = self.start_kill_launcher(); match tokio::time::timeout(BWRAP_CLEANUP_TIMEOUT, self.child.wait()).await { Ok(result) => result.context("wait for killed bwrap launcher"), @@ -383,13 +391,7 @@ impl BwrapProcesses { } } } - .and_then(|status| { - anyhow::ensure!( - !check_exit_status || status.success(), - "bwrap process exited with {status}" - ); - Ok(()) - }); + .and_then(|status| ensure_launcher_exit_status(status, exit_policy)); // Every cleanup action has been attempted, so an individual error // should not cause the blocking fallback to repeat them. @@ -482,6 +484,18 @@ impl Drop for BwrapProcesses { } } +fn ensure_launcher_exit_status(status: ExitStatus, policy: LauncherExitPolicy) -> Result<()> { + // bwrap normalizes a SIGKILL of its PID-namespace init to 128 + SIGKILL. + // Killing the launcher directly leaves it signal-terminated instead. + let expected_teardown_kill = matches!(policy, LauncherExitPolicy::AllowTeardownKill) + && (status.code() == Some(128 + libc::SIGKILL) || status.signal() == Some(libc::SIGKILL)); + anyhow::ensure!( + status.success() || expected_teardown_kill, + "bwrap process exited with {status}" + ); + Ok(()) +} + fn log_panic_cleanup(args: std::fmt::Arguments<'_>) { let _ = writeln!(std::io::stderr().lock(), "{args}"); } diff --git a/bazel/rules/testing/bwrap/src/lib_tests.rs b/bazel/rules/testing/bwrap/src/lib_tests.rs index ede7d816e8..7e217ea819 100644 --- a/bazel/rules/testing/bwrap/src/lib_tests.rs +++ b/bazel/rules/testing/bwrap/src/lib_tests.rs @@ -5,9 +5,11 @@ use std::future::Future; use std::os::fd::AsRawFd; use std::os::fd::FromRawFd; use std::os::fd::OwnedFd; +use std::os::unix::process::ExitStatusExt; use std::panic::AssertUnwindSafe; use std::path::Path; use std::path::PathBuf; +use std::process::ExitStatus; use anyhow::Context; use anyhow::Result; @@ -23,6 +25,8 @@ use tokio::time::timeout; use super::BwrapTestCommand; use super::BwrapTestProcess; +use super::LauncherExitPolicy; +use super::ensure_launcher_exit_status; fn smoke_fixture() -> Result { Ok(codex_utils_cargo_bin::cargo_bin("bwrap-smoke")?) @@ -103,6 +107,53 @@ fn namespace_identity(namespace: &str) -> Result { .into_owned()) } +fn exit_status(code: i32) -> ExitStatus { + ExitStatus::from_raw(code << 8) +} + +fn signal_status(signal: i32) -> ExitStatus { + ExitStatus::from_raw(signal) +} + +#[test] +fn launcher_exit_status_distinguishes_payload_failures_from_teardown() { + let actual = [ + ensure_launcher_exit_status(exit_status(0), LauncherExitPolicy::RequireSuccess).is_ok(), + ensure_launcher_exit_status(exit_status(73), LauncherExitPolicy::RequireSuccess).is_ok(), + ensure_launcher_exit_status( + exit_status(128 + libc::SIGKILL), + LauncherExitPolicy::RequireSuccess, + ) + .is_ok(), + ensure_launcher_exit_status( + signal_status(libc::SIGKILL), + LauncherExitPolicy::RequireSuccess, + ) + .is_ok(), + ensure_launcher_exit_status(exit_status(0), LauncherExitPolicy::AllowTeardownKill).is_ok(), + ensure_launcher_exit_status(exit_status(73), LauncherExitPolicy::AllowTeardownKill).is_ok(), + ensure_launcher_exit_status( + exit_status(128 + libc::SIGKILL), + LauncherExitPolicy::AllowTeardownKill, + ) + .is_ok(), + ensure_launcher_exit_status( + signal_status(libc::SIGKILL), + LauncherExitPolicy::AllowTeardownKill, + ) + .is_ok(), + ensure_launcher_exit_status( + signal_status(libc::SIGTERM), + LauncherExitPolicy::AllowTeardownKill, + ) + .is_ok(), + ]; + assert_eq!( + actual, + [true, false, false, false, true, false, true, true, false] + ); +} + #[tokio::test] async fn dropping_without_teardown_panics_and_cleans_up() -> Result<()> { let process = waiting_smoke_process().await?;