[ThreeFire1]

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:

./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

[ruanwenjun]

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?

[ThreeFire1]

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.

[ruanwenjun]

LGTM

[ruanwenjun]

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.

[SbloodyS]

+1