Repository navigation
Conversation
FailureToError passed the raw *commonpb.Payloads proto straight through as the ApplicationError's details for a reset-workflow failure, instead of wrapping it with newEncodedValues like every sibling branch does. Calling Details() on the resulting error failed instead of decoding the payload.
|
|
|
|
|
Thanks for the contrib. Does this close an existing issue? Please use the PR template we provided and review the contributing guide: https://github.com/temporalio/sdk-go/blob/main/CONTRIBUTING.md |
|
No linked issue — this is a small, scoped bug fix (root cause + fix + regression test in the description above), which your CONTRIBUTING.md says can go straight to a PR. I don't see a PR template file in .github either, just ISSUE_TEMPLATE. Happy to add anything specific if I'm missing something. |
|
The template is org-wide, which is why I believe there is no file in |
…failure-heartbeat-details
|
PR updated as per template. |
| // Before the fix, the raw *commonpb.Payloads proto was passed straight through as the | ||
| // error's details instead of being wrapped by newEncodedValues, so Details() panicked | ||
| // with a reflect type mismatch instead of decoding the payload. |
There was a problem hiding this comment.
I think it would return ErrTooManyArg instead of decoding it,
| // Before the fix, the raw *commonpb.Payloads proto was passed straight through as the | |
| // error's details instead of being wrapped by newEncodedValues, so Details() panicked | |
| // with a reflect type mismatch instead of decoding the payload. | |
| // Before the fix, the raw *commonpb.Payloads proto was treated as a single | |
| // detail value, so Details() returned ErrTooManyArg instead of decoding it. |
The raw *commonpb.Payloads proto was treated as a single detail value before the fix, so Details() returned ErrTooManyArg rather than panicking with a reflect type mismatch as the comment claimed. Found via review on the PR.
…failure-heartbeat-details
1.48.0 was cut and released before this fix merged, so the entry landed in an already-shipped section instead of the one that will actually include it. Also incorporates the corrected wording from review (ErrTooManyArg, not a panic).
|
Rebased onto main and updated the comment per your suggestion. Also moved the CHANGELOG entry -- 1.48.0 had already been cut and released while this was open, so it was sitting in an already-shipped section instead of Unreleased. |
yuandrew
left a comment
yuandrew
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
What was changed
FailureToError'sResetWorkflowFailureInfobranch passed the raw*commonpb.Payloadsproto straight through as theApplicationError's details, instead of wrapping it withnewEncodedValueslike every sibling branch (Timeout, Canceled, Activity, etc.) does. CallingDetails()on the resulting error failed withtoo many argumentsinstead of decoding the payload. Fixed to match the sibling pattern.Why?
Every other failure-info branch in
FailureToErrorwraps its payload the same way before handing it to the caller; this one didn't, soDetails()broke specifically for reset-workflow failures — an easy case to miss since it's not the common path, but a genuine inconsistency with the rest of the function.Checklist
FailurewithResetWorkflowFailureInfodirectly and assertingDetails()decodes correctly; confirmed it fails onmainwithtoo many argumentsand passes after the fix.go run . checkand the fullinternalpackage test suite pass with no regressions.