diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 32799e105..32a4bb99f 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -916,8 +916,8 @@ retros = true .position(|event| matches!(&event.body, EventBody::RetroCompleted(_))) .expect("retro.completed should be present"); assert!( - run_completed_index < retro_completed_index, - "retro should still run after run.completed" + retro_completed_index < run_completed_index, + "retro.completed must precede run.completed: run.completed is the terminal event and retro runs before FINALIZE" ); } diff --git a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs index 88e35ab6a..cd6eb06a8 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs @@ -1,5 +1,4 @@ use std::sync::Arc; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::time::Duration; use async_trait::async_trait; @@ -34,22 +33,17 @@ const ARTIFACT_UPLOAD_RETRY_DELAYS: [Duration; 3] = [ /// Sub-lifecycle responsible for artifact collection, offloading, and syncing. pub(crate) struct ArtifactLifecycle { - pub sandbox: Arc, - pub run_store: RunStoreHandle, - pub emitter: Arc, - pub run_id: RunId, - pub artifact_globs: Vec, - pub artifact_sink: Option, - pub captured_artifact_count: Arc, + pub sandbox: Arc, + pub run_store: RunStoreHandle, + pub emitter: Arc, + pub run_id: RunId, + pub artifact_globs: Vec, + pub artifact_sink: Option, /// Per-attempt state: epoch seconds when the attempt started. - attempt_start_epoch: std::sync::Mutex>, + attempt_start_epoch: std::sync::Mutex>, } impl ArtifactLifecycle { - #[allow( - clippy::too_many_arguments, - reason = "Artifact capture setup needs the run-scoped collaborators up front." - )] pub(crate) fn new( sandbox: Arc, run_store: RunStoreHandle, @@ -57,7 +51,6 @@ impl ArtifactLifecycle { run_id: RunId, artifact_globs: Vec, artifact_sink: Option, - captured_artifact_count: Arc, ) -> Self { Self { sandbox, @@ -66,7 +59,6 @@ impl ArtifactLifecycle { run_id, artifact_globs, artifact_sink, - captured_artifact_count, attempt_start_epoch: std::sync::Mutex::new(None), } } @@ -75,7 +67,6 @@ impl ArtifactLifecycle { #[async_trait] impl RunLifecycle for ArtifactLifecycle { async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> { - self.captured_artifact_count.store(0, Ordering::Relaxed); *self.attempt_start_epoch.lock().unwrap() = None; Ok(()) } @@ -139,7 +130,6 @@ impl RunLifecycle for ArtifactLifecycle { } let scope = stage_scope_for(state, node_id); for asset in &summary.captured_assets { - self.captured_artifact_count.fetch_add(1, Ordering::Relaxed); self.emitter.emit_scoped( &Event::ArtifactCaptured { node_id: node_id.to_string(), diff --git a/lib/crates/fabro-workflow/src/lifecycle/event.rs b/lib/crates/fabro-workflow/src/lifecycle/event.rs index 48e981212..642565526 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/event.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/event.rs @@ -1,5 +1,4 @@ use std::collections::BTreeMap; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -11,12 +10,11 @@ use fabro_core::lifecycle::{ }; use fabro_core::outcome::NodeResult; use fabro_core::state::ExecutionState; -use fabro_types::{BilledTokenCounts, FailureReason, RunId, SuccessReason}; +use fabro_types::RunId; use super::circuit_breaker::CircuitBreakerLifecycle; use super::git::GitCheckpointResult; use crate::context::WorkflowContext; -use crate::error::Error; use crate::event::{Emitter, Event, StageScope}; use crate::graph::{WorkflowGraph, WorkflowNode}; use crate::outcome::{BilledModelUsage, FailureCategory, FailureDetail, Outcome, StageStatus}; @@ -31,25 +29,23 @@ type FailureSignatureSnapshot = ( /// Sub-lifecycle responsible for emitting workflow run events. pub(crate) struct EventLifecycle { - pub emitter: Arc, - pub graph_name: String, - pub run_id: RunId, - pub run_start: Mutex, + pub emitter: Arc, + pub graph_name: String, + pub run_id: RunId, + pub run_start: Mutex, /// Set in on_edge_selected when loop_restart approved; emitted+cleared in /// on_run_start. - pub restarted_from: Arc>>, + pub restarted_from: Arc>>, // Config for WorkflowRunStarted payload - pub base_branch: Option, - pub base_sha: Option, - pub run_branch: Option, - pub worktree_dir: Option, - pub goal: Option, - pub captured_artifact_count: Arc, - // Cross-lifecycle data - pub checkpoint_git_result: Arc>>, - pub last_git_sha: Arc>>, - pub final_patch: Arc>>, - pub circuit_breaker: Arc, + pub base_branch: Option, + pub base_sha: Option, + pub run_branch: Option, + pub worktree_dir: Option, + pub goal: Option, + /// Shared git checkpoint result (written by GitLifecycle, read by + /// EventLifecycle when emitting CheckpointCompleted). + pub checkpoint_git_result: Arc>>, + pub circuit_breaker: Arc, } fn snapshot_failure_signatures( @@ -415,75 +411,4 @@ impl RunLifecycle for EventLifecycle { Ok(()) } - - async fn on_run_end(&self, outcome: &Outcome, state: &WfRunState) { - let duration_ms = crate::millis_u64(self.run_start.lock().unwrap().elapsed()); - let artifact_count = self.captured_artifact_count.load(Ordering::Relaxed); - let last_sha = self.last_git_sha.lock().unwrap().clone(); - let final_patch = self.final_patch.lock().unwrap().clone(); - let run_billing_entries = state - .node_outcomes - .values() - .filter_map(|o| o.usage.clone()) - .collect::>(); - let run_billing = (!run_billing_entries.is_empty()) - .then(|| BilledTokenCounts::from_billed_usage(&run_billing_entries)); - let total_usd_micros = run_billing - .as_ref() - .and_then(|billing| billing.total_usd_micros) - .or_else(|| { - let mut total = 0_i64; - let mut has_total = false; - for usage in state - .node_outcomes - .values() - .filter_map(|o| o.usage.as_ref()) - { - if let Some(value) = usage.total_usd_micros { - total += value; - has_total = true; - } - } - has_total.then_some(total) - }); - - if state.cancelled { - self.emitter.emit(&Event::WorkflowRunFailed { - error: Error::Cancelled, - duration_ms, - reason: FailureReason::Cancelled, - git_commit_sha: last_sha, - final_patch: final_patch.clone(), - }); - return; - } - - if outcome.status == StageStatus::Success || outcome.status == StageStatus::PartialSuccess { - self.emitter.emit(&Event::WorkflowRunCompleted { - duration_ms, - artifact_count, - status: outcome.status.to_string(), - reason: match outcome.status { - StageStatus::PartialSuccess => SuccessReason::PartialSuccess, - _ => SuccessReason::Completed, - }, - total_usd_micros, - final_git_commit_sha: last_sha, - final_patch, - billing: run_billing, - }); - } else { - let error_msg = outcome - .failure - .as_ref() - .map_or_else(|| "run failed".to_string(), |f| f.message.clone()); - self.emitter.emit(&Event::WorkflowRunFailed { - error: Error::engine(error_msg), - duration_ms, - reason: FailureReason::WorkflowError, - git_commit_sha: last_sha, - final_patch, - }); - } - } } diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index aaf468b10..457fa9a91 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -14,11 +14,11 @@ use crate::event::{Emitter, Event, RunNoticeLevel}; use crate::git::MetadataStore; use crate::graph::{WorkflowGraph, WorkflowNode}; use crate::lifecycle::event::stage_scope_for; -use crate::outcome::{BilledModelUsage, Outcome, StageStatus}; +use crate::outcome::BilledModelUsage; use crate::run_dump::RunDump; use crate::run_options::RunOptions; use crate::runtime_store::RunStoreHandle; -use crate::sandbox_git::{git_checkpoint, git_diff, git_diff_with_timeout, git_push_host}; +use crate::sandbox_git::{git_checkpoint, git_diff, git_push_host}; type WfRunState = ExecutionState>; type WfNodeResult = NodeResult>; @@ -71,7 +71,6 @@ pub(crate) struct GitLifecycle { // Cross-lifecycle data (shared with EventLifecycle) pub checkpoint_git_result: Arc>>, pub last_git_sha: Arc>>, - pub final_patch: Arc>>, } #[async_trait] @@ -80,7 +79,6 @@ impl RunLifecycle for GitLifecycle { // Reset last_git_sha (diff base parity) *self.last_git_sha.lock().unwrap() = None; *self.checkpoint_git_result.lock().unwrap() = None; - *self.final_patch.lock().unwrap() = None; // Init metadata branch (best-effort) if let (Some(_), Some(repo_path)) = ( @@ -321,43 +319,4 @@ impl RunLifecycle for GitLifecycle { Ok(()) } - - async fn on_run_end(&self, outcome: &Outcome, _state: &WfRunState) { - // Capture the final diff for event/store projection. - // - // Success/PartialSuccess uses the standard 30 s timeout. Failed runs - // use a shorter 10 s timeout: a pathological workspace (FS locks, - // corrupted index) must not stall terminal event emission downstream - // (Slack notifier, SSE RunFailed, CI hooks). - if self.run_options.git.is_none() { - return; - } - let timeout_ms = match outcome.status { - StageStatus::Success | StageStatus::PartialSuccess => 30_000, - _ => 10_000, - }; - if let Some(base_sha) = self - .run_options - .git - .as_ref() - .and_then(|g| g.base_sha.clone()) - { - match git_diff_with_timeout(&*self.sandbox, &base_sha, timeout_ms).await { - Ok(patch) if !patch.is_empty() => { - *self.final_patch.lock().unwrap() = Some(patch.clone()); - } - Ok(_) => { - *self.final_patch.lock().unwrap() = None; - } - Err(err) => { - *self.final_patch.lock().unwrap() = None; - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "git_diff_failed".to_string(), - message: format!("final diff failed: {err}"), - }); - } - } - } - } } diff --git a/lib/crates/fabro-workflow/src/lifecycle/mod.rs b/lib/crates/fabro-workflow/src/lifecycle/mod.rs index f7b47a72f..cae2cd322 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/mod.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/mod.rs @@ -8,7 +8,7 @@ pub(crate) mod hook; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -96,8 +96,6 @@ impl WorkflowLifecycle { let checkpoint_git_result: Arc>> = Arc::new(Mutex::new(None)); let last_git_sha: Arc>> = Arc::new(Mutex::new(None)); - let final_patch: Arc>> = Arc::new(Mutex::new(None)); - let captured_artifact_count = Arc::new(AtomicUsize::new(0)); let circuit_breaker = Arc::new(CircuitBreakerLifecycle::new(loop_restart_signature_limit)); @@ -114,21 +112,18 @@ impl WorkflowLifecycle { }; let event = EventLifecycle { - emitter: Arc::clone(emitter), - graph_name: graph.name.clone(), - run_id: run_options.run_id, - run_start: Mutex::new(Instant::now()), - restarted_from: Arc::clone(&restarted_from), - base_branch: run_options.base_branch.clone(), - base_sha: run_options.git.as_ref().and_then(|g| g.base_sha.clone()), - run_branch: run_options.git.as_ref().and_then(|g| g.run_branch.clone()), - worktree_dir: working_directory.clone(), - goal: (!graph.goal().is_empty()).then(|| graph.goal().to_string()), - captured_artifact_count: Arc::clone(&captured_artifact_count), - last_git_sha: Arc::clone(&last_git_sha), - final_patch: Arc::clone(&final_patch), - checkpoint_git_result: Arc::clone(&checkpoint_git_result), - circuit_breaker: Arc::clone(&circuit_breaker), + emitter: Arc::clone(emitter), + graph_name: graph.name.clone(), + run_id: run_options.run_id, + run_start: Mutex::new(Instant::now()), + restarted_from: Arc::clone(&restarted_from), + base_branch: run_options.base_branch.clone(), + base_sha: run_options.git.as_ref().and_then(|g| g.base_sha.clone()), + run_branch: run_options.git.as_ref().and_then(|g| g.run_branch.clone()), + worktree_dir: working_directory.clone(), + goal: (!graph.goal().is_empty()).then(|| graph.goal().to_string()), + checkpoint_git_result: Arc::clone(&checkpoint_git_result), + circuit_breaker: Arc::clone(&circuit_breaker), }; let hook = HookLifecycle { @@ -156,8 +151,7 @@ impl WorkflowLifecycle { run_options: Arc::clone(run_options), start_node_id, checkpoint_git_result: Arc::clone(&checkpoint_git_result), - last_git_sha: Arc::clone(&last_git_sha), - final_patch, + last_git_sha, }; let artifact = ArtifactLifecycle::new( @@ -167,7 +161,6 @@ impl WorkflowLifecycle { run_options.run_id, run_options.artifact_globs(), artifact_sink, - captured_artifact_count, ); Self { @@ -419,12 +412,6 @@ impl RunLifecycle for WorkflowLifecycle { } async fn on_run_end(&self, outcome: &Outcome, state: &WfRunState) { - if state.cancelled { - self.event.on_run_end(outcome, state).await; - return; - } - self.git.on_run_end(outcome, state).await; - self.event.on_run_end(outcome, state).await; self.hook.on_run_end(outcome, state).await; } } diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index a3c61ac30..dab98376e 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -32,7 +32,6 @@ use crate::pipeline::initialize; use crate::pipeline::types::{InitOptions, LlmSpec, Persisted, SandboxEnvSpec}; use crate::records::RunSpec; use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions}; -use crate::run_status::{FailureReason, RunStatus}; use crate::test_support::run_graph; fn local_env() -> Arc { @@ -789,11 +788,9 @@ async fn execute_cancelled_mid_run_persists_cancelled_status() { let executed = execute_test_run_with_options(run_options, g, Some(Arc::new(registry))).await; + // The status update is driven by the terminal event, which FINALIZE emits. + // This test only runs EXECUTE, so we assert on the cancellation outcome. assert!(matches!(executed.outcome, Err(Error::Cancelled))); - let status = executed.run_store.state().await.unwrap().status.unwrap(); - assert_eq!(status, RunStatus::Failed { - reason: FailureReason::Cancelled, - }); } #[tokio::test] diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 67dd9011c..dbdee506b 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use fabro_hooks::{HookContext, HookEvent, HookRunner}; -use fabro_types::BilledTokenCounts; +use fabro_types::{BilledTokenCounts, EventBody}; use super::types::{Concluded, FinalizeOptions, Retroed}; use crate::error::Error; @@ -13,7 +13,7 @@ use crate::run_dump::RunDump; use crate::run_options::RunOptions; use crate::run_status::{FailureReason, RunStatus, SuccessReason}; use crate::runtime_store::RunStoreHandle; -use crate::sandbox_git::git_push_host; +use crate::sandbox_git::{git_diff_with_timeout, git_push_host}; fn emit_run_notice( emitter: &Emitter, @@ -153,9 +153,14 @@ fn build_conclusion_from_parts( /// Write a finalize projection snapshot commit to the metadata branch. /// -/// This captures the final `run.json` projection state, including conclusion -/// and retro data. Best-effort: errors are logged as warnings. -pub async fn write_finalize_commit(run_options: &RunOptions, run_store: &RunStoreHandle) { +/// `conclusion` is injected into the projection copy: the terminal event +/// hasn't been emitted yet (FINALIZE emits it after this commit lands), so +/// the run store's `projection.conclusion` is still `None`. +pub async fn write_finalize_commit( + run_options: &RunOptions, + run_store: &RunStoreHandle, + conclusion: &Conclusion, +) { let (Some(meta_branch), Some(repo_path)) = ( run_options .git @@ -168,9 +173,12 @@ pub async fn write_finalize_commit(run_options: &RunOptions, run_store: &RunStor let git_author = run_options.git_author(); let store = MetadataStore::new(repo_path, &git_author); - let Ok(store_state) = run_store.state().await else { + let Ok(mut store_state) = run_store.state().await else { return; }; + if store_state.conclusion.is_none() { + store_state.conclusion = Some(conclusion.clone()); + } let dump = RunDump::from_projection(&store_state); if let Err(e) = dump.write_to_metadata_store(&store, &run_options.run_id.to_string(), "finalize run") @@ -189,6 +197,123 @@ pub async fn write_finalize_commit(run_options: &RunOptions, run_store: &RunStor .await; } +/// Compute the diff between the run's base sha and the workspace head. +/// +/// Failed runs use a shorter timeout: a corrupted workspace must not stall +/// the terminal event downstream consumers (Slack, SSE, CI hooks) are waiting +/// for. +async fn compute_final_patch( + run_options: &RunOptions, + sandbox: &dyn fabro_agent::Sandbox, + status: StageStatus, + emitter: &Emitter, +) -> Option { + let base_sha = run_options.git.as_ref().and_then(|g| g.base_sha.clone())?; + let timeout_ms = match status { + StageStatus::Success | StageStatus::PartialSuccess => 30_000, + _ => 10_000, + }; + match git_diff_with_timeout(sandbox, &base_sha, timeout_ms).await { + Ok(patch) if !patch.is_empty() => Some(patch), + Ok(_) => None, + Err(err) => { + emit_run_notice( + emitter, + RunNoticeLevel::Warn, + "git_diff_failed", + format!("final diff failed: {err}"), + ); + None + } + } +} + +/// Build the terminal `WorkflowRunCompleted`/`WorkflowRunFailed` event. +pub(crate) fn build_terminal_event( + outcome: &Result, + duration_ms: u64, + artifact_count: usize, + final_git_commit_sha: Option, + final_patch: Option, + state: Option<&fabro_store::RunProjection>, +) -> Event { + let cancelled = matches!(outcome, Err(Error::Cancelled)); + let outcome_status = outcome + .as_ref() + .map_or(StageStatus::Fail, |o| o.status.clone()); + + let billed_usage: Vec<_> = state + .and_then(|s| s.checkpoint.as_ref()) + .map(|cp| { + cp.node_outcomes + .values() + .filter_map(|o| o.usage.clone()) + .collect() + }) + .unwrap_or_default(); + let billing = + (!billed_usage.is_empty()).then(|| BilledTokenCounts::from_billed_usage(&billed_usage)); + let total_usd_micros = billing + .as_ref() + .and_then(|b| b.total_usd_micros) + .or_else(|| { + let mut total = 0_i64; + let mut has_total = false; + for usage in &billed_usage { + if let Some(value) = usage.total_usd_micros { + total += value; + has_total = true; + } + } + has_total.then_some(total) + }); + + if cancelled { + return Event::WorkflowRunFailed { + error: Error::Cancelled, + duration_ms, + reason: FailureReason::Cancelled, + git_commit_sha: final_git_commit_sha, + final_patch, + }; + } + + if outcome_status == StageStatus::Success || outcome_status == StageStatus::PartialSuccess { + Event::WorkflowRunCompleted { + duration_ms, + artifact_count, + status: outcome_status.to_string(), + reason: match outcome_status { + StageStatus::PartialSuccess => SuccessReason::PartialSuccess, + _ => SuccessReason::Completed, + }, + total_usd_micros, + final_git_commit_sha, + final_patch, + billing, + } + } else { + let error_msg = outcome + .as_ref() + .err() + .map(ToString::to_string) + .or_else(|| { + outcome + .as_ref() + .ok() + .and_then(|o| o.failure.as_ref().map(|f| f.message.clone())) + }) + .unwrap_or_else(|| "run failed".to_string()); + Event::WorkflowRunFailed { + error: Error::engine(error_msg), + duration_ms, + reason: FailureReason::WorkflowError, + git_commit_sha: final_git_commit_sha, + final_patch, + } + } +} + async fn run_hooks( hook_runner: Option<&HookRunner>, hook_context: &HookContext, @@ -219,7 +344,14 @@ async fn cleanup_sandbox( Ok(()) } -/// FINALIZE phase: classify outcome, build conclusion, persist terminal state. +/// FINALIZE phase: build conclusion, write the meta branch, emit the terminal +/// `WorkflowRunCompleted`/`WorkflowRunFailed` event. +/// +/// The terminal event MUST be emitted from here, not from the executor's +/// `on_run_end` lifecycle hook. Observers (CLI attach, daemon SSE) treat the +/// event as the run's "done" signal — emitting it earlier means they can +/// observe terminal state and act on it (e.g. delete the meta branch in +/// recovery tests) before this function's writes are flushed. /// /// # Errors /// @@ -240,14 +372,33 @@ pub async fn finalize(retroed: Retroed, options: &FinalizeOptions) -> Result PathBuf { let mut hasher = std::collections::hash_map::DefaultHasher::new(); std::process::id().hash(&mut hasher); @@ -165,6 +182,7 @@ pub async fn run_graph( ) .await; let executed = pipeline::execute(initialized.initialized).await; + emit_test_terminal_event(&executed).await; // Tests often reopen the run store immediately after `run()` returns. // Flush the async store logger first so they don't observe partial state. initialized.store_logger.flush().await; @@ -192,6 +210,7 @@ pub async fn run_graph_with_state( ) .await; let executed = pipeline::execute(initialized.initialized).await; + emit_test_terminal_event(&executed).await; initialized.store_logger.flush().await; let outcome = executed.outcome?; let state = executed @@ -225,6 +244,7 @@ pub async fn run_graph_with_hooks( ) .await; let executed = pipeline::execute(initialized.initialized).await; + emit_test_terminal_event(&executed).await; initialized.store_logger.flush().await; executed.outcome } @@ -252,6 +272,7 @@ pub async fn run_graph_with_hooks_and_state( ) .await; let executed = pipeline::execute(initialized.initialized).await; + emit_test_terminal_event(&executed).await; initialized.store_logger.flush().await; let outcome = executed.outcome?; let state = executed @@ -284,6 +305,7 @@ pub async fn run_graph_from_checkpoint( ) .await; let executed = pipeline::execute(initialized.initialized).await; + emit_test_terminal_event(&executed).await; initialized.store_logger.flush().await; executed.outcome } @@ -310,6 +332,7 @@ pub async fn run_graph_from_checkpoint_with_state( ) .await; let executed = pipeline::execute(initialized.initialized).await; + emit_test_terminal_event(&executed).await; initialized.store_logger.flush().await; let outcome = executed.outcome?; let state = executed