From 3b8d712edb52e332d466a1e21c0d652c32000474 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 14 Sep 2026 11:01:24 -0600 Subject: [PATCH] Delete a live Daytona test's sandbox even when the test panics The live Daytona tests create a provider sandbox and delete it on their last line, so any panic or failed assertion before that line leaks a running, billed sandbox. Two leaked that way on 2026-09-14 when a sandbox-driver decoder flake panicked daytona_playwright_mcp_sandbox_transport. Add fabro_sandbox::test_support::DeletedOnDrop, a guard that owns the RunSandbox (Deref keeps the tests reading unchanged), offers an explicit delete(self) for the happy path, and deletes from Drop otherwise. The drop-time delete runs on its own thread and runtime because the test's runtime may be unwinding. It reconnects the provider through the ProviderAccess the test built the sandbox with, because the sandbox's own handle pools HTTP connections whose tasks live on the test's runtime; a live check of that path timed out after 10s. Every Daytona test that creates a sandbox now holds it through the guard. Unit tests over the scripted double prove delete-on-drop runs once, an explicit delete runs once, and a panic inside catch_unwind still deletes with and without a runtime. Co-Authored-By: Claude Fable 5.1 --- .../fabro-sandbox/src/test_support.rs | 4 + .../src/test_support/deleted_on_drop.rs | 281 ++++++++++++++++++ .../tests/it/daytona_integration.rs | 44 +-- 3 files changed, 309 insertions(+), 20 deletions(-) create mode 100644 lib/components/fabro-sandbox/src/test_support/deleted_on_drop.rs diff --git a/lib/components/fabro-sandbox/src/test_support.rs b/lib/components/fabro-sandbox/src/test_support.rs index 05c84b16e..56d611627 100644 --- a/lib/components/fabro-sandbox/src/test_support.rs +++ b/lib/components/fabro-sandbox/src/test_support.rs @@ -29,6 +29,10 @@ use crate::driver_sandbox::RunSandbox; use crate::managed_labels::{MANAGED_LABEL, MANAGED_LABEL_VALUE}; use crate::sandbox::SandboxFile; +mod deleted_on_drop; + +pub use deleted_on_drop::DeletedOnDrop; + /// The id a run record carries for a local sandbox at `working_directory`, /// as the Host provider derives it from the canonical path. A record a test /// writes by hand reconnects the way one fabro wrote would. The directory diff --git a/lib/components/fabro-sandbox/src/test_support/deleted_on_drop.rs b/lib/components/fabro-sandbox/src/test_support/deleted_on_drop.rs new file mode 100644 index 000000000..396ed957c --- /dev/null +++ b/lib/components/fabro-sandbox/src/test_support/deleted_on_drop.rs @@ -0,0 +1,281 @@ +//! A sandbox a test deletes even when it fails. +//! +//! A live test that creates a provider sandbox and deletes it on its last +//! line leaks a running (and billed) sandbox whenever it panics or fails an +//! assertion before that line. [`DeletedOnDrop`] owns the sandbox for the +//! test: the happy path still calls `delete` explicitly, and any other exit +//! deletes it from `Drop`. +//! +//! `Drop` is synchronous and may run while the test's runtime is unwinding +//! a panic, so the cleanup never uses that runtime: it spawns a thread with +//! a small runtime of its own and blocks until the delete finishes or a +//! bounded timeout passes. A live provider's handle cannot be driven from +//! that thread either, because its pooled HTTP connections are tasks on the +//! test's runtime, which nobody polls while it unwinds. The guard therefore +//! connects the provider afresh through the [`ProviderAccess`] the test +//! built the sandbox with and deletes the sandbox by id over that new +//! connection. + +use std::fmt; +use std::ops::Deref; +use std::sync::Arc; +use std::time::Duration; + +use fabro_types::SandboxProviderKind; +use tokio::runtime::Builder as RuntimeBuilder; +use tokio::time; + +use crate::driver::ProviderAccess; +use crate::driver_sandbox::RunSandbox; +use crate::error::display_for_log; +use crate::provider_sandbox; + +/// How long a drop-time delete may take before the guard gives up and +/// reports the sandbox as possibly leaked. Daytona's driver bounds each +/// delete call at 10s and may wait out a state change once; a reconnect +/// adds a few seconds of its own. +const DROP_DELETE_TIMEOUT: Duration = Duration::from_secs(90); + +/// A run sandbox that is deleted when the guard drops, unless the test +/// deleted it explicitly through [`DeletedOnDrop::delete`]. +/// +/// Derefs to the [`RunSandbox`] so a test reads the same as before; code +/// that needs a shared handle takes one from [`DeletedOnDrop::shared`]. +pub struct DeletedOnDrop { + sandbox: Arc, + /// Access for a fresh provider connection at drop time. `None` deletes + /// through the handle the sandbox already holds. + access: Option, + deleted: bool, +} + +impl DeletedOnDrop { + /// Guards a sandbox built through `access`, as every live provider test + /// builds one. A drop-time delete reconnects the provider through + /// `access` and deletes the sandbox by id. + pub fn new(sandbox: impl Into>, access: &ProviderAccess) -> Self { + Self { + sandbox: sandbox.into(), + access: Some(access.clone()), + deleted: false, + } + } + + /// Guards a sandbox whose own handle can finish a delete from any + /// thread: the scripted double, whose delete needs no live connection. + /// Not for a live provider, whose handle is bound to the test's runtime + /// (see the module docs). + pub fn through_handle(sandbox: impl Into>) -> Self { + Self { + sandbox: sandbox.into(), + access: None, + deleted: false, + } + } + + /// A shared handle to the sandbox for code that takes an `Arc`, such as + /// a workflow runner or an agent environment. The guard keeps its own + /// and still deletes the sandbox when it drops. + #[must_use] + pub fn shared(&self) -> Arc { + Arc::clone(&self.sandbox) + } + + /// Deletes the sandbox now, returning the driver's result. After a + /// successful delete the drop does nothing; after a failed one it tries + /// once more so a transient failure still leaves nothing behind. + pub async fn delete(mut self) -> crate::Result<()> { + let result = self.sandbox.delete().await; + self.deleted = result.is_ok(); + result + } +} + +impl Deref for DeletedOnDrop { + type Target = RunSandbox; + + fn deref(&self) -> &RunSandbox { + &self.sandbox + } +} + +impl fmt::Debug for DeletedOnDrop { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("DeletedOnDrop") + .field("kind", self.sandbox.kind()) + .field("id", &self.sandbox.sandbox_info()) + .field("deleted", &self.deleted) + .finish_non_exhaustive() + } +} + +impl Drop for DeletedOnDrop { + #[expect( + clippy::print_stderr, + reason = "The guard runs during a failing test; its report has to reach the captured test output." + )] + fn drop(&mut self) { + if self.deleted { + return; + } + let id = self.sandbox.sandbox_info(); + if id.is_empty() { + // Never created on the provider: nothing to delete. + return; + } + let kind = self.sandbox.kind().clone(); + eprintln!("DeletedOnDrop: deleting the {kind} sandbox {id} the test left behind"); + let outcome = delete_on_own_thread( + kind.clone(), + id.clone(), + Arc::clone(&self.sandbox), + self.access.clone(), + ); + match outcome { + Ok(()) => eprintln!("DeletedOnDrop: deleted the {kind} sandbox {id}"), + Err(error) => { + eprintln!("DeletedOnDrop: the {kind} sandbox {id} may be leaked: {error}"); + } + } + } +} + +/// Runs the delete to completion on a dedicated thread with its own +/// runtime, bounded by [`DROP_DELETE_TIMEOUT`]. +#[expect( + clippy::disallowed_methods, + reason = "Drop is synchronous and the test's runtime may be unwinding; the delete needs a thread and runtime of its own." +)] +fn delete_on_own_thread( + kind: SandboxProviderKind, + id: String, + sandbox: Arc, + access: Option, +) -> Result<(), String> { + let thread = std::thread::Builder::new() + .name("sandbox-delete-on-drop".to_string()) + .spawn(move || -> Result<(), String> { + let runtime = RuntimeBuilder::new_current_thread() + .enable_all() + .build() + .map_err(|error| format!("could not build a runtime for the delete: {error}"))?; + runtime.block_on(async { + time::timeout( + DROP_DELETE_TIMEOUT, + delete_afresh(&kind, &id, &sandbox, access.as_ref()), + ) + .await + .map_err(|_| { + format!( + "the delete did not finish within {}s", + DROP_DELETE_TIMEOUT.as_secs() + ) + })? + }) + }) + .map_err(|error| format!("could not spawn the delete thread: {error}"))?; + thread + .join() + .map_err(|_| "the delete thread panicked".to_string())? +} + +/// Deletes sandbox `id` over a fresh provider connection when `access` is +/// given, through the sandbox's own handle otherwise. +async fn delete_afresh( + kind: &SandboxProviderKind, + id: &str, + sandbox: &RunSandbox, + access: Option<&ProviderAccess>, +) -> Result<(), String> { + let Some(access) = access else { + return sandbox + .delete() + .await + .map_err(|error| display_for_log(&error)); + }; + let fresh = provider_sandbox::attach_provider_sandbox( + kind.clone(), + access, + id, + false, + sandbox.working_directory().to_string(), + None, + None, + None, + ) + .await + .map_err(|error| format!("could not reconnect: {}", display_for_log(&error)))?; + fresh + .delete() + .await + .map_err(|error| display_for_log(&error)) +} + +#[cfg(test)] +mod tests { + use std::panic::AssertUnwindSafe; + + use super::*; + use crate::test_support::MockSandbox; + + #[tokio::test] + async fn deletes_once_when_dropped_without_an_explicit_delete() { + let mock = MockSandbox::default(); + let guard = DeletedOnDrop::through_handle(mock.sandbox()); + assert_eq!( + guard.working_directory(), + "/work", + "reads through to the sandbox" + ); + assert_eq!(mock.driver().delete_count(), 0); + + drop(guard); + + assert_eq!(mock.driver().delete_count(), 1); + } + + #[tokio::test] + async fn an_explicit_delete_runs_once() { + let mock = MockSandbox::default(); + let guard = DeletedOnDrop::through_handle(mock.sandbox()); + let shared = guard.shared(); + + guard.delete().await.unwrap(); + + assert_eq!(mock.driver().delete_count(), 1); + drop(shared); + assert_eq!( + mock.driver().delete_count(), + 1, + "a shared handle does not delete" + ); + } + + #[test] + fn a_panic_before_the_delete_still_deletes_once() { + let mock = MockSandbox::default(); + let sandbox = mock.sandbox(); + + let outcome = std::panic::catch_unwind(AssertUnwindSafe(|| { + let _guard = DeletedOnDrop::through_handle(sandbox); + panic!("the test failed before its delete"); + })); + + assert!(outcome.is_err(), "the panic still propagates"); + assert_eq!(mock.driver().delete_count(), 1); + } + + #[tokio::test] + async fn a_panic_inside_a_runtime_still_deletes_once() { + let mock = MockSandbox::default(); + let sandbox = mock.sandbox(); + + let outcome = std::panic::catch_unwind(AssertUnwindSafe(|| { + let _guard = DeletedOnDrop::through_handle(sandbox); + panic!("the test failed before its delete"); + })); + + assert!(outcome.is_err(), "the panic still propagates"); + assert_eq!(mock.driver().delete_count(), 1); + } +} diff --git a/lib/components/fabro-workflow/tests/it/daytona_integration.rs b/lib/components/fabro-workflow/tests/it/daytona_integration.rs index ed9906ceb..7339dd6e6 100644 --- a/lib/components/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/components/fabro-workflow/tests/it/daytona_integration.rs @@ -23,6 +23,7 @@ use std::path::Path; use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; +use fabro_sandbox::test_support::DeletedOnDrop; use fabro_sandbox::{ CloneRequest, DaytonaCredentials, ProviderAccess, RunSandbox, SandboxProviderKind, provider_sandbox, @@ -197,7 +198,7 @@ fn live_daytona_credentials() -> DaytonaCredentials { DaytonaCredentials::from_api_key(api_key, |name| std::env::var(name).ok()) } -async fn create_env() -> RunSandbox { +async fn create_env() -> DeletedOnDrop { let creds = load_github_app_credentials(); create_env_with_github_app(Some(creds)).await } @@ -212,17 +213,19 @@ fn test_artifact_store(run_dir: &Path) -> ArtifactStore { async fn create_env_with_github_app( github_app: Option, -) -> RunSandbox { - provider_sandbox( +) -> DeletedOnDrop { + let access = daytona_access(live_daytona_credentials()); + let sandbox = provider_sandbox( SandboxProviderKind::DAYTONA, - &daytona_access(live_daytona_credentials()), + &access, SandboxSpec::new(SandboxSource::HostDirectory), &CloneRequest::default(), github_app.as_ref(), None, ) .await - .expect("Failed to create Daytona client — is DAYTONA_API_KEY set?") + .expect("Failed to create Daytona client — is DAYTONA_API_KEY set?"); + DeletedOnDrop::new(sandbox, &access) } fn load_github_app_credentials() -> fabro_github::GitHubCredentials { @@ -425,9 +428,10 @@ async fn daytona_snapshot_sandbox() { .timers(timers); let creds = load_github_app_credentials(); + let access = daytona_access(live_daytona_credentials()); let env = provider_sandbox( SandboxProviderKind::DAYTONA, - &daytona_access(live_daytona_credentials()), + &access, spec, &CloneRequest::default(), Some(&creds), @@ -435,6 +439,7 @@ async fn daytona_snapshot_sandbox() { ) .await .expect("Failed to create Daytona client — is DAYTONA_API_KEY set?"); + let env = DeletedOnDrop::new(env, &access); env.initialize().await.unwrap(); // Verify rg is available (installed by snapshot) @@ -532,7 +537,6 @@ impl Handler for LargeOutputHandler { async fn daytona_pipeline_artifact_offload_and_sync() { let env = create_env().await; env.initialize().await.unwrap(); - let env: Arc = Arc::new(env); // Pipeline: start -> big_output -> exit let mut graph = Graph::new("DaytonaArtifactPipeline"); @@ -570,7 +574,7 @@ async fn daytona_pipeline_artifact_offload_and_sync() { registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.clone()); + let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.shared()); let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), @@ -687,7 +691,6 @@ async fn setup_daytona_git(sandbox: &RunSandbox) -> (RunId, String, String) { async fn daytona_git_checkpoint_remote_emits_events() { let env = create_env().await; env.initialize().await.unwrap(); - let env: Arc = Arc::new(env); // Install git if not available (the default ubuntu:22.04 image may not have it) let git_check = env @@ -759,7 +762,7 @@ async fn daytona_git_checkpoint_remote_emits_events() { registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let engine = WorkflowRunner::new(registry, Arc::new(emitter), env.clone()); + let engine = WorkflowRunner::new(registry, Arc::new(emitter), env.shared()); let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), @@ -836,7 +839,6 @@ async fn daytona_git_checkpoint_remote_emits_events() { async fn daytona_git_checkpoint_without_metadata_branch() { let env = create_env().await; env.initialize().await.unwrap(); - let env: Arc = Arc::new(env); // Install git if not available let git_check = env @@ -901,7 +903,7 @@ async fn daytona_git_checkpoint_without_metadata_branch() { registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.clone()); + let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.shared()); let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), @@ -997,7 +999,6 @@ impl Handler for AssetCreatorHandler { async fn daytona_asset_collection() { let env = create_env().await; env.initialize().await.unwrap(); - let env: Arc = Arc::new(env); let dir = tempfile::tempdir().unwrap(); @@ -1005,7 +1006,7 @@ async fn daytona_asset_collection() { registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.clone()); + let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.shared()); let mut graph = Graph::new("DaytonaAssetTest"); graph.attrs.insert( @@ -1256,7 +1257,6 @@ async fn daytona_git_push_run_branch_to_origin() { let creds = load_github_app_credentials(); let env = create_env_with_github_app(Some(creds)).await; env.initialize().await.unwrap(); - let env: Arc = Arc::new(env); // Install git if not available let git_check = env @@ -1319,7 +1319,7 @@ async fn daytona_git_push_run_branch_to_origin() { registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.clone()); + let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env.shared()); let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), @@ -1623,9 +1623,10 @@ async fn daytona_cp_upload_download_round_trip() { #[fabro_macros::e2e_test(live("DAYTONA_API_KEY"))] async fn daytona_computer_use_browser_screenshot() { + let access = daytona_access(live_daytona_credentials()); let env = provider_sandbox( SandboxProviderKind::DAYTONA, - &daytona_access(live_daytona_credentials()), + &access, SandboxSpec::new(SandboxSource::HostDirectory), &CloneRequest::none(), None, @@ -1633,6 +1634,7 @@ async fn daytona_computer_use_browser_screenshot() { ) .await .expect("DAYTONA_API_KEY must be set"); + let env = DeletedOnDrop::new(env, &access); env.initialize().await.unwrap(); // 1. Start the computer use desktop environment (Xvfb, xfce4, etc.) through the @@ -1764,9 +1766,10 @@ async fn daytona_computer_use_browser_screenshot() { #[fabro_macros::e2e_test(live("DAYTONA_API_KEY"))] async fn daytona_playwright_mcp_sandbox_transport() { // Create sandbox from daytona-medium (has Node.js + Chromium) + let access = daytona_access(live_daytona_credentials()); let sandbox = provider_sandbox( SandboxProviderKind::DAYTONA, - &daytona_access(live_daytona_credentials()), + &access, SandboxSpec::new(SandboxSource::HostDirectory), &CloneRequest::none(), None, @@ -1774,6 +1777,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { ) .await .expect("DAYTONA_API_KEY must be set"); + let sandbox = DeletedOnDrop::new(sandbox, &access); sandbox.initialize().await.unwrap(); // 1. Install Playwright MCP server and its browser @@ -1858,13 +1862,13 @@ async fn daytona_playwright_mcp_sandbox_transport() { ), ]), ); - let sandbox = Arc::new(sandbox); let routes = sandbox + .shared() .port_routes() .expect("Daytona forwards ports through preview URLs"); let mut agent = pebble_coding_agent::CodingAgent::builder( client, - Arc::clone(&sandbox) as Arc, + sandbox.shared() as Arc, ) .model("test/model") .permission_level(pebble_coding_agent::events::PermissionLevel::Full)