[Fix-17758][Master] Add logic to handle tasks that fail during initialization. #17759
Conversation
njnu-seafish
requested review from
SbloodyS,
caishunfeng and
ruanwenjun
as code owners
SbloodyS
changed the title
ruanwenjun
left a comment
ruanwenjun
left a comment
There was a problem hiding this comment.
Should fire a TaskFailedLifecycleEvent when task initialize failed.
yes, add logic to handle TaskFatalLifecycleEvent |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @AllArgsConstructor | ||
| public class TaskFatalLifecycleEvent extends AbstractTaskLifecycleEvent { | ||
|
|
||
| private final ITaskExecutionRunnable taskExecutionRunnable; |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
| try { | ||
| taskExecutionRunnable.initializeTaskExecutionContext(); | ||
| } catch (Exception ex) { | ||
| log.error("Current taskInstance: {} initializeTaskExecutionContext error", taskInstance.getName(), ex); | ||
| final TaskFatalLifecycleEvent taskFatalEvent = TaskFatalLifecycleEvent.builder() | ||
| .taskExecutionRunnable(taskExecutionRunnable) | ||
| .endTime(new Date()) | ||
| .build(); | ||
| taskExecutionRunnable.getWorkflowEventBus().publish(taskFatalEvent); | ||
| return; | ||
| } |
There was a problem hiding this comment.
You need to create a TaskFatalException, and throw TaskFatalException here and handle TaskFatalException at WorkflowEventBusFireWorker.
There was a problem hiding this comment.
Pull request overview
This PR adds a new mechanism to handle tasks that fail during initialization by introducing a TaskFatalLifecycleEvent. The implementation distinguishes fatal initialization failures from regular task failures, ensuring that unrecoverable errors (like missing environment configurations) do not trigger retry logic and are handled appropriately in the workflow execution.
- Adds
TaskFatalLifecycleEventand its handler to manage initialization failures - Updates task state machine to handle fatal events and properly fail workflows or continue if all successors are condition tasks
- Adds comprehensive integration tests covering fatal task scenarios with condition tasks
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| TaskFatalLifecycleEvent.java | New lifecycle event class representing fatal unrecoverable errors during task initialization |
| TaskFatalLifecycleEventHandler.java | Handler that processes fatal events and delegates to state actions |
| TaskLifecycleEventType.java | Added FATAL enum value for the new event type |
| ITaskStateAction.java | Added onFatalEvent interface method for state actions |
| AbstractTaskStateAction.java | Default implementation of fatal event handling with workflow graph logic |
| TaskSubmittedStateAction.java | Added try-catch around initializeTaskExecutionContext to publish fatal events on exceptions |
| WorkflowStartTestCase.java | Added three new integration tests for fatal task scenarios |
| workflow_with_one_fake_task_fatal.yaml | Test fixture for single fatal task scenario |
| workflow_with_one_condition_task_with_one_fake_predecessor_fatal.yaml | Test fixture for condition task with fatal predecessor |
| workflow_with_one_forbidden_condition_task_with_one_fake_predecessor_fatal.yaml | Test fixture for forbidden condition task with fatal predecessor |
| workflow_with_one_forbidden_condition_task_with_one_fake_predecessor_failed.yaml | Updated description for clarity to indicate the condition task is forbidden |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| taskType: LogicFakeTask | ||
| taskParams: '{"localParams":null,"varPool":[],"shellScript":"echo success"}' | ||
| workerGroup: default | ||
| environmentCode : 144873539254368 |
There was a problem hiding this comment.
Inconsistent spacing around colon. The key environmentCode should be followed by a colon without a space before it, consistent with other YAML files in this directory. Change environmentCode : to environmentCode:.
| environmentCode : 144873539254368 | |
| environmentCode: 144873539254368 |
| # limitations under the License. | ||
| # | ||
|
|
||
| # A(fatal-failed) -> B(success) -> D(success) |
There was a problem hiding this comment.
The comment incorrectly describes the workflow structure. Based on the taskRelations, the structure is: A(fatal-failed) -> B(Condition) -> C(success) or D(failed). The comment should be updated to: # A(fatal-failed) -> B(Condition) -> C(success) or D(failed).
| # A(fatal-failed) -> B(success) -> D(success) | |
| # A(fatal-failed) -> B(Condition) -> C(success) or D(failed) |
| # limitations under the License. | ||
| # | ||
|
|
||
| # A(failed) -> B(Condition)(forbidden) -> C(success) |
There was a problem hiding this comment.
The comment says "A(failed)" but based on the filename and test description, this should be "A(fatal)". The comment should be updated to: # A(fatal) -> B(Condition)(forbidden) -> C(success).
| # A(failed) -> B(Condition)(forbidden) -> C(success) | |
| # A(fatal) -> B(Condition)(forbidden) -> C(success) |
| final TaskInstance taskInstance = taskExecutionRunnable.getTaskInstance(); | ||
| taskInstance.setState(TaskExecutionStatus.FAILURE); | ||
| taskInstance.setEndTime(taskFatalEvent.getEndTime()); | ||
| taskInstanceDao.updateById(taskInstance); |
There was a problem hiding this comment.
[nitpick] For consistency with other event handlers in this class (e.g., onFailedEvent, onPausedEvent, onKilledEvent), consider extracting the persistence logic into a private helper method like persistentTaskInstanceFatalEventToDB. This improves code maintainability and follows the established pattern in this class.
| @AllArgsConstructor | ||
| public class TaskFatalLifecycleEvent extends AbstractTaskLifecycleEvent { | ||
|
|
||
| private final ITaskExecutionRunnable taskExecutionRunnable; |
There was a problem hiding this comment.
This method overrides AbstractTaskLifecycleEvent.getTaskExecutionRunnable; it is advisable to add an Override annotation.
|
3 participants
Purpose of the pull request
close #17758
Brief change log
Add logic to handle tasks that fail during initialization.
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.md