[Bug] correctly set task execution phase for terminal array node #5136
Conversation
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5136 +/- ##
=======================================
Coverage 37.02% 37.02%
=======================================
Files 1317 1317
Lines 132523 132537 +14
=======================================
+ Hits 49066 49075 +9
- Misses 79211 79215 +4
- Partials 4246 4247 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Code Review Agent Run #e4c107Actionable Suggestions - 2
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| if !eventsErr.IsAlreadyExists(err) { | ||
| logger.Errorf(ctx, "ArrayNode event recording failed: [%s]", err.Error()) | ||
| return err |
There was a problem hiding this comment.
Consider handling the error case more gracefully by logging the error and continuing execution rather than returning early. The current implementation may cause unnecessary aborts.
Code suggestion
Check the AI-generated fix before applying
| if !eventsErr.IsAlreadyExists(err) { | |
| logger.Errorf(ctx, "ArrayNode event recording failed: [%s]", err.Error()) | |
| return err | |
| if eventsErr.IsAlreadyExists(err) { | |
| return nil | |
| } |
Code Review Run #e4c107
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| // ensure task_execution set to failed - this should already be sent by the abort handler | ||
| if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil { |
There was a problem hiding this comment.
Consider checking if eventRecorder is not nil before calling finalize(). The current code assumes eventRecorder is always initialized.
Code suggestion
Check the AI-generated fix before applying
| // ensure task_execution set to failed - this should already be sent by the abort handler | |
| if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil { | |
| // ensure task_execution set to failed - this should already be sent by the abort handler | |
| if eventRecorder == nil { | |
| logger.Errorf(ctx, "ArrayNode eventRecorder is nil") | |
| return handler.UnknownTransition, fmt.Errorf("eventRecorder is nil") | |
| } | |
| if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil { |
Code Review Run #e4c107
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Tracking issue
#5135
Why are the changes needed?
Task execution phases are not set correctly for successful nor failed ArrayNodes
What changes were proposed in this pull request?
How was this patch tested?
Setup process
ran a workflow with a map task that failed and one that succeeded. Confirmed that the taskExecution closure phase was correctly set.
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR addresses a bug in the array node handler related to task execution phase settings. The changes implement proper phase setting in task execution events, specifically improving the abort handler to distinguish between aborted and failed states. The modifications enhance state reporting accuracy for both successful and failed terminal states.Unit tests added: False
Estimated effort to review (1-5, lower is better): 1