From 866039c0fc8d67c3c989dec158b5474ccc6d631f Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 24 Jul 2026 16:00:01 +0000 Subject: [PATCH] test: add regression test for keyed WAC approval resume; trim comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add backend/tests/wac_approval_resume.rs pinning the fix: a job parked on its second waitForApproval() with two resume_job rows (an older approved row for the first approval, a newer cancelled row for the pending one) must resolve to the pending step's own event, not the oldest row. Verified it fails against the pre-fix `ORDER BY created_at ASC LIMIT 1` query and passes with the resume_id filter. Also trim the resume-match comment to the durable constraint (drops the bug-history narration) per AGENTS.md. approval_resume_id stays `pub` — the integration test crate now consumes it. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/tests/wac_approval_resume.rs | 104 ++++++++++++++++++++ backend/windmill-worker/src/wac_executor.rs | 10 +- 2 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 backend/tests/wac_approval_resume.rs diff --git a/backend/tests/wac_approval_resume.rs b/backend/tests/wac_approval_resume.rs new file mode 100644 index 0000000000..b626c2caac --- /dev/null +++ b/backend/tests/wac_approval_resume.rs @@ -0,0 +1,104 @@ +//! Regression test for WAC `waitForApproval` reading the wrong approval result. +//! +//! Sequential `waitForApproval()` calls share the parent job_id, so the resume +//! lookup must select the `resume_job` row whose `resume_id` matches the pending +//! approval step's key — not the oldest row for the job. Before the fix, a job's +//! second approval re-read the first approval's value (and cancels/timeouts +//! inherited the first approval's `approved=true`). + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use uuid::Uuid; + +use windmill_common::wac::{save_checkpoint, WacCheckpoint, WacPendingSteps}; +use windmill_worker::wac_executor::{approval_resume_id, prepare_checkpoint_for_resume}; + +async fn insert_resume( + db: &Pool, + job_id: Uuid, + key: &str, + value: serde_json::Value, + approved: bool, + age_seconds: i64, +) -> anyhow::Result<()> { + sqlx::query( + "INSERT INTO resume_job (id, job, flow, created_at, value, approver, resume_id, approved) + VALUES ($1, $2, $2, now() - make_interval(secs => $3), $4, 'tester', $5, $6)", + ) + .bind(Uuid::new_v4()) + .bind(job_id) + .bind(age_seconds as f64) + .bind(value) + .bind(approval_resume_id(key) as i32) + .bind(approved) + .execute(db) + .await?; + Ok(()) +} + +#[sqlx::test(fixtures("base"))] +async fn resume_matches_pending_approval_step(db: Pool) -> anyhow::Result<()> { + let job_id = Uuid::new_v4(); + + // Minimal queue row so the resume_job.flow FK and the v2_job_status FK resolve. + sqlx::query( + "INSERT INTO v2_job_queue (id, workspace_id, scheduled_for, suspend) + VALUES ($1, 'test-workspace', now(), 1)", + ) + .bind(job_id) + .execute(&db) + .await?; + + // The workflow is parked on its *second* approval. + let pending_key = "approval_2"; + let checkpoint = WacCheckpoint { + pending_steps: Some(WacPendingSteps { + mode: "approval".to_string(), + keys: vec![pending_key.to_string()], + job_ids: Default::default(), + }), + ..Default::default() + }; + save_checkpoint(&db, &job_id, &checkpoint).await?; + + // Two resume events for the same job: the first approval (older, approved) and + // the pending second approval (newer, cancelled). The pre-fix query + // `WHERE job = $1 ORDER BY created_at ASC LIMIT 1` would return the older row. + insert_resume( + &db, + job_id, + "approval", + json!({ "which": "first" }), + true, + 10, + ) + .await?; + insert_resume( + &db, + job_id, + pending_key, + json!({ "which": "second" }), + false, + 0, + ) + .await?; + + let resumed = prepare_checkpoint_for_resume(&db, &job_id, checkpoint).await?; + + let injected = resumed + .completed_steps + .get(pending_key) + .expect("pending approval step should be resolved"); + assert_eq!( + injected["value"], + json!({ "which": "second" }), + "must read its own resume event, not the workflow's first approval" + ); + assert_eq!( + injected["approved"], + json!(false), + "the cancelled second approval must keep approved=false" + ); + + Ok(()) +} diff --git a/backend/windmill-worker/src/wac_executor.rs b/backend/windmill-worker/src/wac_executor.rs index afb1553d8d..f7c20eeade 100644 --- a/backend/windmill-worker/src/wac_executor.rs +++ b/backend/windmill-worker/src/wac_executor.rs @@ -154,12 +154,10 @@ pub async fn prepare_checkpoint_for_resume( .and_then(|p| p.keys.first().cloned()) .unwrap_or_default(); - // Match the resume event to *this* approval step by its resume_id. Every - // waitForApproval() in the same workflow shares the parent job_id, so keying - // only on `job` (and taking the oldest row) made every step after the first - // re-inject the first approval's value. A canceled step then carries approved=false - // on its own row, and a timed-out step has no row at all -> the None branch below - // yields the approved=false default. + // Match the resume event to *this* approval step by its resume_id: every + // waitForApproval() in a workflow shares the parent job_id, so filtering only on + // `job` would let one step read another's approval. A timed-out step matches no + // row and falls through to the approved=false default in the None branch below. let resume_id = approval_resume_id(&approval_key) as i32; let resume_row = sqlx::query_as::<_, (sqlx::types::Json>, Option, bool)>( "SELECT value, approver, approved FROM resume_job WHERE job = $1 AND resume_id = $2 ORDER BY created_at DESC LIMIT 1",