From 2d94810a7164fe396cbe26604b9ba938d46bcb08 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 10 Sep 2026 16:23:22 -0600 Subject: [PATCH] Keep the sandbox inventory over the driver's providers directly Fabro had its own SandboxProvider trait with a registry over it, a LocalSandboxProvider that listed nothing, and a DriverInventoryProvider that adapted a driver provider to the fabro trait. The trait existed to tag a provider with fabro's kind and to aggregate across providers; both are the inventory's job. SandboxInventory replaces all three: a list of driver providers, each narrowed by fabro's ownership labels and connected on first use, with the cross-provider aggregation, native-id lookup, and conflict detection the registry did. The local kind keeps an entry so a caller can ask whether it is ready, and lists nothing, since its sandboxes are directories the run record names. The delete path nothing called is gone. Tests run against the driver's scripted provider and, for a provider that cannot connect, a plugin kind whose executable does not exist; the fake provider module is deleted. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-server/src/serve.rs | 2 +- lib/apps/fabro-server/src/server.rs | 39 +- .../src/server/handler/automations.rs | 7 +- .../src/server/handler/sandboxes.rs | 129 +++-- lib/apps/fabro-server/src/server/tests.rs | 8 +- lib/apps/fabro-server/src/test_support.rs | 15 +- lib/components/fabro-sandbox/src/lib.rs | 5 +- lib/components/fabro-sandbox/src/provider.rs | 484 +++++++++++------- .../fabro-sandbox/src/provider/driver.rs | 234 --------- .../fabro-sandbox/src/test_support.rs | 120 ++--- 10 files changed, 431 insertions(+), 612 deletions(-) delete mode 100644 lib/components/fabro-sandbox/src/provider/driver.rs diff --git a/lib/apps/fabro-server/src/serve.rs b/lib/apps/fabro-server/src/serve.rs index 0c1c49aaf..dd174630e 100644 --- a/lib/apps/fabro-server/src/serve.rs +++ b/lib/apps/fabro-server/src/serve.rs @@ -799,7 +799,7 @@ where github_api_base_url: None, active_config_path, http_client: None, - sandbox_provider_registry: None, + sandbox_inventory: None, shutdown: shutdown.clone(), #[cfg(test)] worker_control_bus: None, diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index b5da5d57d..ae7d158d6 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -65,10 +65,7 @@ use fabro_redact::redact_jsonl_line; use fabro_sandbox::details::sandbox_details; use fabro_sandbox::driver::{DaytonaCredentials, ProviderAccess, ProviderConnectOptions}; use fabro_sandbox::reconnect::reconnect_for_run; -use fabro_sandbox::{ - DriverInventoryProvider, LocalSandboxProvider, SandboxProvider, SandboxProviderRegistry, - daytona, -}; +use fabro_sandbox::{SandboxInventory, daytona}; use fabro_slack::client::{PostedMessage as SlackPostedMessage, SlackClient}; use fabro_slack::config::{ SlackCredentialResolution, @@ -1137,7 +1134,7 @@ pub struct AppState { pub(crate) github_api_base_url: String, active_config_path: PathBuf, http_client: Option, - sandbox_provider_registry: SandboxProviderRegistry, + sandbox_inventory: SandboxInventory, shutdown: CancellationToken, shutting_down: AtomicBool, registry_factory_override: Option>, @@ -1280,7 +1277,7 @@ pub(crate) struct AppStateConfig { pub(crate) github_api_base_url: Option, pub(crate) active_config_path: PathBuf, pub(crate) http_client: Option, - pub(crate) sandbox_provider_registry: Option, + pub(crate) sandbox_inventory: Option, pub(crate) shutdown: CancellationToken, #[cfg(test)] pub(crate) worker_control_bus: Option>, @@ -1538,8 +1535,8 @@ impl AppState { &self.session_runtimes } - pub(crate) fn sandbox_provider_registry(&self) -> &SandboxProviderRegistry { - &self.sandbox_provider_registry + pub(crate) fn sandbox_inventory(&self) -> &SandboxInventory { + &self.sandbox_inventory } pub(crate) fn server_secret(&self, name: &str) -> Option { @@ -2336,26 +2333,26 @@ fn worker_token_keys_from_server_secrets( .map_err(|err| jwt_auth::session_secret_key_error(&err)) } -fn build_sandbox_provider_registry( +fn build_sandbox_inventory( server_settings: &ServerSettings, daytona_api_key: Option, env_lookup: &EnvLookup, http_client: Option, -) -> SandboxProviderRegistry { +) -> SandboxInventory { let provider_settings = &server_settings.server.sandbox.providers; - let mut providers: Vec> = Vec::new(); + let mut inventory = SandboxInventory::empty(); if provider_settings.is_enabled(&SandboxProviderKind::LOCAL) { - providers.push(Arc::new(LocalSandboxProvider)); + inventory = inventory.with_host_directories(SandboxProviderKind::LOCAL); } if let Some(docker) = provider_settings.get(&SandboxProviderKind::DOCKER) { if docker.enabled { - providers.push(Arc::new(DriverInventoryProvider::lazy( + inventory = inventory.with_lazy( SandboxProviderKind::DOCKER, docker.clone(), ProviderConnectOptions::default(), - ))); + ); } } @@ -2369,18 +2366,18 @@ fn build_sandbox_provider_registry( target: None, http_client, }; - providers.push(Arc::new(DriverInventoryProvider::lazy( + inventory = inventory.with_lazy( SandboxProviderKind::DAYTONA, daytona.clone(), ProviderConnectOptions { host_registry_root: None, daytona: Some(credentials), }, - ))); + ); } } - SandboxProviderRegistry::new(providers) + inventory } pub(crate) fn automation_dir_for_active_config(active_config_path: &std::path::Path) -> PathBuf { @@ -2433,7 +2430,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result anyhow::Result anyhow::Result>, _auth: RequiredRunManagementActor, ) -> Json { - Json(state.sandbox_provider_registry().list_managed().await) + Json(state.sandbox_inventory().list_managed().await) } async fn retrieve_sandbox( @@ -30,7 +30,7 @@ async fn retrieve_sandbox( _auth: RequiredRunManagementActor, ) -> Result, ApiError> { state - .sandbox_provider_registry() + .sandbox_inventory() .get_managed_by_native_id(&id) .await .map(Json) @@ -79,23 +79,49 @@ fn provider_list(providers: &[SandboxProviderKind]) -> String { mod tests { use axum::body::{Body, to_bytes}; use axum::http::{Request, StatusCode}; - use fabro_sandbox::SandboxProviderRegistry; - use fabro_sandbox::test_support::{ - FakeGet, FakeList, FakeSandboxProvider, fake_registry, fake_sandbox_info, - }; + use fabro_sandbox::SandboxInventory; + use fabro_sandbox::driver::{ConnectedProvider, ProviderConnectOptions}; + use fabro_sandbox::test_support::{managed_scripted_sandbox, scripted_inventory_provider}; use fabro_types::SandboxProviderKind; + use fabro_types::settings::server::{SandboxPluginSettings, ServerSandboxProviderSettings}; use serde_json::{Value, json}; use tower::ServiceExt; use crate::test_support::{TestAppStateBuilder, build_test_router}; - fn app_with_registry(registry: SandboxProviderRegistry) -> axum::Router { + fn app_with_inventory(inventory: SandboxInventory) -> axum::Router { let state = TestAppStateBuilder::new() - .sandbox_provider_registry(registry) + .sandbox_inventory(inventory) .build(); build_test_router(state) } + /// A connected provider of `kind` holding fabro-managed sandboxes `ids`. + fn provider(kind: SandboxProviderKind, ids: &[&str]) -> ConnectedProvider { + scripted_inventory_provider( + kind, + ids.iter().map(|id| managed_scripted_sandbox(id)).collect(), + ) + } + + /// A plugin kind whose executable does not exist, so every lookup fails + /// to connect. + fn with_unreachable_plugin(inventory: SandboxInventory, name: &str) -> SandboxInventory { + let settings = ServerSandboxProviderSettings { + enabled: true, + plugin: Some(SandboxPluginSettings { + path: Some(format!("/nonexistent/fabro-sandbox-{name}")), + dev: true, + ..SandboxPluginSettings::default() + }), + }; + inventory.with_lazy( + SandboxProviderKind::try_new(name).expect("valid kind"), + settings, + ProviderConnectOptions::default(), + ) + } + fn req_get(uri: &str) -> Request { Request::builder() .method("GET") @@ -113,12 +139,10 @@ mod tests { #[tokio::test] async fn list_returns_provider_backed_data_without_run_projection_state() { - let docker = fake_sandbox_info(SandboxProviderKind::DOCKER, "docker-native-id"); - let app = app_with_registry(fake_registry(vec![FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(vec![docker]), - FakeGet::Missing, - )])); + let app = app_with_inventory( + SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &["docker-native-id"])), + ); let response = app.oneshot(req_get("/api/v1/sandboxes")).await.unwrap(); @@ -126,24 +150,17 @@ mod tests { let body = body_json(response).await; assert_eq!(body["data"][0]["id"], "docker-native-id"); assert_eq!(body["data"][0]["provider"], "docker"); + assert_eq!(body["data"][0]["state"], "running"); assert_eq!(body["meta"]["provider_errors"], json!([])); } #[tokio::test] async fn retrieve_searches_all_configured_providers() { - let daytona = fake_sandbox_info(SandboxProviderKind::DAYTONA, "native-id"); - let app = app_with_registry(fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Found(Box::new(daytona)), - ), - ])); + let app = app_with_inventory( + SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &[])) + .with_connected(provider(SandboxProviderKind::DAYTONA, &["native-id"])), + ); let response = app .oneshot(req_get("/api/v1/sandboxes/native-id")) @@ -158,18 +175,11 @@ mod tests { #[tokio::test] async fn no_matching_sandbox_returns_404() { - let app = app_with_registry(fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - ])); + let app = app_with_inventory( + SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &[])) + .with_connected(provider(SandboxProviderKind::DAYTONA, &[])), + ); let response = app .oneshot(req_get("/api/v1/sandboxes/missing")) @@ -181,24 +191,11 @@ mod tests { #[tokio::test] async fn duplicate_native_ids_return_409() { - let app = app_with_registry(fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Found(Box::new(fake_sandbox_info( - SandboxProviderKind::DOCKER, - "same-id", - ))), - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Found(Box::new(fake_sandbox_info( - SandboxProviderKind::DAYTONA, - "same-id", - ))), - ), - ])); + let app = app_with_inventory( + SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &["same-id"])) + .with_connected(provider(SandboxProviderKind::DAYTONA, &["same-id"])), + ); let response = app .oneshot(req_get("/api/v1/sandboxes/same-id")) @@ -217,18 +214,10 @@ mod tests { #[tokio::test] async fn provider_lookup_uncertainty_returns_502() { - let app = app_with_registry(fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Err("daytona unavailable"), - ), - ])); + let app = app_with_inventory(with_unreachable_plugin( + SandboxInventory::empty().with_connected(provider(SandboxProviderKind::DOCKER, &[])), + "e2b", + )); let response = app .oneshot(req_get("/api/v1/sandboxes/maybe-missing")) @@ -241,7 +230,7 @@ mod tests { body["errors"][0]["detail"] .as_str() .unwrap_or_default() - .contains("daytona unavailable") + .contains("e2b: Failed to connect to the e2b provider") ); } } diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 9ecf1c98c..1f8cee03e 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -2107,7 +2107,7 @@ fn slack_app_state_with_settings_and_secret_sources( github_api_base_url: None, active_config_path: tempfile::tempdir().unwrap().path().join("settings.toml"), http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), - sandbox_provider_registry: None, + sandbox_inventory: None, shutdown: tokio_util::sync::CancellationToken::new(), worker_control_bus: None, worker_runtime: None, @@ -2268,7 +2268,7 @@ fn slack_service_respects_disabled_server_config_even_with_vault_tokens() { github_api_base_url: None, active_config_path: tempfile::tempdir().unwrap().path().join("settings.toml"), http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), - sandbox_provider_registry: None, + sandbox_inventory: None, shutdown: tokio_util::sync::CancellationToken::new(), worker_control_bus: None, worker_runtime: None, @@ -2622,7 +2622,7 @@ methods = ["dev-token"] github_api_base_url: None, active_config_path: tempfile::tempdir().unwrap().path().join("settings.toml"), http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), - sandbox_provider_registry: None, + sandbox_inventory: None, shutdown: tokio_util::sync::CancellationToken::new(), worker_control_bus: None, worker_runtime: None, @@ -8440,7 +8440,7 @@ fn create_github_token_app_state_with_env_lookup_and_llm_catalog_settings( github_api_base_url, active_config_path, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), - sandbox_provider_registry: None, + sandbox_inventory: None, shutdown: tokio_util::sync::CancellationToken::new(), worker_control_bus: None, worker_runtime: None, diff --git a/lib/apps/fabro-server/src/test_support.rs b/lib/apps/fabro-server/src/test_support.rs index 8522206c1..0f6b0e2d5 100644 --- a/lib/apps/fabro-server/src/test_support.rs +++ b/lib/apps/fabro-server/src/test_support.rs @@ -19,7 +19,7 @@ use fabro_config::{LlmLayer, RunLayer, ServerSettingsBuilder, Storage, envfile}; use fabro_db::DbPool; use fabro_interview::Interviewer; use fabro_llm::lithos_catalog::Catalog; -use fabro_sandbox::SandboxProviderRegistry; +use fabro_sandbox::SandboxInventory; use fabro_static::EnvVars; use fabro_store::{ArtifactStore, Database, test_support as store_test_support}; use fabro_types::settings::ServerAuthMethod; @@ -90,7 +90,7 @@ pub struct TestAppStateBuilder { manifest_run_defaults: RunLayer, max_concurrent_runs: usize, registry_factory_override: Option>, - sandbox_provider_registry: Option, + sandbox_inventory: Option, store_bundle: Option<(Arc, ArtifactStore)>, vault_path: Option, vault_entries: Vec<(String, String)>, @@ -112,7 +112,7 @@ impl Default for TestAppStateBuilder { manifest_run_defaults: RunLayer::default(), max_concurrent_runs: 5, registry_factory_override: None, - sandbox_provider_registry: None, + sandbox_inventory: None, store_bundle: None, vault_path: None, vault_entries: Vec::new(), @@ -160,11 +160,8 @@ impl TestAppStateBuilder { self } - pub fn sandbox_provider_registry( - mut self, - sandbox_provider_registry: SandboxProviderRegistry, - ) -> Self { - self.sandbox_provider_registry = Some(sandbox_provider_registry); + pub fn sandbox_inventory(mut self, sandbox_inventory: SandboxInventory) -> Self { + self.sandbox_inventory = Some(sandbox_inventory); self } @@ -312,7 +309,7 @@ impl TestAppStateBuilder { http_client: Some( fabro_http::test_http_client().expect("test HTTP client should build"), ), - sandbox_provider_registry: self.sandbox_provider_registry, + sandbox_inventory: self.sandbox_inventory, shutdown: CancellationToken::new(), #[cfg(test)] worker_control_bus: None, diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index ed5cbc540..e5524c132 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -52,10 +52,7 @@ pub use options::{ SandboxOptions, local_working_directory_from_environment, options_from_environment, unresolved_env, }; -pub use provider::driver::DriverInventoryProvider; -pub use provider::{ - LocalSandboxProvider, SandboxLookupError, SandboxProvider, SandboxProviderRegistry, -}; +pub use provider::{SandboxInventory, SandboxLookupError}; pub use provider_sandbox::{attach_provider_sandbox, provider_sandbox}; pub use push_credentials::RefreshErrorKind; pub use reconnect::{ diff --git a/lib/components/fabro-sandbox/src/provider.rs b/lib/components/fabro-sandbox/src/provider.rs index d32e5b4f1..6991fd34f 100644 --- a/lib/components/fabro-sandbox/src/provider.rs +++ b/lib/components/fabro-sandbox/src/provider.rs @@ -1,47 +1,116 @@ -pub mod driver; +//! Fabro's inventory of the sandboxes it manages, across the providers a +//! server has configured. +//! +//! Every entry is a sandbox-driver provider narrowed by fabro's ownership +//! labels, so a listing shows only the sandboxes fabro created and an +//! attach to anything else is refused. A provider connects on first use: +//! the inventory is assembled synchronously at startup, and a provider that +//! is down surfaces as a lookup error rather than a startup failure. The +//! `local` kind has an entry too, so a caller can ask whether the kind is +//! ready, but its sandboxes are directories the run record names and there +//! is nothing to list. use std::sync::Arc; -use async_trait::async_trait; +use fabro_types::settings::server::ServerSandboxProviderSettings; use fabro_types::{ SandboxInfo, SandboxListMeta, SandboxListResponse, SandboxProviderKind, SandboxProviderLookupError, }; use fabro_util::error::collect_chain; use futures::future::join_all; +use sandbox_driver::{ + Error as DriverError, OwnedProvider, SandboxFilter, SandboxId, + SandboxProvider as DriverProvider, SandboxState, +}; +use tokio::sync::OnceCell; -#[async_trait] -pub trait SandboxProvider: Send + Sync { - fn kind(&self) -> SandboxProviderKind; - - async fn list(&self) -> crate::Result>; - async fn get(&self, id: &str) -> crate::Result>; - async fn delete(&self, id: &str) -> crate::Result<()>; -} +use crate::driver::{ConnectedProvider, ProviderConnectOptions, connect_provider}; +use crate::{details, managed_labels}; +/// The sandboxes fabro manages, by provider. #[derive(Clone, Default)] -pub struct SandboxProviderRegistry { - providers: Vec>, +pub struct SandboxInventory { + entries: Vec>, } -impl SandboxProviderRegistry { - pub fn new(providers: Vec>) -> Self { - Self { providers } - } +struct InventoryEntry { + kind: SandboxProviderKind, + connection: Connection, +} +enum Connection { + /// Sandboxes on this host are directories the run record names; + /// there is nothing to list. + HostDirectories, + Connected(Arc), + /// Connected through [`connect_provider`] on first use. + Lazy(Box), +} + +struct LazyConnection { + settings: ServerSandboxProviderSettings, + options: ProviderConnectOptions, + provider: OnceCell>, +} + +impl SandboxInventory { + #[must_use] pub fn empty() -> Self { Self::default() } - pub fn providers(&self) -> &[Arc] { - &self.providers + /// A kind whose sandboxes are directories on this host: ready to run, + /// nothing to list. + #[must_use] + pub fn with_host_directories(self, kind: SandboxProviderKind) -> Self { + self.with_entry(kind, Connection::HostDirectories) + } + + /// A provider already connected, tagged with the kind fabro persists + /// for it. + #[must_use] + pub fn with_connected(self, connected: ConnectedProvider) -> Self { + self.with_entry( + connected.kind, + Connection::Connected(owned(connected.provider)), + ) + } + + /// A provider connected through [`connect_provider`] on first use. + #[must_use] + pub fn with_lazy( + self, + kind: SandboxProviderKind, + settings: ServerSandboxProviderSettings, + options: ProviderConnectOptions, + ) -> Self { + self.with_entry( + kind, + Connection::Lazy(Box::new(LazyConnection { + settings, + options, + provider: OnceCell::new(), + })), + ) + } + + fn with_entry(mut self, kind: SandboxProviderKind, connection: Connection) -> Self { + self.entries + .push(Arc::new(InventoryEntry { kind, connection })); + self + } + + /// The provider kinds this inventory covers. + pub fn kinds(&self) -> impl Iterator { + self.entries.iter().map(|entry| &entry.kind) } pub async fn list_managed(&self) -> SandboxListResponse { let results = join_all( - self.providers + self.entries .iter() - .map(|provider| async move { (provider.kind(), provider.list().await) }), + .map(|entry| async move { (&entry.kind, entry.list().await) }), ) .await; @@ -50,7 +119,7 @@ impl SandboxProviderRegistry { for (kind, result) in results { match result { Ok(mut sandboxes) => data.append(&mut sandboxes), - Err(err) => provider_errors.push(provider_error(kind, &err)), + Err(err) => provider_errors.push(provider_error(kind.clone(), &err)), } } @@ -65,9 +134,9 @@ impl SandboxProviderRegistry { id: &str, ) -> Result { let results = join_all( - self.providers + self.entries .iter() - .map(|provider| async move { (provider.kind(), provider.get(id).await) }), + .map(|entry| async move { (&entry.kind, entry.get(id).await) }), ) .await; @@ -77,7 +146,7 @@ impl SandboxProviderRegistry { match result { Ok(Some(sandbox)) => matches.push(sandbox), Ok(None) => {} - Err(err) => provider_errors.push(provider_error(kind, &err)), + Err(err) => provider_errors.push(provider_error(kind.clone(), &err)), } } @@ -101,6 +170,88 @@ impl SandboxProviderRegistry { } } +impl InventoryEntry { + /// The provider narrowed to fabro's sandboxes, connected on first use; + /// `None` when the kind has nothing to list. + async fn provider(&self) -> crate::Result>> { + match &self.connection { + Connection::HostDirectories => Ok(None), + Connection::Connected(provider) => Ok(Some(provider)), + Connection::Lazy(lazy) => lazy + .provider + .get_or_try_init(|| async { + connect_provider(&self.kind, &lazy.settings, &lazy.options) + .await + .map(|connected| owned(connected.provider)) + .map_err(|error| { + crate::Error::context( + format!("Failed to connect to the {} provider", self.kind), + error, + ) + }) + }) + .await + .map(Some), + } + } + + async fn list(&self) -> crate::Result> { + let Some(provider) = self.provider().await? else { + return Ok(Vec::new()); + }; + let statuses = provider + .list(&SandboxFilter::default()) + .await + .map_err(|error| { + crate::Error::context(format!("Failed to list {} sandboxes", self.kind), error) + })?; + Ok(statuses + .iter() + .map(|status| details::info_from_status(&self.kind, status)) + .collect()) + } + + async fn get(&self, id: &str) -> crate::Result> { + let Some(provider) = self.provider().await? else { + return Ok(None); + }; + // An id the driver cannot even name is not one of ours. + let Ok(sandbox_id) = SandboxId::try_new(id) else { + return Ok(None); + }; + let handle = match provider.attach(&sandbox_id, None).await { + Ok(handle) => handle, + // Unknown to the provider, or not fabro's: neither is in the + // inventory. + Err(DriverError::NotFound { .. } | DriverError::NotOwned { .. }) => return Ok(None), + Err(error) => { + return Err(crate::Error::context( + format!("Failed to look up {} sandbox '{id}'", self.kind), + error, + )); + } + }; + let status = handle.describe().await.map_err(|error| { + crate::Error::context( + format!("Failed to describe {} sandbox '{id}'", self.kind), + error, + ) + })?; + if status.state == SandboxState::Deleted { + return Ok(None); + } + Ok(Some(details::info_from_status(&self.kind, &status))) + } +} + +/// The provider narrowed to fabro's sandboxes. +fn owned(provider: Arc) -> Arc { + Arc::new(OwnedProvider::new( + provider, + managed_labels::ownership(None), + )) +} + #[derive(Debug, thiserror::Error)] pub enum SandboxLookupError { #[error("sandbox '{id}' was not found by any configured provider")] @@ -117,28 +268,6 @@ pub enum SandboxLookupError { }, } -#[derive(Debug, Clone, Copy, Default)] -pub struct LocalSandboxProvider; - -#[async_trait] -impl SandboxProvider for LocalSandboxProvider { - fn kind(&self) -> SandboxProviderKind { - SandboxProviderKind::LOCAL - } - - async fn list(&self) -> crate::Result> { - Ok(Vec::new()) - } - - async fn get(&self, _id: &str) -> crate::Result> { - Ok(None) - } - - async fn delete(&self, _id: &str) -> crate::Result<()> { - Ok(()) - } -} - fn provider_error( provider: SandboxProviderKind, err: &(dyn std::error::Error + 'static), @@ -151,170 +280,177 @@ fn provider_error( #[cfg(test)] mod tests { + use fabro_types::settings::server::SandboxPluginSettings; + use sandbox_driver::SandboxState; + use super::*; use crate::test_support::{ - FakeGet, FakeList, FakeSandboxProvider, fake_registry, fake_sandbox_info, + ScriptedSandbox, managed_scripted_sandbox, scripted_inventory_provider, }; + fn kind(name: &str) -> SandboxProviderKind { + SandboxProviderKind::try_new(name).expect("valid kind") + } + + fn provider(kind: SandboxProviderKind, ids: &[&str]) -> ConnectedProvider { + scripted_inventory_provider( + kind, + ids.iter().map(|id| managed_scripted_sandbox(id)).collect(), + ) + } + + /// A plugin kind whose executable does not exist, so every lookup fails + /// to connect. + fn unreachable_plugin(inventory: SandboxInventory, name: &str) -> SandboxInventory { + let settings = ServerSandboxProviderSettings { + enabled: true, + plugin: Some(SandboxPluginSettings { + path: Some(format!("/nonexistent/fabro-sandbox-{name}")), + dev: true, + ..SandboxPluginSettings::default() + }), + }; + inventory.with_lazy(kind(name), settings, ProviderConnectOptions::default()) + } + #[tokio::test] - async fn list_returns_aggregate_data_from_successful_providers() { - let docker = fake_sandbox_info(SandboxProviderKind::DOCKER, "docker-1"); - let daytona = fake_sandbox_info(SandboxProviderKind::DAYTONA, "daytona-1"); - let registry = fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(vec![docker.clone()]), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(vec![daytona.clone()]), - FakeGet::Missing, - ), + async fn list_aggregates_fabro_owned_sandboxes_across_providers() { + let foreign = Arc::new( + ScriptedSandbox::with_id_and_working_dir("someone-elses", "/work") + .state(SandboxState::Running), + ); + let docker = scripted_inventory_provider(SandboxProviderKind::DOCKER, vec![ + managed_scripted_sandbox("docker-1"), + foreign, ]); + let inventory = SandboxInventory::empty() + .with_host_directories(SandboxProviderKind::LOCAL) + .with_connected(docker) + .with_connected(provider(SandboxProviderKind::DAYTONA, &["daytona-1"])); - let response = registry.list_managed().await; + let response = inventory.list_managed().await; - assert_eq!(response.data, vec![docker, daytona]); + let mut ids: Vec<_> = response.data.iter().map(|s| s.id.as_str()).collect(); + ids.sort_unstable(); + assert_eq!(ids, ["daytona-1", "docker-1"]); assert!(response.meta.provider_errors.is_empty()); - } - - #[tokio::test] - async fn list_includes_provider_error_metadata_when_one_provider_fails() { - let docker = fake_sandbox_info(SandboxProviderKind::DOCKER, "docker-1"); - let registry = fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(vec![docker.clone()]), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Err("daytona unavailable"), - FakeGet::Missing, - ), - ]); - - let response = registry.list_managed().await; - - assert_eq!(response.data, vec![docker]); - assert_eq!(response.meta.provider_errors, vec![ - SandboxProviderLookupError { - provider: SandboxProviderKind::DAYTONA, - message: "daytona unavailable".to_string(), - } + let kinds: Vec<_> = inventory.kinds().cloned().collect(); + assert_eq!(kinds, [ + SandboxProviderKind::LOCAL, + SandboxProviderKind::DOCKER, + SandboxProviderKind::DAYTONA ]); } #[tokio::test] - async fn get_returns_one_matching_sandbox() { - let docker = fake_sandbox_info(SandboxProviderKind::DOCKER, "same-id"); - let registry = fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Found(Box::new(docker.clone())), - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - ]); + async fn list_reports_a_provider_that_cannot_connect_beside_the_others() { + let inventory = unreachable_plugin( + SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &["docker-1"])), + "e2b", + ); - assert_eq!( - registry.get_managed_by_native_id("same-id").await.unwrap(), - docker + let response = inventory.list_managed().await; + + assert_eq!(response.data.len(), 1); + assert_eq!(response.meta.provider_errors.len(), 1); + assert_eq!(response.meta.provider_errors[0].provider, kind("e2b")); + assert!( + response.meta.provider_errors[0] + .message + .contains("Failed to connect to the e2b provider"), + "{}", + response.meta.provider_errors[0].message ); } #[tokio::test] - async fn get_returns_not_found_when_all_providers_miss() { - let registry = fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - ]); + async fn get_finds_one_sandbox_by_native_id() { + let inventory = SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &[])) + .with_connected(provider(SandboxProviderKind::DAYTONA, &["native-id"])); - let err = registry + let sandbox = inventory + .get_managed_by_native_id("native-id") + .await + .expect("one provider matches"); + + assert_eq!(sandbox.id, "native-id"); + assert_eq!(sandbox.provider, SandboxProviderKind::DAYTONA); + } + + #[tokio::test] + async fn get_reports_not_found_when_every_provider_misses() { + let inventory = SandboxInventory::empty() + .with_host_directories(SandboxProviderKind::LOCAL) + .with_connected(provider(SandboxProviderKind::DOCKER, &[])); + + let error = inventory .get_managed_by_native_id("missing") .await - .unwrap_err(); + .expect_err("nothing matches"); - assert!(matches!(err, SandboxLookupError::NotFound { id } if id == "missing")); + assert!(matches!(error, SandboxLookupError::NotFound { id } if id == "missing")); } #[tokio::test] - async fn get_returns_conflict_when_two_providers_match() { - let registry = fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Found(Box::new(fake_sandbox_info( - SandboxProviderKind::DOCKER, - "same-id", - ))), - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Found(Box::new(fake_sandbox_info( - SandboxProviderKind::DAYTONA, - "same-id", - ))), - ), - ]); + async fn get_reports_a_conflict_when_two_providers_match() { + let inventory = SandboxInventory::empty() + .with_connected(provider(SandboxProviderKind::DOCKER, &["same-id"])) + .with_connected(provider(SandboxProviderKind::DAYTONA, &["same-id"])); - let err = registry + let error = inventory .get_managed_by_native_id("same-id") .await - .unwrap_err(); + .expect_err("two providers match"); - assert!(matches!( - err, - SandboxLookupError::Conflict { id, providers } - if id == "same-id" - && providers == vec![SandboxProviderKind::DOCKER, SandboxProviderKind::DAYTONA] - )); + let SandboxLookupError::Conflict { providers, .. } = error else { + panic!("expected a conflict, got {error:?}"); + }; + assert_eq!(providers, [ + SandboxProviderKind::DOCKER, + SandboxProviderKind::DAYTONA + ]); } #[tokio::test] - async fn get_returns_provider_unavailable_when_no_match_and_one_provider_fails() { - let registry = fake_registry(vec![ - FakeSandboxProvider::new( - SandboxProviderKind::DOCKER, - FakeList::Ok(Vec::new()), - FakeGet::Missing, - ), - FakeSandboxProvider::new( - SandboxProviderKind::DAYTONA, - FakeList::Ok(Vec::new()), - FakeGet::Err("daytona unavailable"), - ), - ]); + async fn get_is_unavailable_when_no_match_and_a_provider_failed() { + let inventory = unreachable_plugin( + SandboxInventory::empty().with_connected(provider(SandboxProviderKind::DOCKER, &[])), + "e2b", + ); - let err = registry + let error = inventory .get_managed_by_native_id("maybe-missing") .await - .unwrap_err(); + .expect_err("the failed provider may have held it"); - assert!(matches!( - err, - SandboxLookupError::ProviderUnavailable { - id, - provider_errors - } if id == "maybe-missing" - && provider_errors == vec![SandboxProviderLookupError { - provider: SandboxProviderKind::DAYTONA, - message: "daytona unavailable".to_string(), - }] + let SandboxLookupError::ProviderUnavailable { + provider_errors, .. + } = error + else { + panic!("expected provider unavailable, got {error:?}"); + }; + assert_eq!(provider_errors.len(), 1); + assert_eq!(provider_errors[0].provider, kind("e2b")); + } + + #[tokio::test] + async fn get_ignores_a_sandbox_without_the_managed_label() { + let foreign = Arc::new( + ScriptedSandbox::with_id_and_working_dir("foreign", "/work") + .state(SandboxState::Running), + ); + let inventory = SandboxInventory::empty().with_connected(scripted_inventory_provider( + SandboxProviderKind::DOCKER, + vec![foreign], )); + + let error = inventory + .get_managed_by_native_id("foreign") + .await + .expect_err("a foreign sandbox is not in the inventory"); + + assert!(matches!(error, SandboxLookupError::NotFound { .. })); } } diff --git a/lib/components/fabro-sandbox/src/provider/driver.rs b/lib/components/fabro-sandbox/src/provider/driver.rs deleted file mode 100644 index 069469b77..000000000 --- a/lib/components/fabro-sandbox/src/provider/driver.rs +++ /dev/null @@ -1,234 +0,0 @@ -//! Fabro-managed inventory over a sandbox-driver provider. -//! -//! Lists and looks up the sandboxes fabro created, identified by fabro's -//! own `sh.fabro.managed` label. The driver marks every sandbox it creates -//! with its own label too, but that covers every application on the same -//! daemon or account; the provider is connected through the driver's -//! ownership scope, which lists only fabro's sandboxes and refuses to -//! attach to or delete any other. - -use std::sync::Arc; - -use async_trait::async_trait; -use fabro_types::settings::server::ServerSandboxProviderSettings; -use fabro_types::{SandboxInfo, SandboxProviderKind}; -use sandbox_driver::{ - Error as DriverError, OwnedProvider, SandboxFilter, SandboxId, - SandboxProvider as DriverProvider, -}; -use tokio::sync::OnceCell; - -use super::SandboxProvider; -use crate::driver::{ConnectedProvider, ProviderConnectOptions, connect_provider}; -use crate::{details, managed_labels}; - -/// How the driver provider behind the inventory is obtained. -enum Connection { - Connected(Arc), - /// Connected on first use, so a registry can be assembled synchronously - /// and a provider that is down surfaces as a lookup error rather than a - /// startup failure. - Lazy(Box), -} - -struct LazyConnection { - settings: ServerSandboxProviderSettings, - options: ProviderConnectOptions, - provider: OnceCell>, -} - -pub struct DriverInventoryProvider { - kind: SandboxProviderKind, - connection: Connection, -} - -impl DriverInventoryProvider { - #[must_use] - pub fn new(connected: ConnectedProvider) -> Self { - Self { - kind: connected.kind, - connection: Connection::Connected(owned(connected.provider)), - } - } - - /// An inventory over a provider connected through - /// [`connect_provider`] on first use. - #[must_use] - pub fn lazy( - kind: SandboxProviderKind, - settings: ServerSandboxProviderSettings, - options: ProviderConnectOptions, - ) -> Self { - Self { - kind, - connection: Connection::Lazy(Box::new(LazyConnection { - settings, - options, - provider: OnceCell::new(), - })), - } - } - - async fn provider(&self) -> crate::Result<&Arc> { - match &self.connection { - Connection::Connected(provider) => Ok(provider), - Connection::Lazy(lazy) => { - lazy.provider - .get_or_try_init(|| async { - connect_provider(&self.kind, &lazy.settings, &lazy.options) - .await - .map(|connected| owned(connected.provider)) - .map_err(|error| { - crate::Error::context( - format!("Failed to connect to the {} provider", self.kind), - error, - ) - }) - }) - .await - } - } - } - - async fn describe_managed( - &self, - id: &str, - ) -> crate::Result> { - // An id the driver cannot even name is not one of ours. - let Ok(sandbox_id) = SandboxId::try_new(id) else { - return Ok(None); - }; - let handle = match self.provider().await?.attach(&sandbox_id, None).await { - Ok(handle) => handle, - // Unknown to the provider, or not fabro's: neither is in the - // inventory. - Err(DriverError::NotFound { .. } | DriverError::NotOwned { .. }) => return Ok(None), - Err(error) => { - return Err(crate::Error::context( - format!("Failed to look up {} sandbox '{id}'", self.kind), - error, - )); - } - }; - let status = handle.describe().await.map_err(|error| { - crate::Error::context( - format!("Failed to describe {} sandbox '{id}'", self.kind), - error, - ) - })?; - if status.state == sandbox_driver::SandboxState::Deleted { - return Ok(None); - } - Ok(Some(status)) - } -} - -/// The provider narrowed to fabro's sandboxes. -fn owned(provider: Arc) -> Arc { - Arc::new(OwnedProvider::new( - provider, - managed_labels::ownership(None), - )) -} - -#[async_trait] -impl SandboxProvider for DriverInventoryProvider { - fn kind(&self) -> SandboxProviderKind { - self.kind.clone() - } - - async fn list(&self) -> crate::Result> { - let statuses = self - .provider() - .await? - .list(&SandboxFilter::default()) - .await - .map_err(|error| { - crate::Error::context(format!("Failed to list {} sandboxes", self.kind), error) - })?; - Ok(statuses - .iter() - .map(|status| details::info_from_status(&self.kind, status)) - .collect()) - } - - async fn get(&self, id: &str) -> crate::Result> { - Ok(self - .describe_managed(id) - .await? - .map(|status| details::info_from_status(&self.kind, &status))) - } - - async fn delete(&self, id: &str) -> crate::Result<()> { - // Missing or already deleted is an idempotent success; the scope - // refuses a sandbox that is not fabro's, which must never be - // deleted here. - let Ok(sandbox_id) = SandboxId::try_new(id) else { - return Ok(()); - }; - match self.provider().await?.delete(&sandbox_id, None).await { - Ok(()) => Ok(()), - Err(DriverError::NotOwned { .. }) => Err(crate::Error::message(format!( - "Refusing to delete {} sandbox '{id}' because it is missing label {}={}", - self.kind, - managed_labels::MANAGED_LABEL, - managed_labels::MANAGED_LABEL_VALUE - ))), - Err(error) => Err(crate::Error::context( - format!("Failed to delete {} sandbox '{id}'", self.kind), - error, - )), - } - } -} - -#[cfg(test)] -mod tests { - use sandbox_driver::{SandboxSource, SandboxSpec}; - use sandbox_driver_host::HostProvider; - - use super::*; - - fn inventory() -> (DriverInventoryProvider, Arc) { - let host = Arc::new(HostProvider::new()); - let provider = DriverInventoryProvider::new(ConnectedProvider { - kind: SandboxProviderKind::try_new("host").unwrap(), - provider: host.clone(), - }); - (provider, host) - } - - #[tokio::test] - async fn lists_and_deletes_only_fabro_managed_sandboxes() { - let (inventory, host) = inventory(); - let ours = host - .create( - &SandboxSpec::new(SandboxSource::HostDirectory) - .label(managed_labels::MANAGED_LABEL, "true"), - None, - ) - .await - .unwrap(); - let theirs = host - .create(&SandboxSpec::new(SandboxSource::HostDirectory), None) - .await - .unwrap(); - - let listed = inventory.list().await.unwrap(); - assert_eq!(listed.len(), 1); - assert_eq!(listed[0].id, ours.id().as_str()); - assert_eq!(listed[0].provider.as_str(), "host"); - assert!(inventory.get(ours.id().as_str()).await.unwrap().is_some()); - assert!(inventory.get(theirs.id().as_str()).await.unwrap().is_none()); - - let refused = inventory.delete(theirs.id().as_str()).await.unwrap_err(); - assert!( - refused.to_string().contains("Refusing to delete"), - "{refused}" - ); - inventory.delete(ours.id().as_str()).await.unwrap(); - assert!(inventory.get(ours.id().as_str()).await.unwrap().is_none()); - inventory.delete(ours.id().as_str()).await.unwrap(); - inventory.delete("never-existed").await.unwrap(); - } -} diff --git a/lib/components/fabro-sandbox/src/test_support.rs b/lib/components/fabro-sandbox/src/test_support.rs index 734aa9711..b2a389169 100644 --- a/lib/components/fabro-sandbox/src/test_support.rs +++ b/lib/components/fabro-sandbox/src/test_support.rs @@ -15,10 +15,14 @@ use fabro_types::SandboxProviderKind; use sandbox_driver::{ ExecResult, GrepMatch, PlatformInfo, SandboxState, StderrTail, Termination, WalkedFile, }; -pub use sandbox_driver_testing::{ScriptedExec, ScriptedSandbox, ScriptedStdioProcess}; +pub use sandbox_driver_testing::{ + ScriptedExec, ScriptedProvider, ScriptedSandbox, ScriptedStdioProcess, +}; use tokio::io::DuplexStream; +use crate::driver::ConnectedProvider; use crate::driver_sandbox::RunSandbox; +use crate::managed_labels::{MANAGED_LABEL, MANAGED_LABEL_VALUE}; use crate::sandbox::SandboxFile; /// A driver [`ExecResult`] with the given streams, for scripting a mock @@ -418,98 +422,32 @@ impl MockStdioProcess { } } -// --- FakeSandboxProvider --- +// --- Inventory doubles --- -pub use fake_provider::{FakeGet, FakeList, FakeSandboxProvider, fake_registry, fake_sandbox_info}; +/// A running scripted sandbox carrying fabro's managed label, so an owned +/// inventory lists it and attaches to it. +#[must_use] +pub fn managed_scripted_sandbox(id: &str) -> Arc { + Arc::new( + ScriptedSandbox::with_id_and_working_dir(id, "/work") + .state(SandboxState::Running) + .label(MANAGED_LABEL, MANAGED_LABEL_VALUE), + ) +} -mod fake_provider { - use std::collections::BTreeMap; - use std::sync::Arc; - - use async_trait::async_trait; - use fabro_types::{ - SandboxInfo, SandboxNetwork, SandboxProviderKind, SandboxResources, SandboxState, - SandboxTimestamps, - }; - - use crate::provider::{SandboxProvider, SandboxProviderRegistry}; - - #[derive(Clone)] - pub enum FakeList { - Ok(Vec), - Err(&'static str), +/// A connected inventory provider of `kind` holding `sandboxes`, over the +/// driver's scripted provider. +#[must_use] +pub fn scripted_inventory_provider( + kind: SandboxProviderKind, + sandboxes: Vec>, +) -> ConnectedProvider { + let provider = ScriptedProvider::new(kind.as_str()); + for sandbox in sandboxes { + provider.register(sandbox); } - - #[derive(Clone)] - pub enum FakeGet { - Found(Box), - Missing, - Err(&'static str), - } - - pub struct FakeSandboxProvider { - kind: SandboxProviderKind, - list: FakeList, - get: FakeGet, - } - - impl FakeSandboxProvider { - pub fn new(kind: SandboxProviderKind, list: FakeList, get: FakeGet) -> Self { - Self { kind, list, get } - } - } - - #[async_trait] - impl SandboxProvider for FakeSandboxProvider { - fn kind(&self) -> SandboxProviderKind { - self.kind.clone() - } - - async fn list(&self) -> crate::Result> { - match &self.list { - FakeList::Ok(sandboxes) => Ok(sandboxes.clone()), - FakeList::Err(message) => Err(crate::Error::message(*message)), - } - } - - async fn get(&self, _id: &str) -> crate::Result> { - match &self.get { - FakeGet::Found(sandbox) => Ok(Some((**sandbox).clone())), - FakeGet::Missing => Ok(None), - FakeGet::Err(message) => Err(crate::Error::message(*message)), - } - } - - async fn delete(&self, _id: &str) -> crate::Result<()> { - Ok(()) - } - } - - pub fn fake_registry(providers: Vec) -> SandboxProviderRegistry { - SandboxProviderRegistry::new( - providers - .into_iter() - .map(|provider| Arc::new(provider) as Arc) - .collect(), - ) - } - - pub fn fake_sandbox_info(provider: SandboxProviderKind, id: &str) -> SandboxInfo { - SandboxInfo { - provider, - id: id.to_string(), - display_name: None, - state: SandboxState::Running, - native_state: None, - image: None, - snapshot: None, - region: None, - web_url: None, - working_directory: None, - resources: SandboxResources::default(), - network: SandboxNetwork::unknown(), - labels: BTreeMap::new(), - timestamps: SandboxTimestamps::default(), - } + ConnectedProvider { + kind, + provider: Arc::new(provider), } }