You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "ThreeFire1 (via GitHub)" <gi...@apache.org> on 2026/10/19 11:14:27 UTC
[PR] [Fix-18721][Master] Keep workflow finish retryable after transaction rollback [dolphinscheduler]
ThreeFire1 opened a new pull request, #18722:
URL: https://github.com/apache/dolphinscheduler/pull/18722
## Was this PR generated or assisted by AI?
YES. AI assisted with the investigation, code changes, regression tests, and this description. I reviewed the proposed approach and the reported test results.
## Purpose of the pull request
Fixes #18721.
If serial-command cleanup or a pre-commit callback fails after the workflow update, the transaction rolls back but the shared workflow instance already has a final state. The Master then skips the retried finish event, and a following SERIAL_WAIT instance can remain blocked.
This change keeps the shared instance unchanged until the transaction commits, so a rollback leaves it ready for retry.
## Brief change log
- Update a copy of the workflow instance and delete its serial command in the same transaction. After commit, update the shared state and end time, record the completion metric, and publish the finalize event.
- Avoid completing the same workflow twice within an enclosing transaction. If an earlier `afterCommit` callback throws, use `afterCompletion` to finish only when the transaction committed and completion has not already been attempted.
- Keep a completion-metric exception from preventing finalize publication.
- Add rollback, retry, duplicate-event, and transaction-callback tests, plus Master/H2 tests for automatic recovery and serial queue progress.
## Verify this pull request
Added `WorkflowFinishPersistenceRecoveryTestCase` using the existing Master integration test framework and H2.
- The rollback matrix covers nine transitions from RUNNING_EXECUTION, READY_STOP, and READY_PAUSE to SUCCESS, FAILURE, STOP, or PAUSE. It injects failures in the workflow update, serial-command deletion, and `beforeCommit`, then checks rollback, retry, and repeated events. These cases use real handlers and transactions with a mocked execution graph and event bus.
- Separate Master tests verify automatic retry through the real event bus: SUCCESS with two SERIAL_WAIT instances, and FAILURE, STOP, and PAUSE with parallel workflows. They check final workflow/task states and resource release without restarting the Master or manually replaying events.
- Additional tests cover enclosing REQUIRED transactions, duplicate finish calls, callback failures, metrics, and finalize publication attempts.
The original reproduction fails on unmodified production code: the database rolls back while the shared instance stays final. It passes with this change.
Focused clean builds passed 35 selected cases in both Surefire executions on JDK 8 and JDK 11. An earlier JDK 11 build passed 53 cases covering transaction callback boundaries and related workflow regressions with the same production change. These selections overlap; they are not a single full-suite run. Spotless checks and JaCoCo were enabled, and `spotless:apply` was run before testing.
To run the full workflow-finish and related regression selection:
```sh
./mvnw -B -ntp -pl dolphinscheduler-master -am \
-Dtest=WorkflowFinishPersistenceRecoveryTestCase,WorkflowStartSerialStrategyTestCase,WorkflowInstanceMetricsTestCase,WorkflowBackfillTestCase,WorkflowStartSubWorkflowTestCase \
-Dsurefire.failIfNoSpecifiedTests=false clean test
```
The tests inject exceptions into real H2 transactions; they do not simulate a database-server outage. The full project test suite was not run.
## Pull Request Notice
[Pull Request Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-18721][Master] Keep workflow finish retryable after transaction rollback [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #18722:
URL: https://github.com/apache/dolphinscheduler/pull/18722#issuecomment-6282744889
Could we simplify `workflowFinish()` around the current lifecycle-event dispatch path, which does not have an enclosing transaction, instead of introducing `WorkflowFinishTransactionSynchronization`?
The flow could be:
1. Copy the shared `WorkflowInstance`.
2. Inside `transactionTemplate.execute()`, update the copy's final state/end time, persist it, and delete the serial command when applicable.
3. After `execute()` returns successfully, update the shared instance, record the completion metric, and publish the finalize event. Keep the metric exception isolated so it cannot prevent finalize publication.
The database-operation logs can stay inside `execute()`, which also avoids returning a `serialCommandDeleted` flag just for logging. Their wording should describe operations executed within the transaction, rather than claim that the transaction has committed. Any final completion log should remain after the transaction returns.
For this scope, I do not think an additional `isActualTransactionActive()` guard is necessary; we can document the existing transaction boundary. This would remove the synchronization scan, custom synchronization class, and `completionAttempted` flag while retaining rollback/retry coverage for the current event-dispatch path.
This is deliberately a narrower contract, not an equivalent refactor: it would not retain the additional guarantees for an enclosing `REQUIRED` transaction or for another `afterCommit` callback throwing after the database has committed. Could we scope the implementation and tests accordingly, while retaining the rollback/retry and real-event-bus serial-queue recovery tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-18721][Master] Keep workflow finish retryable after transaction rollback [dolphinscheduler]
Posted by "ThreeFire1 (via GitHub)" <gi...@apache.org>.
ThreeFire1 commented on PR #18722:
URL: https://github.com/apache/dolphinscheduler/pull/18722#issuecomment-6286029224
Thanks, I've simplified the implementation around the current event-dispatch path as suggested. Shared state, metrics, and finalize publication now follow a successful return from `execute()`, with metric failures handled separately. I removed the custom transaction synchronization and the tests for the additional transaction guarantees. Rollback/retry, completion ordering, and real-event-bus serial-queue recovery tests are retained. All 57 selected tests pass on JDK 8 and 11.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-18721][Master] Keep workflow finish retryable after transaction rollback [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #18722:
URL: https://github.com/apache/dolphinscheduler/pull/18722#discussion_r4332613801
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/statemachine/AbstractWorkflowStateAction.java:
##########
@@ -153,25 +154,34 @@ protected void tryToTriggerSuccessorsAfterTaskFinish(final IWorkflowExecution wo
protected void workflowFinish(final IWorkflowExecution workflowExecution,
final WorkflowExecutionStatus workflowExecutionStatus) {
- // todo: add transaction configuration in lifecycle event, all sync lifecycle should be in transaction
+ final WorkflowInstance workflowInstance = workflowExecution.getWorkflowInstance();
+ final WorkflowInstance workflowInstanceToUpdate = new WorkflowInstance();
+ BeanUtils.copyProperties(workflowInstance, workflowInstanceToUpdate);
+ // Lifecycle events are dispatched without an enclosing transaction, so execute commits before returning.
transactionTemplate.execute(status -> {
- final WorkflowInstance workflowInstance = workflowExecution.getWorkflowInstance();
- workflowInstance.setEndTime(new Date());
- transformWorkflowInstanceState(workflowExecution, workflowExecutionStatus);
- WorkflowInstanceMetrics.recordWorkflowInstanceFinish(
- workflowExecutionStatus,
- workflowInstance.getWorkflowDefinitionCode());
- if (workflowExecution.getWorkflowExecuteContext().getWorkflowDefinition().getExecutionType()
- .isSerial()) {
- if (serialCommandDao.deleteByWorkflowInstanceId(workflowInstance.getId()) > 0) {
- log.info("Success clear SerialCommand for WorkflowExecuteRunnable: {}",
- workflowExecution.getName());
- }
+ workflowInstanceToUpdate.setState(workflowExecutionStatus);
+ workflowInstanceToUpdate.setEndTime(new Date());
+ workflowInstanceDao.updateById(workflowInstanceToUpdate);
+ if (workflowExecution.getWorkflowExecuteContext().getWorkflowDefinition().getExecutionType().isSerial()
+ && serialCommandDao.deleteByWorkflowInstanceId(workflowInstance.getId()) > 0) {
+ log.info("Deleted SerialCommand within workflow finish transaction for WorkflowExecuteRunnable: {}",
+ workflowExecution.getName());
}
- workflowExecution.getWorkflowEventBus()
- .publish(WorkflowFinalizeLifecycleEvent.of(workflowExecution));
return null;
});
+ final WorkflowExecutionStatus originState = workflowInstance.getState();
+ workflowInstance.setState(workflowInstanceToUpdate.getState());
+ workflowInstance.setEndTime(workflowInstanceToUpdate.getEndTime());
+ log.info("Success set WorkflowExecuteRunnable: {} state from: {} to {}",
+ workflowInstance.getName(), originState.name(), workflowInstance.getState().name());
+ try {
+ WorkflowInstanceMetrics.recordWorkflowInstanceFinish(workflowInstance.getState(),
+ workflowInstance.getWorkflowDefinitionCode());
+ } catch (Exception ex) {
+ // Metrics must not prevent releasing a workflow whose transaction has already committed.
+ log.warn("Record completion metric failed for workflow: {}", workflowExecution.getName(), ex);
+ }
Review Comment:
NIT: We can use another PR to simplify this. Since most of the state changes here involve updating the database and updating memory, we can move the memory updates into a separate method, similar to the implementation for task instance updates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-18721][Master] Keep workflow finish retryable after transaction rollback [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #18722:
URL: https://github.com/apache/dolphinscheduler/pull/18722#discussion_r4332613801
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/workflow/statemachine/AbstractWorkflowStateAction.java:
##########
@@ -153,25 +154,34 @@ protected void tryToTriggerSuccessorsAfterTaskFinish(final IWorkflowExecution wo
protected void workflowFinish(final IWorkflowExecution workflowExecution,
final WorkflowExecutionStatus workflowExecutionStatus) {
- // todo: add transaction configuration in lifecycle event, all sync lifecycle should be in transaction
+ final WorkflowInstance workflowInstance = workflowExecution.getWorkflowInstance();
+ final WorkflowInstance workflowInstanceToUpdate = new WorkflowInstance();
+ BeanUtils.copyProperties(workflowInstance, workflowInstanceToUpdate);
+ // Lifecycle events are dispatched without an enclosing transaction, so execute commits before returning.
transactionTemplate.execute(status -> {
- final WorkflowInstance workflowInstance = workflowExecution.getWorkflowInstance();
- workflowInstance.setEndTime(new Date());
- transformWorkflowInstanceState(workflowExecution, workflowExecutionStatus);
- WorkflowInstanceMetrics.recordWorkflowInstanceFinish(
- workflowExecutionStatus,
- workflowInstance.getWorkflowDefinitionCode());
- if (workflowExecution.getWorkflowExecuteContext().getWorkflowDefinition().getExecutionType()
- .isSerial()) {
- if (serialCommandDao.deleteByWorkflowInstanceId(workflowInstance.getId()) > 0) {
- log.info("Success clear SerialCommand for WorkflowExecuteRunnable: {}",
- workflowExecution.getName());
- }
+ workflowInstanceToUpdate.setState(workflowExecutionStatus);
+ workflowInstanceToUpdate.setEndTime(new Date());
+ workflowInstanceDao.updateById(workflowInstanceToUpdate);
+ if (workflowExecution.getWorkflowExecuteContext().getWorkflowDefinition().getExecutionType().isSerial()
+ && serialCommandDao.deleteByWorkflowInstanceId(workflowInstance.getId()) > 0) {
+ log.info("Deleted SerialCommand within workflow finish transaction for WorkflowExecuteRunnable: {}",
+ workflowExecution.getName());
}
- workflowExecution.getWorkflowEventBus()
- .publish(WorkflowFinalizeLifecycleEvent.of(workflowExecution));
return null;
});
+ final WorkflowExecutionStatus originState = workflowInstance.getState();
+ workflowInstance.setState(workflowInstanceToUpdate.getState());
+ workflowInstance.setEndTime(workflowInstanceToUpdate.getEndTime());
+ log.info("Success set WorkflowExecuteRunnable: {} state from: {} to {}",
+ workflowInstance.getName(), originState.name(), workflowInstance.getState().name());
+ try {
+ WorkflowInstanceMetrics.recordWorkflowInstanceFinish(workflowInstance.getState(),
+ workflowInstance.getWorkflowDefinitionCode());
+ } catch (Exception ex) {
+ // Metrics must not prevent releasing a workflow whose transaction has already committed.
+ log.warn("Record completion metric failed for workflow: {}", workflowExecution.getName(), ex);
+ }
Review Comment:
NIT: We can use another PR to simplify this. Since most of the state changes here involve updating the database and updating memory, we can move the memory updates into a separate method, similar to the implementation for task instance updates.
At the same time, exception handling can be placed in `recordWorkflowInstanceFinish`; I believe that any exceptions that occur while recording metrics should not affect the upper layers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-18721][Master] Keep workflow finish retryable after transaction rollback [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS merged PR #18722:
URL: https://github.com/apache/dolphinscheduler/pull/18722
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org