fix: Validate first yielded value in orchestrator run() method#164
Open
YunchuWang wants to merge 2 commits intomainfrom
Open
fix: Validate first yielded value in orchestrator run() method#164YunchuWang wants to merge 2 commits intomainfrom
YunchuWang wants to merge 2 commits intomainfrom
Conversation
The run() method in RuntimeOrchestrationContext did not validate that the first value yielded by an orchestrator generator is a Task instance. The resume() method already validates subsequent yields with an instanceof check, but run() silently accepted non-Task values (null, undefined, plain objects, primitives), causing the orchestration to hang indefinitely with no error message. This adds the same validation to run() that already exists in resume(), throwing a clear error when a non-Task value is yielded. This also removes the now-addressed TODO comment and the redundant instanceof guard on the isComplete check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a runtime correctness gap in the Durable Task JS orchestration engine by validating that the first value yielded by an orchestrator generator is a Task, aligning run() behavior with the existing validation in resume() to prevent silent orchestration hangs (Issue #160).
Changes:
- Add
instanceof Taskvalidation for the first yielded value inRuntimeOrchestrationContext.run(), throwing a clear error on invalid yields. - Simplify the “already complete” check after the first yield now that
_previousTaskis guaranteed to be aTask. - Add Jest coverage to verify orchestrations fail (not hang) when the first yield is
null,undefined, an object, or a primitive.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/worker/runtime-orchestration-context.ts | Adds first-yield Task validation in run() to match resume() and prevent silent hangs. |
| packages/durabletask-js/test/orchestration_executor.spec.ts | Adds tests ensuring invalid first yields fail the orchestration with an informative error. |
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add validation that the first value yielded by an orchestrator generator is a Task instance, matching the existing validation in
esume().
Closes #160
Problem
un() silently accepts any value (null, undefined, plain objects, primitives) as the first yield and assigns it to _previousTask without validation. This causes orchestrations to hang indefinitely with no error when a non-Task value is yielded.
Fix
Add the same instanceof Task validation that
esume() already has, replacing the TODO comment that acknowledged this gap.