Repository navigation
fix: resolve activity inputs in the workflow before scheduling - #615
Conversation
a61b82e to
b4d7734
Compare
|
Good catch, fixed in the same commit: map keys are now walked in sorted order, so the first error and the deferred list are deterministic on replay (the extra sort of |
mrsimonemms
added
ai-review
Activities received their inputs as unresolved ${ ... } strings and
evaluated them themselves, so the history recorded the expressions rather
than the values the activity used.
The workflow now resolves the inputs of call: http, call: grpc and run
(shell, script and container) before scheduling the activity: exactly the
fields each activity evaluated before, with the same coercions. The
activity evaluates only the expressions the workflow left for it, listed by
JSON Pointer on the state it receives: those that read $data.activity,
which exists only once an attempt runs, and those whose result JSON cannot
carry exactly (an integer beyond 2^53, NaN or infinity), since Temporal
decodes activity arguments as JSON numbers. Every expression is evaluated
once, so resolved data that looks like an expression reaches the activity
as text. Map keys are walked in sorted order, so the first error and the
deferred list are the same on replay.
New executions record the zigflow.resolve-activity-inputs version marker.
An execution without it keeps scheduling unresolved inputs, and the
activities still evaluate those, so open histories replay unchanged.
Fixes zigflow#462
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yash B Joshi <68319416+YJ-928@users.noreply.github.com>
b4d7734 to
14d03a2
Compare
|
Good catch, fixed in the same commit, as a class rather than the one case. An expression whose result JSON can't carry exactly (an integer beyond 2^53, NaN, infinity) is now left for the activity, as before this PR. The same precision loss would have hit gRPC arguments through Temporal's decoding, so that's covered too. New regression tests run the old and new paths over a matrix of value shapes through Temporal's data converter, and check that the actual HTTP request, gRPC arguments and shell output are unchanged. They fail without the fix for both HTTP and gRPC. |
mrsimonemms
added
ai-review
Codex PR Review
No blocking issues. Ship it. Tests could not be run in this read-only environment because Go could not create its module cache. |
mrsimonemms
left a comment
mrsimonemms
left a comment
There was a problem hiding this comment.
Reviewed the workflow resolution, deferred activity metadata, JSON precision handling, shared task isolation and version-gated legacy path. No blocking findings.
Validation: go test ./pkg/... passes locally with TMPDIR=/private/tmp (the default macOS temporary path causes an existing /var vs /private/var assertion mismatch). The reviewed commit also has passing CI unit tests, race tests, end-to-end tests, linting and Helm checks.
The mixed-worker upgrade caveat documented in the PR remains: an older activity worker can re-evaluate resolved text that resembles an expression.
|
Thank you @YJ-928. I’ve really appreciated your contributions and how you handle reviews. Would you be interested in joining me in more of a maintainer role for the Zigflow project? |
|
Thank you, @mrsimonemms — that genuinely means a lot, and I'd be glad to accept. Working on Zigflow with you has been a real pleasure. Your reviews are fast, clear and fair, and I've learned a lot from how you run the project. We run Zigflow in production every day, so I care about it being solid, and I'd like to help carry more of the load. Where I think I can help most:
To set expectations honestly, I can reliably commit around 5 hours a week. That's enough to review PRs and respond to new issues within two working days, with more time when something is urgent or a release is close. A few questions so I can fit in the way you'd like:
Thanks again for the trust. Looking forward to it. |
|
That's great news @YJ-928, thank you. Your "where I can help" list is a great start. My overall intention is to make Zigflow sustainable in the long-term - ie, My main ambition is getting Zigflow to v1 status, which involves getting the identified issues resolved (there may be more, but this is what we know about today). In answer to your questions:
|
|
Outside of this project, another priority is getting Zigflow Studio into a releasable state. That may or may not interest you, but there's definitely a desire for that project. |
|
Thanks Simon, that's really clear and it's great to have it laid out. Equal maintainers, open discussion when unsure, and releases as a discussion all sound right to me. I'll join the Zigflow Slack channel today. On the v1 list, my plan is:
On Zigflow Studio: thanks for thinking of me. I need to check one thing on my side before I pick anything up there, and I'll come back to you on it. Looking forward to getting Zigflow to v1 together. |
2 participants
Description
Activities received their inputs as unresolved
${ ... }strings and evaluated them themselves, so the history recorded expressions instead of the values the activity used.Inputs are now resolved once, in the workflow, before the activity is scheduled. The activity evaluates only what the workflow explicitly leaves for it.
What is resolved: exactly the fields each activity evaluated before, with the same coercions.
call: httpwithcall: grpcmethod,argumentsrun: shell,run: scriptarguments,environment, a script's externalsourcerun: containerarguments,environment, andimageon Docker (the Kubernetes runtime never evaluated it)call: activityandrun: workflowalready resolve in the workflow and are unchanged.The
$data.activityboundary.$data.activitydescribes the attempt that is running: attempt, start time, deadline, task token. One scheduled input serves every retry attempt, so it cannot be known before scheduling.ActivityInputs.Deferredon the state the activity receives, and the activity evaluates those locations and nothing else.$datacounts unless its first access is a static key other thanactivity. So$data | .activity.attemptand$data as $d | $d.activity.attemptare deferred.$data.activityis resolved there like anything else; only values JSON cannot carry (below) are deferred.Evaluated once. A resolved value that itself looks like an expression reaches the endpoint as text, for example
$input.commentholding"${ $env.API_TOKEN }". Nothing evaluates it again, and nothing is quoted.Values JSON cannot carry. Temporal decodes activity arguments as JSON, where every number is a
float64. An expression whose result contains an integer beyond 2^53, NaN or infinity is therefore left for the activity, which evaluates it exactly as before. Otherwise a body or gRPC argument such as${ 9007199254740993 }would arrive as9007199254740992.Compatibility
zigflow.resolve-activity-inputsversion marker, using the sameworkflow.GetVersionpattern aszigflow.per-task-activity-aliases. An execution without the marker keeps scheduling unresolved inputs, and the activities still evaluate those, so open histories replay unchanged.mainwas handed to this branch's worker during await. It replayed, finished its remaining activities on the old path, and completed with no non-determinism errors.zigflow validateand when the worker loads the workflow (checked for http headers, shell arguments and container environment). Evaluating them in workflow code is therefore replay-safe.ZIGGY_*environment is already part of the state every activity receives.Behaviour changes
httpandrunevery retry the retry policy allowed. Atrystill catches it;$data.errorhas noactivityblock because no activity ran.Related issue
Fixes #462
How to test
New tests:
pkg/utils/activity_inputs_test.go: the detection table, including the forms from the Evaluate expressions before executing activities #483 review; deferred locations, including results JSON cannot carry; values evaluated once; the same error reported every time when several expressions fail; and parity withTraverseAndEvaluateObjfor activities scheduled before this change.pkg/zigflow/tasks/task_builder_activity_inputs_test.go: what http, grpc, shell, script and container (Docker and Kubernetes) are scheduled with; a failing expression; an execution without the version marker.pkg/zigflow/activities/activity_inputs_test.go: the http, shell, Docker and Kubernetes paths evaluate only the deferred locations, including when nothing is deferred. Old-vs-new tests run a matrix of values (small and large integers, floats, booleans, null, nested values, numbers from$input) through both paths and Temporal's real data converter, and check that the HTTP request, the gRPC arguments and the shell output are unchanged.activity-inputschecks two things:withand the deferred list.echoof a resolved${ $env.HTTP_MOCK }and a deferred attempt prints${ $env.HTTP_MOCK } 1.Checklist
Fixes #...,Closes #...orRelates to #...tests/chartwas not run locally because it needshelm; the chart is untouched.