diff --git a/AGENTS.md b/AGENTS.md index d7ebf2452d..90c0060ae2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,7 @@ Open-source platform for internal tools, workflows, API integrations, background ## Documentation - **Validation**: `docs/validation.md` — what checks to run based on what you changed +- **Unreleased SDK changes**: `docs/wac-sdk-e2e.md` — exercising a client change on a real worker - **Enterprise**: `docs/enterprise.md` — EE file conventions and PR workflow - **Backend patterns**: use the `rust-backend` skill when writing Rust code - **Frontend patterns**: use the `svelte-frontend` skill when writing Svelte code. Do NOT edit svelte files unless you have read that skill. 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..e4498159cd --- /dev/null +++ b/backend/windmill-common/src/wac_failure_corpus.json @@ -0,0 +1,108 @@ +{ + "_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.", + "", + "Second known divergence, also not covered: an attribute named like a field", + "the executors report separately \u2014 `name`, `message`, `stack` \u2014 reaches", + "`extra` in python and not in typescript. Each client mirrors its own", + "executor, which differ the same way: the python one copies `__dict__`", + "wholesale, the bun/deno one filters that skip-list out." + ], + "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" + ] + } + }, + { + "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/docs/wac-sdk-e2e.md b/docs/wac-sdk-e2e.md new file mode 100644 index 0000000000..5c6682d93d --- /dev/null +++ b/docs/wac-sdk-e2e.md @@ -0,0 +1,68 @@ +# Exercising an unreleased SDK change end to end + +A job installs the **published** `windmill-client` / `wmill`, so a change in this +repo is invisible to a real job until it is injected into the worker's dependency +cache. The SDK unit suites cover neither the worker nor the SDK the worker +installs, and that gap is where the Workflow-as-Code failure contract kept coming +apart: every finding behind #10366, #10367 and #10368 came from review or from a +run like the one below, never from a green suite. + +## Recipe + +Use a private `WINDMILL_DIR`. `/tmp/windmill/` is shared by every worktree's +backend, so patching it in place leaks a modified SDK into other people's jobs. + +```bash +# Run from the repository root. The backend holds a terminal of its own; every +# other command is a subshell, so nothing depends on where the last one left you. + +# 1. a backend of your own, with its own cache root — in its own terminal +(cd backend && DATABASE_URL=... PORT=8062 WINDMILL_DIR=/tmp/windmill-mytest \ + cargo run --features quickjs) # add ,python to run python jobs + +# 2. one job to populate the cache with the published SDK +# (any preview job importing the client will do) + +# 3. build the client and overwrite what the cache holds +C=$(echo /tmp/windmill-mytest/cache_nomount/bun/windmill-client@*@@@1) +(cd typescript-client && ./build.sh && npx tsdown --format esm --no-dts \ + && cp dist/index.mjs "$C/dist/index.mjs" \ + && cp dist/index.mjs "$C/dist/client.mjs") # code-split package: cover both + +# python instead: copy the source file straight over, then drop the bytecode +cp python-client/wmill/wmill/client.py \ + /tmp/windmill-mytest/cache/python_3_12/wmill==*/wmill/client.py +find /tmp/windmill-mytest -name __pycache__ -type d -exec rm -rf {} + + +# 4. RESTART the backend — see below +# 5. run your scenarios, and rm -rf /tmp/windmill-mytest when done +``` + +## Restart the workers after injecting + +A worker materializes the package once and keeps using its copy, so patching the +cache under a running backend leaves some workers on the old code. With more than +one worker the results then **alternate run to run** as jobs land on one worker or +the other, which reads like flakiness in the product rather than in the harness. +Restarting after the swap makes it deterministic. + +Symptom worth recognising: identical jobs returning two different answers in a +stable pattern, with each run internally consistent. + +## Run your scenarios twice + +Once against the published SDK, once against the injected one. A scenario that +behaves the same either way is not testing what you think it is, and it is easy +to write several of those without noticing. + +As a calibration: a spread of WAC scenarios written this way scored 10/17 (bun) +and 7/18 (python) against the published SDK and 17/17 and 18/18 against a client +carrying #10366, #10367 and #10368. The ones that did not move were covering +behaviour those PRs never touched — worth knowing before concluding that a green +run means anything. + +## Worth covering, and easy to miss + +The deno path, `taskScript` / `taskFlow`, `waitForApproval`, and failures +interleaved with parallelism. None of these were exercised while the failure +record was being unified. diff --git a/python-client/wmill/tests/test_workflow.py b/python-client/wmill/tests/test_workflow.py index 3f80fb06b3..f0b9d4b801 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 @@ -9,6 +10,16 @@ from datetime import datetime, timezone from wmill.client import WorkflowCtx, _StepSuspend, TaskError, workflow, task, step, sleep, parallel, wait_for_approval, _run_workflow, _run_workflow_async +class _FakeItems(dict): + """A mapping whose ``items()`` yields something that is not a key/value pair.""" + + def __bool__(self): + return True + + def items(self): + return [None] + + class _StubInlineClient: """Stands in for the httpx client the inline fast path POSTs with. @@ -968,6 +979,59 @@ class TestErrorPropagation: assert "step failed" in r["result"]["caught"] +class TestFailureRecordCorpus: + """The cases both SDKs must agree on, from the corpus the typescript suite + also reads. Its `_readme` states the contract and why it is shared.""" + + 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``. @@ -1092,6 +1156,52 @@ class TestRaisingInlineStepIsCheckpointed: _json.dumps(marker) assert marker["result"]["error"]["name"] == "HostileDict" + # ...or an overridden __dict__, whatever shape it takes + class NotAMapping(Exception): + @property + def __dict__(self): + return "definitely not a mapping" + + marker = _step_error_marker("k", NotAMapping()) + _json.dumps(marker) + assert marker["result"]["error"]["name"] == "NotAMapping" + + class YieldsNonPairs(Exception): + @property + def __dict__(self): + return _FakeItems() + + marker = _step_error_marker("k", YieldsNonPairs()) + _json.dumps(marker) + assert marker["result"]["error"]["name"] == "YieldsNonPairs" + + class OddKey(Exception): + def __init__(self): + super().__init__("boom") + self.code = 429 + + odd = OddKey() + odd.__dict__[(1, 2)] = "tuple key" + marker = _step_error_marker("k", odd) + _json.dumps(marker) # a surviving tuple key would raise here + assert marker["result"]["error"]["extra"] == {"code": 429} + + class ExplodingKey: + def __init__(self): + self.hashed = 0 + + def __hash__(self): + self.hashed += 1 + if self.hashed > 1: + raise RuntimeError("second hash") + return 1 + + exploding = OddKey() + exploding.__dict__[ExplodingKey()] = "x" + marker = _step_error_marker("k", exploding) + _json.dumps(marker) + assert marker["result"]["error"]["extra"] == {"code": 429} + # A float is serializable, so `default=` never sees NaN — it would go out # as a bare `NaN` literal, which is not JSON and which the backend # rejects, so the step could not be checkpointed at all. diff --git a/python-client/wmill/wmill/client.py b/python-client/wmill/wmill/client.py index 230530fdab..01589d4ef7 100644 --- a/python-client/wmill/wmill/client.py +++ b/python-client/wmill/wmill/client.py @@ -2772,33 +2772,36 @@ def _step_error_marker(key: str, exc: BaseException) -> dict: # there is — ``resp.raise_for_status()``, whose ``__dict__`` holds a request # and a response object — would otherwise fail to serialize and silently # drop every such failure onto the slow suspend-and-replay path. - # ``getattr``'s default only swallows ``AttributeError``; an exception - # overriding ``__getattribute__`` raises whatever it likes from this read. + # Everything about the failing exception can fight back, and this runs inside + # the ``except`` reporting it, so an escape replaces the user's error and + # skips the checkpoint. Only a genuine ``dict`` is walked: an overridden + # ``__dict__`` can raise on access, on ``.items()``, or yield non-pairs. try: - extra = getattr(exc, "__dict__", None) + _raw_extra = getattr(exc, "__dict__", None) except Exception: - extra = None - if extra: - # ``default`` is where json hands back the objects it cannot represent, - # and ``str()`` on a detached ORM row or a proxy over a closed - # 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 + _raw_extra = None + if type(_raw_extra) is dict and _raw_extra: + safe_extra = {} + for _k, _v in _raw_extra.items(): + # Rebuilding the pair hashes the key again, so only the types json + # can represent, and exactly those: a subclass may define __hash__. + if type(_k) not in (str, int, float, bool, type(None)): + continue + # Per attribute so one bad value cannot take the rest, and as a pair + # so an int/bool/None key arrives as the string a replay reads. + # ``parse_constant`` catches what ``default`` cannot: a float is + # serializable, so NaN/Infinity would go out as invalid JSON. + try: + safe_extra.update( + json.loads( + json.dumps({_k: _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..08a817bc54 100644 --- a/typescript-client/tests/workflow.test.ts +++ b/typescript-client/tests/workflow.test.ts @@ -5,11 +5,41 @@ */ import { expect, test, describe } from "bun:test"; -// The two functions that decide what a caught failure looks like come from the -// shipped module, not from the mirror below: they are what drifted from the -// backend's shape before, so a copy of them here would guard nothing. +// From the shipped module, not the mirror below: these decide what a caught +// failure looks like and what is the SDK's own control flow, so a copy here +// would guard nothing. import { isSuspendSignal, stepErrorMarker, taskErrorFromMarker } from "../wacError"; +// The cases both SDKs must agree on, from the corpus the python suite also +// reads. Its `_readme` states the contract and why it is shared. +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 { diff --git a/typescript-client/wacError.ts b/typescript-client/wacError.ts index 025de78a7b..4145630565 100644 --- a/typescript-client/wacError.ts +++ b/typescript-client/wacError.ts @@ -3,8 +3,8 @@ // Deliberately dependency-free so the tests can import it directly: the rest of // client.ts pulls in the generated API modules, which is why the workflow test // suite re-implements WorkflowCtx inline. What a caught failure looks like, and -// what is a failure at all rather than the SDK's own control flow, is decided -// here — so it is decided against the shipped code rather than against a copy. +// what counts as a failure rather than the SDK's own control flow, is decided +// here, against the shipped code rather than a copy of it. /** Error properties the executors already report as named fields, so they must * not be repeated inside `extra`. Kept identical to the skip-list in the