From 1ac60aeea04b325d6d5163ced59223f5cdea3b0c Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sun, 27 Sep 2026 17:37:16 +0100 Subject: [PATCH 1/2] fix(e2e): stop sandbox leaks from async Drop cleanup Closes #2922 SandboxGuard::Drop spawned a detached thread to delete the sandbox. The thread got killed with the test process before the delete finished. Switch to a blocking command in Drop, like ManagedCleanup already does. Also wrap two tests' manual cleanup in RAII guards so a panic does not leak a sandbox. Signed-off-by: Eric Curtin --- e2e/rust/src/harness/sandbox.rs | 37 +++++++++++++---------------- e2e/rust/tests/sandbox_labels.rs | 22 ++++++----------- e2e/rust/tests/sandbox_lifecycle.rs | 8 +++---- 3 files changed, 27 insertions(+), 40 deletions(-) diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index 0d742792e3..d8d8dd87f8 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. @@ -723,27 +723,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..9004e06980 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,18 +116,23 @@ async fn sandbox_labels_are_stored_and_filterable() { let prod_frontend = format!("lbl-pf-{suffix}"); let dev_data = format!("lbl-dd-{suffix}"); - // Create sandboxes with different labels + // Create sandboxes with different labels. Each guard deletes its sandbox + // on drop, even if a later assertion panics. let name1 = create_sandbox_with_labels(&dev_backend, &[("env", "dev"), ("team", "backend")]).await; + let _cleanup1 = SandboxGuard::manage_existing(name1.clone()); let name2 = create_sandbox_with_labels(&staging_backend, &[("env", "staging"), ("team", "backend")]) .await; + let _cleanup2 = SandboxGuard::manage_existing(name2.clone()); let name3 = create_sandbox_with_labels(&prod_frontend, &[("env", "prod"), ("team", "frontend")]).await; + let _cleanup3 = SandboxGuard::manage_existing(name3.clone()); let name4 = create_sandbox_with_labels(&dev_data, &[("env", "dev"), ("team", "data")]).await; + let _cleanup4 = SandboxGuard::manage_existing(name4.clone()); // Test 1: Verify labels are stored in sandbox metadata let details = get_sandbox_details(&name1).await; @@ -228,10 +226,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..777c9ceff0 100644 --- a/e2e/rust/tests/sandbox_lifecycle.rs +++ b/e2e/rust/tests/sandbox_lifecycle.rs @@ -659,9 +659,10 @@ async fn canonical_main_exit_zero_completes_persistent_sandbox() { ); let sandbox_name = extract_sandbox_name(&combined).expect("sandbox name should be present in output"); + // Deletes the sandbox on drop, including on panic from the asserts below. + let _cleanup = SandboxGuard::manage_existing(sandbox_name.clone()); 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,8 +688,6 @@ 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] @@ -721,6 +720,8 @@ async fn canonical_main_nonzero_exit_preserves_status() { ); let sandbox_name = extract_sandbox_name(&combined).expect("sandbox name should be present in output"); + // Deletes the sandbox on drop, including on panic from the asserts below. + let _cleanup = SandboxGuard::manage_existing(sandbox_name.clone()); let mut get_cmd = openshell_cmd(); get_cmd @@ -741,7 +742,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] From 3a2245ba647c841beab460ba1cf50f4ff0aa0b56 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Mon, 28 Sep 2026 22:07:00 +0100 Subject: [PATCH 2/2] test(e2e): arm sandbox guards before create Address review: install guards with explicit names first. Signed-off-by: Eric Curtin --- e2e/rust/src/harness/sandbox.rs | 15 ++++++++++----- e2e/rust/tests/sandbox_labels.rs | 13 +++++++------ e2e/rust/tests/sandbox_lifecycle.rs | 30 +++++++++++++++++++---------- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index d8d8dd87f8..9922db02b4 100644 --- a/e2e/rust/src/harness/sandbox.rs +++ b/e2e/rust/src/harness/sandbox.rs @@ -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()); } } diff --git a/e2e/rust/tests/sandbox_labels.rs b/e2e/rust/tests/sandbox_labels.rs index 9004e06980..937171ead3 100644 --- a/e2e/rust/tests/sandbox_labels.rs +++ b/e2e/rust/tests/sandbox_labels.rs @@ -116,23 +116,24 @@ async fn sandbox_labels_are_stored_and_filterable() { let prod_frontend = format!("lbl-pf-{suffix}"); let dev_data = format!("lbl-dd-{suffix}"); - // Create sandboxes with different labels. Each guard deletes its sandbox - // on drop, even if a later assertion panics. + // 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; - let _cleanup1 = SandboxGuard::manage_existing(name1.clone()); let name2 = create_sandbox_with_labels(&staging_backend, &[("env", "staging"), ("team", "backend")]) .await; - let _cleanup2 = SandboxGuard::manage_existing(name2.clone()); let name3 = create_sandbox_with_labels(&prod_frontend, &[("env", "prod"), ("team", "frontend")]).await; - let _cleanup3 = SandboxGuard::manage_existing(name3.clone()); let name4 = create_sandbox_with_labels(&dev_data, &[("env", "dev"), ("team", "data")]).await; - let _cleanup4 = SandboxGuard::manage_existing(name4.clone()); // Test 1: Verify labels are stored in sandbox metadata let details = get_sandbox_details(&name1).await; diff --git a/e2e/rust/tests/sandbox_lifecycle.rs b/e2e/rust/tests/sandbox_lifecycle.rs index 777c9ceff0..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,10 +669,6 @@ 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"); - // Deletes the sandbox on drop, including on panic from the asserts below. - let _cleanup = SandboxGuard::manage_existing(sandbox_name.clone()); if let Err(last_sandbox_list) = assert_sandbox_presence_eventually(&sandbox_name, true).await { panic!( @@ -693,9 +701,15 @@ async fn canonical_main_exit_zero_completes_persistent_sandbox() { #[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", @@ -718,10 +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"); - // Deletes the sandbox on drop, including on panic from the asserts below. - let _cleanup = SandboxGuard::manage_existing(sandbox_name.clone()); let mut get_cmd = openshell_cmd(); get_cmd