From a558d876d8a939d08106147b96e3d1090dd12166 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 28 Jul 2026 11:25:26 +0000 Subject: [PATCH] test(wac): pin the failure record with one corpus both SDKs read Co-Authored-By: Claude Opus 5 (1M context) --- .../src/wac_failure_corpus.json | 80 +++++++++++++++++++ python-client/wmill/tests/test_workflow.py | 59 ++++++++++++++ python-client/wmill/wmill/client.py | 36 +++++---- typescript-client/tests/workflow.test.ts | 32 ++++++++ 4 files changed, 192 insertions(+), 15 deletions(-) create mode 100644 backend/windmill-common/src/wac_failure_corpus.json diff --git a/backend/windmill-common/src/wac_failure_corpus.json b/backend/windmill-common/src/wac_failure_corpus.json new file mode 100644 index 0000000000..9361b9a044 --- /dev/null +++ b/backend/windmill-common/src/wac_failure_corpus.json @@ -0,0 +1,80 @@ +{ + "_readme": [ + "Cases the python and the typescript SDK must both satisfy when they serialize", + "a failed step() body into a `__wmill_error` marker.", + "", + "The two drifted apart twice while #10368 was in review: `name` was taken from", + "the constructor in one client and from `e.name` in the other, and `stack` was", + "written in two different formats. Each was caught by a reviewer reading the", + "diff, not by a test, because each suite only ever checked its own language.", + "A shared corpus turns the next divergence into a failure in both suites.", + "", + "It sits next to `wac_failure_record`, the function that decides the record's", + "final shape, because that is the contract these markers are the raw material", + "for. The SDK test suites read it by relative path.", + "", + "`thrown` describes a value each language constructs natively:", + " name the name the record must report", + " message the message the record must report", + " props own properties/attributes that must reach `extra`", + " circular_prop an own property holding a cycle, which must be dropped", + " without taking the rest of `extra` with it", + "`expect.stack` is only \"present\" or \"absent\": its text is language-specific", + "(each client matches its own executor's format) and is asserted there.", + "", + "Known divergence, deliberately not covered here: a non-finite float reaches", + "`extra` as the string \"NaN\" in python and as null in typescript, because", + "JSON.stringify has no hook for it. The executors differ the same way." + ], + "cases": [ + { + "case": "a named error keeps its name and message", + "thrown": { "name": "HttpError", "message": "429 too many requests" }, + "expect": { + "name": "HttpError", + "message": "429 too many requests", + "stack": "present", + "absent": ["extra", "extra_omitted"] + } + }, + { + "case": "custom properties reach extra", + "thrown": { + "name": "HttpError", + "message": "429", + "props": { "code": 429, "retry_after": 5, "endpoint": "/v1/jobs" } + }, + "expect": { + "name": "HttpError", + "message": "429", + "stack": "present", + "extra": { "code": 429, "retry_after": 5, "endpoint": "/v1/jobs" } + } + }, + { + "case": "a property that cannot be serialized is dropped on its own", + "thrown": { + "name": "RequestError", + "message": "socket hang up", + "props": { "code": "ECONNRESET" }, + "circular_prop": "request" + }, + "expect": { + "name": "RequestError", + "message": "socket hang up", + "stack": "present", + "extra": { "code": "ECONNRESET" } + } + }, + { + "case": "an error carrying nothing of its own has no extra", + "thrown": { "name": "ValueError", "message": "nope" }, + "expect": { + "name": "ValueError", + "message": "nope", + "stack": "present", + "absent": ["extra"] + } + } + ] +} diff --git a/python-client/wmill/tests/test_workflow.py b/python-client/wmill/tests/test_workflow.py index 3f80fb06b3..22263f10a8 100644 --- a/python-client/wmill/tests/test_workflow.py +++ b/python-client/wmill/tests/test_workflow.py @@ -2,6 +2,7 @@ import asyncio import json +import pathlib import pytest from datetime import datetime, timezone @@ -968,6 +969,64 @@ class TestErrorPropagation: assert "step failed" in r["result"]["caught"] +class TestFailureRecordCorpus: + """The cases both SDKs must agree on, read from one shared file. + + `name` and `stack` drifted between the two clients twice while the record + was being unified, and each time only a reviewer noticed: a suite that only + knows its own language cannot see a divergence. The corpus is the same file + the typescript suite reads. + """ + + CORPUS = json.loads( + ( + pathlib.Path(__file__).resolve().parents[3] + / "backend/windmill-common/src/wac_failure_corpus.json" + ).read_text() + ) + + @staticmethod + def _construct(spec: dict) -> BaseException: + exc = type(spec["name"], (Exception,), {})(spec["message"]) + for key, value in (spec.get("props") or {}).items(): + setattr(exc, key, value) + if spec.get("circular_prop"): + cyclic: dict = {} + cyclic["self"] = cyclic + setattr(exc, spec["circular_prop"], cyclic) + return exc + + @pytest.mark.parametrize("case", CORPUS["cases"], ids=lambda c: c["case"]) + def test_marker_matches_the_shared_contract(self, case): + exc = self._construct(case["thrown"]) + + def raiser(): + raise exc + + # through the real step path, so the stack is the one a failure actually + # records rather than one this test happens to construct + async def run(): + ctx = WorkflowCtx({}) + try: + await ctx._run_inline_step("k", raiser) + except _StepSuspend as suspended: + return suspended.dispatch_info["result"] + raise AssertionError("a raising step did not suspend") + + error = asyncio.run(run())["result"]["error"] + + expect = case["expect"] + assert error["name"] == expect["name"] + assert error["message"] == expect["message"] + assert ("stack" in error) == (expect["stack"] == "present") + if "extra" in expect: + assert error["extra"] == expect["extra"] + for absent in expect.get("absent", []): + assert absent not in error + # whatever it kept has to survive the trip to the checkpoint + json.dumps(error, allow_nan=False) + + class TestRaisingInlineStepIsCheckpointed: """A ``step()`` whose body raises must still land in ``completed_steps``. diff --git a/python-client/wmill/wmill/client.py b/python-client/wmill/wmill/client.py index 05496f0724..4928da5d3e 100644 --- a/python-client/wmill/wmill/client.py +++ b/python-client/wmill/wmill/client.py @@ -2784,21 +2784,27 @@ def _step_error_marker(key: str, exc: BaseException) -> dict: # connection raises in turn. This runs inside the ``except`` that is # reporting the user's failure, so an escape here would replace their # error with an unrelated one and skip the checkpoint entirely. - # Narrow: what json raises for something it cannot represent. A broader - # catch would hide a mistake in this function as a silently missing - # field, which is how it read before. - try: - # ``parse_constant`` catches the one thing ``default`` cannot: a - # float is serializable, so ``NaN``/``Infinity`` pass through as - # bare literals that are not JSON and that the backend's extractor - # rejects — taking the whole checkpoint down with them. Kept as - # their text rather than dropped, so the attribute still says - # something. - error["extra"] = json.loads( - json.dumps(extra, default=_safe_str), parse_constant=lambda c: c - ) - except (TypeError, ValueError, RecursionError): - pass + # + # Per attribute, so one that cannot be represented does not take the + # rest with it — an ``AxiosError``-shaped exception carrying both a + # ``code`` and a cyclic ``request`` keeps the ``code``. The typescript + # client admits properties one at a time for the same reason. + safe_extra = {} + for _k, _v in extra.items(): + # Narrow: what json raises for something it cannot represent. A + # broader catch would hide a mistake here as a silently missing + # field, which is how it read before. ``parse_constant`` catches the + # one thing ``default`` cannot: a float is serializable, so + # ``NaN``/``Infinity`` pass through as bare literals that are not + # JSON and that the backend's extractor rejects. + try: + safe_extra[_k] = json.loads( + json.dumps(_v, default=_safe_str), parse_constant=lambda c: c + ) + except (TypeError, ValueError, RecursionError): + pass + if safe_extra: + error["extra"] = safe_extra return { "__wmill_error": True, "message": _safe_str(exc), diff --git a/typescript-client/tests/workflow.test.ts b/typescript-client/tests/workflow.test.ts index 179244314b..adbeac68e2 100644 --- a/typescript-client/tests/workflow.test.ts +++ b/typescript-client/tests/workflow.test.ts @@ -10,6 +10,38 @@ import { expect, test, describe } from "bun:test"; // backend's shape before, so a copy of them here would guard nothing. import { isSuspendSignal, stepErrorMarker, taskErrorFromMarker } from "../wacError"; +// The cases both SDKs must agree on, read from the same file the python suite +// reads. `name` and `stack` drifted between the two clients twice while the +// record was being unified, and a suite that only knows its own language cannot +// see that. +import corpus from "../../backend/windmill-common/src/wac_failure_corpus.json"; + +describe("shared failure-record corpus", () => { + const construct = (spec: any): any => { + const e: any = Object.assign(new Error(spec.message), { name: spec.name }); + for (const [k, v] of Object.entries(spec.props ?? {})) e[k] = v; + if (spec.circular_prop) { + const cyclic: any = {}; + cyclic.self = cyclic; + e[spec.circular_prop] = cyclic; + } + return e; + }; + + for (const c of (corpus as any).cases) { + test(c.case, () => { + const error = stepErrorMarker("k", construct(c.thrown)).result.error; + expect(error.name).toBe(c.expect.name); + expect(error.message).toBe(c.expect.message); + expect("stack" in error).toBe(c.expect.stack === "present"); + if (c.expect.extra) expect(error.extra).toEqual(c.expect.extra); + for (const absent of c.expect.absent ?? []) expect(absent in error).toBe(false); + // whatever it kept has to survive the trip to the checkpoint + expect(() => JSON.stringify(error)).not.toThrow(); + }); + } +}); + // --- Inline SDK (mirrors client.ts implementation) --- class StepSuspend extends Error {