fix: Add missing non-Task validation in resume() isFailed path#165
Open
YunchuWang wants to merge 1 commit intomainfrom
Open
fix: Add missing non-Task validation in resume() isFailed path#165YunchuWang wants to merge 1 commit intomainfrom
YunchuWang wants to merge 1 commit intomainfrom
Conversation
… path When an orchestrator catches an exception from a failed task and yields a non-Task value, the runtime silently falls through without reporting an error. This leaves _previousTask pointing to the old failed task, corrupting the generator state. On the next resume() call, the same exception is thrown to the generator again in an unexpected location. The isComplete path already validates this (line 184-186) and throws a clear error: 'The orchestrator generator yielded a non-Task object'. The isFailed path lacked this same validation. This fix adds the same non-Task validation to the isFailed path, ensuring consistent behavior and a clear error message. 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 generator state-corruption bug in the DurableTask JS orchestration runtime by enforcing Task-only yields when resuming after a failed task, aligning the isFailed resume path with the existing isComplete validation behavior (closes #161).
Changes:
- Add non-
Taskyield validation inRuntimeOrchestrationContext.resume()when resuming viagenerator.throw(...)after task failure. - Add regression coverage for yielding a non-
Taskafter catching a failure, plus a positive test for catching a failure and yielding a newTask.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/worker/runtime-orchestration-context.ts | Throws a clear error if a generator yields a non-Task after catching a failed task exception, preventing silent state corruption. |
| packages/durabletask-js/test/orchestration_executor.spec.ts | Adds tests to verify orchestration fails with the expected error on non-Task yield after a caught failure, and succeeds when a new Task is yielded. |
You can also share your feedback on Copilot code review. Take the survey.
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 non-Task validation in the isFailed path of
esume(), making it consistent with the isComplete path.
Closes #161
Problem
In
esume(), the isComplete path correctly validates that the yielded value is a Task instance and throws a clear error. The isFailed path lacks this validation entirely - it silently returns when the yielded value is not a Task, leaving _previousTask pointing to the old failed task and causing confusing behavior on subsequent events.
Fix
Add hrow new Error('The orchestrator generator yielded a non-Task object') after the instanceof Task check in the isFailed path.