Fix three bugs in create→start→attach path that caused test hangs

1. _run_engine crashed for .fabro workflows because it hardcoded
   run.toml as the workflow path, but create_run only writes run.toml
   for .toml configs. Now falls back to graph.fabro when run.toml is
   absent.

2. attach_run couldn't detect a crashed engine because start_run
   dropped the Child handle, creating a zombie that kill(pid, 0)
   reported as alive. Now start_run returns the Child and attach_run
   uses try_wait() to safely detect exit.

3. create_run ignored --run-id, always generating a new ULID. Now
   uses args.run_id when provided.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-03-21 15:41:56 -04:00
parent 9792d96901
commit e47d00a371
4 changed files with 47 additions and 25 deletions

View file

@ -18,6 +18,7 @@ pub async fn attach_run(
run_dir: &Path,
kill_on_detach: bool,
styles: &'static Styles,
mut engine_child: Option<std::process::Child>,
) -> Result<ExitCode> {
let progress_path = run_dir.join("progress.jsonl");
let conclusion_path = run_dir.join("conclusion.json");
@ -127,19 +128,29 @@ pub async fn attach_run(
break;
}
// Check if engine process is still alive (cache PID after first read)
let engine_alive = match cached_pid {
Some(pid) => process_alive(pid),
None => {
if let Ok(pid_str) = std::fs::read_to_string(&pid_path) {
if let Ok(pid) = pid_str.trim().parse::<u32>() {
cached_pid = Some(pid);
process_alive(pid)
// Check if engine process is still alive
let engine_alive = if let Some(ref mut child) = engine_child {
// We own the child handle — use try_wait (safe, reaps zombies)
match child.try_wait() {
Ok(Some(_)) => false, // child exited
Ok(None) => true, // still running
Err(_) => false, // error, treat as dead
}
} else {
// Standalone attach — use kill-based check via PID file
match cached_pid {
Some(pid) => process_alive(pid),
None => {
if let Ok(pid_str) = std::fs::read_to_string(&pid_path) {
if let Ok(pid) = pid_str.trim().parse::<u32>() {
cached_pid = Some(pid);
process_alive(pid)
} else {
true
}
} else {
true
true // no PID file yet, assume alive
}
} else {
true // no PID file yet, assume alive
}
}
};

View file

@ -27,7 +27,10 @@ pub async fn create_run(
let goal = prep.graph.goal();
// Create run directory
let run_id = ulid::Ulid::new().to_string();
let run_id = args
.run_id
.clone()
.unwrap_or_else(|| ulid::Ulid::new().to_string());
let run_dir = args
.run_dir
.clone()

View file

@ -5,8 +5,8 @@ use anyhow::{bail, Result};
/// Spawn a detached engine process for the given run directory.
///
/// The engine process reads `spec.json` from the run directory and executes the
/// workflow. Returns the child process PID.
pub fn start_run(run_dir: &Path) -> Result<u32> {
/// workflow. Returns the child process handle (use `.id()` for the PID).
pub fn start_run(run_dir: &Path) -> Result<std::process::Child> {
// Validate status is Submitted
let status_path = run_dir.join("status.json");
match fabro_workflows::run_status::RunStatusRecord::load(&status_path) {
@ -53,12 +53,11 @@ pub fn start_run(run_dir: &Path) -> Result<u32> {
}
let child = cmd.spawn()?;
let pid = child.id();
// Write PID file
std::fs::write(run_dir.join("run.pid"), pid.to_string())?;
std::fs::write(run_dir.join("run.pid"), child.id().to_string())?;
Ok(pid)
Ok(child)
}
#[cfg(test)]

View file

@ -679,13 +679,14 @@ async fn main_inner() -> (String, Result<()>) {
#[cfg(feature = "sleep_inhibitor")]
let _sleep_guard = fabro_beastie::guard(cli_config.prevent_idle_sleep);
commands::start::start_run(&run_dir)?;
let child = commands::start::start_run(&run_dir)?;
if args.detach {
println!("{run_id}");
} else {
let exit_code =
commands::attach::attach_run(&run_dir, true, styles).await?;
commands::attach::attach_run(&run_dir, true, styles, Some(child))
.await?;
commands::run::print_run_summary(&run_dir, &run_id, styles);
if exit_code != std::process::ExitCode::SUCCESS {
std::process::exit(1);
@ -705,15 +706,16 @@ async fn main_inner() -> (String, Result<()>) {
Command::Start { run } => {
let base = fabro_workflows::run_lookup::default_runs_base();
let run_info = fabro_workflows::run_lookup::resolve_run(&base, &run)?;
let pid = commands::start::start_run(&run_info.path)?;
eprintln!("Started engine process (PID {pid})");
let child = commands::start::start_run(&run_info.path)?;
eprintln!("Started engine process (PID {})", child.id());
}
Command::Attach { run } => {
let styles: &'static fabro_util::terminal::Styles =
Box::leak(Box::new(fabro_util::terminal::Styles::detect_stderr()));
let base = fabro_workflows::run_lookup::default_runs_base();
let run_info = fabro_workflows::run_lookup::resolve_run(&base, &run)?;
let exit_code = commands::attach::attach_run(&run_info.path, false, styles).await?;
let exit_code =
commands::attach::attach_run(&run_info.path, false, styles, None).await?;
if exit_code != std::process::ExitCode::SUCCESS {
std::process::exit(1);
}
@ -739,9 +741,16 @@ async fn main_inner() -> (String, Result<()>) {
)
})?;
// Prefer the cached run.toml. prepare_workflow() falls back to the
// sibling graph snapshot for older detached runs that predate run.toml.
let workflow_path = commands::run::cached_run_config_path(&run_dir);
// Prefer the cached run.toml when present (workflow was a .toml
// config). For plain .fabro graphs, only graph.fabro exists.
let workflow_path = {
let config_path = commands::run::cached_run_config_path(&run_dir);
if config_path.exists() {
config_path
} else {
commands::run::cached_graph_path(&run_dir)
}
};
let run_args = commands::run::RunArgs {
workflow: Some(workflow_path),