From 75f2148f43f665ebe23b78bb3ed16b7725c2a396 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 26 Mar 2026 13:19:41 -0400 Subject: [PATCH] fix(resume): reject succeeded runs and check PID before checkpoint parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Succeeded runs now rejected — a completed run keeps checkpoint.json around, so resume would happily restart and overwrite start.json and conclusion.json. Now checks status.json and bails on Succeeded. 2. PID liveness check moved before checkpoint validation. The engine writes checkpoint.json with a plain fs::write, so a concurrent resume could see a half-written file and report "corrupt" for a run that is simply still alive. Order is now: PID → status → checkpoint parse → cleanup → spawn. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/resume.rs | 23 +++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/resume.rs b/lib/crates/fabro-cli/src/commands/resume.rs index dbd449817..f6487b04d 100644 --- a/lib/crates/fabro-cli/src/commands/resume.rs +++ b/lib/crates/fabro-cli/src/commands/resume.rs @@ -2,7 +2,7 @@ use anyhow::bail; use clap::Args; use fabro_util::terminal::Styles; use fabro_workflows::records::{Checkpoint, RunRecord}; -use fabro_workflows::run_status::RunStatus; +use fabro_workflows::run_status::{RunStatus, RunStatusRecord}; #[derive(Debug, Args)] pub struct ResumeArgs { @@ -29,6 +29,22 @@ pub async fn resume_command(args: ResumeArgs, styles: &'static Styles) -> anyhow } let run_id = RunRecord::load(&run_dir)?.run_id; + // Guard against resuming a live run — must happen before checkpoint + // validation because the engine writes checkpoint.json with a plain + // fs::write, so a mid-write read would see a truncated file and + // report "corrupt" for a run that is simply still alive. + if is_pid_alive(&run_dir.join("run.pid")) { + bail!("an engine process is still running for this run — cannot resume"); + } + + // Reject runs that completed successfully — only failed/interrupted + // runs should be resumed. + if let Ok(record) = RunStatusRecord::load(&run_dir.join("status.json")) { + if record.status == RunStatus::Succeeded { + bail!("run already succeeded — nothing to resume"); + } + } + // Validate checkpoint is parseable before touching any state. // A crash during the original run can leave a truncated file; // we must not destroy the old conclusion/failure evidence and @@ -42,11 +58,6 @@ pub async fn resume_command(args: ResumeArgs, styles: &'static Styles) -> anyhow } })?; - // Guard against resuming a live run - if is_pid_alive(&run_dir.join("run.pid")) { - bail!("an engine process is still running for this run — cannot resume"); - } - // Clean stale artifacts from previous execution for name in &[ "conclusion.json",