From 4d7bb36957b8e905cb8a8c8592982a8361fd1e1d Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Thu, 30 Jul 2026 16:28:25 -0700 Subject: [PATCH 1/2] test(e2e): coordinate sandbox cleanup with creation Signed-off-by: Piotr Mlocek --- e2e/rust/src/harness/sandbox.rs | 70 +++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index 0aeb25038c..6db5e6e451 100644 --- a/e2e/rust/src/harness/sandbox.rs +++ b/e2e/rust/src/harness/sandbox.rs @@ -7,10 +7,11 @@ //! is dropped, replacing the `trap cleanup EXIT` pattern from the bash tests. use std::process::Stdio; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, OnceLock}; use std::time::Duration; use tokio::io::{AsyncBufReadExt, BufReader}; +use tokio::sync::{OwnedRwLockReadGuard, OwnedRwLockWriteGuard, RwLock}; use tokio::time::timeout; use super::binary::openshell_cmd; @@ -31,6 +32,44 @@ fn extract_sandbox_name(output: &str) -> Option { /// startup. const SANDBOX_READY_TIMEOUT: Duration = Duration::from_secs(600); +/// Upper bound for best-effort sandbox deletion during test teardown. +const SANDBOX_CLEANUP_TIMEOUT: Duration = Duration::from_secs(60); + +/// Coordinates sandbox creation and cleanup across the E2E suite. +/// +/// Parallel tests share an external gateway and compute runtime. Without +/// coordination, automatic cleanup from one test can overlap sandbox creation +/// in another test, leaking teardown across test boundaries and exposing +/// backend-specific lifecycle races. +/// +/// Creates take a shared lock so concurrent-create coverage is preserved. +/// Cleanup takes an exclusive lock so lifecycle teardown never overlaps +/// creation in this test harness. This is intentionally not a +/// production-driver lock: serializing a driver would reduce supported +/// lifecycle concurrency. +static E2E_LIFECYCLE_GATE: OnceLock>> = OnceLock::new(); + +async fn create_guard() -> OwnedRwLockReadGuard<()> { + Arc::clone(E2E_LIFECYCLE_GATE.get_or_init(|| Arc::new(RwLock::new(())))) + .read_owned() + .await +} + +async fn cleanup_guard() -> OwnedRwLockWriteGuard<()> { + Arc::clone(E2E_LIFECYCLE_GATE.get_or_init(|| Arc::new(RwLock::new(())))) + .write_owned() + .await +} + +async fn delete_sandbox(name: &str) { + let _lifecycle_guard = cleanup_guard().await; + let mut cmd = openshell_cmd(); + cmd.arg("sandbox").arg("delete").arg(name); + cmd.stdout(Stdio::null()).stderr(Stdio::null()); + + let _ = timeout(SANDBOX_CLEANUP_TIMEOUT, cmd.status()).await; +} + /// RAII guard that deletes a sandbox on drop. /// /// For sandboxes created with `--keep` (long-running background command), the @@ -67,6 +106,7 @@ impl SandboxGuard { /// Returns an error if the CLI exits with a non-zero status or the sandbox /// name cannot be parsed from the output. pub async fn create(args: &[&str]) -> Result { + let _lifecycle_guard = create_guard().await; let mut cmd = openshell_cmd(); cmd.arg("sandbox").arg("create"); for arg in args { @@ -139,6 +179,7 @@ impl SandboxGuard { command: &[&str], ready_marker: &str, ) -> Result { + let _lifecycle_guard = create_guard().await; let mut cmd = openshell_cmd(); cmd.arg("sandbox").arg("create").arg("--keep"); for arg in create_args { @@ -264,6 +305,7 @@ impl SandboxGuard { uploads: &[(&str, &str)], command: &[&str], ) -> Result { + let _lifecycle_guard = create_guard().await; let mut cmd = openshell_cmd(); cmd.arg("sandbox").arg("create"); for (local, dest) in uploads { @@ -511,12 +553,7 @@ impl SandboxGuard { let _ = child.wait().await; } - // Delete the sandbox. - let mut cmd = openshell_cmd(); - cmd.arg("sandbox").arg("delete").arg(&self.name); - cmd.stdout(Stdio::null()).stderr(Stdio::null()); - - let _ = cmd.status().await; + delete_sandbox(&self.name).await; } } @@ -526,14 +563,15 @@ impl Drop for SandboxGuard { return; } - // We need to run async cleanup in a sync Drop. Use block_in_place to - // avoid blocking the tokio runtime. This is acceptable for test code. let name = self.name.clone(); let mut child = self.child.take(); - // Attempt cleanup with a new runtime if we're not inside one, or - // block_in_place if we are. - std::thread::spawn(move || { + // Cleanup uses a separate runtime because Drop cannot await. Join the + // thread so teardown completes before the guard disappears; detaching + // it allowed cleanup to leak into a later test or be terminated when + // the test process exited. + let cleanup_name = name.clone(); + let cleanup = std::thread::spawn(move || { let rt = tokio::runtime::Runtime::new().expect("create cleanup runtime"); rt.block_on(async { if let Some(ref mut child) = child { @@ -541,11 +579,11 @@ impl Drop for SandboxGuard { let _ = child.wait().await; } - let mut cmd = openshell_cmd(); - cmd.arg("sandbox").arg("delete").arg(&name); - cmd.stdout(Stdio::null()).stderr(Stdio::null()); - let _ = cmd.status().await; + delete_sandbox(&name).await; }); }); + if cleanup.join().is_err() { + eprintln!("sandbox cleanup thread panicked for {cleanup_name}"); + } } } From 71f7d2495e87e365e386a48e95d70cf6a2864786 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Thu, 30 Jul 2026 16:39:16 -0700 Subject: [PATCH 2/2] test(e2e): scope lifecycle gate to podman Signed-off-by: Piotr Mlocek --- e2e/rust/src/harness/sandbox.rs | 51 +++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index 6db5e6e451..d9b274a20f 100644 --- a/e2e/rust/src/harness/sandbox.rs +++ b/e2e/rust/src/harness/sandbox.rs @@ -35,30 +35,43 @@ const SANDBOX_READY_TIMEOUT: Duration = Duration::from_secs(600); /// Upper bound for best-effort sandbox deletion during test teardown. const SANDBOX_CLEANUP_TIMEOUT: Duration = Duration::from_secs(60); -/// Coordinates sandbox creation and cleanup across the E2E suite. +/// Coordinates sandbox creation and cleanup in the Podman E2E lane. /// -/// Parallel tests share an external gateway and compute runtime. Without -/// coordination, automatic cleanup from one test can overlap sandbox creation -/// in another test, leaking teardown across test boundaries and exposing -/// backend-specific lifecycle races. +/// A standalone Podman reproducer confirmed that removing a container with an +/// image volume can race with another container attaching the same image. The +/// E2E suite can trigger that Podman issue when automatic cleanup from one test +/// overlaps sandbox creation in another test. /// /// Creates take a shared lock so concurrent-create coverage is preserved. -/// Cleanup takes an exclusive lock so lifecycle teardown never overlaps -/// creation in this test harness. This is intentionally not a -/// production-driver lock: serializing a driver would reduce supported -/// lifecycle concurrency. -static E2E_LIFECYCLE_GATE: OnceLock>> = OnceLock::new(); - -async fn create_guard() -> OwnedRwLockReadGuard<()> { - Arc::clone(E2E_LIFECYCLE_GATE.get_or_init(|| Arc::new(RwLock::new(())))) - .read_owned() - .await +/// Cleanup takes an exclusive lock so Podman image-volume detach never overlaps +/// attach in this test harness. This is intentionally not a production-driver +/// lock: serializing the driver would reduce supported lifecycle concurrency. +static PODMAN_E2E_LIFECYCLE_GATE: OnceLock>> = OnceLock::new(); + +fn is_podman_e2e() -> bool { + std::env::var("OPENSHELL_E2E_DRIVER").as_deref() == Ok("podman") } -async fn cleanup_guard() -> OwnedRwLockWriteGuard<()> { - Arc::clone(E2E_LIFECYCLE_GATE.get_or_init(|| Arc::new(RwLock::new(())))) - .write_owned() - .await +async fn create_guard() -> Option> { + if !is_podman_e2e() { + return None; + } + Some( + Arc::clone(PODMAN_E2E_LIFECYCLE_GATE.get_or_init(|| Arc::new(RwLock::new(())))) + .read_owned() + .await, + ) +} + +async fn cleanup_guard() -> Option> { + if !is_podman_e2e() { + return None; + } + Some( + Arc::clone(PODMAN_E2E_LIFECYCLE_GATE.get_or_init(|| Arc::new(RwLock::new(())))) + .write_owned() + .await, + ) } async fn delete_sandbox(name: &str) {