mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-24 16:01:42 +00:00
fix: keep the same_worker pin when a suspend ends without approval (#10552)
* fix: keep the same_worker pin when a suspend ends without approval A disapproved or timed-out approval gate hands the flow back through the UpdateFlow channel with unrecoverable = true. That flag means "the previous step's worker died", and it is read by six sites. Five of them happen to want what it does here, but continue_on_same_worker and continue_with_runners do not: the worker that ran the approval step is alive, so unpinning the error handler and routing it by tag breaks the ./shared contract of a same_worker flow and can land it on a worker group that cannot run it — the same defect #10551 fixed for the three producers that hand back a live flow. Replace the boolean with StepFailureKind so the suspend producer can say "worker alive, but this failure is not the module's to handle" instead of overstating a worker death. The failed module's error policy is deliberately still bypassed: the failure is recorded against the step the gate was holding back, which never ran, so its retry would re-open the gate and its continue_on_error would skip it outright (verified: the gated step is marked Failure with a nil job id and the flow jumps past it). suspend. continue_on_disapprove_timeout remains the way to continue past a gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(flow-editor): flag that continue on error does not cover the approval gate A resolved approval is recorded against the step the gate holds back, not the step carrying the suspend, so continue_on_error never sees it: the flow still stops on a disapproval or timeout. Point users at suspend.continue_on_disapprove_timeout, which is what actually continues past a gate, whenever both settings are on and that one is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+15
-10
@@ -119,15 +119,15 @@ use windmill_queue::{
|
||||
};
|
||||
use windmill_worker::{
|
||||
result_processor::handle_job_error, JobCompletedSender, JobIsolationLevel,
|
||||
OtelTracingProxySettings, SameWorkerSender, WorkspaceRegistryMap, BUNFIG_INSTALL_SCOPES,
|
||||
BUN_INSTALL_MIN_RELEASE_AGE, CARGO_REGISTRIES, INSTANCE_PYTHON_VERSION, JAVA_HOME_DIR,
|
||||
JOB_DEFAULT_TIMEOUT, JOB_ISOLATION, KEEP_JOB_DIR, MAVEN_REPOS, MAVEN_SETTINGS_XML,
|
||||
NO_DEFAULT_MAVEN, NPMRC, NPM_CONFIG_REGISTRY, NSJAIL_AVAILABLE, NSJAIL_TMPFS_SIZE_MB,
|
||||
NSJAIL_TMP_BACKING, NUGET_CONFIG, OTEL_TRACING_PROXY_SETTINGS, PIP_EXTRA_INDEX_URL,
|
||||
PIP_INDEX_URL, POWERSHELL_REPO_PAT, POWERSHELL_REPO_URL, SANDBOX_IMAGE_CACHE_MAX_MB,
|
||||
SANDBOX_IMAGE_DEFAULT_REGISTRY, SANDBOX_IMAGE_MAX_SIZE_MB, SANDBOX_IMAGE_PULL_POLICY,
|
||||
SANDBOX_REGISTRY_AUTH, UNSHARE_PATH, UV_EXCLUDE_NEWER, UV_INDEX_STRATEGY,
|
||||
UV_PYTHON_INSTALL_MIRROR, WORKSPACE_REGISTRIES,
|
||||
OtelTracingProxySettings, SameWorkerSender, StepFailureKind, WorkspaceRegistryMap,
|
||||
BUNFIG_INSTALL_SCOPES, BUN_INSTALL_MIN_RELEASE_AGE, CARGO_REGISTRIES, INSTANCE_PYTHON_VERSION,
|
||||
JAVA_HOME_DIR, JOB_DEFAULT_TIMEOUT, JOB_ISOLATION, KEEP_JOB_DIR, MAVEN_REPOS,
|
||||
MAVEN_SETTINGS_XML, NO_DEFAULT_MAVEN, NPMRC, NPM_CONFIG_REGISTRY, NSJAIL_AVAILABLE,
|
||||
NSJAIL_TMPFS_SIZE_MB, NSJAIL_TMP_BACKING, NUGET_CONFIG, OTEL_TRACING_PROXY_SETTINGS,
|
||||
PIP_EXTRA_INDEX_URL, PIP_INDEX_URL, POWERSHELL_REPO_PAT, POWERSHELL_REPO_URL,
|
||||
SANDBOX_IMAGE_CACHE_MAX_MB, SANDBOX_IMAGE_DEFAULT_REGISTRY, SANDBOX_IMAGE_MAX_SIZE_MB,
|
||||
SANDBOX_IMAGE_PULL_POLICY, SANDBOX_REGISTRY_AUTH, UNSHARE_PATH, UV_EXCLUDE_NEWER,
|
||||
UV_INDEX_STRATEGY, UV_PYTHON_INSTALL_MIRROR, WORKSPACE_REGISTRIES,
|
||||
};
|
||||
|
||||
#[cfg(feature = "parquet")]
|
||||
@@ -4700,7 +4700,12 @@ async fn handle_zombie_jobs(db: &Pool<Postgres>, base_internal_url: &str, node_n
|
||||
memory_peak,
|
||||
None,
|
||||
error::Error::ExecutionErr(error_message.clone()),
|
||||
matches!(error_kind, ErrorMessage::SameWorker), // unrecoverable if the job is a same worker zombie
|
||||
// a same worker zombie means the worker itself is gone
|
||||
if matches!(error_kind, ErrorMessage::SameWorker) {
|
||||
StepFailureKind::Unrecoverable
|
||||
} else {
|
||||
StepFailureKind::Normal
|
||||
},
|
||||
Some(&same_worker_tx_never_used),
|
||||
"",
|
||||
node_name,
|
||||
|
||||
@@ -235,6 +235,88 @@ mod suspend_resume {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A suspend gate that ends without approval leaves the worker that ran the approval step
|
||||
/// alive, so the error handler it routes to must stay pinned to that worker rather than
|
||||
/// being unpinned and routed by tag — which would break the `./shared` contract of a
|
||||
/// `same_worker` flow.
|
||||
#[cfg(feature = "deno_core")]
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn disapproved_suspend_keeps_same_worker_pin(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
|
||||
let value: FlowValue = serde_json::from_value(json!({
|
||||
"same_worker": true,
|
||||
"modules": [{
|
||||
"id": "a",
|
||||
"value": {
|
||||
"input_transforms": {
|
||||
"port": { "type": "javascript", "expr": "flow_input.port" },
|
||||
},
|
||||
"type": "rawscript",
|
||||
"language": "deno",
|
||||
"content": "\
|
||||
export async function main(port) {\
|
||||
const job = Deno.env.get('WM_JOB_ID');\
|
||||
const token = Deno.env.get('WM_TOKEN');\
|
||||
const secret = await (await fetch(\
|
||||
`http://localhost:${port}/api/w/test-workspace/jobs/job_signature/${job}/0?token=${token}&approver=ruben`,\
|
||||
{ headers: { 'Authorization': `Bearer ${token}` } }\
|
||||
)).text();\
|
||||
await fetch(\
|
||||
`http://localhost:${port}/api/w/test-workspace/jobs_u/cancel/${job}/0/${secret}?approver=ruben`,\
|
||||
{ method: 'POST', body: JSON.stringify('from job'), headers: { 'content-type': 'application/json' } }\
|
||||
);\
|
||||
return 'a ran';\
|
||||
}",
|
||||
},
|
||||
"suspend": { "required_events": 1 },
|
||||
}, {
|
||||
"id": "b",
|
||||
"value": {
|
||||
"input_transforms": {},
|
||||
"type": "rawscript",
|
||||
"language": "deno",
|
||||
"content": "export function main() { return 'b ran' }",
|
||||
},
|
||||
// The gate holds `b` back, so `b` never runs and its error policy describes
|
||||
// nothing: honouring it here would skip `b` instead of reaching the handler.
|
||||
"continue_on_error": true,
|
||||
}],
|
||||
"failure_module": {
|
||||
"id": "failure",
|
||||
"value": {
|
||||
"input_transforms": {},
|
||||
"type": "rawscript",
|
||||
"language": "deno",
|
||||
"content": "export function main() { return 'handled' }",
|
||||
},
|
||||
},
|
||||
}))?;
|
||||
|
||||
let completed =
|
||||
RunJob::from(JobPayload::RawFlow { value, path: None, restarted_from: None })
|
||||
.arg("port", json!(port))
|
||||
.run_until_complete(&db, false, port)
|
||||
.await;
|
||||
|
||||
server.close().await.unwrap();
|
||||
|
||||
assert_eq!(json!("handled"), completed.json_result().unwrap());
|
||||
|
||||
let same_worker: Option<bool> = sqlx::query_scalar(
|
||||
"SELECT same_worker FROM v2_job WHERE parent_job = $1 AND flow_step_id = 'failure'",
|
||||
)
|
||||
.bind(completed.id)
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(Some(true), same_worker);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Test that self-approval is blocked when self_approval_disabled is true.
|
||||
///
|
||||
/// This test verifies that when a flow has an approval step with self_approval_disabled=true,
|
||||
|
||||
@@ -4055,7 +4055,7 @@ async fn test_failure_module(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
|
||||
/// Push `flow`, run it on a real worker until its first step is running, then simulate
|
||||
/// `monitor::handle_zombie_jobs` reaping that step unrecoverably (its worker crashed/OOM'd)
|
||||
/// by calling `handle_job_error(..., unrecoverable = true, ...)` exactly as the monitor does.
|
||||
/// by calling `handle_job_error(..., StepFailureKind::Unrecoverable, ...)` exactly as the monitor does.
|
||||
/// Returns the flow's completed result.
|
||||
#[cfg(feature = "deno_core")]
|
||||
async fn run_flow_until_step_running_then_fail_unrecoverably(
|
||||
@@ -4070,7 +4070,7 @@ async fn run_flow_until_step_running_then_fail_unrecoverably(
|
||||
use windmill_common::client::AuthedClient;
|
||||
use windmill_common::KillpillSender;
|
||||
use windmill_queue::{get_queued_job_v2, MiniCompletedJob, SameWorkerPayload};
|
||||
use windmill_worker::{JobCompletedSender, SameWorkerSender};
|
||||
use windmill_worker::{JobCompletedSender, SameWorkerSender, StepFailureKind};
|
||||
|
||||
let flow_id =
|
||||
RunJob::from(JobPayload::RawFlow { value: flow, path: None, restarted_from: None })
|
||||
@@ -4135,7 +4135,7 @@ async fn run_flow_until_step_running_then_fail_unrecoverably(
|
||||
windmill_common::error::Error::ExecutionErr(
|
||||
"simulated worker OOM crash".to_string(),
|
||||
),
|
||||
true, // unrecoverable
|
||||
StepFailureKind::Unrecoverable,
|
||||
Some(&sw_tx),
|
||||
"",
|
||||
"test-monitor",
|
||||
|
||||
@@ -52,7 +52,7 @@ use crate::{
|
||||
otel_oss::add_root_flow_job_to_otlp,
|
||||
worker_flow::update_flow_status_after_job_completion,
|
||||
JobCompletedReceiver, JobCompletedSender, SameWorkerSender, SendResult, SendResultPayload,
|
||||
UpdateFlow, SAME_WORKER_REQUIREMENTS,
|
||||
StepFailureKind, UpdateFlow, SAME_WORKER_REQUIREMENTS,
|
||||
};
|
||||
use windmill_common::client::AuthedClient;
|
||||
|
||||
@@ -465,7 +465,7 @@ pub fn start_background_processor(
|
||||
worker_dir,
|
||||
stop_early_override,
|
||||
token,
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
}),
|
||||
time,
|
||||
}) => {
|
||||
@@ -486,7 +486,7 @@ pub fn start_background_processor(
|
||||
None,
|
||||
Arc::new(result),
|
||||
None,
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
&same_worker_tx,
|
||||
&worker_dir,
|
||||
stop_early_override,
|
||||
@@ -792,7 +792,7 @@ pub async fn handle_receive_completed_job(
|
||||
mem_peak,
|
||||
canceled_by,
|
||||
err,
|
||||
false,
|
||||
StepFailureKind::Normal,
|
||||
same_worker_tx.clone(),
|
||||
&worker_dir,
|
||||
worker_name,
|
||||
@@ -1594,7 +1594,7 @@ pub async fn process_completed_job(
|
||||
canceled_by,
|
||||
result,
|
||||
started_at.map(|x| FlowJobDuration { started_at: x, duration_ms: duration }),
|
||||
false,
|
||||
StepFailureKind::Normal,
|
||||
&same_worker_tx.expect(SAME_WORKER_REQUIREMENTS).to_owned(),
|
||||
&worker_dir,
|
||||
None,
|
||||
@@ -1701,7 +1701,7 @@ pub async fn process_completed_job(
|
||||
duration_ms: d,
|
||||
})
|
||||
}),
|
||||
false,
|
||||
StepFailureKind::Normal,
|
||||
&same_worker_tx.expect(SAME_WORKER_REQUIREMENTS).to_owned(),
|
||||
&worker_dir,
|
||||
None,
|
||||
@@ -1979,7 +1979,7 @@ pub async fn handle_job_error(
|
||||
mem_peak: i32,
|
||||
canceled_by: Option<CanceledBy>,
|
||||
err: Error,
|
||||
unrecoverable: bool,
|
||||
step_failure: StepFailureKind,
|
||||
same_worker_tx: Option<&SameWorkerSender>,
|
||||
worker_dir: &str,
|
||||
worker_name: &str,
|
||||
@@ -2029,7 +2029,7 @@ pub async fn handle_job_error(
|
||||
canceled_by.clone(),
|
||||
Arc::new(serde_json::value::to_raw_value(&wrapped_error).unwrap()),
|
||||
None,
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
&same_worker_tx.expect(SAME_WORKER_REQUIREMENTS).clone(),
|
||||
worker_dir,
|
||||
None,
|
||||
|
||||
@@ -1747,7 +1747,7 @@ pub async fn handle_all_job_kind_error(
|
||||
0,
|
||||
None,
|
||||
err,
|
||||
false,
|
||||
StepFailureKind::Normal,
|
||||
same_worker_tx,
|
||||
&worker_dir,
|
||||
&worker_name,
|
||||
@@ -3639,10 +3639,38 @@ pub struct UpdateFlow {
|
||||
pub worker_dir: String,
|
||||
pub stop_early_override: Option<bool>,
|
||||
pub token: String,
|
||||
/// Whether the flow must be resumed as if the previous step's worker had died: no retry,
|
||||
/// straight to the failure module, and nothing pinned to this worker. Only true when the
|
||||
/// flow is handed back from a state no live step can recover from.
|
||||
pub unrecoverable: bool,
|
||||
pub step_failure: StepFailureKind,
|
||||
}
|
||||
|
||||
/// Why the step a flow is being resumed from failed, which bounds what the engine may do next.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum StepFailureKind {
|
||||
/// The step failed by running, or did not fail at all.
|
||||
Normal,
|
||||
/// A suspend gate was disapproved or timed out. The worker that ran the approval step is
|
||||
/// still alive, but the failure is recorded against the step the gate was holding back,
|
||||
/// which never ran — so that step's `retry` and `continue_on_error` describe nothing that
|
||||
/// happened, and honouring them would re-open the gate or skip the step outright.
|
||||
/// `suspend.continue_on_disapprove_timeout` is how a flow opts into continuing past a gate.
|
||||
SuspendNotApproved,
|
||||
/// The step's worker died (OOM/zombie), or the flow status update itself errored. Neither
|
||||
/// leaves state worth pinning to: in the first case that worker is gone, in the second the
|
||||
/// flow's own bookkeeping is what just broke.
|
||||
Unrecoverable,
|
||||
}
|
||||
|
||||
impl StepFailureKind {
|
||||
/// Whether the failed module's own `retry` / `continue_on_error` still describe the
|
||||
/// failure at hand. When they don't, the failure module is the only way forward.
|
||||
pub fn honors_step_error_policy(self) -> bool {
|
||||
matches!(self, Self::Normal)
|
||||
}
|
||||
|
||||
/// Whether follow-up work may still be pinned to the worker that ran the previous step,
|
||||
/// via `same_worker` or dedicated flow-module runners.
|
||||
pub fn keeps_worker_pin(self) -> bool {
|
||||
!matches!(self, Self::Unrecoverable)
|
||||
}
|
||||
}
|
||||
|
||||
async fn do_nativets(
|
||||
@@ -3944,8 +3972,8 @@ pub async fn handle_queued_job(
|
||||
flow_runners,
|
||||
&killpill_rx,
|
||||
// A freshly pulled flow job is being executed by a live worker; the prior
|
||||
// step (if any) completed normally, so this is never unrecoverable here.
|
||||
false,
|
||||
// step (if any) completed normally.
|
||||
StepFailureKind::Normal,
|
||||
))
|
||||
.warn_after_seconds(10)
|
||||
.await?;
|
||||
|
||||
@@ -15,8 +15,8 @@ use crate::common::{cached_result_path, get_root_job_id, save_in_cache, transfor
|
||||
use crate::js_eval::{eval_timeout, IdContext};
|
||||
use crate::worker_utils::get_tag_and_concurrency;
|
||||
use crate::{
|
||||
JobCompletedSender, PreviousResult, SameWorkerSender, SendResultPayload, UpdateFlow,
|
||||
KEEP_JOB_DIR,
|
||||
JobCompletedSender, PreviousResult, SameWorkerSender, SendResultPayload, StepFailureKind,
|
||||
UpdateFlow, KEEP_JOB_DIR,
|
||||
};
|
||||
|
||||
use anyhow::Context;
|
||||
@@ -169,7 +169,7 @@ pub async fn update_flow_status_after_job_completion(
|
||||
canceled_by: Option<CanceledBy>,
|
||||
result: Arc<Box<RawValue>>,
|
||||
flow_job_duration: Option<FlowJobDuration>,
|
||||
unrecoverable: bool,
|
||||
step_failure: StepFailureKind,
|
||||
same_worker_tx: &SameWorkerSender,
|
||||
worker_dir: &str,
|
||||
stop_early_override: Option<bool>,
|
||||
@@ -192,7 +192,7 @@ pub async fn update_flow_status_after_job_completion(
|
||||
stop_early_override,
|
||||
has_triggered_error_handler: false,
|
||||
};
|
||||
let mut unrecoverable = unrecoverable;
|
||||
let mut step_failure = step_failure;
|
||||
loop {
|
||||
potentially_crash_for_testing();
|
||||
let nrec = match Box::pin(update_flow_status_after_job_completion_internal(
|
||||
@@ -205,7 +205,7 @@ pub async fn update_flow_status_after_job_completion(
|
||||
rec.canceled_by,
|
||||
rec.flow_job_duration.clone(),
|
||||
rec.result,
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
same_worker_tx,
|
||||
worker_dir,
|
||||
rec.stop_early_override,
|
||||
@@ -234,7 +234,7 @@ pub async fn update_flow_status_after_job_completion(
|
||||
Arc::new(to_raw_value(&Json(&WrappedError {
|
||||
error: json!(e.to_string()),
|
||||
}))),
|
||||
true,
|
||||
StepFailureKind::Unrecoverable,
|
||||
same_worker_tx,
|
||||
worker_dir,
|
||||
rec.stop_early_override,
|
||||
@@ -249,7 +249,7 @@ pub async fn update_flow_status_after_job_completion(
|
||||
.await?
|
||||
}
|
||||
};
|
||||
unrecoverable = false;
|
||||
step_failure = StepFailureKind::Normal;
|
||||
|
||||
match nrec {
|
||||
UpdateFlowStatusAfterJobCompletion::Done(job) => {
|
||||
@@ -421,7 +421,7 @@ pub async fn update_flow_status_after_job_completion_internal(
|
||||
canceled_by: Option<CanceledBy>,
|
||||
mut flow_job_duration: Option<FlowJobDuration>,
|
||||
result: Arc<Box<RawValue>>,
|
||||
unrecoverable: bool,
|
||||
step_failure: StepFailureKind,
|
||||
same_worker_tx: &SameWorkerSender,
|
||||
worker_dir: &str,
|
||||
stop_early_override: Option<bool>,
|
||||
@@ -1298,11 +1298,11 @@ pub async fn update_flow_status_after_job_completion_internal(
|
||||
}),
|
||||
)
|
||||
} else {
|
||||
// An unrecoverable failure (worker crash/OOM) must reach the error handler
|
||||
// even on a continue_on_error step, so don't advance the step counter past
|
||||
// the failed module — otherwise the flow would silently continue to the next
|
||||
// step and hide the worker death.
|
||||
let inc = if !unrecoverable && continue_on_error {
|
||||
// A failure the module's error policy does not describe must reach the error
|
||||
// handler even on a continue_on_error step, so don't advance the step
|
||||
// counter past the failed module — otherwise the flow would silently
|
||||
// continue to the next step and hide it.
|
||||
let inc = if step_failure.honors_step_error_policy() && continue_on_error {
|
||||
let retry = current_module
|
||||
.as_ref()
|
||||
.and_then(|x| x.retry.clone())
|
||||
@@ -1741,19 +1741,20 @@ pub async fn update_flow_status_after_job_completion_internal(
|
||||
// enclosing job/subflow. Detect that case and treat the flow as successful.
|
||||
let recoverable_failure_at_last_step = !success
|
||||
&& is_last_step
|
||||
&& !unrecoverable
|
||||
&& step_failure.honors_step_error_policy()
|
||||
&& (skip_seq_branch_failure || skip_loop_failures || continue_on_error);
|
||||
|
||||
let should_continue_flow = match success {
|
||||
_ if stop_early => stop_early_err_msg.is_some() && flow_value.failure_module.is_some(), // if stop_early_err_msg some, we want to trigger the error handler before stopping the flow, if any
|
||||
_ if flow_job.is_canceled() => false,
|
||||
true => !is_last_step,
|
||||
// An unrecoverable failure (a step killed by a worker crash/OOM and surfaced by
|
||||
// the zombie handler, or an error raised while updating the flow status itself)
|
||||
// must not be retried or silently skipped, but it should still trigger the flow's
|
||||
// error handler: an OOM/worker death is precisely when the error handler is expected
|
||||
// to run. Continue the flow only to reach the failure module, never to retry.
|
||||
false if unrecoverable => {
|
||||
// A failure the module's error policy does not describe (a worker crash/OOM
|
||||
// surfaced by the zombie handler, an error raised while updating the flow status,
|
||||
// a suspend gate that ended without approval) must not be retried or silently
|
||||
// skipped, but it should still trigger the flow's error handler — that is
|
||||
// precisely when the error handler is expected to run. Continue the flow only to
|
||||
// reach the failure module, never to retry.
|
||||
false if !step_failure.honors_step_error_policy() => {
|
||||
!is_failure_step
|
||||
&& !has_triggered_error_handler
|
||||
&& flow_value.failure_module.is_some()
|
||||
@@ -1776,7 +1777,7 @@ pub async fn update_flow_status_after_job_completion_internal(
|
||||
success = true;
|
||||
}
|
||||
|
||||
tracing::info!(id = %flow_job.id, root_id = %job_root, success = %success, stop_early = %stop_early, is_last_step = %is_last_step, unrecoverable = %unrecoverable,
|
||||
tracing::info!(id = %flow_job.id, root_id = %job_root, success = %success, stop_early = %stop_early, is_last_step = %is_last_step, step_failure = ?step_failure,
|
||||
skip_seq_branch_failure = %skip_seq_branch_failure, skip_loop_failures = %skip_loop_failures,
|
||||
current_module_id = %current_module.map(|x| x.id.clone()).unwrap_or_default(),
|
||||
continue_on_error = %continue_on_error, should_continue_flow = %should_continue_flow, "computed if flow should continue");
|
||||
@@ -2108,7 +2109,7 @@ pub async fn update_flow_status_after_job_completion_internal(
|
||||
worker_name,
|
||||
flow_runners,
|
||||
&killpill_rx,
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
))
|
||||
.warn_after_seconds(10)
|
||||
.await
|
||||
@@ -2799,10 +2800,9 @@ pub async fn handle_flow(
|
||||
worker_name: &str,
|
||||
flow_runners: Option<Arc<FlowRunners>>,
|
||||
killpill_rx: &tokio::sync::broadcast::Receiver<()>,
|
||||
// The previous step failed unrecoverably (e.g. a worker crash/OOM surfaced by the
|
||||
// zombie handler). The next pushed step can only be the error handler (failure
|
||||
// module), and it must not be pinned to the dead worker via same_worker.
|
||||
unrecoverable: bool,
|
||||
// How the step this flow is resuming from failed, which bounds what may be pushed next:
|
||||
// see [`StepFailureKind`].
|
||||
step_failure: StepFailureKind,
|
||||
) -> anyhow::Result<()> {
|
||||
let flow = flow_data.value();
|
||||
|
||||
@@ -2984,7 +2984,7 @@ pub async fn handle_flow(
|
||||
flow_runners.clone(),
|
||||
job_completed_tx.clone(),
|
||||
&killpill_rx,
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
))
|
||||
.warn_after_seconds(10)
|
||||
.await?;
|
||||
@@ -3167,10 +3167,9 @@ async fn push_next_flow_job(
|
||||
flow_runners: Option<Arc<FlowRunners>>,
|
||||
job_completed_tx: JobCompletedSender,
|
||||
killpill_rx: &tokio::sync::broadcast::Receiver<()>,
|
||||
// The prior step failed unrecoverably (worker crash/OOM). The only step pushed
|
||||
// from here is the error handler, which must run on a live worker rather than
|
||||
// being pinned to the dead one via same_worker / dedicated runners.
|
||||
unrecoverable: bool,
|
||||
// How the prior step failed, which bounds what may be pushed next: see
|
||||
// [`StepFailureKind`].
|
||||
step_failure: StepFailureKind,
|
||||
) -> error::Result<PushNextFlowJob> {
|
||||
let job_root = flow_job
|
||||
.flow_innermost_root_job
|
||||
@@ -3231,7 +3230,7 @@ async fn push_next_flow_job(
|
||||
w_id: flow_job.workspace_id.clone(),
|
||||
worker_dir: worker_dir.to_string(),
|
||||
token: client.token.clone(),
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
})));
|
||||
}
|
||||
|
||||
@@ -3286,7 +3285,7 @@ async fn push_next_flow_job(
|
||||
w_id: flow_job.workspace_id.clone(),
|
||||
worker_dir: worker_dir.to_string(),
|
||||
token: client.token.clone(),
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
}
|
||||
)));
|
||||
}
|
||||
@@ -3331,7 +3330,7 @@ async fn push_next_flow_job(
|
||||
w_id: flow_job.workspace_id.clone(),
|
||||
worker_dir: worker_dir.to_string(),
|
||||
token: client.token.clone(),
|
||||
unrecoverable,
|
||||
step_failure,
|
||||
})));
|
||||
}
|
||||
}
|
||||
@@ -3664,10 +3663,7 @@ async fn push_next_flow_job(
|
||||
w_id: flow_job.workspace_id.clone(),
|
||||
worker_dir: worker_dir.to_string(),
|
||||
token: client.token.clone(),
|
||||
// A suspend that was disapproved or ran out its timeout cannot be
|
||||
// resumed by anything the flow does next, so the failure module is the
|
||||
// only way forward.
|
||||
unrecoverable: true,
|
||||
step_failure: StepFailureKind::SuspendNotApproved,
|
||||
})));
|
||||
}
|
||||
}
|
||||
@@ -3737,10 +3733,11 @@ async fn push_next_flow_job(
|
||||
}
|
||||
};
|
||||
|
||||
// An unrecoverable failure (worker crash/OOM) must not be retried — the original worker
|
||||
// and its state are gone — so skip retry evaluation and fall straight through to the
|
||||
// failure module below.
|
||||
let retry = if !unrecoverable && matches!(&status_module, FlowStatusModule::Failure { .. },) {
|
||||
// Retry is a policy on the step's own execution: skip it for a failure the step did not
|
||||
// produce by running, and fall straight through to the failure module below.
|
||||
let retry = if step_failure.honors_step_error_policy()
|
||||
&& matches!(&status_module, FlowStatusModule::Failure { .. },)
|
||||
{
|
||||
let retry = &module.retry.clone().unwrap_or_default();
|
||||
evaluate_retry(
|
||||
retry,
|
||||
@@ -3755,13 +3752,13 @@ async fn push_next_flow_job(
|
||||
None
|
||||
};
|
||||
let get_args_from_id = match &status_module {
|
||||
// `|| unrecoverable`: a worker crash/OOM routes to the failure module even on a
|
||||
// continue_on_error step (whose failures are normally tolerated), matching the
|
||||
// `unrecoverable` decision in update_flow_status_after_job_completion_internal.
|
||||
// `|| !honors_step_error_policy()`: such a failure routes to the failure module even
|
||||
// on a continue_on_error step (whose failures are normally tolerated), matching the
|
||||
// decision in update_flow_status_after_job_completion_internal.
|
||||
FlowStatusModule::Failure { job, .. }
|
||||
if retry.as_ref().is_some()
|
||||
|| !module.continue_on_error.is_some_and(|x| x)
|
||||
|| unrecoverable =>
|
||||
|| !step_failure.honors_step_error_policy() =>
|
||||
{
|
||||
if let Some((fail_count, retry_in)) = retry {
|
||||
tracing::debug!(
|
||||
@@ -4082,7 +4079,7 @@ async fn push_next_flow_job(
|
||||
None,
|
||||
result,
|
||||
None,
|
||||
false,
|
||||
StepFailureKind::Normal,
|
||||
same_worker_tx,
|
||||
worker_dir,
|
||||
None,
|
||||
@@ -4110,7 +4107,7 @@ async fn push_next_flow_job(
|
||||
.as_ref()
|
||||
.is_some_and(|fr| fr.job_id == flow_job.id);
|
||||
|
||||
let continue_with_runners = !unrecoverable
|
||||
let continue_with_runners = step_failure.keeps_worker_pin()
|
||||
&& (start_runners || (flow_runners.is_some() && !do_not_pass_runners))
|
||||
&& module.suspend.is_none()
|
||||
&& module.sleep.is_none();
|
||||
@@ -4120,10 +4117,10 @@ async fn push_next_flow_job(
|
||||
let job_same_worker = flow_job.same_worker
|
||||
&& matches!(flow_job.kind, JobKind::Flow)
|
||||
&& flow_job.runnable_id.is_some();
|
||||
// After an unrecoverable failure the original worker is gone, so the error handler
|
||||
// step is pushed as a regular queued job (any live worker can pick it up) instead of
|
||||
// being signaled to the dead worker via same_worker — which would strand it forever.
|
||||
let continue_on_same_worker = !unrecoverable
|
||||
// Without a worker worth pinning to, the error handler step is pushed as a regular queued
|
||||
// job (any live worker can pick it up) instead of being signaled via same_worker to a
|
||||
// worker that may be dead — which would strand it forever.
|
||||
let continue_on_same_worker = step_failure.keeps_worker_pin()
|
||||
&& (flow.same_worker || job_same_worker)
|
||||
&& module.suspend.is_none()
|
||||
&& module.sleep.is_none();
|
||||
|
||||
@@ -107,6 +107,13 @@
|
||||
const s3Snippet = $derived(s3Language ? s3Scripts[s3Language][s3Kind] : undefined)
|
||||
const concurrencyOn = $derived(hasInlineConcurrency(flowModule))
|
||||
const concurrencyOff = $derived(!$enterpriseLicense || !concurrencyOn)
|
||||
// A resolved approval is recorded against the step the gate holds back, not this one, so
|
||||
// `continue_on_error` never sees it — the suspend option is the only way to continue past it.
|
||||
const suspendNeedsItsOwnContinueToggle = $derived(
|
||||
Boolean(flowModule.continue_on_error) &&
|
||||
Boolean(flowModule.suspend) &&
|
||||
!flowModule.suspend?.continue_on_disapprove_timeout
|
||||
)
|
||||
</script>
|
||||
|
||||
{#snippet sectionHeader(title: string)}
|
||||
@@ -153,7 +160,7 @@
|
||||
<FlowRetries bind:flowModuleRetry={flowModule.retry} bind:flowModule {isAgentTool} />
|
||||
</div>
|
||||
|
||||
<div data-setting="error-handling">
|
||||
<div data-setting="error-handling" class="flex flex-col gap-2">
|
||||
<Toggle
|
||||
size="xs"
|
||||
textClass="text-xs font-normal text-primary"
|
||||
@@ -164,6 +171,13 @@
|
||||
"The flow continues to the next step even if this step fails (after exhausting retries, if any). The step's error becomes its return, so a following branch can handle it."
|
||||
}}
|
||||
/>
|
||||
{#if suspendNeedsItsOwnContinueToggle}
|
||||
<Alert type="info" title="Does not cover the approval" size="xs">
|
||||
This only applies when the step's own code fails. A disapproval or an approval timeout
|
||||
is not a failure of this step, so it still stops the flow. To continue past those,
|
||||
turn on "Continue on disapproval/timeout" in the approval settings.
|
||||
</Alert>
|
||||
{/if}
|
||||
</div>
|
||||
|
||||
<!-- The error handler runs outside the flow's control graph; only its own
|
||||
|
||||
Reference in New Issue
Block a user