diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index 0d742792e3..9922db02b4 100644 --- a/e2e/rust/src/harness/sandbox.rs +++ b/e2e/rust/src/harness/sandbox.rs @@ -14,7 +14,7 @@ use std::time::Duration; use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::time::timeout; -use super::binary::openshell_cmd; +use super::binary::{openshell_bin, openshell_cmd}; use super::output::{extract_field, strip_ansi}; /// Tool-capable workload image used by the E2E harness. @@ -67,13 +67,18 @@ fn add_test_image_if_missing(command: &mut tokio::process::Command, args: &[&str } } +/// Generate a sandbox name that is unique within and across test processes. +pub fn unique_sandbox_name() -> String { + format!( + "e2e-{}-{}", + std::process::id(), + NEXT_SANDBOX_NAME.fetch_add(1, Ordering::Relaxed) + ) +} + fn add_unique_name_if_missing(command: &mut tokio::process::Command, args: &[&str]) { if !has_explicit_sandbox_name(args) { - command.arg("--name").arg(format!( - "e2e-{}-{}", - std::process::id(), - NEXT_SANDBOX_NAME.fetch_add(1, Ordering::Relaxed) - )); + command.arg("--name").arg(unique_sandbox_name()); } } @@ -723,27 +728,22 @@ 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 || { - let rt = tokio::runtime::Runtime::new().expect("create cleanup runtime"); - rt.block_on(async { - if let Some(ref mut child) = child { - let _: Result<(), _> = child.kill().await; - let _ = child.wait().await; - } + // A detached thread here would get killed along with the test + // process before the delete command finishes, leaking the sandbox. + // Use a blocking std::process::Command instead, matching the + // ManagedCleanup pattern in workspace_namespace_managed.rs, so + // cleanup completes before this function returns. + if let Some(mut child) = self.child.take() { + let _ = child.start_kill(); + } - let mut cmd = openshell_cmd(); - cmd.arg("sandbox").arg("delete").arg(&name); - cmd.stdout(Stdio::null()).stderr(Stdio::null()); - let _ = cmd.status().await; - }); - }); + let _ = std::process::Command::new(openshell_bin()) + .arg("sandbox") + .arg("delete") + .arg(&self.name) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); } } diff --git a/e2e/rust/tests/sandbox_labels.rs b/e2e/rust/tests/sandbox_labels.rs index 90351266c5..937171ead3 100644 --- a/e2e/rust/tests/sandbox_labels.rs +++ b/e2e/rust/tests/sandbox_labels.rs @@ -5,6 +5,7 @@ use std::process::Stdio; use openshell_e2e::harness::binary::openshell_cmd; use openshell_e2e::harness::output::{extract_field, strip_ansi}; +use openshell_e2e::harness::sandbox::SandboxGuard; fn normalize_output(output: &str) -> String { let stripped = strip_ansi(output).replace('\r', ""); @@ -104,14 +105,6 @@ async fn get_sandbox_details(name: &str) -> String { combined } -async fn delete_sandbox(name: &str) { - let mut cmd = openshell_cmd(); - cmd.args(["sandbox", "delete", name]) - .stdout(Stdio::null()) - .stderr(Stdio::null()); - let _ = cmd.status().await; -} - #[tokio::test] #[allow(clippy::too_many_lines)] // end-to-end test exercises full label lifecycle async fn sandbox_labels_are_stored_and_filterable() { @@ -123,6 +116,12 @@ async fn sandbox_labels_are_stored_and_filterable() { let prod_frontend = format!("lbl-pf-{suffix}"); let dev_data = format!("lbl-dd-{suffix}"); + // Arm guards before create so a partial create still cleans up. + let _cleanup: Vec<_> = [&dev_backend, &staging_backend, &prod_frontend, &dev_data] + .into_iter() + .map(|name| SandboxGuard::manage_existing(name.clone())) + .collect(); + // Create sandboxes with different labels let name1 = create_sandbox_with_labels(&dev_backend, &[("env", "dev"), ("team", "backend")]).await; @@ -228,10 +227,4 @@ async fn sandbox_labels_are_stored_and_filterable() { all_sandboxes.contains(&name4), "list without filter should include all test sandboxes" ); - - // Cleanup - delete_sandbox(&name1).await; - delete_sandbox(&name2).await; - delete_sandbox(&name3).await; - delete_sandbox(&name4).await; } diff --git a/e2e/rust/tests/sandbox_lifecycle.rs b/e2e/rust/tests/sandbox_lifecycle.rs index 4df9f24eba..7f5a30b2ee 100644 --- a/e2e/rust/tests/sandbox_lifecycle.rs +++ b/e2e/rust/tests/sandbox_lifecycle.rs @@ -11,7 +11,7 @@ use std::time::Duration; use openshell_e2e::harness::binary::{openshell_cmd, openshell_tty_cmd}; use openshell_e2e::harness::cli::{run_cli, wait_for_sandbox_phase}; use openshell_e2e::harness::output::{extract_field, strip_ansi}; -use openshell_e2e::harness::sandbox::SandboxGuard; +use openshell_e2e::harness::sandbox::{SandboxGuard, unique_sandbox_name}; use serial_test::serial; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::time::{Instant, sleep}; @@ -644,7 +644,19 @@ async fn sandbox_can_be_deleted_while_stopped() { #[tokio::test] #[serial(sandbox_lifecycle)] async fn canonical_main_exit_zero_completes_persistent_sandbox() { - let mut cmd = openshell_tty_cmd(&["sandbox", "create", "--", "echo", "OK"]); + // Armed before create so a failed create or parse still cleans up. + let sandbox_name = unique_sandbox_name(); + let _cleanup = SandboxGuard::manage_existing(sandbox_name.clone()); + + let mut cmd = openshell_tty_cmd(&[ + "sandbox", + "create", + "--name", + &sandbox_name, + "--", + "echo", + "OK", + ]); cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); let output = cmd.output().await.expect("spawn openshell sandbox create"); @@ -657,11 +669,8 @@ async fn canonical_main_exit_zero_completes_persistent_sandbox() { combined.contains("OK"), "main output was not streamed:\n{combined}" ); - let sandbox_name = - extract_sandbox_name(&combined).expect("sandbox name should be present in output"); if let Err(last_sandbox_list) = assert_sandbox_presence_eventually(&sandbox_name, true).await { - delete_sandbox(&sandbox_name).await; panic!( "sandbox {sandbox_name} should still exist by default after {SANDBOX_PRESENCE_TIMEOUT:?}; \ last observed sandbox list: {last_sandbox_list:?}" @@ -687,16 +696,20 @@ async fn canonical_main_exit_zero_completes_persistent_sandbox() { details.contains("Phase: Completed"), "expected terminal sandbox phase:\n{details}" ); - - delete_sandbox(&sandbox_name).await; } #[tokio::test] #[serial(sandbox_lifecycle)] async fn canonical_main_nonzero_exit_preserves_status() { + // Armed before create so a failed create or parse still cleans up. + let sandbox_name = unique_sandbox_name(); + let _cleanup = SandboxGuard::manage_existing(sandbox_name.clone()); + let mut cmd = openshell_tty_cmd(&[ "sandbox", "create", + "--name", + &sandbox_name, "--", "sh", "-c", @@ -719,8 +732,6 @@ async fn canonical_main_nonzero_exit_preserves_status() { combined.contains("failed-main"), "main output was not streamed:\n{combined}" ); - let sandbox_name = - extract_sandbox_name(&combined).expect("sandbox name should be present in output"); let mut get_cmd = openshell_cmd(); get_cmd @@ -741,7 +752,6 @@ async fn canonical_main_nonzero_exit_preserves_status() { details.contains("Exit Code: 7"), "missing exit code:\n{details}" ); - delete_sandbox(&sandbox_name).await; } #[tokio::test]