mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-07 03:00:29 +00:00
fix(workflow): emit terminal run event from FINALIZE, not on_run_end
The `WorkflowRunCompleted` / `WorkflowRunFailed` event was emitted from `EventLifecycle::on_run_end`, a callback the executor fires at the end of the EXECUTE phase. But the run isn't done at that point — RETRO and FINALIZE still need to run, and FINALIZE writes the meta branch's finalize commit. Observers that treat the event as "done" (CLI attach, daemon SSE consumers) could observe terminal state and act on it before the worker flushed its remaining writes. The recovery scenario test exposed this: it deletes the meta branch right after `fabro run` returns, then asserts the branch is empty. On loaded CI runners the worker's finalize commit landed after the delete, recreating the branch and failing the assertion. Move the terminal event emission to `pipeline::finalize::finalize`, after `write_finalize_commit`. The lifecycle's `on_run_end` overrides for event and git become empty (deleted — the trait already provides a no-op default). Three pieces of cross-cutting state (`final_patch`, `captured_artifact_count`, the dead `EventLifecycle` reads of `last_git_sha`) only existed to ferry data from EXECUTE to the terminal event; deleted those too. The aggregator collapses to a one-line delegate to `hook.on_run_end`. `write_finalize_commit` now takes the conclusion as a parameter and injects it into the projection copy, since the terminal event hasn't run through the run store yet when the meta branch is written. `build_terminal_event` is `pub(crate)` so `test_support` helpers (which stop at EXECUTE) can mirror the production payload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
3f807f42a6
commit
41c47dbe12
9 changed files with 227 additions and 195 deletions
|
|
@ -916,8 +916,8 @@ retros = true
|
||||||
.position(|event| matches!(&event.body, EventBody::RetroCompleted(_)))
|
.position(|event| matches!(&event.body, EventBody::RetroCompleted(_)))
|
||||||
.expect("retro.completed should be present");
|
.expect("retro.completed should be present");
|
||||||
assert!(
|
assert!(
|
||||||
run_completed_index < retro_completed_index,
|
retro_completed_index < run_completed_index,
|
||||||
"retro should still run after run.completed"
|
"retro.completed must precede run.completed: run.completed is the terminal event and retro runs before FINALIZE"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,4 @@
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
|
|
||||||
use async_trait::async_trait;
|
use async_trait::async_trait;
|
||||||
|
|
@ -40,16 +39,11 @@ pub(crate) struct ArtifactLifecycle {
|
||||||
pub run_id: RunId,
|
pub run_id: RunId,
|
||||||
pub artifact_globs: Vec<String>,
|
pub artifact_globs: Vec<String>,
|
||||||
pub artifact_sink: Option<ArtifactSink>,
|
pub artifact_sink: Option<ArtifactSink>,
|
||||||
pub captured_artifact_count: Arc<AtomicUsize>,
|
|
||||||
/// Per-attempt state: epoch seconds when the attempt started.
|
/// Per-attempt state: epoch seconds when the attempt started.
|
||||||
attempt_start_epoch: std::sync::Mutex<Option<f64>>,
|
attempt_start_epoch: std::sync::Mutex<Option<f64>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ArtifactLifecycle {
|
impl ArtifactLifecycle {
|
||||||
#[allow(
|
|
||||||
clippy::too_many_arguments,
|
|
||||||
reason = "Artifact capture setup needs the run-scoped collaborators up front."
|
|
||||||
)]
|
|
||||||
pub(crate) fn new(
|
pub(crate) fn new(
|
||||||
sandbox: Arc<dyn fabro_sandbox::Sandbox>,
|
sandbox: Arc<dyn fabro_sandbox::Sandbox>,
|
||||||
run_store: RunStoreHandle,
|
run_store: RunStoreHandle,
|
||||||
|
|
@ -57,7 +51,6 @@ impl ArtifactLifecycle {
|
||||||
run_id: RunId,
|
run_id: RunId,
|
||||||
artifact_globs: Vec<String>,
|
artifact_globs: Vec<String>,
|
||||||
artifact_sink: Option<ArtifactSink>,
|
artifact_sink: Option<ArtifactSink>,
|
||||||
captured_artifact_count: Arc<AtomicUsize>,
|
|
||||||
) -> Self {
|
) -> Self {
|
||||||
Self {
|
Self {
|
||||||
sandbox,
|
sandbox,
|
||||||
|
|
@ -66,7 +59,6 @@ impl ArtifactLifecycle {
|
||||||
run_id,
|
run_id,
|
||||||
artifact_globs,
|
artifact_globs,
|
||||||
artifact_sink,
|
artifact_sink,
|
||||||
captured_artifact_count,
|
|
||||||
attempt_start_epoch: std::sync::Mutex::new(None),
|
attempt_start_epoch: std::sync::Mutex::new(None),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -75,7 +67,6 @@ impl ArtifactLifecycle {
|
||||||
#[async_trait]
|
#[async_trait]
|
||||||
impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
||||||
async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> {
|
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;
|
*self.attempt_start_epoch.lock().unwrap() = None;
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
@ -139,7 +130,6 @@ impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
||||||
}
|
}
|
||||||
let scope = stage_scope_for(state, node_id);
|
let scope = stage_scope_for(state, node_id);
|
||||||
for asset in &summary.captured_assets {
|
for asset in &summary.captured_assets {
|
||||||
self.captured_artifact_count.fetch_add(1, Ordering::Relaxed);
|
|
||||||
self.emitter.emit_scoped(
|
self.emitter.emit_scoped(
|
||||||
&Event::ArtifactCaptured {
|
&Event::ArtifactCaptured {
|
||||||
node_id: node_id.to_string(),
|
node_id: node_id.to_string(),
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,4 @@
|
||||||
use std::collections::BTreeMap;
|
use std::collections::BTreeMap;
|
||||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
|
||||||
use std::sync::{Arc, Mutex};
|
use std::sync::{Arc, Mutex};
|
||||||
use std::time::Instant;
|
use std::time::Instant;
|
||||||
|
|
||||||
|
|
@ -11,12 +10,11 @@ use fabro_core::lifecycle::{
|
||||||
};
|
};
|
||||||
use fabro_core::outcome::NodeResult;
|
use fabro_core::outcome::NodeResult;
|
||||||
use fabro_core::state::ExecutionState;
|
use fabro_core::state::ExecutionState;
|
||||||
use fabro_types::{BilledTokenCounts, FailureReason, RunId, SuccessReason};
|
use fabro_types::RunId;
|
||||||
|
|
||||||
use super::circuit_breaker::CircuitBreakerLifecycle;
|
use super::circuit_breaker::CircuitBreakerLifecycle;
|
||||||
use super::git::GitCheckpointResult;
|
use super::git::GitCheckpointResult;
|
||||||
use crate::context::WorkflowContext;
|
use crate::context::WorkflowContext;
|
||||||
use crate::error::Error;
|
|
||||||
use crate::event::{Emitter, Event, StageScope};
|
use crate::event::{Emitter, Event, StageScope};
|
||||||
use crate::graph::{WorkflowGraph, WorkflowNode};
|
use crate::graph::{WorkflowGraph, WorkflowNode};
|
||||||
use crate::outcome::{BilledModelUsage, FailureCategory, FailureDetail, Outcome, StageStatus};
|
use crate::outcome::{BilledModelUsage, FailureCategory, FailureDetail, Outcome, StageStatus};
|
||||||
|
|
@ -44,11 +42,9 @@ pub(crate) struct EventLifecycle {
|
||||||
pub run_branch: Option<String>,
|
pub run_branch: Option<String>,
|
||||||
pub worktree_dir: Option<String>,
|
pub worktree_dir: Option<String>,
|
||||||
pub goal: Option<String>,
|
pub goal: Option<String>,
|
||||||
pub captured_artifact_count: Arc<AtomicUsize>,
|
/// Shared git checkpoint result (written by GitLifecycle, read by
|
||||||
// Cross-lifecycle data
|
/// EventLifecycle when emitting CheckpointCompleted).
|
||||||
pub checkpoint_git_result: Arc<Mutex<Option<GitCheckpointResult>>>,
|
pub checkpoint_git_result: Arc<Mutex<Option<GitCheckpointResult>>>,
|
||||||
pub last_git_sha: Arc<Mutex<Option<String>>>,
|
|
||||||
pub final_patch: Arc<Mutex<Option<String>>>,
|
|
||||||
pub circuit_breaker: Arc<CircuitBreakerLifecycle>,
|
pub circuit_breaker: Arc<CircuitBreakerLifecycle>,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -415,75 +411,4 @@ impl RunLifecycle<WorkflowGraph> for EventLifecycle {
|
||||||
|
|
||||||
Ok(())
|
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::<Vec<_>>();
|
|
||||||
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,
|
|
||||||
});
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -14,11 +14,11 @@ use crate::event::{Emitter, Event, RunNoticeLevel};
|
||||||
use crate::git::MetadataStore;
|
use crate::git::MetadataStore;
|
||||||
use crate::graph::{WorkflowGraph, WorkflowNode};
|
use crate::graph::{WorkflowGraph, WorkflowNode};
|
||||||
use crate::lifecycle::event::stage_scope_for;
|
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_dump::RunDump;
|
||||||
use crate::run_options::RunOptions;
|
use crate::run_options::RunOptions;
|
||||||
use crate::runtime_store::RunStoreHandle;
|
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<Option<BilledModelUsage>>;
|
type WfRunState = ExecutionState<Option<BilledModelUsage>>;
|
||||||
type WfNodeResult = NodeResult<Option<BilledModelUsage>>;
|
type WfNodeResult = NodeResult<Option<BilledModelUsage>>;
|
||||||
|
|
@ -71,7 +71,6 @@ pub(crate) struct GitLifecycle {
|
||||||
// Cross-lifecycle data (shared with EventLifecycle)
|
// Cross-lifecycle data (shared with EventLifecycle)
|
||||||
pub checkpoint_git_result: Arc<Mutex<Option<GitCheckpointResult>>>,
|
pub checkpoint_git_result: Arc<Mutex<Option<GitCheckpointResult>>>,
|
||||||
pub last_git_sha: Arc<Mutex<Option<String>>>,
|
pub last_git_sha: Arc<Mutex<Option<String>>>,
|
||||||
pub final_patch: Arc<Mutex<Option<String>>>,
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[async_trait]
|
#[async_trait]
|
||||||
|
|
@ -80,7 +79,6 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
||||||
// Reset last_git_sha (diff base parity)
|
// Reset last_git_sha (diff base parity)
|
||||||
*self.last_git_sha.lock().unwrap() = None;
|
*self.last_git_sha.lock().unwrap() = None;
|
||||||
*self.checkpoint_git_result.lock().unwrap() = None;
|
*self.checkpoint_git_result.lock().unwrap() = None;
|
||||||
*self.final_patch.lock().unwrap() = None;
|
|
||||||
|
|
||||||
// Init metadata branch (best-effort)
|
// Init metadata branch (best-effort)
|
||||||
if let (Some(_), Some(repo_path)) = (
|
if let (Some(_), Some(repo_path)) = (
|
||||||
|
|
@ -321,43 +319,4 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
||||||
|
|
||||||
Ok(())
|
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}"),
|
|
||||||
});
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -8,7 +8,7 @@ pub(crate) mod hook;
|
||||||
|
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use std::path::{Path, PathBuf};
|
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::sync::{Arc, Mutex};
|
||||||
use std::time::Instant;
|
use std::time::Instant;
|
||||||
|
|
||||||
|
|
@ -96,8 +96,6 @@ impl WorkflowLifecycle {
|
||||||
let checkpoint_git_result: Arc<Mutex<Option<GitCheckpointResult>>> =
|
let checkpoint_git_result: Arc<Mutex<Option<GitCheckpointResult>>> =
|
||||||
Arc::new(Mutex::new(None));
|
Arc::new(Mutex::new(None));
|
||||||
let last_git_sha: Arc<Mutex<Option<String>>> = Arc::new(Mutex::new(None));
|
let last_git_sha: Arc<Mutex<Option<String>>> = Arc::new(Mutex::new(None));
|
||||||
let final_patch: Arc<Mutex<Option<String>>> = 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));
|
let circuit_breaker = Arc::new(CircuitBreakerLifecycle::new(loop_restart_signature_limit));
|
||||||
|
|
||||||
|
|
@ -124,9 +122,6 @@ impl WorkflowLifecycle {
|
||||||
run_branch: run_options.git.as_ref().and_then(|g| g.run_branch.clone()),
|
run_branch: run_options.git.as_ref().and_then(|g| g.run_branch.clone()),
|
||||||
worktree_dir: working_directory.clone(),
|
worktree_dir: working_directory.clone(),
|
||||||
goal: (!graph.goal().is_empty()).then(|| graph.goal().to_string()),
|
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),
|
checkpoint_git_result: Arc::clone(&checkpoint_git_result),
|
||||||
circuit_breaker: Arc::clone(&circuit_breaker),
|
circuit_breaker: Arc::clone(&circuit_breaker),
|
||||||
};
|
};
|
||||||
|
|
@ -156,8 +151,7 @@ impl WorkflowLifecycle {
|
||||||
run_options: Arc::clone(run_options),
|
run_options: Arc::clone(run_options),
|
||||||
start_node_id,
|
start_node_id,
|
||||||
checkpoint_git_result: Arc::clone(&checkpoint_git_result),
|
checkpoint_git_result: Arc::clone(&checkpoint_git_result),
|
||||||
last_git_sha: Arc::clone(&last_git_sha),
|
last_git_sha,
|
||||||
final_patch,
|
|
||||||
};
|
};
|
||||||
|
|
||||||
let artifact = ArtifactLifecycle::new(
|
let artifact = ArtifactLifecycle::new(
|
||||||
|
|
@ -167,7 +161,6 @@ impl WorkflowLifecycle {
|
||||||
run_options.run_id,
|
run_options.run_id,
|
||||||
run_options.artifact_globs(),
|
run_options.artifact_globs(),
|
||||||
artifact_sink,
|
artifact_sink,
|
||||||
captured_artifact_count,
|
|
||||||
);
|
);
|
||||||
|
|
||||||
Self {
|
Self {
|
||||||
|
|
@ -419,12 +412,6 @@ impl RunLifecycle<WorkflowGraph> for WorkflowLifecycle {
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn on_run_end(&self, outcome: &Outcome, state: &WfRunState) {
|
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;
|
self.hook.on_run_end(outcome, state).await;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -32,7 +32,6 @@ use crate::pipeline::initialize;
|
||||||
use crate::pipeline::types::{InitOptions, LlmSpec, Persisted, SandboxEnvSpec};
|
use crate::pipeline::types::{InitOptions, LlmSpec, Persisted, SandboxEnvSpec};
|
||||||
use crate::records::RunSpec;
|
use crate::records::RunSpec;
|
||||||
use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions};
|
use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions};
|
||||||
use crate::run_status::{FailureReason, RunStatus};
|
|
||||||
use crate::test_support::run_graph;
|
use crate::test_support::run_graph;
|
||||||
|
|
||||||
fn local_env() -> Arc<dyn Sandbox> {
|
fn local_env() -> Arc<dyn Sandbox> {
|
||||||
|
|
@ -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;
|
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)));
|
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]
|
#[tokio::test]
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,7 @@
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
use fabro_hooks::{HookContext, HookEvent, HookRunner};
|
use fabro_hooks::{HookContext, HookEvent, HookRunner};
|
||||||
use fabro_types::BilledTokenCounts;
|
use fabro_types::{BilledTokenCounts, EventBody};
|
||||||
|
|
||||||
use super::types::{Concluded, FinalizeOptions, Retroed};
|
use super::types::{Concluded, FinalizeOptions, Retroed};
|
||||||
use crate::error::Error;
|
use crate::error::Error;
|
||||||
|
|
@ -13,7 +13,7 @@ use crate::run_dump::RunDump;
|
||||||
use crate::run_options::RunOptions;
|
use crate::run_options::RunOptions;
|
||||||
use crate::run_status::{FailureReason, RunStatus, SuccessReason};
|
use crate::run_status::{FailureReason, RunStatus, SuccessReason};
|
||||||
use crate::runtime_store::RunStoreHandle;
|
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(
|
fn emit_run_notice(
|
||||||
emitter: &Emitter,
|
emitter: &Emitter,
|
||||||
|
|
@ -153,9 +153,14 @@ fn build_conclusion_from_parts(
|
||||||
|
|
||||||
/// Write a finalize projection snapshot commit to the metadata branch.
|
/// Write a finalize projection snapshot commit to the metadata branch.
|
||||||
///
|
///
|
||||||
/// This captures the final `run.json` projection state, including conclusion
|
/// `conclusion` is injected into the projection copy: the terminal event
|
||||||
/// and retro data. Best-effort: errors are logged as warnings.
|
/// hasn't been emitted yet (FINALIZE emits it after this commit lands), so
|
||||||
pub async fn write_finalize_commit(run_options: &RunOptions, run_store: &RunStoreHandle) {
|
/// 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)) = (
|
let (Some(meta_branch), Some(repo_path)) = (
|
||||||
run_options
|
run_options
|
||||||
.git
|
.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 git_author = run_options.git_author();
|
||||||
let store = MetadataStore::new(repo_path, &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;
|
return;
|
||||||
};
|
};
|
||||||
|
if store_state.conclusion.is_none() {
|
||||||
|
store_state.conclusion = Some(conclusion.clone());
|
||||||
|
}
|
||||||
let dump = RunDump::from_projection(&store_state);
|
let dump = RunDump::from_projection(&store_state);
|
||||||
if let Err(e) =
|
if let Err(e) =
|
||||||
dump.write_to_metadata_store(&store, &run_options.run_id.to_string(), "finalize run")
|
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;
|
.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<String> {
|
||||||
|
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<Outcome, Error>,
|
||||||
|
duration_ms: u64,
|
||||||
|
artifact_count: usize,
|
||||||
|
final_git_commit_sha: Option<String>,
|
||||||
|
final_patch: Option<String>,
|
||||||
|
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(
|
async fn run_hooks(
|
||||||
hook_runner: Option<&HookRunner>,
|
hook_runner: Option<&HookRunner>,
|
||||||
hook_context: &HookContext,
|
hook_context: &HookContext,
|
||||||
|
|
@ -219,7 +344,14 @@ async fn cleanup_sandbox(
|
||||||
Ok(())
|
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
|
/// # Errors
|
||||||
///
|
///
|
||||||
|
|
@ -240,14 +372,33 @@ pub async fn finalize(retroed: Retroed, options: &FinalizeOptions) -> Result<Con
|
||||||
let (final_status, failure_reason, _run_status) = classify_engine_result(&outcome);
|
let (final_status, failure_reason, _run_status) = classify_engine_result(&outcome);
|
||||||
let conclusion = build_conclusion_from_store(
|
let conclusion = build_conclusion_from_store(
|
||||||
&options.run_store,
|
&options.run_store,
|
||||||
final_status,
|
final_status.clone(),
|
||||||
failure_reason,
|
failure_reason,
|
||||||
duration_ms,
|
duration_ms,
|
||||||
options.last_git_sha.clone(),
|
options.last_git_sha.clone(),
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
write_finalize_commit(&run_options, &options.run_store).await;
|
let final_patch = compute_final_patch(&run_options, &*sandbox, final_status, &emitter).await;
|
||||||
|
|
||||||
|
write_finalize_commit(&run_options, &options.run_store, &conclusion).await;
|
||||||
|
|
||||||
|
let events = options.run_store.list_events().await.unwrap_or_default();
|
||||||
|
let artifact_count = events
|
||||||
|
.iter()
|
||||||
|
.filter(|envelope| matches!(envelope.event.body, EventBody::ArtifactCaptured(_)))
|
||||||
|
.count();
|
||||||
|
let state_for_event = options.run_store.state().await.ok();
|
||||||
|
|
||||||
|
let terminal_event = build_terminal_event(
|
||||||
|
&outcome,
|
||||||
|
duration_ms,
|
||||||
|
artifact_count,
|
||||||
|
options.last_git_sha.clone(),
|
||||||
|
final_patch,
|
||||||
|
state_for_event.as_ref(),
|
||||||
|
);
|
||||||
|
emitter.emit(&terminal_event);
|
||||||
|
|
||||||
if options.preserve_sandbox {
|
if options.preserve_sandbox {
|
||||||
let info = sandbox.sandbox_info();
|
let info = sandbox.sandbox_info();
|
||||||
|
|
|
||||||
|
|
@ -11,7 +11,7 @@ mod validate;
|
||||||
|
|
||||||
pub use execute::execute;
|
pub use execute::execute;
|
||||||
pub use fabro_types::PullRequestRecord;
|
pub use fabro_types::PullRequestRecord;
|
||||||
pub(crate) use finalize::build_conclusion_from_store;
|
pub(crate) use finalize::{build_conclusion_from_store, build_terminal_event};
|
||||||
pub use finalize::{classify_engine_result, finalize, write_finalize_commit};
|
pub use finalize::{classify_engine_result, finalize, write_finalize_commit};
|
||||||
pub use initialize::initialize;
|
pub use initialize::initialize;
|
||||||
pub use parse::parse;
|
pub use parse::parse;
|
||||||
|
|
|
||||||
|
|
@ -15,10 +15,27 @@ use crate::event::{Emitter, Event, StoreProgressLogger, append_event};
|
||||||
use crate::handler::HandlerRegistry;
|
use crate::handler::HandlerRegistry;
|
||||||
use crate::outcome::Outcome;
|
use crate::outcome::Outcome;
|
||||||
use crate::pipeline;
|
use crate::pipeline;
|
||||||
use crate::pipeline::types::Initialized;
|
use crate::pipeline::build_terminal_event;
|
||||||
|
use crate::pipeline::types::{Executed, Initialized};
|
||||||
use crate::records::Checkpoint;
|
use crate::records::Checkpoint;
|
||||||
use crate::run_options::RunOptions;
|
use crate::run_options::RunOptions;
|
||||||
|
|
||||||
|
/// FINALIZE emits the terminal event in production. These helpers stop at
|
||||||
|
/// EXECUTE, so they emit it here to keep test consumers seeing the same
|
||||||
|
/// end-of-run signal.
|
||||||
|
async fn emit_test_terminal_event(executed: &Executed) {
|
||||||
|
let state = executed.run_store.state().await.ok();
|
||||||
|
let event = build_terminal_event(
|
||||||
|
&executed.outcome,
|
||||||
|
executed.duration_ms,
|
||||||
|
0,
|
||||||
|
None,
|
||||||
|
None,
|
||||||
|
state.as_ref(),
|
||||||
|
);
|
||||||
|
executed.emitter.emit(&event);
|
||||||
|
}
|
||||||
|
|
||||||
pub fn test_store_dir(run_dir: &std::path::Path) -> PathBuf {
|
pub fn test_store_dir(run_dir: &std::path::Path) -> PathBuf {
|
||||||
let mut hasher = std::collections::hash_map::DefaultHasher::new();
|
let mut hasher = std::collections::hash_map::DefaultHasher::new();
|
||||||
std::process::id().hash(&mut hasher);
|
std::process::id().hash(&mut hasher);
|
||||||
|
|
@ -165,6 +182,7 @@ pub async fn run_graph(
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let executed = pipeline::execute(initialized.initialized).await;
|
let executed = pipeline::execute(initialized.initialized).await;
|
||||||
|
emit_test_terminal_event(&executed).await;
|
||||||
// Tests often reopen the run store immediately after `run()` returns.
|
// Tests often reopen the run store immediately after `run()` returns.
|
||||||
// Flush the async store logger first so they don't observe partial state.
|
// Flush the async store logger first so they don't observe partial state.
|
||||||
initialized.store_logger.flush().await;
|
initialized.store_logger.flush().await;
|
||||||
|
|
@ -192,6 +210,7 @@ pub async fn run_graph_with_state(
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let executed = pipeline::execute(initialized.initialized).await;
|
let executed = pipeline::execute(initialized.initialized).await;
|
||||||
|
emit_test_terminal_event(&executed).await;
|
||||||
initialized.store_logger.flush().await;
|
initialized.store_logger.flush().await;
|
||||||
let outcome = executed.outcome?;
|
let outcome = executed.outcome?;
|
||||||
let state = executed
|
let state = executed
|
||||||
|
|
@ -225,6 +244,7 @@ pub async fn run_graph_with_hooks(
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let executed = pipeline::execute(initialized.initialized).await;
|
let executed = pipeline::execute(initialized.initialized).await;
|
||||||
|
emit_test_terminal_event(&executed).await;
|
||||||
initialized.store_logger.flush().await;
|
initialized.store_logger.flush().await;
|
||||||
executed.outcome
|
executed.outcome
|
||||||
}
|
}
|
||||||
|
|
@ -252,6 +272,7 @@ pub async fn run_graph_with_hooks_and_state(
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let executed = pipeline::execute(initialized.initialized).await;
|
let executed = pipeline::execute(initialized.initialized).await;
|
||||||
|
emit_test_terminal_event(&executed).await;
|
||||||
initialized.store_logger.flush().await;
|
initialized.store_logger.flush().await;
|
||||||
let outcome = executed.outcome?;
|
let outcome = executed.outcome?;
|
||||||
let state = executed
|
let state = executed
|
||||||
|
|
@ -284,6 +305,7 @@ pub async fn run_graph_from_checkpoint(
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let executed = pipeline::execute(initialized.initialized).await;
|
let executed = pipeline::execute(initialized.initialized).await;
|
||||||
|
emit_test_terminal_event(&executed).await;
|
||||||
initialized.store_logger.flush().await;
|
initialized.store_logger.flush().await;
|
||||||
executed.outcome
|
executed.outcome
|
||||||
}
|
}
|
||||||
|
|
@ -310,6 +332,7 @@ pub async fn run_graph_from_checkpoint_with_state(
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let executed = pipeline::execute(initialized.initialized).await;
|
let executed = pipeline::execute(initialized.initialized).await;
|
||||||
|
emit_test_terminal_event(&executed).await;
|
||||||
initialized.store_logger.flush().await;
|
initialized.store_logger.flush().await;
|
||||||
let outcome = executed.outcome?;
|
let outcome = executed.outcome?;
|
||||||
let state = executed
|
let state = executed
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue