From a992a7d76b22f6206d5ea107feadeba852677da1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 27 May 2026 18:02:29 -0400 Subject: [PATCH] feat(runs): allow retrying succeeded runs Broaden manual retry eligibility to all unarchived terminal runs while preserving active and archived precondition failures. --- apps/fabro-web/app/lib/run-actions.test.ts | 5 +- apps/fabro-web/app/lib/run-actions.ts | 2 +- docs/public/api-reference/fabro-api.yaml | 6 +- lib/crates/fabro-server/src/server/tests.rs | 66 ++++++++++++- .../fabro-workflow/src/operations/retry.rs | 98 ++++++++++++++----- .../fabro-api-client/src/api/runs-api.ts | 8 +- 6 files changed, 151 insertions(+), 34 deletions(-) diff --git a/apps/fabro-web/app/lib/run-actions.test.ts b/apps/fabro-web/app/lib/run-actions.test.ts index 8acde997f..ffb208aa7 100644 --- a/apps/fabro-web/app/lib/run-actions.test.ts +++ b/apps/fabro-web/app/lib/run-actions.test.ts @@ -427,13 +427,14 @@ describe("run lifecycle actions", () => { expect(canApprove(makeRun({ kind: "runnable" }))).toBe(false); }); - test("canRetry allows failed (including cancelled) and dead runs except archived runs", () => { + test("canRetry allows terminal runs except archived runs", () => { expect(canRetry(makeRun({ kind: "failed", reason: "workflow_error" }))).toBe(true); expect(canRetry(makeRun({ kind: "dead" }))).toBe(true); expect(canRetry(makeRun({ kind: "failed", reason: "cancelled" }))).toBe(true); - expect(canRetry(makeRun({ kind: "succeeded", reason: "completed" }))).toBe(false); + expect(canRetry(makeRun({ kind: "succeeded", reason: "completed" }))).toBe(true); expect(canRetry(makeRun({ kind: "running" }))).toBe(false); expect(canRetry(makeRun({ kind: "failed", reason: "workflow_error" }, true))).toBe(false); + expect(canRetry(makeRun({ kind: "succeeded", reason: "completed" }, true))).toBe(false); }); test("isTerminalCancelledRun distinguishes immediate cancel success from in-flight cancellation", () => { diff --git a/apps/fabro-web/app/lib/run-actions.ts b/apps/fabro-web/app/lib/run-actions.ts index 53ccab596..d51c1210a 100644 --- a/apps/fabro-web/app/lib/run-actions.ts +++ b/apps/fabro-web/app/lib/run-actions.ts @@ -152,7 +152,7 @@ export function canUnarchive(status: string | null | undefined): boolean { export function canRetry(run: Pick | null | undefined): boolean { if (!run || run.lifecycle.archived) return false; const status = run.lifecycle.status; - return status.kind === "failed" || status.kind === "dead"; + return status.kind === "succeeded" || status.kind === "failed" || status.kind === "dead"; } export function canDelete(status: string | null | undefined): boolean { diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index eaa272e0c..aff64c205 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -2076,10 +2076,10 @@ paths: tags: [Runs] summary: Retry Run description: > - Creates a fresh run from the failed or dead source run's captured + Creates a fresh run from the terminal source run's captured durable definition, records `retried_from` on the new run, and schedules - it for execution. The source run is left unchanged. Active, succeeded, - and archived runs are not retryable. + it for execution. The source run is left unchanged. Active and archived + runs are not retryable. parameters: - $ref: "#/components/parameters/RunId" responses: diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 78432c1b2..e53312627 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -10417,8 +10417,8 @@ async fn retry_missing_run_returns_not_found() { } #[tokio::test] -async fn retry_non_retryable_run_returns_conflict() { - let state = test_app_state(); +async fn retry_succeeded_run_creates_and_queues_new_run() { + let state = test_app_state_with_isolated_storage(); let app = crate::test_support::build_test_router(Arc::clone(&state)); let source_run_id = RunId::new(); create_durable_run_with_events(&state, source_run_id, &[ @@ -10435,6 +10435,68 @@ async fn retry_non_retryable_run_returns_conflict() { }, ]) .await; + let source_events_before = state + .store + .open_run(&source_run_id) + .await + .unwrap() + .list_events() + .await + .unwrap() + .len(); + + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(api(&format!("/runs/{source_run_id}/retry"))) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let body = response_json!(response, StatusCode::CREATED).await; + let new_run_id = body["id"].as_str().unwrap().parse::().unwrap(); + + assert_ne!(new_run_id, source_run_id); + assert_eq!(body["retried_from"], source_run_id.to_string()); + assert_eq!(run_json_status(&body)["kind"], "runnable"); + + let source_store = state.store.open_run(&source_run_id).await.unwrap(); + assert_eq!( + source_store.list_events().await.unwrap().len(), + source_events_before + ); + assert_eq!( + source_store.state().await.unwrap().status, + RunStatus::Succeeded { + reason: SuccessReason::Completed, + } + ); + + let new_state = state + .store + .open_run(&new_run_id) + .await + .unwrap() + .state() + .await + .unwrap(); + assert_eq!(new_state.retried_from, Some(source_run_id)); + assert_eq!(new_state.status, RunStatus::Runnable); +} + +#[tokio::test] +async fn retry_active_run_returns_conflict() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let source_run_id = RunId::new(); + create_durable_run_with_events(&state, source_run_id, &[ + workflow_event::Event::RunSubmitted { + definition_blob: None, + }, + ]) + .await; let response = app .oneshot( diff --git a/lib/crates/fabro-workflow/src/operations/retry.rs b/lib/crates/fabro-workflow/src/operations/retry.rs index 7a80033df..be7793d3e 100644 --- a/lib/crates/fabro-workflow/src/operations/retry.rs +++ b/lib/crates/fabro-workflow/src/operations/retry.rs @@ -103,11 +103,12 @@ pub async fn retry_run( } fn ensure_retryable(status: RunStatus, run_id: &RunId) -> std::result::Result<(), Error> { - match status { - RunStatus::Failed { .. } | RunStatus::Dead => Ok(()), - other => Err(Error::Precondition(format!( - "run {run_id} cannot be retried from status {other}; expected failed or dead" - ))), + if status.is_terminal() { + Ok(()) + } else { + Err(Error::Precondition(format!( + "run {run_id} cannot be retried from status {status}; expected terminal" + ))) } } @@ -235,6 +236,23 @@ mod tests { event::append_event(store, &run_id, &event).await.unwrap(); } + async fn append_succeeded(store: &fabro_store::RunDatabase, run_id: RunId) { + append_started(store, run_id).await; + event::append_event(store, &run_id, &Event::WorkflowRunCompleted { + timing: RunTiming::wall_only(10), + artifact_count: 0, + status: "succeeded".to_string(), + reason: fabro_types::SuccessReason::Completed, + total_usd_micros: None, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, + }) + .await + .unwrap(); + } + async fn seed_retryable_failed_source( store: &Database, source_run_id: RunId, @@ -424,26 +442,51 @@ mod tests { } #[tokio::test] - async fn retry_rejects_non_retryable_sources() { + async fn retry_creates_fresh_run_from_succeeded_source() { let store = memory_store(); - - let succeeded = fixtures::RUN_1; - let succeeded_store = store.create_run(&succeeded).await.unwrap(); - append_created(&succeeded_store, succeeded, None, None).await; - append_started(&succeeded_store, succeeded).await; - event::append_event(&succeeded_store, &succeeded, &Event::WorkflowRunCompleted { - timing: RunTiming::wall_only(10), - artifact_count: 0, - status: "succeeded".to_string(), - reason: fabro_types::SuccessReason::Completed, - total_usd_micros: None, - final_git_commit_sha: None, - final_patch: None, - diff_summary: None, - billing: None, + let source_run_id = fixtures::RUN_1; + let source_store = store.create_run(&source_run_id).await.unwrap(); + append_created(&source_store, source_run_id, None, None).await; + let definition_blob = Some( + source_store + .write_blob(br#"{\"definition\":true}"#) + .await + .unwrap(), + ); + event::append_event(&source_store, &source_run_id, &Event::RunSubmitted { + definition_blob, }) .await .unwrap(); + append_succeeded(&source_store, source_run_id).await; + + let outcome = retry_run(&store, &RetryRunInput { + source_run_id, + new_run_id: RunId::new(), + provenance: Some(provenance("retry-user")), + web_url: None, + }) + .await + .unwrap(); + + let retry_store = store.open_run(&outcome.new_run_id).await.unwrap(); + let retry_events = retry_store.list_events().await.unwrap(); + let retry_state = fabro_store::RunProjection::apply_events(&retry_events).unwrap(); + assert_eq!(retry_events.len(), 2); + assert_eq!(retry_state.status, RunStatus::Submitted); + assert_eq!(retry_state.retried_from, Some(source_run_id)); + assert_eq!(retry_state.spec.definition_blob, definition_blob); + assert_eq!( + source_store.state().await.unwrap().status, + RunStatus::Succeeded { + reason: fabro_types::SuccessReason::Completed, + } + ); + } + + #[tokio::test] + async fn retry_rejects_active_and_archived_sources() { + let store = memory_store(); let active = fixtures::RUN_2; let active_store = store.create_run(&active).await.unwrap(); @@ -470,7 +513,7 @@ mod tests { .await .unwrap(); - for run_id in [succeeded, active, archived] { + for run_id in [active, archived] { let err = retry_run(&store, &RetryRunInput { source_run_id: run_id, new_run_id: RunId::new(), @@ -509,6 +552,17 @@ mod tests { ensure_retryable(RunStatus::Dead, &fixtures::RUN_1).unwrap(); } + #[test] + fn succeeded_status_is_retryable() { + ensure_retryable( + RunStatus::Succeeded { + reason: fabro_types::SuccessReason::Completed, + }, + &fixtures::RUN_1, + ) + .unwrap(); + } + #[test] fn cancelled_status_is_retryable() { ensure_retryable( diff --git a/lib/packages/fabro-api-client/src/api/runs-api.ts b/lib/packages/fabro-api-client/src/api/runs-api.ts index 0977f14f1..963fe112a 100644 --- a/lib/packages/fabro-api-client/src/api/runs-api.ts +++ b/lib/packages/fabro-api-client/src/api/runs-api.ts @@ -1120,7 +1120,7 @@ export const RunsApiAxiosParamCreator = function (configuration?: Configuration) }; }, /** - * Creates a fresh run from the failed or dead source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active, succeeded, and archived runs are not retryable. + * Creates a fresh run from the terminal source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active and archived runs are not retryable. * @summary Retry Run * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option. @@ -1868,7 +1868,7 @@ export const RunsApiFp = function(configuration?: Configuration) { return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, /** - * Creates a fresh run from the failed or dead source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active, succeeded, and archived runs are not retryable. + * Creates a fresh run from the terminal source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active and archived runs are not retryable. * @summary Retry Run * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option. @@ -2264,7 +2264,7 @@ export const RunsApiFactory = function (configuration?: Configuration, basePath? return localVarFp.retrieveRunGraphSource(id, options).then((request) => request(axios, basePath)); }, /** - * Creates a fresh run from the failed or dead source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active, succeeded, and archived runs are not retryable. + * Creates a fresh run from the terminal source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active and archived runs are not retryable. * @summary Retry Run * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option. @@ -2652,7 +2652,7 @@ export class RunsApi extends BaseAPI { } /** - * Creates a fresh run from the failed or dead source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active, succeeded, and archived runs are not retryable. + * Creates a fresh run from the terminal source run\'s captured durable definition, records `retried_from` on the new run, and schedules it for execution. The source run is left unchanged. Active and archived runs are not retryable. * @summary Retry Run * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option.