From e9c35911bba71b8f811f6491f104cb1b389c6db7 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 9 Oct 2026 12:08:28 -0400 Subject: [PATCH] Require finalization for every Fabro run FabroHooks always declares required run finalization instead of only when the run publishes or checkpoints. Every run takes one path, the best-effort diff branch in run_finished goes away, and a fork always declares what its worker's hooks will declare on resume, so the fork no longer carries its source's flag across. Co-Authored-By: Claude Opus 5.5 --- docs/internal/run-finalization.md | 10 +++---- lib/components/fabro-petri/src/fork.rs | 26 +++++++++--------- lib/components/fabro-petri/src/hooks.rs | 35 +++++++++---------------- 3 files changed, 30 insertions(+), 41 deletions(-) diff --git a/docs/internal/run-finalization.md b/docs/internal/run-finalization.md index 4dc01406e..50a35da51 100644 --- a/docs/internal/run-finalization.md +++ b/docs/internal/run-finalization.md @@ -1,8 +1,8 @@ # Required run finalization -Petri owns the durable run result. Fabro declares required finalization when -its hooks checkpoint or have a publisher, and implements it in -`FabroHooks::finalize_run`. A failed checkpoint rejects with +Petri owns the durable run result. Fabro declares required finalization for +every run and implements it in `FabroHooks::finalize_run`, so a resume or a +fork always matches the stored declaration. A failed checkpoint rejects with `checkpoint_failed` and skips publication. The checkpoint failure cancelled the run, so Petri may commit `cancelled`; Fabro reports that finish as a workflow failure with the checkpoint's message. @@ -45,8 +45,8 @@ historical-view repair requires a separate, bounded process over preserved source records. No publication is invoked by projection or replay. A committed resume returns the same overall result without publishing again. -A fork inherits the source run’s required-finalization declaration while seeding -its records; its worker restores the actual publication hooks before resume. +A fork declares required finalization while seeding its records, as every +Fabro run does; its worker restores the actual publication hooks before resume. Unfinished recovery must restore the same finalization requirement. Petri may call the restored finalizer again after interruption, including a crash after the callback returns but before the terminal record commits. Existing GitHub diff --git a/lib/components/fabro-petri/src/fork.rs b/lib/components/fabro-petri/src/fork.rs index 3b96c023f..bf79af0eb 100644 --- a/lib/components/fabro-petri/src/fork.rs +++ b/lib/components/fabro-petri/src/fork.rs @@ -132,12 +132,11 @@ pub async fn check( .open(&RunKey::new(source.to_string()), Access::Read) .await .map_err(ForkError::Open)?; - check_logs(&*logs, &position).await.map(|_| ()) + check_logs(&*logs, &position).await } -/// [`check`] over the source's opened logs. Returns whether the source -/// requires run finalization, which the fork inherits. -async fn check_logs(logs: &dyn RunLogs, position: &ForkPosition) -> Result { +/// [`check`] over the source's opened logs. +async fn check_logs(logs: &dyn RunLogs, position: &ForkPosition) -> Result<(), ForkError> { let state = host::stored_state(logs).await.map_err(ForkError::Seed)?; let Some(execution) = state.executions.get(&position.execution) else { return Err(ForkError::Refused(format!( @@ -173,20 +172,19 @@ async fn check_logs(logs: &dyn RunLogs, position: &ForkPosition) -> Result bool { - self.required + true } async fn finalize_run( @@ -212,14 +210,14 @@ pub async fn fork(request: ForkRequest) -> Result { .await .map_err(ForkError::Open)?; - let required = check_logs(&*source_logs, &request.position).await?; + check_logs(&*source_logs, &request.position).await?; let mut options = RunOptions::new(&request.fork_run_dir); options.run_key = Some(fork_key.clone()); // A fork only copies records and acquires no sandbox, so it needs no // provider configuration. let runtime = providers::standard_runtime(&SandboxProviderConfig::default()) .options(options) - .hooks(Arc::new(ForkFinalizationRequirement { required })) + .hooks(Arc::new(ForkFinalizationRequirement)) .store(Arc::clone(&request.store)); let forked = host::fork_from(&runtime, &*source_logs, request.position, ForkOptions { rerun_last: request.rerun_last, diff --git a/lib/components/fabro-petri/src/hooks.rs b/lib/components/fabro-petri/src/hooks.rs index 36208453b..207e8975e 100644 --- a/lib/components/fabro-petri/src/hooks.rs +++ b/lib/components/fabro-petri/src/hooks.rs @@ -31,14 +31,13 @@ //! `artifact.collected` record, unless the same file with the same content //! was already collected earlier in the run. A failed write is a recorded //! problem on the transition, never a blocked route. -//! - `finalize_run`, required when the run checkpoints or publishes: the run's -//! diff, its run branch against its base commit, as the `run.diff` platform -//! record with the patch as a blob; a failed checkpoint, which fails the run -//! and skips publication; for a successful run, its publication -//! ([`RunPublisher`]: the platform pushes the run branch and opens a pull -//! request), whose failure fails the run before its terminal record. -//! - `run_finished`: best-effort diff preparation for runs without required -//! finalization, then the forwarded point, so the local service runs +//! - `finalize_run`, required for every run: the run's diff, its run branch +//! against its base commit, as the `run.diff` platform record with the patch +//! as a blob; a failed checkpoint, which fails the run and skips publication; +//! for a successful run, its publication ([`RunPublisher`]: the platform +//! pushes the run branch and opens a pull request), whose failure fails the +//! run before its terminal record. +//! - `run_finished`: the forwarded point, so the local service runs //! `run_complete` and `run_failed` with the sandbox in place. //! - `scope_acquired`: a fresh run's Git target checked out into the workspace //! from inside the scope ([`crate::source`]); a resumed run uses its @@ -1607,10 +1606,11 @@ impl ExecutionHooks for FabroHooks { Ok(report) } + /// Every Fabro run requires finalization, whether or not it publishes + /// or checkpoints: one path for every run, and a declaration that a + /// resume or a fork always matches. fn requires_run_finalization(&self) -> bool { - self.publisher.is_some() - || self.checkpoint_enabled - || self.inner.requires_run_finalization() + true } async fn finalize_run( @@ -1667,17 +1667,8 @@ impl ExecutionHooks for FabroHooks { } async fn run_finished(&self, context: &HookContext, finished: RunFinished) -> Vec { - // Nonpublishing runs keep best-effort diff preparation. Required work - // has already run in finalize_run, before Petri commits its outcome. - if !self.requires_run_finalization() { - if let Err(error) = self.record_run_diff().await { - warn!( - run_id = %self.run_id, - error = %error.render(), - "the run's diff was not recorded" - ); - } - } + // The diff and publication already ran in finalize_run, before Petri + // committed the outcome. self.inner.run_finished(context, finished).await }