[Fix-17758][Master] Mark task as failed if TaskExecutionContext initialization fails #17821
Conversation
…l when try to dispatch task
njnu-seafish
requested review from
SbloodyS,
caishunfeng and
ruanwenjun
as code owners
|
The second version of the code has been verified to work in our actual test environment. The specific logs are as follows:
|
![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
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #17758 by implementing proper failure handling when TaskExecutionContext initialization fails during task dispatch. Previously, when initialization failed, tasks would remain in an incomplete state rather than being marked as failed.
Key Changes:
- Introduced TaskFatalLifecycleEvent to handle catastrophic task failures
- Added exception handling in TaskSubmittedStateAction to catch initialization failures
- Implemented automatic task failure marking when initialization fails, with support for retries and condition task workflows
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| TaskExecutionContextCreateException.java | Changed exception hierarchy from MasterException to RuntimeException |
| ExceptionUtils.java | Added utility method to check for TaskExecutionContextCreateException |
| TaskSubmittedStateAction.java | Added try-catch block around task context initialization to throw TaskExecutionContextCreateException |
| TaskLifecycleEventType.java | Added new FATAL event type for catastrophic failures |
| TaskFatalLifecycleEvent.java | New event class representing fatal task failures with end time tracking |
| TaskFatalLifecycleEventHandler.java | New handler to process fatal lifecycle events |
| ITaskStateAction.java | Added onFatalEvent method interface for handling fatal events |
| AbstractTaskStateAction.java | Implemented onFatalEvent with retry logic, condition task handling, and failure chain marking |
| WorkflowEventBusFireWorker.java | Added logic to publish TaskFatalLifecycleEvent when TaskExecutionContextCreateException occurs |
| WorkflowStartTestCase.java | Added three test methods to verify fatal task handling scenarios |
| workflow_with_one_fake_task_fatal.yaml | Test configuration for single fatal task scenario |
| workflow_with_one_condition_task_with_one_fake_predecessor_fatal.yaml | Test configuration for condition task with fatal predecessor |
| workflow_with_one_forbidden_condition_task_with_one_fake_predecessor_fatal.yaml | Test configuration for forbidden condition task with fatal predecessor |
| workflow_with_one_forbidden_condition_task_with_one_fake_predecessor_failed.yaml | Updated name and description for clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AbstractTaskLifecycleEvent taskLifecycleEvent = (AbstractTaskLifecycleEvent) lifecycleEvent; | ||
| final TaskFatalLifecycleEvent taskFatalEvent = TaskFatalLifecycleEvent.builder() | ||
| .taskExecutionRunnable(taskLifecycleEvent.getTaskExecutionRunnable()) | ||
| .endTime(new Date()) | ||
| .build(); | ||
| workflowEventBus.publish(taskFatalEvent); |
There was a problem hiding this comment.
Unsafe type casting without verification. While the TaskExecutionContextCreateException is currently only thrown when handling TaskDispatchLifecycleEvent (which extends AbstractTaskLifecycleEvent), it would be safer to check the type before casting to prevent potential ClassCastException if the code structure changes. Consider adding an instanceof check before the cast.
| AbstractTaskLifecycleEvent taskLifecycleEvent = (AbstractTaskLifecycleEvent) lifecycleEvent; | |
| final TaskFatalLifecycleEvent taskFatalEvent = TaskFatalLifecycleEvent.builder() | |
| .taskExecutionRunnable(taskLifecycleEvent.getTaskExecutionRunnable()) | |
| .endTime(new Date()) | |
| .build(); | |
| workflowEventBus.publish(taskFatalEvent); | |
| if (lifecycleEvent instanceof AbstractTaskLifecycleEvent) { | |
| AbstractTaskLifecycleEvent taskLifecycleEvent = (AbstractTaskLifecycleEvent) lifecycleEvent; | |
| final TaskFatalLifecycleEvent taskFatalEvent = TaskFatalLifecycleEvent.builder() | |
| .taskExecutionRunnable(taskLifecycleEvent.getTaskExecutionRunnable()) | |
| .endTime(new Date()) | |
| .build(); | |
| workflowEventBus.publish(taskFatalEvent); | |
| } else { | |
| log.warn( | |
| "TaskExecutionContextCreateException occurred for non-task lifecycle event: {}", | |
| lifecycleEvent.getClass().getName(), | |
| ex); | |
| } |
| } catch (Exception ex) { | ||
| log.error("Failed to initialize task execution context, taskName: {}", taskInstance.getName(), ex); | ||
| throw new TaskExecutionContextCreateException(ex.getMessage()); | ||
|
|
There was a problem hiding this comment.
Unnecessary blank line after the throw statement. This extra blank line should be removed to maintain consistent code formatting.
| code: 1 | ||
| version: 1 | ||
| projectCode: 1 | ||
| description: This is a fake workflow with one forbidden condition task which has one predecessor fatal |
There was a problem hiding this comment.
The description reads awkwardly. It should be "This is a fake workflow with one forbidden condition task that has one predecessor that failed fatally" or "...with one predecessor that encountered a fatal error". The current phrasing "has one predecessor fatal" is grammatically incorrect.
| description: This is a fake workflow with one forbidden condition task which has one predecessor fatal | |
| description: This is a fake workflow with one forbidden condition task that has one predecessor that failed fatally |
| code: 1 | ||
| version: 1 | ||
| projectCode: 1 | ||
| description: This is a fake workflow with one condition task which has one predecessor fatal |
There was a problem hiding this comment.
The description reads awkwardly. It should be "This is a fake workflow with one condition task that has one predecessor that failed fatally" or "...with one predecessor that encountered a fatal error". The current phrasing "has one predecessor fatal" is grammatically incorrect.
| description: This is a fake workflow with one condition task which has one predecessor fatal | |
| description: This is a fake workflow with one condition task that has one predecessor that failed fatally |
| @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.
2 participants
Purpose of the pull request
close #17758
Brief change log
publish TaskFatalLifecycleEvent if initializeTaskExecutionContext fail when try to dispatch task
Verify this pull request
This pull request is already covered by existing tests.
First, add it test case
Second, We have already verified and tested this in our actual production environment.
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