diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 77bdff64e0..353cc59836 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -1800,9 +1800,9 @@ pub async fn handle_bun_job( }; // Kept comment-free — this string is written out per job. - // `_takePendingSuspend` returns a StepSuspend the body caught and swallowed - // (it is an `Error`), so honour it instead of reporting a `complete` whose - // step never reached the checkpoint. Optional: npm clients may predate it. + // `_takePendingStepFailure` / `_takePendingSuspend` hand back what the body + // caught and swallowed; honour them instead of reporting a `complete` (see + // `_pendingStepFailure` in client.ts). Optional: npm clients may predate them. let wrapper_content = if is_wac_v2 { format!( r#" @@ -1847,6 +1847,10 @@ async function run() {{ try {{ const result = await workflowFn(...argsArr); setWorkflowCtx(null); + const failed = ctx._takePendingStepFailure?.(); + if (failed) {{ + throw failed.error; + }} const swallowed = ctx._takePendingSuspend?.(); if (swallowed) {{ throw swallowed; @@ -1875,6 +1879,10 @@ async function run() {{ }} return {{ type: "dispatch", mode: dispatch.mode ?? "sequential", steps: dispatch.steps ?? [] }}; }} + const failed = ctx._takePendingStepFailure?.(); + if (failed) {{ + throw failed.error; + }} throw e; }} }} diff --git a/python-client/wmill/tests/test_workflow.py b/python-client/wmill/tests/test_workflow.py index 5e49f7f595..54abff21ca 100644 --- a/python-client/wmill/tests/test_workflow.py +++ b/python-client/wmill/tests/test_workflow.py @@ -406,6 +406,24 @@ class TestChildMode: assert result["type"] == "complete" assert result["result"] == 14 + def test_child_cannot_swallow_the_failure_of_the_step_it_executes(self): + # If `except Exception` could catch it, the child would report a success + # returning "swallowed" and the parent would record that as the step's value. + @task + async def boom(): + raise ValueError("nope") + + @workflow + async def wf(): + try: + await boom() + except Exception: + return "swallowed" + return "unreachable" + + with pytest.raises(ValueError, match="nope"): + _run_workflow(wf, {"_executing_key": "boom"}, {}) + def test_child_replays_cached_steps(self): checkpoint = { "completed_steps": {"extract_data": {"data": [1, 2, 3]}}, diff --git a/python-client/wmill/wmill/client.py b/python-client/wmill/wmill/client.py index a4b8e9c476..c68431ae80 100644 --- a/python-client/wmill/wmill/client.py +++ b/python-client/wmill/wmill/client.py @@ -2673,6 +2673,19 @@ class _StepSuspend(BaseException): self.dispatch_info = dispatch_info +class _StepFailure(BaseException): + """Carries the exception raised by the step a child round executes directly. + + That exception *is* the round's result, so a broad ``except Exception`` in the + body must not be able to turn it into a successful complete — the parent would + then record the caught branch's value as the step result. BaseException for the + same reason ``_StepSuspend`` is; a bare ``except:`` still swallows both. + """ + + def __init__(self, exc: BaseException): + self.exc = exc + + class TaskError(Exception): """Raised when a WAC task step failed. @@ -2799,9 +2812,12 @@ class WorkflowCtx: return value async def _execute_directly(self, func, **kwargs): - result = func(**kwargs) - if _asyncio.iscoroutine(result): - result = await result + try: + result = func(**kwargs) + if _asyncio.iscoroutine(result): + result = await result + except Exception as exc: + raise _StepFailure(exc) from exc raise _StepSuspend({"mode": "step_complete", "steps": [], "result": result}) async def _never_resolve(self): @@ -3266,6 +3282,9 @@ async def _run_workflow_async(func, checkpoint: dict, input_args: dict): "steps": steps, } return {"type": "complete", "result": result} + except _StepFailure as e: + # Re-raise the step's own exception so the child job fails with it. + raise e.exc except _StepSuspend as e: info = e.dispatch_info mode = info.get("mode") diff --git a/typescript-client/client.ts b/typescript-client/client.ts index a17e7e361f..228fbdff62 100644 --- a/typescript-client/client.ts +++ b/typescript-client/client.ts @@ -1594,6 +1594,11 @@ export class WorkflowCtx { * `complete` whose step never reached `completed_steps`. Python is immune: * `_StepSuspend` derives from `BaseException`. */ private _pendingSuspend: StepSuspend | null = null; + /** The failure raised by the step this child round is executing. That exception + * *is* the round's result, so a `catch` in the body must not be able to turn it + * into a `complete` — the parent would then record the caught branch's value as + * a successful step. Boxed: the thrown value may be any falsy value. */ + private _pendingStepFailure: { error: unknown } | null = null; /** When set, the task matching this key executes its inner function directly */ _executingKey: string | null; /** Serializes fast-path POSTs across concurrent step() calls within one @@ -1635,7 +1640,7 @@ export class WorkflowCtx { dispatch_type: string = "inline", options?: TaskOptions, ): PromiseLike { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); const key = this._allocKey(name || script || "step"); if (key in this.completed) { @@ -1707,7 +1712,7 @@ export class WorkflowCtx { selfApproval?: boolean; key?: string; }): PromiseLike<{ value: any; approver: string; approved: boolean }> { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); if (options?.key !== undefined) assertUsableStepKey(options.key, "waitForApproval key"); const key = this._allocKey(options?.key || "approval"); @@ -1745,7 +1750,7 @@ export class WorkflowCtx { } _sleep(seconds: number): PromiseLike { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); const key = this._allocKey("sleep"); if (key in this.completed) { @@ -1766,7 +1771,7 @@ export class WorkflowCtx { } async _runInlineStep(name: string, fn: () => T | Promise): Promise { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); const key = this._allocKey(name || "step"); if (key in this.completed) { @@ -1889,11 +1894,19 @@ export class WorkflowCtx { throw suspend; } - /** Re-throw a swallowed suspend at the next SDK call. It happened before - * whatever the body is doing now, so it wins: the run is unwinding either - * way and everything after it re-runs on the replay. Left set, so a body - * that catches in a loop can't swallow it a second time. */ - private _rethrowSwallowedSuspend(): void { + /** Park then raise the executing step's failure. Child mode only — in a parent + * round a task failure is an ordinary `TaskError` the body may handle. */ + _raiseStepFailure(error: unknown): never { + this._pendingStepFailure = { error }; + throw error; + } + + /** Re-throw a swallowed suspend or step failure at the next SDK call. It + * happened before whatever the body is doing now, so it wins: the run is + * unwinding either way and everything after it re-runs on the replay. Left + * set, so a body that catches in a loop can't swallow it a second time. */ + private _rethrowSwallowed(): void { + if (this._pendingStepFailure) throw this._pendingStepFailure.error; if (this._pendingSuspend) throw this._pendingSuspend; } @@ -1905,6 +1918,13 @@ export class WorkflowCtx { this._pendingSuspend = null; return s; } + + /** Same for the executing step's failure, so the runner can fail the child job. */ + _takePendingStepFailure(): { error: unknown } | null { + const f = this._pendingStepFailure; + this._pendingStepFailure = null; + return f; + } } export async function sleep(seconds: number): Promise { @@ -1978,7 +1998,13 @@ export function task Promise>( // and throw StepSuspend with mode "step_complete" to signal that we're done if ((stepResult as any)?._execute_directly) { return (async () => { - const result = await fn(...args); + let result: any; + try { + result = await fn(...args); + } catch (e) { + if ((e as any)?.name === "StepSuspend" || e instanceof StepSuspend) throw e; + ctx._raiseStepFailure(e); + } ctx._raiseSuspend({ mode: "step_complete", steps: [], result }); })(); } diff --git a/typescript-client/tests/workflow.test.ts b/typescript-client/tests/workflow.test.ts index 6a4fed30c4..48a20b5374 100644 --- a/typescript-client/tests/workflow.test.ts +++ b/typescript-client/tests/workflow.test.ts @@ -27,6 +27,7 @@ class WorkflowCtx { }> = []; private _suspended = false; private _pendingSuspend: StepSuspend | null = null; + private _pendingStepFailure: { error: unknown } | null = null; _executingKey: string | null; _raiseSuspend(dispatchInfo: Record): never { @@ -35,7 +36,13 @@ class WorkflowCtx { throw suspend; } - private _rethrowSwallowedSuspend(): void { + _raiseStepFailure(error: unknown): never { + this._pendingStepFailure = { error }; + throw error; + } + + private _rethrowSwallowed(): void { + if (this._pendingStepFailure) throw this._pendingStepFailure.error; if (this._pendingSuspend) throw this._pendingSuspend; } @@ -45,6 +52,12 @@ class WorkflowCtx { return s; } + _takePendingStepFailure(): { error: unknown } | null { + const f = this._pendingStepFailure; + this._pendingStepFailure = null; + return f; + } + constructor(checkpoint: Record = {}) { this.completed = checkpoint?.completed_steps ?? {}; this._executingKey = checkpoint?._executing_key ?? null; @@ -60,7 +73,7 @@ class WorkflowCtx { args: Record = {}, options?: Record, ): PromiseLike { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); const key = this._allocKey(); if (key in this.completed) { @@ -118,7 +131,7 @@ class WorkflowCtx { } _sleep(seconds: number): PromiseLike { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); const key = this._allocKey(); if (key in this.completed) { return { then: (resolve: any) => resolve(undefined) }; @@ -138,7 +151,7 @@ class WorkflowCtx { name: string, fn: () => T | Promise ): Promise { - this._rethrowSwallowedSuspend(); + this._rethrowSwallowed(); const key = this._allocKey(); if (key in this.completed) { @@ -231,7 +244,13 @@ function task Promise>( const stepResult = ctx._nextStep(taskName, script, kwargs, taskOptions); if ((stepResult as any)?._execute_directly) { return (async () => { - const result = await fn(...args); + let result: any; + try { + result = await fn(...args); + } catch (e: any) { + if (e?.name === "StepSuspend" || e instanceof StepSuspend) throw e; + ctx._raiseStepFailure(e); + } ctx._raiseSuspend({ mode: "step_complete", steps: [], @@ -302,7 +321,10 @@ async function runWorkflow( _workflowCtx = ctx; try { const result = await fn(...args); - // Mirrors bun_executor.rs: honour a suspend the body caught and swallowed. + // Mirrors bun_executor.rs: honour a step failure or suspend the body caught + // and swallowed. + const failed = ctx._takePendingStepFailure?.(); + if (failed) throw failed.error; const swallowed = ctx._takePendingSuspend?.(); if (swallowed) throw swallowed; // Flush unawaited tasks @@ -336,6 +358,8 @@ async function runWorkflow( } return { type: "dispatch", ...info }; } + const failed = ctx._takePendingStepFailure?.(); + if (failed) throw failed.error; throw e; } finally { _workflowCtx = null; @@ -1481,6 +1505,49 @@ describe("throwing inline step is checkpointed", () => { expect(result.result).toBe(10); }); + test("a child job cannot swallow the failure of the step it executes", async () => { + // Without parking, the catch below turns the child into a success returning + // "swallowed" and the parent records that as the step's value. + const boom = task(async function boom() { + throw new TypeError("nope"); + }); + const wf = workflow(async () => { + try { + await boom(); + } catch { + return "swallowed"; + } + return "unreachable"; + }); + await expect(runWorkflow(wf, { _executing_key: "step_0" }, [])).rejects.toThrow("nope"); + }); + + test("a parked failure is re-raised at the next SDK call, not left to hang", async () => { + // A body that catches and carries on reaches an SDK call that, in child mode, + // never resolves — so without the re-raise the child parks there and hangs + // until timeout instead of reporting the failure. Raced against a deadline so + // that regression fails the test rather than wedging the suite. + const boom = task(async function boom() { + throw new TypeError("nope"); + }); + for (const carryOn of [() => double(1), () => sleep(1)]) { + const wf = workflow(async () => { + try { + await boom(); + } catch { + // swallowed on purpose + } + await carryOn(); + return "unreachable"; + }); + const run = Promise.race([ + runWorkflow(wf, { _executing_key: "step_0" }, []), + new Promise((_, reject) => setTimeout(() => reject(new Error("parked")), 500)), + ]); + await expect(run).rejects.toThrow("nope"); + } + }); + test("a swallowed suspend from a task dispatch still reaches the runner", async () => { const wf = workflow(async (x: number) => { try {