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.
This commit is contained in:
Bryan Helmkamp 2026-04-14 18:15:23 -04:00
parent d6ed6b3cda
commit 286eee7efa
9 changed files with 392 additions and 12 deletions

1
Cargo.lock generated
View file

@ -1588,6 +1588,7 @@ dependencies = [
"fabro-devcontainer",
"fabro-github",
"fabro-graphviz",
"fabro-graphviz-sys",
"fabro-hooks",
"fabro-http",
"fabro-interview",

View file

@ -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" }

View file

@ -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",
}

View file

@ -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;

View file

@ -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
}
}
}

View file

@ -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<String> = 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<String> = 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");

View file

@ -29,6 +29,7 @@ mod preflight;
mod provider;
mod provider_login;
mod ps;
mod render_graph;
mod repo;
mod repo_deinit;
mod repo_init;

View file

@ -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("<svg"),
"expected SVG output, got: {}",
&stdout[..stdout.len().min(200)]
);
}
#[test]
fn render_graph_bad_input_uses_render_error_protocol() {
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"not valid dot")
.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.starts_with("RENDER_ERROR:"),
"expected render error protocol, got: {stdout}"
);
}

View file

@ -3,7 +3,7 @@ use std::path::PathBuf;
use std::process::Stdio;
use std::str::FromStr;
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::{Arc, Mutex, RwLock};
use std::sync::{Arc, LazyLock, Mutex, RwLock};
use std::time::{Duration, Instant};
use axum::body::Body;
@ -96,7 +96,7 @@ use tokio::fs;
use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
use tokio::process::{ChildStderr, ChildStdin, Command};
use tokio::sync::broadcast::error::RecvError;
use tokio::sync::{Notify, RwLock as AsyncRwLock, broadcast, mpsc, oneshot};
use tokio::sync::{Notify, RwLock as AsyncRwLock, Semaphore, broadcast, mpsc, oneshot};
use tokio::task::spawn_blocking;
use tokio::time::{sleep, timeout};
use tokio_stream::StreamExt;
@ -258,6 +258,23 @@ const ARTIFACT_UPLOAD_TOKEN_SCOPE: &str = "stage_artifacts:upload";
const ARTIFACT_UPLOAD_TOKEN_TTL_SECS: u64 = 24 * 60 * 60;
const MAX_SINGLE_ARTIFACT_BYTES: u64 = 10 * 1024 * 1024;
const MAX_MULTIPART_ARTIFACTS: usize = 100;
const RENDER_ERROR_PREFIX: &[u8] = b"RENDER_ERROR:";
const GRAPHVIZ_RENDER_CONCURRENCY_LIMIT: usize = 4;
static GRAPHVIZ_RENDER_SEMAPHORE: LazyLock<Semaphore> =
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<PathBuf, RenderSubprocessError> {
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<Vec<u8>, 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"<?xml") || output.stdout.starts_with(b"<svg") {
return Ok(output.stdout);
}
let stdout = String::from_utf8_lossy(&output.stdout);
let stderr = String::from_utf8_lossy(&output.stderr);
Err(RenderSubprocessError::ProtocolViolation(format!(
"stdout did not contain SVG or error protocol (stdout: {:?}, stderr: {:?})",
stdout.trim(),
stderr.trim()
)))
}
async fn render_graph_response(
dot_source: &str,
exe_override: Option<&std::path::Path>,
) -> 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<Arc<AppState>>,
@ -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();