From 286eee7efa294f510990cea0d0d1a3db566f4a5f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 14 Apr 2026 18:15:23 -0400 Subject: [PATCH] fix(graphviz): isolate rendering in a subprocess Run Graphviz through an internal fabro subprocess so renderer failures no longer share process fate with the server. Keep expected DOT parse failures on the 400 path via an explicit stdout protocol, and treat child crashes or protocol violations as 500s. --- Cargo.lock | 1 + lib/crates/fabro-cli/Cargo.toml | 1 + lib/crates/fabro-cli/src/args.rs | 4 + lib/crates/fabro-cli/src/commands/mod.rs | 1 + .../fabro-cli/src/commands/render_graph.rs | 25 ++ lib/crates/fabro-cli/src/main.rs | 18 +- lib/crates/fabro-cli/tests/it/cmd/mod.rs | 1 + .../fabro-cli/tests/it/cmd/render_graph.rs | 105 ++++++++ lib/crates/fabro-server/src/server.rs | 248 +++++++++++++++++- 9 files changed, 392 insertions(+), 12 deletions(-) create mode 100644 lib/crates/fabro-cli/src/commands/render_graph.rs create mode 100644 lib/crates/fabro-cli/tests/it/cmd/render_graph.rs diff --git a/Cargo.lock b/Cargo.lock index 14c6292d2..731a30eb3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1588,6 +1588,7 @@ dependencies = [ "fabro-devcontainer", "fabro-github", "fabro-graphviz", + "fabro-graphviz-sys", "fabro-hooks", "fabro-http", "fabro-interview", diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index edeb11cba..741f76edc 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -34,6 +34,7 @@ fabro-retro = { path = "../fabro-retro" } fabro-sandbox = { path = "../fabro-sandbox", features = ["daytona"] } fabro-checkpoint = { path = "../fabro-checkpoint" } fabro-graphviz = { path = "../fabro-graphviz" } +fabro-graphviz-sys = { path = "../fabro-graphviz-sys" } fabro-validate = { path = "../fabro-validate" } fabro-workflow = { path = "../fabro-workflow" } fabro-server = { path = "../fabro-server" } diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index a8d98959a..441e60643 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -1023,6 +1023,9 @@ pub(crate) enum Commands { /// Path to the JSON event file path: PathBuf, }, + /// Render a DOT graph to SVG (internal) + #[command(name = "__render-graph", hide = true)] + RenderGraph, /// Build a panic event and write JSON to stdout (internal testing) #[cfg(debug_assertions)] #[command(name = "__test_panic", hide = true)] @@ -1100,6 +1103,7 @@ impl Commands { }, Self::SendAnalytics { .. } => "__send_analytics", Self::SendPanic { .. } => "__send_panic", + Self::RenderGraph => "__render-graph", #[cfg(debug_assertions)] Self::TestPanic { .. } => "__test_panic", } diff --git a/lib/crates/fabro-cli/src/commands/mod.rs b/lib/crates/fabro-cli/src/commands/mod.rs index 72995e763..928e3b2fb 100644 --- a/lib/crates/fabro-cli/src/commands/mod.rs +++ b/lib/crates/fabro-cli/src/commands/mod.rs @@ -9,6 +9,7 @@ pub(crate) mod parse; pub(crate) mod pr; pub(crate) mod preflight; pub(crate) mod provider; +pub(crate) mod render_graph; pub(crate) mod repo; pub(crate) mod run; pub(crate) mod runs; diff --git a/lib/crates/fabro-cli/src/commands/render_graph.rs b/lib/crates/fabro-cli/src/commands/render_graph.rs new file mode 100644 index 000000000..ce1ec4e16 --- /dev/null +++ b/lib/crates/fabro-cli/src/commands/render_graph.rs @@ -0,0 +1,25 @@ +use std::io::{Read, Write}; + +const RENDER_ERROR_PREFIX: &str = "RENDER_ERROR:"; + +pub(crate) fn execute() -> i32 { + let mut dot_source = String::new(); + if std::io::stdin().read_to_string(&mut dot_source).is_err() { + return 1; + } + + match fabro_graphviz_sys::render_dot_to_svg(&dot_source) { + Ok(svg) => { + if std::io::stdout().write_all(&svg).is_err() { + return 1; + } + 0 + } + Err(err) => { + if write!(std::io::stdout(), "{RENDER_ERROR_PREFIX}{err}").is_err() { + return 1; + } + 0 + } + } +} diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 35481ee2c..15d3344f6 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -61,11 +61,17 @@ impl Cli { #[expect(clippy::print_stderr, reason = "fatal error reporting before exit")] #[tokio::main] async fn main() { + let raw_args: Vec = std::env::args().collect(); + let subcommand = raw_args.get(1).map(String::as_str); + let subcommand_arg = raw_args.get(2).map(String::as_str); + if subcommand == Some("__render-graph") && !matches!(subcommand_arg, Some("--help" | "-h")) { + std::process::exit(commands::render_graph::execute()); + } + tel_panic::install_panic_hook(); fabro_telemetry::init_cli(); let start = std::time::Instant::now(); - let raw_args: Vec = std::env::args().collect(); let (command_name, result) = Box::pin(main_inner()).await; let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap(); @@ -368,6 +374,7 @@ async fn main_inner() -> (String, Result<()>) { let _ = std::fs::remove_file(&path); result?; } + Commands::RenderGraph => unreachable!("__render-graph handled before CLI bootstrap"), #[cfg(debug_assertions)] Commands::TestPanic { message } => { let event = tel_panic::build_event(&message); @@ -701,6 +708,15 @@ mod tests { } } + #[test] + fn parse_render_graph_command() { + let cli = Cli::try_parse_from(["fabro", "__render-graph"]).expect("should parse"); + match *cli.command { + Commands::RenderGraph => {} + _ => panic!("unexpected command variant"), + } + } + #[test] fn parse_settings_command() { let cli = Cli::try_parse_from(["fabro", "settings"]).expect("should parse"); diff --git a/lib/crates/fabro-cli/tests/it/cmd/mod.rs b/lib/crates/fabro-cli/tests/it/cmd/mod.rs index aad21bc9a..673fcd3ae 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mod.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mod.rs @@ -29,6 +29,7 @@ mod preflight; mod provider; mod provider_login; mod ps; +mod render_graph; mod repo; mod repo_deinit; mod repo_init; diff --git a/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs new file mode 100644 index 000000000..088cec424 --- /dev/null +++ b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs @@ -0,0 +1,105 @@ +use std::io::Write; +use std::process::{Command, Stdio}; + +use fabro_test::{fabro_snapshot, test_context}; + +fn render_graph_command(context: &fabro_test::TestContext) -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_fabro")); + cmd.current_dir(&context.temp_dir); + cmd.env("NO_COLOR", "1"); + cmd.env("HOME", &context.home_dir); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); + cmd +} + +#[test] +fn help() { + let context = test_context!(); + let mut cmd = context.command(); + cmd.args(["__render-graph", "--help"]); + fabro_snapshot!(context.filters(), cmd, @" + success: true + exit_code: 0 + ----- stdout ----- + Render a DOT graph to SVG (internal) + + Usage: fabro __render-graph [OPTIONS] + + Options: + --json Output as JSON [env: FABRO_JSON=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); +} + +#[test] +fn render_graph_outputs_svg() { + let context = test_context!(); + let mut cmd = render_graph_command(&context); + cmd.args(["__render-graph"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("render-graph subprocess should spawn"); + child + .stdin + .as_mut() + .expect("stdin should be piped") + .write_all(b"digraph { a -> b }") + .expect("stdin write should succeed"); + + let output = child + .wait_with_output() + .expect("render-graph subprocess should exit"); + + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).expect("stdout should be valid UTF-8"); + assert!( + stdout.contains(" = + LazyLock::new(|| Semaphore::new(GRAPHVIZ_RENDER_CONCURRENCY_LIMIT)); + +#[derive(Debug, thiserror::Error)] +enum RenderSubprocessError { + #[error("failed to spawn render subprocess: {0}")] + SpawnFailed(String), + #[error("render subprocess crashed: {0}")] + ChildCrashed(String), + #[error("render subprocess returned invalid output: {0}")] + ProtocolViolation(String), + #[error("{0}")] + RenderFailed(String), +} const MAX_MULTIPART_REQUEST_BYTES: u64 = 50 * 1024 * 1024; const MAX_MULTIPART_MANIFEST_BYTES: usize = 256 * 1024; @@ -6181,21 +6198,162 @@ async fn create_completion( } } -/// Render DOT source to a styled SVG image via `render_dot` on a blocking -/// thread. -pub(crate) async fn render_graph_bytes(dot_source: &str) -> Response { - use fabro_graphviz::render::render_dot; +fn render_graph_subprocess_exe( + exe_override: Option<&std::path::Path>, +) -> Result { + match exe_override { + Some(path) => Ok(path.to_path_buf()), + None => { + if let Some(path) = std::env::var_os("CARGO_BIN_EXE_fabro").map(PathBuf::from) { + return Ok(path); + } - let source = dot_source.to_owned(); - match spawn_blocking(move || render_dot(&source)).await { - Ok(Ok(bytes)) => { + let current = std::env::current_exe() + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + let current_name = current.file_stem().and_then(|name| name.to_str()); + if current_name == Some("fabro") { + return Ok(current); + } + + let candidate = current + .parent() + .and_then(|parent| parent.parent()) + .map(|parent| parent.join(if cfg!(windows) { "fabro.exe" } else { "fabro" })); + if let Some(candidate) = candidate.filter(|path| path.is_file()) { + return Ok(candidate); + } + + Ok(current) + } + } +} + +fn render_subprocess_failure( + status: &std::process::ExitStatus, + stderr: &[u8], +) -> RenderSubprocessError { + #[cfg(unix)] + { + use std::os::unix::process::ExitStatusExt; + + if let Some(signal) = status.signal() { + let stderr = String::from_utf8_lossy(stderr).trim().to_string(); + let detail = if stderr.is_empty() { + format!("terminated by signal {signal}") + } else { + format!("terminated by signal {signal}: {stderr}") + }; + return RenderSubprocessError::ChildCrashed(detail); + } + } + + let stderr = String::from_utf8_lossy(stderr).trim().to_string(); + let detail = match status.code() { + Some(code) if stderr.is_empty() => format!("exited with status {code}"), + Some(code) => format!("exited with status {code}: {stderr}"), + None if stderr.is_empty() => "child exited unsuccessfully".to_string(), + None => format!("child exited unsuccessfully: {stderr}"), + }; + RenderSubprocessError::ChildCrashed(detail) +} + +async fn render_dot_subprocess( + styled_source: &str, + exe_override: Option<&std::path::Path>, +) -> Result, RenderSubprocessError> { + let _permit = GRAPHVIZ_RENDER_SEMAPHORE + .acquire() + .await + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + let exe = render_graph_subprocess_exe(exe_override)?; + let mut cmd = Command::new(exe); + cmd.arg("__render-graph") + .env("FABRO_TELEMETRY", "off") + .env_remove("FABRO_JSON") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd + .spawn() + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + let mut stdin = child.stdin.take().ok_or_else(|| { + RenderSubprocessError::SpawnFailed("render subprocess stdin was not piped".to_string()) + })?; + if let Err(err) = stdin.write_all(styled_source.as_bytes()).await { + drop(stdin); + let output = child + .wait_with_output() + .await + .map_err(|wait_err| RenderSubprocessError::SpawnFailed(wait_err.to_string()))?; + return Err(RenderSubprocessError::ChildCrashed(format!( + "failed writing DOT to child stdin: {err}; {}", + render_subprocess_failure(&output.status, &output.stderr) + ))); + } + drop(stdin); + + let output = child + .wait_with_output() + .await + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + + if !output.status.success() { + return Err(render_subprocess_failure(&output.status, &output.stderr)); + } + + if let Some(error) = output.stdout.strip_prefix(RENDER_ERROR_PREFIX) { + return Err(RenderSubprocessError::RenderFailed( + String::from_utf8_lossy(error).trim().to_string(), + )); + } + + if output.stdout.starts_with(b", +) -> Response { + use fabro_graphviz::render::{inject_dot_style_defaults, postprocess_svg}; + + let styled_source = inject_dot_style_defaults(dot_source); + match render_dot_subprocess(&styled_source, exe_override).await { + Ok(raw) => { + let bytes = postprocess_svg(raw); (StatusCode::OK, [("content-type", "image/svg+xml")], bytes).into_response() } - Ok(Err(e)) => ApiError::new(StatusCode::BAD_REQUEST, e.to_string()).into_response(), - Err(e) => ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(), + Err(RenderSubprocessError::RenderFailed(err)) => { + ApiError::new(StatusCode::BAD_REQUEST, err).into_response() + } + Err(err) => { + ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() + } } } +pub(crate) async fn render_graph_bytes(dot_source: &str) -> Response { + render_graph_response(dot_source, None).await +} + +#[cfg(test)] +async fn render_graph_bytes_with_exe_override( + dot_source: &str, + exe_override: Option<&std::path::Path>, +) -> Response { + render_graph_response(dot_source, exe_override).await +} + async fn get_graph( _auth: AuthenticatedService, State(state): State>, @@ -6230,6 +6388,10 @@ async fn get_graph( #[cfg(test)] mod tests { + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; + #[cfg(unix)] + use std::path::{Path, PathBuf}; #[cfg(unix)] use std::process::Stdio; @@ -7834,6 +7996,70 @@ slug = "fabro" ); } + #[cfg(unix)] + #[tokio::test] + async fn render_graph_bytes_returns_bad_request_for_render_error_protocol() { + let (_dir, script_path) = write_test_executable( + "#!/bin/sh\nprintf 'RENDER_ERROR:failed to parse DOT source'\nexit 0\n", + ); + + let response = + render_graph_bytes_with_exe_override("not valid dot {{{", Some(&script_path)).await; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + } + + #[cfg(unix)] + fn write_test_executable(script: &str) -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().expect("temp dir should exist"); + let path = dir.path().join("fake-fabro"); + std::fs::write(&path, script).expect("script should be written"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("script should be executable"); + (dir, path) + } + + #[cfg(unix)] + async fn render_graph_with_override(dot_source: &str, exe_path: &Path) -> Response { + render_graph_bytes_with_exe_override(dot_source, Some(exe_path)).await + } + + #[cfg(unix)] + #[tokio::test] + async fn render_dot_subprocess_returns_child_crashed_for_nonzero_exit() { + let (_dir, script_path) = write_test_executable("#!/bin/sh\nexit 1\n"); + + let result = render_dot_subprocess("digraph { a -> b }", Some(&script_path)).await; + + assert!(matches!( + result, + Err(RenderSubprocessError::ChildCrashed(_)) + )); + } + + #[cfg(unix)] + #[tokio::test] + async fn render_graph_bytes_returns_internal_server_error_for_child_crash() { + let (_dir, script_path) = write_test_executable("#!/bin/sh\nexit 1\n"); + + let response = render_graph_with_override("digraph { a -> b }", &script_path).await; + + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + } + + #[cfg(unix)] + #[tokio::test] + async fn render_dot_subprocess_returns_protocol_violation_for_garbage_stdout() { + let (_dir, script_path) = write_test_executable("#!/bin/sh\nprintf 'garbage'\nexit 0\n"); + + let result = render_dot_subprocess("digraph { a -> b }", Some(&script_path)).await; + + assert!(matches!( + result, + Err(RenderSubprocessError::ProtocolViolation(_)) + )); + } + #[tokio::test] async fn get_graph_not_found() { let app = test_app_with();