diff --git a/Cargo.lock b/Cargo.lock index f70c0515b..bcc5170f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2294,6 +2294,7 @@ dependencies = [ "libc", "lithos-llm", "paste", + "sandbox-driver", "sandbox-driver-testing", "serde", "serde_json", diff --git a/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx b/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx index 38403196e..a20c95a49 100644 --- a/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx +++ b/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx @@ -26,10 +26,7 @@ function makeIdlePreview(): PreviewMutationShape { } function makeServicesData(data: SandboxService[]) { - return { - data, - meta: { source: "ss" as const }, - }; + return { data }; } const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; @@ -116,46 +113,6 @@ describe("ServicesPanelView", () => { expect(titles).toHaveLength(1); }); - test("shows an iproute2 tip when services were discovered from procfs", () => { - const service: SandboxService = { - port: 3000, - addresses: ["0.0.0.0:3000"], - processes: [], - preview_supported: true, - }; - const renderer = renderView({ - servicesQuery: { - ...makeIdleQuery(), - data: { - data: [service], - meta: { source: "procfs" }, - }, - }, - previewMutation: makeIdlePreview(), - }); - - const tipLabels = renderer.root.findAll( - (node) => - node.type === "span" && - Array.isArray(node.children) && - node.children.includes("Tip:"), - ); - expect(tipLabels).toHaveLength(1); - - const commands = renderer.root.findAll( - (node) => - node.type === "code" && - Array.isArray(node.children) && - node.children.includes("apt-get install iproute2"), - ); - expect(commands).toHaveLength(1); - - const tipText = JSON.stringify(renderer.toJSON()); - expect(tipText).toContain("Install "); - expect(tipText).toContain("ss"); - expect(tipText).toContain(" in the sandbox for improved services listing:"); - }); - test("shows API error state with the error message", () => { const renderer = renderView({ servicesQuery: { diff --git a/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx b/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx index 313cac6ba..400cb6365 100644 --- a/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx +++ b/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx @@ -77,7 +77,6 @@ export function ServicesPanelView({ const [previewError, setPreviewError] = useState(null); const services = servicesQuery.data?.data ?? []; - const discoverySource = servicesQuery.data?.meta.source; const queryErrorMessage = describeQueryError(servicesQuery.error); const showLoading = servicesQuery.isLoading && !servicesQuery.data; const showError = queryErrorMessage !== null && !servicesQuery.data; @@ -150,7 +149,6 @@ export function ServicesPanelView({ ) : ( <> - {discoverySource === "procfs" ? : null} - Tip:{" "} - Install ss in the sandbox - for improved services listing:{" "} - apt-get install iproute2 - - ); -} - function ServicesTable({ services, pendingPort, diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index cef7dc3be..f1647b3b2 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14119,7 +14119,7 @@ components: $ref: "#/components/schemas/SandboxFileEntry" SandboxService: - description: A listening TCP service discovered inside a run sandbox. + description: A TCP port a process inside a run sandbox listens on, as the sandbox driver reports it. type: object required: - port @@ -14135,16 +14135,16 @@ components: example: 3000 addresses: type: array - description: Local bind addresses discovered from `ss` or `/proc/net/tcp*`. + description: Local bind addresses the sandbox reports for the port. items: type: string example: ["127.0.0.1:3000", "[::]:3000"] processes: type: array - description: Visible process summaries when available. Empty when the sandbox only supports `/proc/net/tcp*` discovery. + description: The listening processes, when the sandbox can name them (`node`, or `pid=1234`). Empty when it cannot. items: type: string - example: ['users:(("node",pid=42,fd=23))'] + example: ["node"] preview_supported: type: boolean description: Whether the provider supports an external preview URL for this port. @@ -14155,30 +14155,11 @@ components: type: object required: - data - - meta properties: data: type: array items: $ref: "#/components/schemas/SandboxService" - meta: - $ref: "#/components/schemas/SandboxServiceListMeta" - - SandboxServiceListMeta: - description: Metadata about sandbox service discovery. - type: object - required: - - source - properties: - source: - $ref: "#/components/schemas/SandboxServiceDiscoverySource" - - SandboxServiceDiscoverySource: - description: Tool or kernel interface used to discover sandbox services. - type: string - enum: - - ss - - procfs VncPreviewResponse: description: Response containing a signed noVNC preview URL for a Daytona sandbox. diff --git a/lib/apps/fabro-server/src/demo/mod.rs b/lib/apps/fabro-server/src/demo/mod.rs index 7d23d8c7a..4d890b591 100644 --- a/lib/apps/fabro-server/src/demo/mod.rs +++ b/lib/apps/fabro-server/src/demo/mod.rs @@ -23,7 +23,6 @@ use fabro_api::types::{ RunFilesMeta, RunFilesMetaScope, RunFilesMetaSource, SandboxService, SandboxServiceListResponse, }; -use fabro_types::{SandboxServiceDiscoverySource, SandboxServiceListMeta}; use serde_json::json; use crate::error::ApiError; @@ -405,19 +404,16 @@ pub(crate) async fn list_sandbox_services_stub( SandboxService { port: 3000, addresses: vec!["0.0.0.0:3000".to_string()], - processes: vec![r#"users:(("node",pid=42,fd=23))"#.to_string()], + processes: vec!["node".to_string()], preview_supported: true, }, SandboxService { port: 2500, addresses: vec!["127.0.0.1:2500".to_string()], - processes: vec![r#"users:(("debug",pid=84,fd=19))"#.to_string()], + processes: vec!["debug".to_string()], preview_supported: false, }, ], - meta: SandboxServiceListMeta { - source: SandboxServiceDiscoverySource::Ss, - }, }), ) .into_response() diff --git a/lib/apps/fabro-server/src/server/handler/sandbox.rs b/lib/apps/fabro-server/src/server/handler/sandbox.rs index af65b4f71..6e12be966 100644 --- a/lib/apps/fabro-server/src/server/handler/sandbox.rs +++ b/lib/apps/fabro-server/src/server/handler/sandbox.rs @@ -1,5 +1,4 @@ use std::collections::BTreeMap; -use std::net::{Ipv4Addr, Ipv6Addr}; use std::num::NonZeroU64; use std::sync::Arc; use std::time::Duration; @@ -8,11 +7,10 @@ use axum::extract::ws::{Message as WsMessage, WebSocket, WebSocketUpgrade}; use fabro_sandbox::{ FileKind, ProviderAccess, PtySize, RunSandbox, open_terminal_for_run, reconnect_driver_for_run, }; -use fabro_types::{ - RunSandboxInstance, SandboxProviderKind, SandboxServiceDiscoverySource, SandboxServiceListMeta, -}; +use fabro_types::{RunSandboxInstance, SandboxProviderKind}; use futures_util::FutureExt; use futures_util::future::BoxFuture; +use sandbox_driver::{ListeningPort, Services as _}; use super::super::{ ApiError, AppState, Bytes, HeaderMap, IntoResponse, Json, NamedTempFile, Path, @@ -28,20 +26,7 @@ const DEFAULT_VNC_NO_VNC_PORT: u16 = 6080; const DEFAULT_VNC_TTL_SECS: i32 = 3600; /// Header a Daytona unsigned preview needs; surfaced as the response token. const PREVIEW_TOKEN_HEADER: &str = "x-daytona-preview-token"; -const LIST_SANDBOX_SERVICES_COMMAND: &str = r#"if command -v ss >/dev/null 2>&1; then - ss -H -ltnp && exit 0 -fi -printf 'FABRO_PROC_NET_TCP procfs\n' -for file in /proc/net/tcp /proc/net/tcp6; do - if [ -r "$file" ]; then - printf 'FABRO_PROC_NET_TCP %s\n' "$file" - while IFS= read -r line; do - printf '%s\n' "$line" - done < "$file" - fi -done"#; -const LIST_SANDBOX_SERVICES_FAILURE_LABEL: &str = "sandbox service discovery command"; -const LIST_SANDBOX_SERVICES_TIMEOUT_MS: u64 = 5_000; +const LIST_SANDBOX_SERVICES_FAILURE_LABEL: &str = "sandbox service discovery"; // Daytona's signed preview points at the noVNC service root, which serves a // directory listing. Force the iframe to the actual viewer page with // autoconnect+scale so the user lands on the desktop, not a file index. @@ -586,145 +571,43 @@ async fn list_sandbox_services( Ok(sandbox) => sandbox, Err(response) => return response, }; - let result = match sandbox - .exec_command( - LIST_SANDBOX_SERVICES_COMMAND, - LIST_SANDBOX_SERVICES_TIMEOUT_MS, - None, - None, - None, - ) - .await - { - Ok(result) => result, + let services = match sandbox.services() { + Ok(services) => services, Err(err) => { - return ApiError::new(StatusCode::CONFLICT, err.display_with_causes()).into_response(); + return ApiError::new(StatusCode::NOT_IMPLEMENTED, err.display_with_causes()) + .into_response(); + } + }; + let ports = match services.listening_ports().await { + Ok(ports) => ports, + Err(err) => { + return ApiError::new( + StatusCode::CONFLICT, + format!("{LIST_SANDBOX_SERVICES_FAILURE_LABEL} failed: {err}"), + ) + .into_response(); } }; - if !result.success() { - return ApiError::new( - StatusCode::CONFLICT, - sandbox_service_command_failure_detail(&result), - ) - .into_response(); - } - - let discovery = parse_sandbox_services(&result.stdout_lossy(), &provider); Json(SandboxServiceListResponse { - data: discovery.services, - meta: SandboxServiceListMeta { - source: discovery.source, - }, + data: services_from_ports(ports, &provider), }) .into_response() } -fn sandbox_service_command_failure_detail(result: &fabro_sandbox::ExecResult) -> String { - let stderr = result.stderr_lossy(); - let stderr = stderr.trim(); - if !stderr.is_empty() { - return stderr.to_string(); - } - let stdout = result.stdout_lossy(); - let stdout = stdout.trim(); - if !stdout.is_empty() { - return stdout.to_string(); - } - format!("{LIST_SANDBOX_SERVICES_FAILURE_LABEL} failed") -} - -struct SandboxServiceDiscovery { - services: Vec, - source: SandboxServiceDiscoverySource, -} - -fn parse_sandbox_services(output: &str, provider: &SandboxProviderKind) -> SandboxServiceDiscovery { - if output - .lines() - .any(|line| line.trim_start().starts_with("FABRO_PROC_NET_TCP ")) - { - SandboxServiceDiscovery { - services: parse_proc_net_listening_services(output, provider), - source: SandboxServiceDiscoverySource::Procfs, - } - } else { - SandboxServiceDiscovery { - services: parse_ss_listening_services(output, provider), - source: SandboxServiceDiscoverySource::Ss, - } - } -} - -fn parse_ss_listening_services( - output: &str, +/// The driver's listeners grouped by port, previewable ports first. +fn services_from_ports( + ports: Vec, provider: &SandboxProviderKind, ) -> Vec { let mut services = BTreeMap::::new(); - for line in output - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - { - let fields = line.split_whitespace().collect::>(); - let Some(address) = fields.get(3).copied() else { - continue; - }; - let Some(port) = parse_ss_local_port(address) else { - continue; - }; - let process = (fields.len() > 5).then(|| fields[5..].join(" ")); - push_service(&mut services, provider, port, address.to_string(), process); - } - sorted_services(services) -} - -fn parse_ss_local_port(address: &str) -> Option { - let port = address.rsplit_once(':')?.1.parse::().ok()?; - (port > 0).then_some(port) -} - -#[derive(Clone, Copy)] -enum ProcNetFamily { - Ipv4, - Ipv6, -} - -fn parse_proc_net_listening_services( - output: &str, - provider: &SandboxProviderKind, -) -> Vec { - let mut services = BTreeMap::::new(); - let mut family = None; - for line in output - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - { - if let Some(path) = line.strip_prefix("FABRO_PROC_NET_TCP ") { - family = if path.ends_with("/tcp6") { - Some(ProcNetFamily::Ipv6) - } else { - Some(ProcNetFamily::Ipv4) - }; - continue; - } - if line.starts_with("sl") { - continue; - } - let Some(family) = family else { - continue; - }; - let fields = line.split_whitespace().collect::>(); - let (Some(local_address), Some(state)) = (fields.get(1), fields.get(3)) else { - continue; - }; - if *state != "0A" { - continue; - } - let Some((address, port)) = parse_proc_net_local_address(local_address, family) else { - continue; - }; - push_service(&mut services, provider, port, address, None); + for listener in ports { + push_service( + &mut services, + provider, + listener.port, + listener.address, + listener.process, + ); } sorted_services(services) } @@ -735,40 +618,6 @@ fn sorted_services(services: BTreeMap) -> Vec Option<(String, u16)> { - let (address_hex, port_hex) = value.split_once(':')?; - let port = u16::from_str_radix(port_hex, 16).ok()?; - if port == 0 { - return None; - } - let address = match family { - ProcNetFamily::Ipv4 => format!("{}:{port}", parse_proc_net_ipv4(address_hex)?), - ProcNetFamily::Ipv6 => format!("[{}]:{port}", parse_proc_net_ipv6(address_hex)?), - }; - Some((address, port)) -} - -fn parse_proc_net_ipv4(value: &str) -> Option { - if value.len() != 8 { - return None; - } - let raw = u32::from_str_radix(value, 16).ok()?; - Some(Ipv4Addr::from(raw.to_le_bytes())) -} - -fn parse_proc_net_ipv6(value: &str) -> Option { - if value.len() != 32 { - return None; - } - let mut bytes = [0_u8; 16]; - for (chunk_index, chunk) in value.as_bytes().chunks_exact(8).enumerate() { - let chunk = std::str::from_utf8(chunk).ok()?; - let raw = u32::from_str_radix(chunk, 16).ok()?; - bytes[chunk_index * 4..chunk_index * 4 + 4].copy_from_slice(&raw.to_le_bytes()); - } - Some(Ipv6Addr::from(bytes)) -} - fn push_service( services: &mut BTreeMap, provider: &SandboxProviderKind, @@ -927,8 +776,6 @@ async fn load_run_sandbox_instance( #[cfg(test)] mod tests { use axum::http::{HeaderMap, HeaderValue}; - use fabro_sandbox::Termination; - use fabro_sandbox::test_support::exec_result; use futures_util::FutureExt; use super::*; @@ -1017,104 +864,28 @@ mod tests { } #[test] - fn ss_parser_extracts_addresses_processes_and_preview_support() { - let services = parse_ss_listening_services( - r#" -LISTEN 0 4096 127.0.0.1:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 0.0.0.0:5173 0.0.0.0:* users:(("vite",pid=84,fd=19)) -LISTEN 0 4096 [::]:8080 [::]:* users:(("server",pid=126,fd=9)) -LISTEN 0 4096 [::1]:2500 [::]:* users:(("debug",pid=168,fd=7)) -"#, - &SandboxProviderKind::DAYTONA, - ); - - assert_eq!(services.len(), 4); - assert_eq!(services[0].port, 3000); - assert_eq!(services[0].addresses, vec!["127.0.0.1:3000"]); - assert_eq!(services[0].processes, vec![ - r#"users:(("node",pid=42,fd=23))"# - ]); - assert!(services[0].preview_supported); - assert_eq!(services[1].port, 5173); - assert_eq!(services[1].addresses, vec!["0.0.0.0:5173"]); - assert!(services[1].preview_supported); - assert_eq!(services[2].port, 8080); - assert_eq!(services[2].addresses, vec!["[::]:8080"]); - assert!(services[2].preview_supported); - assert_eq!(services[3].port, 2500); - assert_eq!(services[3].addresses, vec!["[::1]:2500"]); - assert_eq!(services[3].processes, vec![ - r#"users:(("debug",pid=168,fd=7))"# - ]); - assert!(!services[3].preview_supported); - } - - #[test] - fn ss_parser_ignores_malformed_and_non_numeric_ports() { - let services = parse_ss_listening_services( - r#" -LISTEN 0 4096 127.0.0.1:not-a-port 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 missing-peer -not enough fields -LISTEN 0 4096 127.0.0.1:0 0.0.0.0:* users:(("zero",pid=1,fd=2)) -LISTEN 0 4096 127.0.0.1:65536 0.0.0.0:* users:(("large",pid=1,fd=2)) -"#, - &SandboxProviderKind::DAYTONA, - ); - - assert!(services.is_empty()); - } - - #[test] - fn ss_parser_groups_duplicate_ports_and_deduplicates_values() { - let services = parse_ss_listening_services( - r#" -LISTEN 0 4096 127.0.0.1:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 0.0.0.0:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 127.0.0.1:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 [::]:3000 [::]:* users:(("vite",pid=84,fd=19)) -"#, - &SandboxProviderKind::DAYTONA, - ); - - assert_eq!(services, vec![SandboxService { - port: 3000, - addresses: vec![ - "127.0.0.1:3000".to_string(), - "0.0.0.0:3000".to_string(), - "[::]:3000".to_string(), + fn listening_ports_group_by_port_and_sort_previewable_first() { + let mut node = ListeningPort::new(3000, "127.0.0.1:3000"); + node.process = Some("node".to_string()); + let mut node_v6 = ListeningPort::new(3000, "[::]:3000"); + node_v6.process = Some("node".to_string()); + let mut debug = ListeningPort::new(2500, "[::1]:2500"); + debug.process = Some("pid=168".to_string()); + let services = services_from_ports( + vec![ + debug, + node, + node_v6, + ListeningPort::new(5173, "0.0.0.0:5173"), ], - processes: vec![ - r#"users:(("node",pid=42,fd=23))"#.to_string(), - r#"users:(("vite",pid=84,fd=19))"#.to_string(), - ], - preview_supported: true, - }]); - } - - #[test] - fn proc_net_parser_extracts_listening_tcp_services_without_processes() { - let discovery = parse_sandbox_services( - r" -FABRO_PROC_NET_TCP /proc/net/tcp - sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode - 0: 0100007F:0BB8 00000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 11111 - 1: 00000000:1435 00000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 22222 - 2: 0100007F:2328 00000000:0000 01 00000000:00000000 00:00000000 00000000 501 0 33333 -FABRO_PROC_NET_TCP /proc/net/tcp6 - sl local_address remote_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode - 0: 00000000000000000000000000000000:1F90 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 44444 - 1: 00000000000000000000000001000000:09C4 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 55555 -", &SandboxProviderKind::DAYTONA, ); - assert_eq!(discovery.source, SandboxServiceDiscoverySource::Procfs); - assert_eq!(discovery.services, vec![ + assert_eq!(services, vec![ SandboxService { port: 3000, - addresses: vec!["127.0.0.1:3000".to_string()], - processes: vec![], + addresses: vec!["127.0.0.1:3000".to_string(), "[::]:3000".to_string()], + processes: vec!["node".to_string()], preview_supported: true, }, SandboxService { @@ -1123,16 +894,10 @@ FABRO_PROC_NET_TCP /proc/net/tcp6 processes: vec![], preview_supported: true, }, - SandboxService { - port: 8080, - addresses: vec!["[::]:8080".to_string()], - processes: vec![], - preview_supported: true, - }, SandboxService { port: 2500, addresses: vec!["[::1]:2500".to_string()], - processes: vec![], + processes: vec!["pid=168".to_string()], preview_supported: false, }, ]); @@ -1147,33 +912,6 @@ FABRO_PROC_NET_TCP /proc/net/tcp6 assert!(!preview_supported(&SandboxProviderKind::DOCKER, 3000)); } - #[test] - fn sandbox_service_command_failure_prefers_stderr_then_stdout() { - let mut result = exec_result( - "stdout detail", - "stderr detail", - Some(127), - Termination::Exited, - 10, - ); - assert_eq!( - sandbox_service_command_failure_detail(&result), - "stderr detail" - ); - - result.stderr.clear(); - assert_eq!( - sandbox_service_command_failure_detail(&result), - "stdout detail" - ); - - result.stdout.clear(); - assert_eq!( - sandbox_service_command_failure_detail(&result), - "sandbox service discovery command failed" - ); - } - struct FakeVncSandbox { error: Option<&'static str>, viewer_url: &'static str, diff --git a/lib/components/fabro-agent/Cargo.toml b/lib/components/fabro-agent/Cargo.toml index 53aa693ac..73d05998e 100644 --- a/lib/components/fabro-agent/Cargo.toml +++ b/lib/components/fabro-agent/Cargo.toml @@ -29,6 +29,7 @@ lithos-llm = { workspace = true, features = ["runtime"] } fabro-llm = { path = "../fabro-llm" } fabro-mcp = { path = "../fabro-mcp" } fabro-sandbox = { path = "../fabro-sandbox" } +sandbox-driver.workspace = true fabro-static.workspace = true fabro-template = { path = "../../foundation/fabro-template" } fabro-util = { path = "../../foundation/fabro-util" } diff --git a/lib/components/fabro-agent/src/session.rs b/lib/components/fabro-agent/src/session.rs index 152de768a..60a5d6975 100644 --- a/lib/components/fabro-agent/src/session.rs +++ b/lib/components/fabro-agent/src/session.rs @@ -22,6 +22,7 @@ use lithos_llm::types::{ ContentPart, Message as LlmMessage, ReasoningEffort, Role, Speed, TokenCounts, ToolCall, ToolChoice, }; +use sandbox_driver::{ServiceId, ServiceSpec, Services as _, ServicesFacet}; use tokio::sync::{Notify, broadcast}; use tokio::time; use tokio_util::sync::CancellationToken; @@ -852,14 +853,14 @@ impl Session { Ok(resolved) } - /// Start an MCP server inside the sandbox and return (url, headers) for - /// HTTP connection. + /// Start an MCP server inside the sandbox as a driver service and return + /// (url, headers) for HTTP connection. /// /// The outer `Result` surfaces fatal cancellation as - /// `Error::Interrupted(InterruptReason::Cancelled)` (the running MCP - /// process group is terminated before returning). The inner `Result` - /// captures non-fatal startup failures that the caller logs and turns - /// into an `McpServerFailed` event. + /// `Error::Interrupted(InterruptReason::Cancelled)` (a service already + /// started is stopped before returning). The inner `Result` captures + /// non-fatal startup failures that the caller logs and turns into an + /// `McpServerFailed` event. async fn start_sandbox_mcp_server( &self, command: &[String], @@ -868,82 +869,56 @@ impl Session { cancel_token: &CancellationToken, ) -> Result), String>, Error> { let sandbox = self.sandbox.as_ref(); - - let launch_script = sandbox_mcp_launch_script(command); - let env_ref = if env.is_empty() { None } else { Some(env) }; + let services = match sandbox.services() { + Ok(services) => services, + Err(error) => { + return Ok(Err(format!( + "Failed to launch MCP server: {}", + error.display_with_causes() + ))); + } + }; + let mut spec = ServiceSpec::new(mcp_service_command(command)); + for (key, value) in env { + spec = spec.env_var(key.clone(), value.clone()); + } if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } - let launch_result = match sandbox - .exec_command( - &launch_script, - 30_000, - None, - env_ref, - Some(cancel_token.child_token()), - ) - .await - { - Ok(result) => result, - Err(e) => { + let service = match services.spawn(&spec).await { + Ok(service) => service, + Err(error) => { if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } - return Ok(Err(format!( - "Failed to launch MCP server: {}", - e.display_with_causes() - ))); + return Ok(Err(format!("Failed to launch MCP server: {error}"))); } }; - - let pid = launch_result.stdout_lossy().trim().to_string(); - info!(pid = %pid, port, "MCP server process launched in sandbox"); - - // Wait for the server to start listening on the port - let poll_cmd = format!( - "for i in $(seq 1 30); do ss -tln | grep -q ':{port} ' && echo ready && exit 0; sleep 1; done; echo timeout" + info!( + service = service.as_str(), + port, "MCP server started as a sandbox service" ); - let poll_result = sandbox - .exec_command( - &poll_cmd, - 60_000, - None, - None, - Some(cancel_token.child_token()), - ) - .await; - if cancel_token.is_cancelled() { - kill_mcp_pid(sandbox, &pid).await; - return Err(Error::Interrupted(InterruptReason::Cancelled)); - } - - let poll_result = match poll_result { - Ok(result) => result, - Err(e) => { - return Ok(Err(format!( - "Failed to poll MCP server readiness: {}", - e.display_with_causes() - ))); + // Wait for the server to listen; a cancellation stops it. + let ready = tokio::select! { + () = cancel_token.cancelled() => { + stop_mcp_service(&services, &service).await; + return Err(Error::Interrupted(InterruptReason::Cancelled)); } + ready = services.wait_for_port(port, MCP_SERVER_READY_TIMEOUT) => ready, }; - - if poll_result.stdout_lossy().trim() != "ready" { - // Grab stderr for debugging - let stderr = sandbox - .exec_command( - "cat /tmp/mcp_server_stderr.log 2>/dev/null | tail -20", - 10_000, - None, - None, - Some(cancel_token.child_token()), - ) + if let Err(error) = ready { + let logs = services + .logs(&service, MCP_SERVER_LOG_TAIL_BYTES) .await - .map(|r| r.stdout_lossy()) + .map(|bytes| String::from_utf8_lossy(&bytes).into_owned()) .unwrap_or_default(); + stop_mcp_service(&services, &service).await; return Ok(Err(format!( - "MCP server did not start listening on port {port} within 30s. stderr:\n{stderr}" + "MCP server did not start listening on port {port} within {}s ({error}). \ + logs:\n{logs}", + MCP_SERVER_READY_TIMEOUT.as_secs() ))); } @@ -955,7 +930,7 @@ impl Session { }; if cancel_token.is_cancelled() { - kill_mcp_pid(sandbox, &pid).await; + stop_mcp_service(&services, &service).await; return Err(Error::Interrupted(InterruptReason::Cancelled)); } @@ -2192,49 +2167,31 @@ impl Session { } } -/// Build the script that launches a sandbox MCP server detached and echoes its -/// PID. +/// How long a sandbox MCP server gets to start listening on its port. +const MCP_SERVER_READY_TIMEOUT: Duration = Duration::from_secs(30); +/// How much of a failed MCP server's output the failure message carries. +const MCP_SERVER_LOG_TAIL_BYTES: usize = 4096; + +/// The Bash source a sandbox MCP server runs as a service. /// -/// `setsid` fully detaches the server so Daytona's exec doesn't block on it. -/// The inner command is shell-quoted for the wrapper so a single quote or -/// metacharacter in any argv element can't break out, and the wrapper itself is -/// the current `$BASH` because the sandbox evaluates this string as non-login -/// Bash and may resolve that executable outside `/bin` (for example on NixOS). -fn sandbox_mcp_launch_script(command: &[String]) -> String { - let command_source = match command { - // Sandbox MCP `script` entries resolve to this exact argv shape. The - // surrounding launcher is already the provider-selected Bash, so - // evaluate the source in that process instead of PATH-resolving a - // second interpreter. Grouping keeps the log redirections scoped to - // the whole script, including multi-command and trailing-comment - // forms. - [interpreter, flag, source] if interpreter == "bash" && flag == "-c" => { - format!("{{\n{source}\n}}") - } +/// Sandbox MCP `script` entries resolve to the argv shape `bash -c `. +/// The service already runs in the provider-selected Bash, so the source +/// runs there as it is instead of PATH-resolving a second interpreter (which +/// may live outside `/bin`, for example on NixOS). Any other argv is quoted +/// into one command line, so a quote or metacharacter in an element stays +/// inert. +fn mcp_service_command(command: &[String]) -> String { + match command { + [interpreter, flag, source] if interpreter == "bash" && flag == "-c" => source.clone(), _ => shell::shell_join(command), - }; - let inner = - format!("{command_source} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log"); - format!( - "setsid \"$BASH\" -c {quoted} /dev/null 2>&1 &\necho $!", - quoted = shell::shell_quote(&inner) - ) + } } -/// Best-effort kill of a sandbox MCP server process group. Used when -/// `start_sandbox_mcp_server` is cancelled after spawning a detached -/// `setsid` child but before reporting readiness. Errors from the sandbox -/// are logged and swallowed; the caller is already returning a Cancelled -/// error. -async fn kill_mcp_pid(sandbox: &RunSandbox, pid: &str) { - let pid = pid.trim(); - if pid.is_empty() { - return; - } - let script = - format!("kill -TERM -{pid} 2>/dev/null; sleep 1; kill -KILL -{pid} 2>/dev/null; true"); - if let Err(err) = sandbox.exec_command(&script, 5_000, None, None, None).await { - warn!(pid, error = %err.display_with_causes(), "Failed to kill MCP server process group during cancellation"); +/// Best-effort stop of a sandbox MCP service that will not be used: the +/// caller is already returning a cancellation or a startup failure. +async fn stop_mcp_service(services: &ServicesFacet<'_>, service: &ServiceId) { + if let Err(error) = services.stop(service).await { + warn!(service = service.as_str(), error = %error, "Failed to stop the MCP server service"); } } @@ -2267,82 +2224,33 @@ mod tests { use crate::tool_registry::{RegisteredTool, ToolContext, ToolRegistry, ToolSource}; #[test] - fn sandbox_mcp_launch_wrapper_uses_bash() { - // The sandbox evaluates this string as non-login Bash, so the detached - // wrapper reuses the executable selected by the provider. - let script = sandbox_mcp_launch_script(&[ - "npx".to_string(), - "@playwright/mcp@latest".to_string(), - "--port".to_string(), - "3100".to_string(), - ]); - - assert!( - script.starts_with("setsid \"$BASH\" -c "), - "launch wrapper should detach through the provider-selected Bash: {script}" - ); - assert!( - script.ends_with(" /dev/null 2>&1 &\necho $!"), - "launch wrapper should stay detached and report its PID: {script}" - ); - assert!( - script.contains("/tmp/mcp_server_stdout.log") - && script.contains("2>/tmp/mcp_server_stderr.log"), - "launch wrapper should keep its log redirection: {script}" - ); - } - - #[test] - fn sandbox_mcp_launch_wrapper_evaluates_scripts_in_the_selected_bash() { + fn mcp_service_command_runs_script_entries_in_the_service_bash() { let source = "PATH=/mcp-only\nprintf 'starting server\\n'\nexec my-server --port 3100 # ready"; - let script = - sandbox_mcp_launch_script(&["bash".to_string(), "-c".to_string(), source.to_string()]); - - let wrapper_argument = script - .strip_prefix("setsid \"$BASH\" -c ") - .and_then(|rest| rest.strip_suffix(" /dev/null 2>&1 &\necho $!")) - .expect("launch wrapper should have the canonical shape"); - let unwrapped = shlex::split(wrapper_argument).expect("wrapper argument should parse"); - - assert_eq!(unwrapped, vec![format!( - "{{\n{source}\n}} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log" - )]); - assert!( - !unwrapped[0].contains("bash -c"), - "script entries must not PATH-resolve a nested Bash: {}", - unwrapped[0] + let command = + mcp_service_command(&["bash".to_string(), "-c".to_string(), source.to_string()]); + assert_eq!( + command, source, + "script entries must not PATH-resolve a nested Bash" ); } #[test] - fn sandbox_mcp_launch_wrapper_quotes_arbitrary_argv() { - // A quote or metacharacter in any argv element must not break out of - // the wrapper; it has to arrive as one argument. - let script = sandbox_mcp_launch_script(&[ + fn mcp_service_command_quotes_arbitrary_argv() { + // A quote or metacharacter in any argv element must not break out + // of the command line; it has to arrive as one argument. + let command = mcp_service_command(&[ "my-server".to_string(), "--flag=it's a value".to_string(), "$(touch /tmp/pwned)".to_string(), ]); - - let wrapper_argument = script - .strip_prefix("setsid \"$BASH\" -c ") - .and_then(|rest| rest.strip_suffix(" /dev/null 2>&1 &\necho $!")) - .expect("launch wrapper should have the canonical shape"); - - // Unwrap the wrapper's own quoting: the whole inner script must arrive - // as one argument to `bash -c`, with each argv element still quoted so - // the substitution stays inert. - let unwrapped = shlex::split(wrapper_argument).expect("wrapper argument should parse"); assert_eq!( - unwrapped.len(), - 1, - "the command must stay a single argument" + command, + "my-server \"--flag=it's a value\" '$(touch /tmp/pwned)'" ); assert_eq!( - unwrapped[0], - "my-server \"--flag=it's a value\" '$(touch /tmp/pwned)' > \ - /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log" + mcp_service_command(&["npx".to_string(), "@playwright/mcp@latest".to_string()]), + "npx @playwright/mcp@latest" ); } diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 7daa4dc92..d66e76c12 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -438,6 +438,20 @@ impl RunSandbox { }) } + /// The driver's services facet for this sandbox: background processes + /// that outlive their exec (the agent's MCP servers, dev servers), the + /// wait for a port to answer, and the list of listeners. Absent until a + /// pending sandbox is initialized, or when the provider has no + /// services. + pub fn services(&self) -> crate::Result> { + self.handle()?.services().ok_or_else(|| { + crate::Error::message(format!( + "sandbox provider `{}` does not support background services", + self.kind + )) + }) + } + fn search(&self) -> crate::Result> { self.handle()?.search().ok_or_else(|| { crate::Error::message(format!( diff --git a/lib/foundation/fabro-api/build.rs b/lib/foundation/fabro-api/build.rs index a05845c2b..df08c012c 100644 --- a/lib/foundation/fabro-api/build.rs +++ b/lib/foundation/fabro-api/build.rs @@ -671,16 +671,6 @@ fn main() { &[], ), ("SandboxService", "fabro_types::SandboxService", &[]), - ( - "SandboxServiceDiscoverySource", - "fabro_types::SandboxServiceDiscoverySource", - &[], - ), - ( - "SandboxServiceListMeta", - "fabro_types::SandboxServiceListMeta", - &[], - ), ( "SandboxServiceListResponse", "fabro_types::SandboxServiceListResponse", diff --git a/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs b/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs index 706ef59f2..4c62059ec 100644 --- a/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs +++ b/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs @@ -4,10 +4,7 @@ use fabro_api::types::{ SandboxService as ApiSandboxService, SandboxServiceListResponse as ApiSandboxServiceListResponse, }; -use fabro_types::{ - SandboxService, SandboxServiceDiscoverySource, SandboxServiceListMeta, - SandboxServiceListResponse, -}; +use fabro_types::{SandboxService, SandboxServiceListResponse}; use serde_json::json; #[test] @@ -22,15 +19,9 @@ fn sandbox_services_json_matches_openapi_shape() { data: vec![SandboxService { port: 3000, addresses: vec!["127.0.0.1:3000".to_string(), "[::]:3000".to_string()], - processes: vec![ - r#"users:(("node",pid=42,fd=23))"#.to_string(), - r#"users:(("vite",pid=84,fd=19))"#.to_string(), - ], + processes: vec!["node".to_string()], preview_supported: true, }], - meta: SandboxServiceListMeta { - source: SandboxServiceDiscoverySource::Ss, - }, }; assert_eq!( @@ -39,27 +30,19 @@ fn sandbox_services_json_matches_openapi_shape() { "data": [{ "port": 3000, "addresses": ["127.0.0.1:3000", "[::]:3000"], - "processes": [ - r#"users:(("node",pid=42,fd=23))"#, - r#"users:(("vite",pid=84,fd=19))"#, - ], + "processes": ["node"], "preview_supported": true - }], - "meta": { - "source": "ss" - } + }] }) ); } #[test] fn sandbox_services_deserializes_empty_response() { - let response: SandboxServiceListResponse = - serde_json::from_value(json!({ "data": [], "meta": { "source": "procfs" } })) - .expect("empty service response should deserialize"); + let response: SandboxServiceListResponse = serde_json::from_value(json!({ "data": [] })) + .expect("empty service response should deserialize"); assert!(response.data.is_empty()); - assert_eq!(response.meta.source, SandboxServiceDiscoverySource::Procfs); } fn assert_same_type() { diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 0926cbdfd..d6fb7b9b6 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -164,10 +164,7 @@ pub use sandbox_inventory::{ pub use sandbox_provider::{ BundledProvider, InvalidSandboxProviderKind, SandboxProviderKind, WorkspacePolicy, }; -pub use sandbox_services::{ - SandboxService, SandboxServiceDiscoverySource, SandboxServiceListMeta, - SandboxServiceListResponse, -}; +pub use sandbox_services::{SandboxService, SandboxServiceListResponse}; pub use secret::{OAuthConfig, OAuthCredential, OAuthTokens, SecretMetadata, SecretType}; pub use session::{ PermissionLevel, SessionDetail, SessionId, SessionMessage, SessionRecord, SessionStatus, diff --git a/lib/foundation/fabro-types/src/sandbox_services.rs b/lib/foundation/fabro-types/src/sandbox_services.rs index 48f61be32..7212c174b 100644 --- a/lib/foundation/fabro-types/src/sandbox_services.rs +++ b/lib/foundation/fabro-types/src/sandbox_services.rs @@ -1,15 +1,3 @@ -#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum SandboxServiceDiscoverySource { - Ss, - Procfs, -} - -#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -pub struct SandboxServiceListMeta { - pub source: SandboxServiceDiscoverySource, -} - #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct SandboxService { pub port: u16, @@ -21,5 +9,4 @@ pub struct SandboxService { #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct SandboxServiceListResponse { pub data: Vec, - pub meta: SandboxServiceListMeta, } diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 664fc9e7e..7f6af8f50 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -443,8 +443,6 @@ models/sandbox-network-policy.ts models/sandbox-plugin-settings.ts models/sandbox-provider-lookup-error.ts models/sandbox-resources.ts -models/sandbox-service-discovery-source.ts -models/sandbox-service-list-meta.ts models/sandbox-service-list-response.ts models/sandbox-service.ts models/sandbox-state.ts diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index c407a60b6..669a9c8a8 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -414,8 +414,6 @@ export * from './sandbox-plugin-settings'; export * from './sandbox-provider-lookup-error'; export * from './sandbox-resources'; export * from './sandbox-service'; -export * from './sandbox-service-discovery-source'; -export * from './sandbox-service-list-meta'; export * from './sandbox-service-list-response'; export * from './sandbox-state'; export * from './sandbox-status'; diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts b/lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts deleted file mode 100644 index 1c6db31fe..000000000 --- a/lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts +++ /dev/null @@ -1,26 +0,0 @@ -/* tslint:disable */ -/* eslint-disable */ -/** - * Fabro Run API - * HTTP API for managing Fabro workflow run executions. - * - * The version of the OpenAPI document: 0.2.0 - * - * - * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). - * https://openapi-generator.tech - * Do not edit the class manually. - */ - - - -/** - * Tool or kernel interface used to discover sandbox services. - */ - -export const SandboxServiceDiscoverySource = { - SS: 'ss', - PROCFS: 'procfs' -} as const; - -export type SandboxServiceDiscoverySource = typeof SandboxServiceDiscoverySource[keyof typeof SandboxServiceDiscoverySource]; diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts b/lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts deleted file mode 100644 index c24d6c81d..000000000 --- a/lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts +++ /dev/null @@ -1,25 +0,0 @@ -/* tslint:disable */ -/* eslint-disable */ -/** - * Fabro Run API - * HTTP API for managing Fabro workflow run executions. - * - * The version of the OpenAPI document: 0.2.0 - * - * - * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). - * https://openapi-generator.tech - * Do not edit the class manually. - */ - - -// May contain unused imports in some cases -// @ts-ignore -import type { SandboxServiceDiscoverySource } from './sandbox-service-discovery-source'; - -/** - * Metadata about sandbox service discovery. - */ -export interface SandboxServiceListMeta { - 'source': SandboxServiceDiscoverySource; -} diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts b/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts index a41676b3b..e68ad21df 100644 --- a/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts +++ b/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts @@ -16,14 +16,10 @@ // May contain unused imports in some cases // @ts-ignore import type { SandboxService } from './sandbox-service'; -// May contain unused imports in some cases -// @ts-ignore -import type { SandboxServiceListMeta } from './sandbox-service-list-meta'; /** * Non-paginated list of listening TCP services in a run sandbox. */ export interface SandboxServiceListResponse { 'data': Array; - 'meta': SandboxServiceListMeta; } diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service.ts b/lib/packages/fabro-api-client/src/models/sandbox-service.ts index 9e89797e7..d0089d47c 100644 --- a/lib/packages/fabro-api-client/src/models/sandbox-service.ts +++ b/lib/packages/fabro-api-client/src/models/sandbox-service.ts @@ -15,7 +15,7 @@ /** - * A listening TCP service discovered inside a run sandbox. + * A TCP port a process inside a run sandbox listens on, as the sandbox driver reports it. */ export interface SandboxService { /** @@ -23,11 +23,11 @@ export interface SandboxService { */ 'port': number; /** - * Local bind addresses discovered from `ss` or `/proc/net/tcp*`. + * Local bind addresses the sandbox reports for the port. */ 'addresses': Array; /** - * Visible process summaries when available. Empty when the sandbox only supports `/proc/net/tcp*` discovery. + * The listening processes, when the sandbox can name them (`node`, or `pid=1234`). Empty when it cannot. */ 'processes': Array; /**