-
Notifications
You must be signed in to change notification settings - Fork 105
Add support for closure scope vars in step functions #358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: cd44969 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Suggestion:
The observability code checks if step.input is an array before hydrating it, but the PR changes new step inputs to be objects with args and closureVars properties, so they won't be hydrated for display.
View Details
📝 Patch Details
diff --git a/packages/core/src/observability.ts b/packages/core/src/observability.ts
index 9422e6f..814d1ac 100644
--- a/packages/core/src/observability.ts
+++ b/packages/core/src/observability.ts
@@ -48,18 +48,42 @@ const hydrateStepIO = <
>(
step: T
): T => {
+ // Handle both old format (array) and new format (object with args and closureVars)
+ let hydratedInput = step.input;
+
+ if (step.input) {
+ if (Array.isArray(step.input) && step.input.length) {
+ // Old format: input is an array of arguments
+ hydratedInput = hydrateStepArguments(
+ step.input,
+ [],
+ step.runId as string,
+ globalThis,
+ streamPrintRevivers
+ );
+ } else if (
+ typeof step.input === 'object' &&
+ 'args' in step.input &&
+ Array.isArray(step.input.args) &&
+ step.input.args.length
+ ) {
+ // New format: input is { args: [...], closureVars: {...} }
+ hydratedInput = {
+ ...step.input,
+ args: hydrateStepArguments(
+ step.input.args,
+ [],
+ step.runId as string,
+ globalThis,
+ streamPrintRevivers
+ ),
+ };
+ }
+ }
+
return {
...step,
- input:
- step.input && Array.isArray(step.input) && step.input.length
- ? hydrateStepArguments(
- step.input,
- [],
- step.runId as string,
- globalThis,
- streamPrintRevivers
- )
- : step.input,
+ input: hydratedInput,
output: step.output
? hydrateStepReturnValue(step.output, globalThis, streamPrintRevivers)
: step.output,
Analysis
Step input not hydrated for display when using closure variables
What fails: hydrateStepIO() in packages/core/src/observability.ts (line 54) skips hydration for new step inputs, causing closure variable-based steps to display in serialized form instead of human-readable format in logging and observability features.
How to reproduce:
- Create a workflow with step functions that use closure variables
- Call
hydrateResourceIO()with a retrieved step object that has the new input format - Observe that
step.inputis returned unhydrated (still in serialized form)
Expected vs actual behavior:
- Old format (array):
step.input = [1, 2]→Array.isArray(step.input)istrue→ hydrated ✓ - New format (object):
step.input = { args: [1, 2], closureVars: {...} }→Array.isArray(step.input)isfalse→ NOT hydrated ✗
Root cause: The runtime (packages/core/src/runtime.ts line 392-398) changed how step inputs are persisted when the closure variables feature was added. Steps are now stored with structure { args: [...], closureVars: {...} } instead of just [...]. The observability code at line 54 only checked for array format using Array.isArray(step.input), missing the new object format.
Fix: Updated hydrateStepIO() to handle both formats:
- Check if input is an array (old format) and hydrate directly
- Check if input is an object with
argsproperty (new format) and hydrate theargsfield while preservingclosureVars
| workflowRunId | ||
| ); | ||
|
|
||
| const args = hydratedInput.args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing backward compatibility handling for step input format. The code assumes hydratedInput has an args property, but old steps created before this change will have step.input as a serialized array, causing a runtime error when trying to access .args.
View Details
📝 Patch Details
diff --git a/packages/core/src/runtime.ts b/packages/core/src/runtime.ts
index a788e48..e57e3ba 100644
--- a/packages/core/src/runtime.ts
+++ b/packages/core/src/runtime.ts
@@ -681,7 +681,9 @@ export const stepEntrypoint =
workflowRunId
);
- const args = hydratedInput.args;
+ // Handle both new format { args, closureVars } and legacy format (just args array)
+ const args = 'args' in hydratedInput ? hydratedInput.args : hydratedInput;
+ const closureVars = 'closureVars' in hydratedInput ? hydratedInput.closureVars : undefined;
span?.setAttributes({
...Attribute.StepArgumentsCount(args.length),
@@ -704,7 +706,7 @@ export const stepEntrypoint =
: `http://localhost:${port ?? 3000}`,
},
ops,
- closureVars: hydratedInput.closureVars,
+ closureVars,
},
() => stepFn.apply(null, args)
);
Analysis
Missing backward compatibility handling for step input format in stepEntrypoint
What fails: stepEntrypoint() in runtime.ts lines 684 and 707 assume hydratedInput has args and closureVars properties. When old steps created before the closureVars feature was added are replayed, they have step.input as a serialized array (not wrapped in an object). Upon hydration, this returns the array directly, and accessing .args on an array returns undefined. This causes line 686 args.length to throw: TypeError: Cannot read properties of undefined (reading 'length')
How to reproduce:
- A step was created and stored with the old format:
step.input = dehydrateStepArguments([arg1, arg2], globalThis)(just an array) - Replay/resume the workflow run after the code was updated to wrap inputs in
{ args, closureVars } - stepEntrypoint calls
hydrateStepArguments(step.input, ...) - Since the stored input is a plain array, hydration returns the array directly
- Code attempts
const args = hydratedInput.argswhich isundefinedon an array - Code tries
args.lengthonundefined, throwing TypeError
Expected behavior: Code should handle both:
- New format:
{ args: [...], closureVars: {...} } - Legacy format: plain array
[...]
This pattern is already implemented in observability.ts line 63: 'args' in hydratedInput ? hydratedInput.args : hydratedInput
Fix applied: Added defensive checks that mirror the observability.ts pattern:
const args = 'args' in hydratedInput ? hydratedInput.args : hydratedInput;
const closureVars = 'closureVars' in hydratedInput ? hydratedInput.closureVars : undefined;This ensures backward compatibility with steps stored in the old format while supporting the new closureVars feature.
2f8dded to
cd44969
Compare
What changed?
AsyncLocalStorage contextExample
Why make this change?
Previously, nested step functions couldn't access variables from their parent workflow scope, limiting their usefulness and requiring workarounds like passing all needed values as parameters. This change enables a more natural programming model where step functions can access variables from their enclosing scope, making workflow code more intuitive and reducing the need for explicit parameter passing.