From a39b97c94061f5b892ab8aa0802f9b66bf84c7bc Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 11 Sep 2026 12:36:29 -0600 Subject: [PATCH] Reconnect a local sandbox by attaching to its directory A local sandbox was rebuilt by creating a fresh Host sandbox over the recorded working directory, so reconnect, sandbox details, the console URL, the recorded id, and the terminal each carried a local branch. The Host provider now derives a designated directory's id from its path and attaches to it from any provider instance, so reconnect goes through the one attach path: the record carries that id, a record written before directories had ids recomputes it from the directory, and describe works for local like every other kind. The local provider skips the ownership scope because a designated directory carries no labels and nothing else shares the host's directories with fabro. Co-Authored-By: Claude Fable 5.1 --- lib/components/fabro-sandbox/src/details.rs | 61 ++--------------- .../fabro-sandbox/src/driver_sandbox.rs | 27 +++----- .../fabro-sandbox/src/provider_sandbox.rs | 15 +++-- lib/components/fabro-sandbox/src/reconnect.rs | 67 +++++++++---------- .../fabro-sandbox/src/sandbox_spec.rs | 24 ++----- .../fabro-workflow/src/operations/start.rs | 4 +- .../fabro-workflow/src/pipeline/initialize.rs | 4 +- 7 files changed, 64 insertions(+), 138 deletions(-) diff --git a/lib/components/fabro-sandbox/src/details.rs b/lib/components/fabro-sandbox/src/details.rs index 3a9c04b7c..3505028cc 100644 --- a/lib/components/fabro-sandbox/src/details.rs +++ b/lib/components/fabro-sandbox/src/details.rs @@ -1,28 +1,21 @@ -use std::collections::BTreeMap; - use anyhow::Result; use chrono::{DateTime, Utc}; use fabro_types::{ - BundledProvider, RunId, RunSandboxInstance, SandboxDetails, SandboxNetwork, SandboxResources, - SandboxState, SandboxTimestamps, + RunId, RunSandboxInstance, SandboxDetails, SandboxNetwork, SandboxResources, SandboxState, + SandboxTimestamps, }; use crate::driver::ProviderAccess; use crate::reconnect; /// Inspect the sandbox identified by `record` and return provider-neutral -/// details for control-plane display. -/// -/// `local` always returns a minimal record describing the host; every other -/// provider is described through the sandbox driver. +/// details for control-plane display, described through the sandbox driver +/// on every provider. pub async fn sandbox_details( record: &RunSandboxInstance, access: &ProviderAccess, run_id: Option, ) -> Result { - if record.provider.bundled() == Some(BundledProvider::Local) { - return Ok(local_details(record)); - } let sandbox = reconnect::reconnect_driver_for_run(record, access, run_id, None).await?; let status = sandbox.handle()?.describe().await.map_err(|err| { anyhow::anyhow!( @@ -34,20 +27,6 @@ pub async fn sandbox_details( Ok(details_from_status(record, &status)) } -fn local_details(record: &RunSandboxInstance) -> SandboxDetails { - SandboxDetails { - sandbox: record.clone(), - state: SandboxState::Running, - native_state: None, - region: None, - web_url: None, - resources: SandboxResources::default(), - network: SandboxNetwork::unknown(), - labels: BTreeMap::new(), - timestamps: SandboxTimestamps::default(), - } -} - /// Projection of a sandbox-driver [`sandbox_driver::SandboxStatus`] into /// fabro's inventory shape. The driver reports what a provider exposes /// through its public facets; fields no facet carries (network policy) stay @@ -226,36 +205,4 @@ mod tests { assert_eq!(details.sandbox.runtime.id, "container-abc123"); assert_eq!(details.network, SandboxNetwork::unknown()); } - - #[test] - fn local_details_returns_running_with_no_metadata() { - let record = RunSandboxInstance { - provider: SandboxProviderKind::LOCAL, - image: None, - snapshot: None, - runtime: fabro_types::RunSandboxRuntime { - id: "local:01JNQVR7M0EJ5GKAT2SC4ERS1Z".to_string(), - working_directory: "/Users/client/project".to_string(), - repo_cloned: None, - clone_origin_url: None, - clone_branch: None, - workspace_root: None, - repos_root: None, - primary_repo_path: None, - primary_repo_link: None, - }, - }; - let details = local_details(&record); - assert_eq!(details.sandbox.provider, SandboxProviderKind::LOCAL); - assert_eq!(details.state, SandboxState::Running); - let runtime = &details.sandbox.runtime; - assert_eq!(runtime.id, "local:01JNQVR7M0EJ5GKAT2SC4ERS1Z"); - assert_eq!(runtime.working_directory, "/Users/client/project"); - assert!(details.region.is_none()); - assert!(details.sandbox.image.is_none()); - assert!(details.labels.is_empty()); - assert_eq!(details.resources, SandboxResources::default()); - assert_eq!(details.network, SandboxNetwork::unknown()); - assert_eq!(details.timestamps, SandboxTimestamps::default()); - } } diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 932c0e3e2..498dc3843 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -923,12 +923,8 @@ impl RunSandbox { } /// The provider's console page for this sandbox, when it has one. Best - /// effort: a failed describe reports no page. The local sandbox is the - /// host and has none. + /// effort: a failed describe reports no page. pub async fn console_url(&self) -> Option { - if self.kind.is_local() { - return None; - } self.handle() .ok()? .describe() @@ -1000,14 +996,10 @@ impl RunSandbox { ) } - /// The provider's id for this sandbox, or empty for `local`: a local - /// sandbox is its working directory, which the run record already - /// carries, and its Host registry id does not outlive the process. - /// Empty for a pending sandbox that has not been created. + /// The provider's id for this sandbox; for `local`, the id the Host + /// provider derives from the working directory. Empty for a pending + /// sandbox that has not been created. pub fn sandbox_info(&self) -> String { - if self.kind.is_local() { - return String::new(); - } self.handle .get() .map(|handle| handle.id().to_string()) @@ -1429,12 +1421,13 @@ mod tests { }; assert_eq!(sandbox.platform(), expected); assert!(sandbox.os_version().starts_with(expected)); - assert_eq!( - sandbox.sandbox_info(), - "", - "local sandboxes are identified by directory" - ); let handle = Arc::clone(sandbox.handle().unwrap()); + assert_eq!(sandbox.sandbox_info(), handle.id().to_string()); + assert!( + sandbox.sandbox_info().starts_with("host-dir-"), + "a local sandbox is identified by its directory: {}", + sandbox.sandbox_info() + ); let isolated = RunSandbox::new(SandboxProviderKind::DOCKER, Arc::clone(&handle)); assert_eq!(isolated.sandbox_info(), handle.id().to_string()); assert_eq!(sandbox.console_url().await, None); diff --git a/lib/components/fabro-sandbox/src/provider_sandbox.rs b/lib/components/fabro-sandbox/src/provider_sandbox.rs index 565903ac7..e136acbaa 100644 --- a/lib/components/fabro-sandbox/src/provider_sandbox.rs +++ b/lib/components/fabro-sandbox/src/provider_sandbox.rs @@ -75,11 +75,11 @@ pub async fn provider_sandbox( /// Reattach to a run's sandbox on `kind` by its persisted id. The driver /// reports the sandbox's lifecycle from here on through `events`. /// -/// The sandbox must carry fabro's managed label and, when a run id is -/// known, the matching run label: the provider shares its backend with -/// every other application, and fabro never operates on a sandbox it did -/// not create. The ownership scope the provider is connected through -/// refuses anything else. +/// On a shared backend the sandbox must carry fabro's managed label and, +/// when a run id is known, the matching run label: fabro never operates on +/// a sandbox it did not create, and the ownership scope the provider is +/// connected through refuses anything else. A local sandbox attaches by +/// the id the Host provider derives from its directory. pub async fn attach_provider_sandbox( kind: SandboxProviderKind, access: &ProviderAccess, @@ -166,6 +166,11 @@ async fn connect( .map_err(|error| { crate::Error::context(format!("Failed to connect to the {kind} provider"), error) })?; + // A local sandbox is a directory the caller designated; it carries no + // labels, and nothing else shares the host's directories with fabro. + if kind.bundled() == Some(BundledProvider::Local) { + return Ok(connected.provider); + } Ok(Arc::new(OwnedProvider::new( connected.provider, managed_labels::ownership(run_id), diff --git a/lib/components/fabro-sandbox/src/reconnect.rs b/lib/components/fabro-sandbox/src/reconnect.rs index e5757a6c6..a1b1c0175 100644 --- a/lib/components/fabro-sandbox/src/reconnect.rs +++ b/lib/components/fabro-sandbox/src/reconnect.rs @@ -1,11 +1,12 @@ -use std::path::PathBuf; +use std::path::Path; use anyhow::{Context, Result}; use fabro_types::{BundledProvider, RunId, RunSandboxInstance}; use sandbox_driver::{EventContext, PtySession, PtySize}; +use sandbox_driver_host::HostProvider; use crate::driver::ProviderAccess; -use crate::driver_sandbox::{RunSandbox, local_sandbox_with_events}; +use crate::driver_sandbox::RunSandbox; use crate::provider_sandbox; /// Reconnect to a sandbox from a saved record. @@ -43,34 +44,35 @@ pub async fn reconnect_driver_for_run( events: Option, ) -> Result { let runtime = &record.runtime; - // A local sandbox is its working directory: rebuilding the handle over - // that directory is the reconnect. The per-process Host registry holds - // no state worth attaching to. - let sandbox = if record.provider.bundled() == Some(BundledProvider::Local) { - local_sandbox_with_events(PathBuf::from(&runtime.working_directory), events) - .await - .context("Failed to reconnect local sandbox")? - } else { - let repo_cloned = runtime.repo_cloned.with_context(|| { - format!( - "{} run sandbox missing repo_cloned metadata", - record.provider - ) - })?; - provider_sandbox::attach_provider_sandbox( - record.provider.clone(), - access, - &runtime.id, - repo_cloned, - runtime.working_directory.clone(), - runtime.clone_origin_url.clone(), - run_id, - events, - ) - .await - .with_context(|| format!("Failed to reconnect {} sandbox", record.provider))? - }; - Ok(sandbox) + let sandbox_id = sandbox_id(record).await; + provider_sandbox::attach_provider_sandbox( + record.provider.clone(), + access, + &sandbox_id, + // A record without the flag was written for a sandbox fabro never + // cloned into. + runtime.repo_cloned.unwrap_or(false), + runtime.working_directory.clone(), + runtime.clone_origin_url.clone(), + run_id, + events, + ) + .await + .with_context(|| format!("Failed to reconnect {} sandbox", record.provider)) +} + +/// The id the record's sandbox attaches by. A local sandbox is its working +/// directory, and the Host provider derives the directory's id from its +/// path, so the record's id is recomputed from the directory: a record +/// written before directories had ids attaches the same way. +async fn sandbox_id(record: &RunSandboxInstance) -> String { + let runtime = &record.runtime; + if record.provider.bundled() == Some(BundledProvider::Local) { + if let Some(id) = HostProvider::directory_id(Path::new(&runtime.working_directory)).await { + return id.to_string(); + } + } + runtime.id.clone() } /// Opens an interactive shell in a run's sandbox over the driver's Pty @@ -82,11 +84,6 @@ pub async fn open_terminal_for_run( run_id: Option, size: PtySize, ) -> crate::Result> { - if record.provider.bundled() == Some(BundledProvider::Local) { - return Err(crate::Error::message( - "Local sandboxes do not support embedded terminals", - )); - } let sandbox = reconnect_driver_for_run(record, access, run_id, None) .await .map_err(|err| crate::Error::context_anyhow("Failed to reconnect sandbox", err))?; diff --git a/lib/components/fabro-sandbox/src/sandbox_spec.rs b/lib/components/fabro-sandbox/src/sandbox_spec.rs index 8345e1bb3..8198afdba 100644 --- a/lib/components/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/components/fabro-sandbox/src/sandbox_spec.rs @@ -60,20 +60,9 @@ impl SandboxSpec { } /// Build initialized sandbox metadata for persistence. - pub fn to_run_sandbox_instance( - &self, - sandbox: &RunSandbox, - run_id: RunId, - ) -> RunSandboxInstance { + pub fn to_run_sandbox_instance(&self, sandbox: &RunSandbox) -> RunSandboxInstance { let working_directory = sandbox.working_directory().to_string(); - let id = { - let info = sandbox.sandbox_info(); - if info.is_empty() { - format!("local:{run_id}") - } else { - info - } - }; + let id = sandbox.sandbox_info(); match self { Self::Provider(spec) => { @@ -136,7 +125,7 @@ impl SandboxSpec { runtime: RunSandboxRuntime { id, working_directory, - repo_cloned: None, + repo_cloned: Some(false), clone_origin_url: None, clone_branch: None, workspace_root: None, @@ -204,7 +193,6 @@ fn runtime_layout_metadata( #[cfg(test)] mod tests { - use fabro_types::RunId; use sandbox_driver::SandboxSource; use sandbox_driver_testing::ScriptedSandbox; @@ -240,8 +228,7 @@ mod tests { }))); let sandbox = sandbox_at("/workspace/rack-test"); - let run_id: RunId = "01HY0000000000000000000000".parse().unwrap(); - let record = spec.to_run_sandbox_instance(&sandbox, run_id); + let record = spec.to_run_sandbox_instance(&sandbox); let runtime = record.runtime; assert_eq!(runtime.working_directory, "/workspace/rack-test"); @@ -295,8 +282,7 @@ mod tests { }))); let sandbox = sandbox_at("/workspace"); - let run_id: RunId = "01HY0000000000000000000000".parse().unwrap(); - let record = spec.to_run_sandbox_instance(&sandbox, run_id); + let record = spec.to_run_sandbox_instance(&sandbox); let runtime = record.runtime; assert_eq!(runtime.working_directory, "/workspace"); diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 657538022..406cb5023 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -1868,7 +1868,7 @@ mod tests { .. } = session; let runtime = sandbox - .to_run_sandbox_instance(&MockSandbox::linux().sandbox(), fixtures::RUN_1) + .to_run_sandbox_instance(&MockSandbox::linux().sandbox()) .runtime; assert_eq!(runtime.repo_cloned, Some(false)); assert_eq!(runtime.clone_origin_url, None); @@ -1930,7 +1930,7 @@ mod tests { .. } = session; let runtime = sandbox - .to_run_sandbox_instance(&MockSandbox::linux().sandbox(), fixtures::RUN_1) + .to_run_sandbox_instance(&MockSandbox::linux().sandbox()) .runtime; assert_eq!(runtime.repo_cloned, Some(false)); assert_eq!(runtime.clone_origin_url, None); diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 0d5989bb0..d7101a5b2 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -522,9 +522,7 @@ pub async fn initialize( } if !attach_existing { - let run_sandbox = options - .sandbox - .to_run_sandbox_instance(&sandbox, options.run_options.run_id); + let run_sandbox = options.sandbox.to_run_sandbox_instance(&sandbox); let runtime = &run_sandbox.runtime; options.emitter.emit(&Event::SandboxInitialized { working_directory: runtime.working_directory.clone(),