From b5de404354bf5a2e076e33acf78d48ceb6544481 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Sat, 30 May 2026 15:07:41 -0400 Subject: [PATCH] ci: lock Cargo resolution and fix cancellation flake (#461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Enforce Cargo lockfile use across CI and release automation so jobs fail on stale `Cargo.lock` state instead of resolving dependencies implicitly. This adds `--locked` to Rust CI, release builds/tests, nightly release tagging, the TypeScript workflow's embedded Rust build, and helper-owned Cargo calls in `fabro-dev`. This also fixes the Linux CI flake exposed by the PR: canceling a durably blocked in-process run could take the abort path while the workflow was still unwinding a human-input gate, causing `run.failed(cancelled)` to be followed by `run.unblocked`. That invalid event order broke projection rebuilds and made `GET /runs/{id}` return 404. Cancellation now uses the durable lifecycle status when selecting the in-process blocked-run path, so the pending interview is cancelled before the terminal event is emitted. The release command's intentional `cargo update --workspace` step is unchanged, because that step updates `Cargo.lock` after bumping the workspace version. ## Testing - `cargo nextest run --locked -p fabro-dev --features dev -E 'test(dry_run_computes_stable_version_from_date) | test(dry_run_prints_equivalent_build_commands)'` - `cargo --locked dev release --dry-run --skip-tests --release-date 2026-01-01` - `cargo --locked dev docs check` - `cargo nextest run --locked -p fabro-server --features test-support cancel_durably_blocked_in_process_run_cancels_pending_interview_without_abort_signal --status-level fail --final-status-level fail --show-progress none` - `cargo nextest run --locked -p fabro-server --features test-support --test it scenario::lifecycle --profile ci --status-level fail --final-status-level fail --show-progress none --no-fail-fast` - Linux Docker stress reproduction: `cargo nextest run --locked -p fabro-server --features test-support --test it scenario::lifecycle::full_http_lifecycle_cancel --profile ci --stress-count 200 --status-level fail --final-status-level fail --show-progress none` - `cargo +nightly-2026-04-14 fmt --check --all` - `cargo +nightly-2026-04-14 clippy --locked -p fabro-dev --features dev --all-targets -- -D warnings` - `cargo +nightly-2026-04-14 clippy --locked -p fabro-server --features test-support --all-targets -- -D warnings` - `git diff --check` Full `cargo nextest run --locked -p fabro-dev --features dev` currently has two unrelated policy-test failures: `policy::catalog_builtin_references_stay_in_allowlist` and `policy::workflow_template_rendering_call_sites_stay_in_allowlist`. --- [![Compound Engineering](https://img.shields.io/badge/Compound_Engineering-6366f1)](https://github.com/EveryInc/compound-engineering-plugin) 🤖 Generated with GPT-5 via [Codex](https://openai.com/codex) --------- Co-authored-by: Release Repro --- .github/workflows/nightly.yml | 2 +- .github/workflows/release.yml | 10 +-- .github/workflows/rust.yml | 8 +- .github/workflows/typescript.yml | 2 +- .../fabro-dev/src/commands/docker_build.rs | 3 +- .../src/commands/docs_cli_reference.rs | 1 + lib/crates/fabro-dev/src/commands/release.rs | 2 + lib/crates/fabro-dev/tests/it/docker_build.rs | 6 +- lib/crates/fabro-dev/tests/it/release.rs | 4 +- .../src/server/handler/lifecycle.rs | 79 ++++++++++++------- lib/crates/fabro-server/src/server/tests.rs | 66 ++++++++++++++++ 11 files changed, 138 insertions(+), 45 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index f63324d41..5dcc0ce3c 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -67,4 +67,4 @@ jobs: git remote set-url origin \ "https://x-access-token:${release_token}@github.com/${GITHUB_REPOSITORY}.git" unset release_token - cargo dev release --nightly + cargo --locked dev release --nightly diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 91519569b..4410f71f7 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -84,7 +84,7 @@ jobs: - uses: taiki-e/install-action@773334c0e05d7e699e4d78234494308223f3a2cf # nextest - name: Refresh embedded SPA - run: cargo dev spa refresh + run: cargo --locked dev spa refresh - name: Test (x86_64-musl) # nextest still shells through cargo test for this target, so @@ -93,22 +93,22 @@ jobs: env: CC_x86_64_unknown_linux_musl: musl-gcc CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER: musl-gcc - run: cargo nextest run --workspace --target ${{ matrix.target }} --release --status-level slow --profile ci + run: cargo nextest run --locked --workspace --target ${{ matrix.target }} --release --status-level slow --profile ci - name: Test # aarch64-musl test runs have not been validated on the compile # runner yet; shipping binary is exercised via Docker smoke tests. # Re-enable after verifying the workspace passes on this target. if: matrix.target != 'aarch64-unknown-linux-musl' && matrix.target != 'x86_64-unknown-linux-musl' - run: cargo nextest run --workspace --target ${{ matrix.target }} --release --status-level slow --profile ci + run: cargo nextest run --locked --workspace --target ${{ matrix.target }} --release --status-level slow --profile ci - name: Build (musl via cargo-zigbuild) if: matrix.musl - run: cargo zigbuild --target ${{ matrix.target }} --release -p fabro-cli + run: cargo zigbuild --locked --target ${{ matrix.target }} --release -p fabro-cli - name: Build if: ${{ !matrix.musl }} - run: cargo build --target ${{ matrix.target }} --release -p fabro-cli + run: cargo build --locked --target ${{ matrix.target }} --release -p fabro-cli - name: Package run: | diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index cc3582ecf..c20fe22de 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -76,7 +76,7 @@ jobs: run: | ! rg -n 'AuthMode::Disabled|RunAuthMethod|RunSubjectProvenance|\bActorRef\b|\bActorKind\b|AuthenticatedSubject|AuthenticatedService|AuthorizeRunScoped|AuthorizeRunBlob|AuthorizeStageArtifact|AuthorizeCommandLog|auth_method\s*==\s*"disabled"' \ lib/crates apps lib/packages docs/public/api-reference/fabro-api.yaml - - run: cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings + - run: cargo +nightly-2026-04-14 clippy --locked --workspace --all-targets -- -D warnings generated-docs: name: Generated Docs @@ -91,7 +91,7 @@ jobs: - uses: Swatinem/rust-cache@779680da715d629ac1d338a641029a2f4372abb5 # v2 with: cache-on-failure: true - - run: cargo dev docs check + - run: cargo --locked dev docs check test: name: Test (Linux) @@ -107,7 +107,7 @@ jobs: with: cache-on-failure: true - uses: taiki-e/install-action@773334c0e05d7e699e4d78234494308223f3a2cf # nextest - - run: cargo nextest run --workspace --status-level slow --profile ci + - run: cargo nextest run --locked --workspace --status-level slow --profile ci test-macos: name: Test (macOS) @@ -124,4 +124,4 @@ jobs: with: cache-on-failure: true - uses: taiki-e/install-action@773334c0e05d7e699e4d78234494308223f3a2cf # nextest - - run: cargo nextest run --workspace --status-level slow --profile ci + - run: cargo nextest run --locked --workspace --status-level slow --profile ci diff --git a/.github/workflows/typescript.yml b/.github/workflows/typescript.yml index c240e3837..e2f63a047 100644 --- a/.github/workflows/typescript.yml +++ b/.github/workflows/typescript.yml @@ -73,5 +73,5 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 - run: bun install --frozen-lockfile - uses: dtolnay/rust-toolchain@631a55b12751854ce901bb631d5902ceb48146f7 # stable - - run: cargo dev build -- -p fabro-cli --release + - run: cargo --locked dev build -- --locked -p fabro-cli --release - run: wc -c < target/release/fabro diff --git a/lib/crates/fabro-dev/src/commands/docker_build.rs b/lib/crates/fabro-dev/src/commands/docker_build.rs index db914cb13..5f048fe7c 100644 --- a/lib/crates/fabro-dev/src/commands/docker_build.rs +++ b/lib/crates/fabro-dev/src/commands/docker_build.rs @@ -148,6 +148,7 @@ impl DockerBuildPlan { fn spa_refresh_command() -> PlannedCommand { PlannedCommand::new("cargo") + .arg("--locked") .arg("dev") .arg("spa") .arg("refresh") @@ -235,6 +236,6 @@ fn build_script(target: &str, zig_arch: &str) -> String { cargo install --locked --root /opt/cargo-tools cargo-zigbuild; \ fi; \ rustup target add {target}; \ - cargo zigbuild --release -p fabro-cli --target {target}" + cargo zigbuild --locked --release -p fabro-cli --target {target}" ) } diff --git a/lib/crates/fabro-dev/src/commands/docs_cli_reference.rs b/lib/crates/fabro-dev/src/commands/docs_cli_reference.rs index fda0a3e69..ffab221ee 100644 --- a/lib/crates/fabro-dev/src/commands/docs_cli_reference.rs +++ b/lib/crates/fabro-dev/src/commands/docs_cli_reference.rs @@ -44,6 +44,7 @@ pub(crate) fn docs_cli_reference_root(root: &Path, check: bool) -> Result<()> { fn render_cli_reference() -> Result { let command = PlannedCommand::new("cargo") .arg("run") + .arg("--locked") .arg("-p") .arg("fabro-cli") .arg("--") diff --git a/lib/crates/fabro-dev/src/commands/release.rs b/lib/crates/fabro-dev/src/commands/release.rs index 8b8ef8029..e088f38d3 100644 --- a/lib/crates/fabro-dev/src/commands/release.rs +++ b/lib/crates/fabro-dev/src/commands/release.rs @@ -237,6 +237,7 @@ impl ReleasePlan { fn spa_refresh_command() -> PlannedCommand { PlannedCommand::new("cargo") + .arg("--locked") .arg("dev") .arg("spa") .arg("refresh") @@ -249,6 +250,7 @@ impl ReleasePlan { .env("SEGMENT_WRITE_KEY", RELEASE_TEST_SEGMENT_WRITE_KEY) .arg("nextest") .arg("run") + .arg("--locked") .arg("--workspace") .arg("--release") .arg("--profile") diff --git a/lib/crates/fabro-dev/tests/it/docker_build.rs b/lib/crates/fabro-dev/tests/it/docker_build.rs index 14f7c9b92..458569e20 100644 --- a/lib/crates/fabro-dev/tests/it/docker_build.rs +++ b/lib/crates/fabro-dev/tests/it/docker_build.rs @@ -53,7 +53,7 @@ fn dry_run_prints_equivalent_build_commands() { let stdout = output_text(&output.stdout); assert!( - stdout.contains("cargo dev spa refresh"), + stdout.contains("cargo --locked dev spa refresh"), "dry-run should print SPA refresh command:\n{stdout}" ); assert!( @@ -61,7 +61,9 @@ fn dry_run_prints_equivalent_build_commands() { "dry-run should print builder docker run:\n{stdout}" ); assert!( - stdout.contains("cargo zigbuild --release -p fabro-cli --target x86_64-unknown-linux-musl"), + stdout.contains( + "cargo zigbuild --locked --release -p fabro-cli --target x86_64-unknown-linux-musl" + ), "dry-run should print cargo-zigbuild target:\n{stdout}" ); assert!( diff --git a/lib/crates/fabro-dev/tests/it/release.rs b/lib/crates/fabro-dev/tests/it/release.rs index c71a3642d..f6949725e 100644 --- a/lib/crates/fabro-dev/tests/it/release.rs +++ b/lib/crates/fabro-dev/tests/it/release.rs @@ -96,7 +96,7 @@ fn dry_run_computes_stable_version_from_date() { "dry-run should compute base version from date:\n{stdout}" ); assert!( - stdout.contains("cargo dev spa refresh"), + stdout.contains("cargo --locked dev spa refresh"), "dry-run should print one SPA refresh command:\n{stdout}" ); assert!( @@ -109,7 +109,7 @@ fn dry_run_computes_stable_version_from_date() { ); assert!( stdout.contains( - "unset GH_TOKEN GITHUB_TOKEN && SEGMENT_WRITE_KEY=fake-for-local-smoke cargo nextest run" + "unset GH_TOKEN GITHUB_TOKEN && SEGMENT_WRITE_KEY=fake-for-local-smoke cargo nextest run --locked" ), "dry-run should show release tests without inherited GitHub tokens:\n{stdout}" ); diff --git a/lib/crates/fabro-server/src/server/handler/lifecycle.rs b/lib/crates/fabro-server/src/server/handler/lifecycle.rs index 975f50154..06beb7c0b 100644 --- a/lib/crates/fabro-server/src/server/handler/lifecycle.rs +++ b/lib/crates/fabro-server/src/server/handler/lifecycle.rs @@ -371,46 +371,67 @@ async fn cancel_run( if let Some(response) = reject_if_archived(state.as_ref(), &id).await { return response; } - let pending_control = match load_pending_control(state.as_ref(), id).await { - Ok(pending_control) => pending_control, + let durable_summary = match state.store.runs().find(&id).await { + Ok(summary) => summary, Err(err) => { return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) .into_response(); } }; + let pending_control = durable_summary + .as_ref() + .and_then(|summary| summary.lifecycle.pending_control); + let durable_status = durable_summary + .as_ref() + .map(|summary| summary.lifecycle.status); let cancel_target = { let mut runs = state.runs.lock().expect("runs lock poisoned"); match runs.get_mut(&id) { - Some(managed_run) => match managed_run.status { - RunStatus::Submitted - | RunStatus::Pending { .. } - | RunStatus::Runnable - | RunStatus::Starting - | RunStatus::Running - | RunStatus::Blocked { .. } - | RunStatus::Paused { .. } => { - let persist_cancelled_status = matches!( - managed_run.status, - RunStatus::Submitted | RunStatus::Pending { .. } | RunStatus::Runnable - ); - if persist_cancelled_status { - managed_run.status = RunStatus::Failed { - reason: FailureReason::Cancelled, + Some(managed_run) => { + let managed_status = managed_run.status; + match managed_status { + RunStatus::Submitted + | RunStatus::Pending { .. } + | RunStatus::Runnable + | RunStatus::Starting + | RunStatus::Running + | RunStatus::Blocked { .. } + | RunStatus::Paused { .. } => { + let answer_transport = managed_run.answer_transport.clone(); + let should_cancel_pending_interview = + matches!( + &answer_transport, + Some(RunAnswerTransport::InProcess { .. }) + ) && (matches!(managed_status, RunStatus::Blocked { .. }) + || matches!(durable_status, Some(RunStatus::Blocked { .. }))); + let persist_cancelled_status = matches!( + managed_status, + RunStatus::Submitted | RunStatus::Pending { .. } | RunStatus::Runnable + ) && !should_cancel_pending_interview; + if persist_cancelled_status { + managed_run.status = RunStatus::Failed { + reason: FailureReason::Cancelled, + }; + } + let cancel_tx = if should_cancel_pending_interview { + None + } else { + managed_run.cancel_tx.take() }; + Some(( + persist_cancelled_status, + answer_transport, + managed_run.cancel_token.clone(), + cancel_tx, + managed_run.worker_ref.clone(), + )) + } + _ => { + return ApiError::new(StatusCode::CONFLICT, "Run is not cancellable.") + .into_response(); } - Some(( - persist_cancelled_status, - managed_run.answer_transport.clone(), - managed_run.cancel_token.clone(), - managed_run.cancel_tx.take(), - managed_run.worker_ref.clone(), - )) } - _ => { - return ApiError::new(StatusCode::CONFLICT, "Run is not cancellable.") - .into_response(); - } - }, + } None => None, } }; diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 86e3bfeb3..1076b3f7e 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -13626,6 +13626,72 @@ async fn cancel_run_requests_worker_runtime_stop_when_control_unavailable() { assert_eq!(runtime.requested_refs(), vec![worker_ref]); } +#[tokio::test] +async fn cancel_durably_blocked_in_process_run_cancels_pending_interview_without_abort_signal() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let run_id = fixtures::RUN_1; + create_durable_run_with_events(&state, run_id, &[ + workflow_event::Event::RunRunning, + workflow_event::Event::RunBlocked { + blocked_reason: BlockedReason::HumanInputRequired, + }, + ]) + .await; + + let interviewer = Arc::new(ControlInterviewer::new()); + let mut question = Question::new("approve?", QuestionType::YesNo); + question.id = "q-1".to_string(); + let ask_interviewer = Arc::clone(&interviewer); + let ask = tokio::spawn(async move { ask_interviewer.ask(question).await }); + tokio::task::yield_now().await; + + let (cancel_tx, mut cancel_rx) = oneshot::channel(); + let cancel_token = CancellationToken::new(); + let temp_dir = tempfile::tempdir().unwrap(); + let mut run = managed_run( + MINIMAL_DOT.to_string(), + RunStatus::Running, + Utc::now(), + temp_dir.path().join(run_id.to_string()), + RunExecutionMode::Start, + ); + run.answer_transport = Some(RunAnswerTransport::InProcess { + interviewer, + steering_hub: Arc::new(fabro_workflow::SteeringHub::new(Arc::new( + fabro_workflow::event::Emitter::new(run_id), + ))), + }); + run.cancel_token = Some(cancel_token); + run.cancel_tx = Some(cancel_tx); + state + .runs + .lock() + .expect("runs lock poisoned") + .insert(run_id, run); + + let req = Request::builder() + .method("POST") + .uri(api(&format!("/runs/{run_id}/cancel"))) + .body(Body::empty()) + .unwrap(); + let response = app.oneshot(req).await.unwrap(); + assert_status!(response, StatusCode::OK).await; + + let submission = tokio::time::timeout(std::time::Duration::from_millis(100), ask) + .await + .expect("cancel should resolve the pending in-process interview") + .expect("interview task should not panic"); + assert_eq!(submission.answer.value, AnswerValue::Cancelled); + assert!( + matches!( + cancel_rx.try_recv(), + Err(tokio::sync::oneshot::error::TryRecvError::Empty) + ), + "blocked in-process cancellation should let the workflow unwind instead of aborting it" + ); +} + #[tokio::test] async fn pause_run_rejects_when_control_is_already_pending() { let state = test_app_state();