From baa2fc8b5cf0f5223ba3a92f70458c885402f160 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 18 Sep 2026 18:39:58 -0400 Subject: [PATCH] Keep every workspace under Petri's retention and say why Petri's retention decides whether a released workspace is kept or removed. Fabro's lifecycle settings decide whether a sandbox keeps running after the run and whether a delete may remove it; none asks for removal at the run's end, and the sandbox tab, `fabro cp`, the run's delete and the sandbox scenarios read the container after the run. So the mapping is `Retention::Always` for every setting, named once as `engine::RETENTION` with the reasoning, instead of a per-setting function that released a finished sandbox. Co-Authored-By: Claude Fable 5.1 --- .../src/commands/run/petri_worker.rs | 1 - .../fabro-server/src/server/petri_runs.rs | 1 - lib/components/fabro-petri/README.md | 15 ++-- lib/components/fabro-petri/src/engine.rs | 81 ++++--------------- lib/components/fabro-petri/src/hooks.rs | 18 ++--- lib/components/fabro-petri/tests/hooks.rs | 4 +- .../fabro-petri/tests/support/mod.rs | 3 +- 7 files changed, 36 insertions(+), 87 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/petri_worker.rs b/lib/apps/fabro-cli/src/commands/run/petri_worker.rs index 55a333437..23576c3ce 100644 --- a/lib/apps/fabro-cli/src/commands/run/petri_worker.rs +++ b/lib/apps/fabro-cli/src/commands/run/petri_worker.rs @@ -205,7 +205,6 @@ pub(super) async fn execute(worker: PetriWorker<'_>) -> Result<()> { .environment .provider .clone(), - retention: engine::retention(&worker.run_state.spec.settings.run.environment), cancel: cancel_token.clone(), controls: controls.clone(), interviewer: Arc::new(petri_interviewer), diff --git a/lib/apps/fabro-server/src/server/petri_runs.rs b/lib/apps/fabro-server/src/server/petri_runs.rs index 6b762914e..5a14d2c13 100644 --- a/lib/apps/fabro-server/src/server/petri_runs.rs +++ b/lib/apps/fabro-server/src/server/petri_runs.rs @@ -318,7 +318,6 @@ pub(crate) async fn execute(state: Arc, run_id: RunId) { .observe_store(Arc::new(SqliteRunStore::new(state.db_pool.clone()))), runtime: runtime_spec(&state, &eligible, dry_run), provider: run_state.spec.settings.run.environment.provider.clone(), - retention: engine::retention(&run_state.spec.settings.run.environment), cancel, // The in-process test path drives no pause or steer: the server's // transports for those name the worker. diff --git a/lib/components/fabro-petri/README.md b/lib/components/fabro-petri/README.md index 2f4803354..c4007396e 100644 --- a/lib/components/fabro-petri/README.md +++ b/lib/components/fabro-petri/README.md @@ -133,12 +133,15 @@ notifications and pairings (recorded, not shown). ### Retention -`engine::retention` maps the run's environment settings onto Petri's -workspace retention: `preserve = true` or `stop_on_terminal = false` keeps -every workspace (`Retention::Always`), as does the local provider, whose -host workspaces live under the run's scratch directory and go with it; -otherwise a failed scope's workspace is kept for debugging and a successful -one is released (`Retention::OnFailure`, Petri's default). +Petri's retention decides, at a scope's release, whether its workspace is +kept or removed. Fabro's environment lifecycle settings decide something +else: `stop_on_terminal` whether a sandbox keeps running after the run, +`preserve` whether the run's delete may remove it. Neither asks for a +sandbox to be removed when the run ends (the legacy executor stopped a +container and left it for the sandbox tab, `fabro cp`, the delete and +`fabro system prune`; a host workspace goes with the run's scratch +directory), so `engine::RETENTION` maps every setting to +`Retention::Always`, and no Fabro setting names `OnFailure` or `Never`. Every run executes on Petri. The server side is `fabro-server`'s `server::petri_runs`; the worker side is `fabro-cli`'s diff --git a/lib/components/fabro-petri/src/engine.rs b/lib/components/fabro-petri/src/engine.rs index 034ba838c..591ca6917 100644 --- a/lib/components/fabro-petri/src/engine.rs +++ b/lib/components/fabro-petri/src/engine.rs @@ -23,10 +23,9 @@ //! hook service: the checkpoint commit before every durable finish and its //! platform record after every route, with a failed commit ending the run //! as a `checkpoint_failed` failure. What the standalone runner's defaults -//! give the run: Petri's local hook service for `[[run.hooks]]` and no host -//! tools. The workspaces' retention comes from the run's environment -//! settings through [`retention`]. Cancellation rides the caller's token: -//! when it fires, the root +//! give the run: Petri's local hook service for `[[run.hooks]]`, no host +//! tools, and `Retention::Always` for every workspace (see [`RETENTION`]). +//! Cancellation rides the caller's token: when it fires, the root //! invocation is cancelled politely and Petri records why. The run's other //! controls (pause, unpause, steer) are the caller's [`RunControls`]: its //! pause gate is installed over the run's hooks, it observes the run, and @@ -48,7 +47,6 @@ use std::path::PathBuf; use std::sync::Arc; -use fabro_types::settings::run::RunEnvironmentSettings; use fabro_types::{FailureReason, RunId, SandboxProviderKind}; use petri_execution::host::{self, HostError, HostRun}; use petri_execution::inspect::{self, InspectError, RunInspection}; @@ -96,8 +94,6 @@ pub struct RunRequest { pub runtime: RuntimeSpec, /// The sandbox provider Fabro resolved for the run's environment. pub provider: SandboxProviderKind, - /// When the run's workspaces are kept after their scope is released. - pub retention: Retention, /// Fires to cancel the run. pub cancel: CancellationToken, /// The run's pause, unpause and steer controls, which the caller keeps @@ -178,7 +174,7 @@ pub async fn run(request: RunRequest) -> Result { let key = RunKey::new(request.run_id.as_str()); let mut options = RunOptions::new(&request.run_dir); options.run_key = Some(key.clone()); - options.retention = request.retention; + options.retention = RETENTION; options.sandbox.backend = backend; // Fabro's hooks restore a sandbox workspace from its snapshots at the // scope's acquisition, so a lease whose sandbox is gone gets a fresh @@ -285,30 +281,20 @@ pub async fn run(request: RunRequest) -> Result { Ok(outcome) } -/// When Petri keeps a run's workspaces after their scope is released, from -/// the run's environment settings: +/// When Petri keeps a run's workspaces after their scope is released. /// -/// - `[environments..lifecycle] preserve = true` asks for the sandbox to -/// stay after the run, so every workspace is kept (`Retention::Always`). -/// - The local provider keeps every workspace too: a host workspace lives under -/// the run's own scratch directory, which `fabro system prune` removes with -/// the run, and the legacy executor never removed it on its own. -/// - `stop_on_terminal = false` asks for the sandbox to outlive the run, so its -/// workspaces are kept (`Retention::Always`). -/// - Otherwise the sandbox is released with the run and Petri's default -/// applies: a failed scope's workspace is kept for debugging, a successful -/// one is not (`Retention::OnFailure`). -#[must_use] -pub fn retention(environment: &RunEnvironmentSettings) -> Retention { - let keep = environment.lifecycle.preserve - || !environment.lifecycle.stop_on_terminal - || environment.provider == SandboxProviderKind::LOCAL; - if keep { - Retention::Always - } else { - Retention::OnFailure - } -} +/// Petri's retention makes one choice at release: keep the workspace (a +/// host directory, a container, a remote sandbox) or remove it. Fabro's +/// environment lifecycle settings make different choices: `stop_on_terminal` +/// says whether a sandbox keeps running after the run, and `preserve` says +/// whether deleting the run may remove it. Neither asks for a sandbox to be +/// removed when the run ends: the legacy executor stopped a container at +/// the end and left it for the sandbox tab, `fabro cp`, the run's delete +/// and `fabro system prune`, and a host workspace lives under the run's +/// scratch directory, which goes with the run. So every setting maps to +/// `Retention::Always`, and `Retention::OnFailure` and `Retention::Never` +/// have no Fabro setting that names them. +pub const RETENTION: Retention = Retention::Always; /// The Fabro run id the run key names. A key that is not one (a test's /// bare key) still gets hooks, under a fresh id for its platform records. @@ -491,41 +477,8 @@ async fn write_receipt(run_dir: &std::path::Path, receipt: &petri_execution::Int #[cfg(test)] mod tests { - use fabro_types::settings::run::EnvironmentLifecycleSettings; - use super::*; - fn environment(provider: SandboxProviderKind) -> RunEnvironmentSettings { - let mut environment = RunEnvironmentSettings::from_environment( - "test".to_string(), - fabro_types::settings::run::EnvironmentSettings::default(), - ); - environment.provider = provider; - environment - } - - #[test] - fn retention_follows_the_environment_lifecycle() { - let mut docker = environment(SandboxProviderKind::DOCKER); - assert_eq!(retention(&docker), Retention::OnFailure); - docker.lifecycle = EnvironmentLifecycleSettings { - preserve: true, - stop_on_terminal: true, - auto_stop: None, - }; - assert_eq!(retention(&docker), Retention::Always); - docker.lifecycle = EnvironmentLifecycleSettings { - preserve: false, - stop_on_terminal: false, - auto_stop: None, - }; - assert_eq!(retention(&docker), Retention::Always); - assert_eq!( - retention(&environment(SandboxProviderKind::LOCAL)), - Retention::Always - ); - } - fn outcome_with(status: RunStatus, failure: Option<&str>, complete: bool) -> RunOutcome { RunOutcome { status, diff --git a/lib/components/fabro-petri/src/hooks.rs b/lib/components/fabro-petri/src/hooks.rs index 6b2244d72..2688571ff 100644 --- a/lib/components/fabro-petri/src/hooks.rs +++ b/lib/components/fabro-petri/src/hooks.rs @@ -36,8 +36,8 @@ //! with the sandbox in place. Fabro's own end-of-run work (the terminal //! lifecycle event, notifications on it) is the run lifecycle path's, on the //! worker's and server's side of the engine, and the workspace's retention is -//! Petri's, mapped from the run's environment settings by -//! [`engine::retention`](crate::engine::retention). +//! Petri's, `Retention::Always` for every Fabro setting +//! ([`engine::RETENTION`](crate::engine::RETENTION)). //! //! # Operation identities //! @@ -965,14 +965,12 @@ impl FabroHooks { .get_or_try_init(|| self.load_recorded()) .await?; let branch = match self.branch.get() { - Some(branch) => branch.clone(), - None => match self.stored_branch().await? { - Some(branch) => branch, - None => { - debug!(run_id = %self.run_id, "no run branch is recorded; no run diff"); - return Ok(()); - } - }, + Some(branch) => Some(branch.clone()), + None => self.stored_branch().await?, + }; + let Some(branch) = branch else { + debug!(run_id = %self.run_id, "no run branch is recorded; no run diff"); + return Ok(()); }; let Some(base_sha) = branch.base_sha.clone() else { return Ok(()); diff --git a/lib/components/fabro-petri/tests/hooks.rs b/lib/components/fabro-petri/tests/hooks.rs index 9b07b9861..1ba189a95 100644 --- a/lib/components/fabro-petri/tests/hooks.rs +++ b/lib/components/fabro-petri/tests/hooks.rs @@ -26,7 +26,7 @@ use fabro_petri::blobs::Blobs; use fabro_petri::check::{self, Bundle, CheckRequest, Launch}; use fabro_petri::checkpoint::{CHECKPOINT_FAILED_CLASS, CheckpointKey, RunWorkspaces}; use fabro_petri::controls::RunControls; -use fabro_petri::engine::{self, Execution, Retention, RunRequest, RunStatus}; +use fabro_petri::engine::{self, Execution, RunRequest, RunStatus}; use fabro_petri::hooks::HooksSpec; use fabro_petri::platform_records::PlatformRecords; use fabro_petri::recovery::{self, Recovery, RecoveryRequest}; @@ -199,7 +199,6 @@ impl Harness { store: Arc::clone(&self.store) as Arc, runtime: RuntimeSpec::default(), provider, - retention: Retention::Always, cancel: CancellationToken::new(), controls: RunControls::new(), interviewer, @@ -816,7 +815,6 @@ async fn a_run_hook_blocks_a_tool_effect_through_the_forwarded_service() { ..RuntimeSpec::default() }, provider: SandboxProviderKind::LOCAL, - retention: Retention::Always, cancel: CancellationToken::new(), controls: RunControls::new(), interviewer, diff --git a/lib/components/fabro-petri/tests/support/mod.rs b/lib/components/fabro-petri/tests/support/mod.rs index 4db4604a0..7cc8fd0c3 100644 --- a/lib/components/fabro-petri/tests/support/mod.rs +++ b/lib/components/fabro-petri/tests/support/mod.rs @@ -16,7 +16,7 @@ use std::time::{Duration, Instant}; use fabro_petri::admission::AdmittedGraphs; use fabro_petri::check::{self, Bundle, CheckRequest, Launch}; use fabro_petri::controls::RunControls; -use fabro_petri::engine::{Execution, Retention, RunRequest}; +use fabro_petri::engine::{Execution, RunRequest}; use fabro_petri::interview::{Approval, FabroInterviewer, QuestionNotice, QuestionSink}; use fabro_petri::runtime::RuntimeSpec; use fabro_types::SandboxProviderKind; @@ -115,7 +115,6 @@ pub(crate) fn run_request( store, runtime, provider: SandboxProviderKind::LOCAL, - retention: Retention::Always, cancel: CancellationToken::new(), controls: RunControls::new(), observers: vec![interviewer.observer()],