mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(flows): stop re-evaluating skip_if once a loop is in progress (#11008)
* fix(flows): stop re-evaluating skip_if once a loop is in progress skip_if is a one-time entry gate, but the flow stays at the same step for a loop's whole lifetime, so it gets re-evaluated on every iteration. previous_id stays pinned to the module preceding the loop, but once the loop is InProgress the last completed job is an inner iteration, and the results proxy in windmill-jseval aliases results.<previous_id> to that job's result. skip_if then reads the wrong value and can flip the loop's module to skipped after one iteration. Skip the check once status_module is already InProgress. * fix(flows): match skip_if gate to sibling entry-state allowlists Rewrite the skip_if gate as a positive allowlist (WaitingForPriorSteps | WaitingForEvents | WaitingForExecutor), matching the shape already used by the BranchOne/BranchAll predicate gates, instead of a negative filter on InProgress. Restart-at-iteration also enters as InProgress; document it as a separate case rather than folding it into the aliasing reason, which does not apply there. Add a regression test pinning skip_if to run once at while-loop entry.
This commit is contained in:
@@ -5649,6 +5649,83 @@ async fn test_whileloop_propagates_inner_iterator_eval_failure(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(all(feature = "quickjs", feature = "python"))]
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn test_whileloop_skip_if_evaluated_once_at_entry(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
|
||||
// Regression test for #11007: `skip_if` on a while-loop module must be
|
||||
// evaluated once, at loop entry, using the preceding step's result.
|
||||
// Re-evaluating it on every iteration aliases `results.first` to the
|
||||
// previous iteration's own result instead, which here lacks `.ok` and
|
||||
// makes `skip_if` incorrectly turn true after the first iteration.
|
||||
let port = 123;
|
||||
let flow: FlowValue = serde_json::from_value(serde_json::json!({
|
||||
"modules": [
|
||||
{
|
||||
"id": "first",
|
||||
"value": {
|
||||
"type": "rawscript",
|
||||
"language": "python3",
|
||||
"content": "def main(): return {\"ok\": True}",
|
||||
},
|
||||
},
|
||||
{
|
||||
"id": "outer",
|
||||
"value": {
|
||||
"type": "whileloopflow",
|
||||
"skip_failures": false,
|
||||
"modules": [
|
||||
{
|
||||
"id": "inner",
|
||||
"value": {
|
||||
"input_transforms": {
|
||||
"i": {
|
||||
"type": "javascript",
|
||||
"expr": "flow_input.iter.index",
|
||||
},
|
||||
},
|
||||
"type": "rawscript",
|
||||
"language": "python3",
|
||||
"content": "def main(i): return i",
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
"skip_if": { "expr": "!results.first.ok" },
|
||||
"stop_after_if": {
|
||||
"expr": "result >= 2",
|
||||
"skip_if_stopped": false,
|
||||
},
|
||||
},
|
||||
],
|
||||
}))
|
||||
.unwrap();
|
||||
let job = JobPayload::RawFlow { value: flow, path: None, restarted_from: None };
|
||||
|
||||
let cjob = RunJob::from(job).run_until_complete(&db, false, port).await;
|
||||
|
||||
assert!(cjob.success, "flow should succeed");
|
||||
|
||||
let outer_module = get_module(&cjob, "outer").expect("outer module status");
|
||||
match outer_module {
|
||||
windmill_common::flow_status::FlowStatusModule::Success { skipped, flow_jobs, .. } => {
|
||||
assert!(
|
||||
!skipped,
|
||||
"while-loop must not be skipped: skip_if should only run once, at entry"
|
||||
);
|
||||
assert_eq!(
|
||||
flow_jobs.map(|v| v.len()),
|
||||
Some(3),
|
||||
"while-loop should run 3 iterations before stop_after_if halts it"
|
||||
);
|
||||
}
|
||||
other => panic!("expected outer module to be Success, got {other:?}"),
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(all(feature = "quickjs", feature = "python"))]
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn test_stop_after_all_iters_if_bad_expr_parallel_branchall(
|
||||
|
||||
@@ -3891,7 +3891,18 @@ async fn push_next_flow_job(
|
||||
|
||||
drop(resume_messages);
|
||||
|
||||
let is_skipped = if let Some(skip_if) = &module.skip_if {
|
||||
// `skip_if` is a one-time entry gate, so only first-entry statuses evaluate it.
|
||||
// Once the module is looping, the last completed job is an inner iteration, not
|
||||
// `previous_id`'s, and re-evaluating would alias `results.<previous_id>` to it.
|
||||
// A restart-at-iteration also enters as `InProgress`: it resumes without re-gating.
|
||||
let is_skipped = if let Some(skip_if) = module.skip_if.as_ref().filter(|_| {
|
||||
matches!(
|
||||
status_module,
|
||||
FlowStatusModule::WaitingForPriorSteps { .. }
|
||||
| FlowStatusModule::WaitingForEvents { .. }
|
||||
| FlowStatusModule::WaitingForExecutor { .. }
|
||||
)
|
||||
}) {
|
||||
let idcontext = get_transform_context(&flow_job, previous_id.as_str(), &status);
|
||||
let skip_if_res = compute_bool_from_expr(
|
||||
&skip_if.expr,
|
||||
|
||||
Reference in New Issue
Block a user