fix: Preserve failure details when error message or type is empty string#198
Open
YunchuWang wants to merge 1 commit intomainfrom
Open
fix: Preserve failure details when error message or type is empty string#198YunchuWang wants to merge 1 commit intomainfrom
YunchuWang wants to merge 1 commit intomainfrom
Conversation
… error message or type is empty string The newOrchestrationState function in orchestration/index.ts used falsy string checks (failureDetailsErrorMessage && failureDetailsErrorType) to determine whether to create FailureDetails. Empty strings are falsy in JavaScript, so failure details with an empty error message or empty error type were silently dropped, returning undefined failureDetails even for failed orchestrations. This fix changes the check to verify the protobuf TaskFailureDetails object exists (consistent with the pattern used in client.ts and history-event-converter.ts). Also fixes getStacktrace()?.toString() to use the canonical .getValue() method used everywhere else in the codebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a client-side bug in the DurableTask JavaScript SDK where newOrchestrationState() could drop FailureDetails when the protobuf error message/type is an empty string, and aligns stack-trace extraction with the rest of the codebase.
Changes:
- Preserve
FailureDetailsbased on presence of the protobufTaskFailureDetailsmessage (not truthiness of its string fields). - Extract stack traces via
getStacktrace()?.getValue()instead oftoString(). - Add unit tests covering empty-string failure details and stack-trace handling.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/orchestration/index.ts | Fixes failure-details extraction logic to preserve empty-string fields and uses getValue() for stack traces. |
| packages/durabletask-js/test/new-orchestration-state.spec.ts | Adds targeted Jest coverage for the failure-details preservation scenarios (including empty strings). |
| package-lock.json | Contains lockfile metadata changes unrelated to the functional fix. |
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.
Fixes #187
Problem
newOrchestrationState silently drops FailureDetails when either the error message or error type is an empty string, due to JavaScript falsy string checks. Also uses .toString() instead of .getValue() for stack trace extraction.
Changes