[ThreeFire1]

Was this PR generated or assisted by AI?

YES. I used AI assistance for the investigation, code changes, regression tests, and PR description. I reviewed the issue description, the proposed fix, and the reported before-and-after test results.

Purpose of the pull request

Fixes #18707.

When persisting a task success result throws, the shared task instance is already marked SUCCESS. A retried success event is then skipped and can be acknowledged while the database still contains RUNNING_EXECUTION.

The old TaskResultEventHandler handled this class of failure after #10866 (for #10854) by restoring the task instance when persistence threw. Source comparison shows that this protection was not retained in the state-machine success path introduced in #16327. This PR restores the same retry guarantee by persisting a copy before updating the shared instance, rather than restoring it after an exception.

Brief change log

• Persist the success state, end time, and merged output variables using a copy of the task instance, then update the shared instance after the DAO call returns normally.
• Keep the persisted success state when sending the ACK fails; a repeated report can be acknowledged without persisting or publishing the topology event again.
• Add regression tests for persistence failure and recovery, repeated reports, output variables, event ordering, ACK send failure, and an uncertain commit result.
• Add a Master integration test with the real DAO and an isolated H2 database, verifying that retrying a failed success update lets a serial workflow finish.

The DAO's boolean return handling is unchanged. This patch does not add database read-back checks or broaden retry exception handling.

Verify this pull request

This change added TaskSuccessPersistenceRecoveryTest. It uses the real state actions and success handler, a mocked DAO with an independent persisted snapshot, and the real event bus for the retry case. The ACK failure case uses the real TaskExecutorClient with a mocked transport delegator.

TaskSuccessPersistenceRecoveryTestCase starts a serial workflow A → B through the existing Master integration test framework. A DAO spy throws once before A's first success update; all other calls use the real DAO. The test verifies the database state before failure, two attempts to persist A's success, exactly two successful task instances, workflow completion, and release of Master resources. No manual event replay or Master restart is used.

With the unmodified production code and a clean build, this test fails: the retry skips A's success event and the workflow remains RUNNING_EXECUTION. With the fix, the integration test passes.

Run the focused tests with:

./mvnw -B -ntp -pl dolphinscheduler-master -am \
  -Dtest=TaskSuccessPersistenceRecoveryTest,TaskSuccessLifecycleEventHandlerTest,TaskForceSuccessStateActionTest,TaskSuccessPersistenceRecoveryTestCase \
  -Dsurefire.failIfNoSpecifiedTests=false clean test

A clean local build on JDK 17 passed all 17 selected tests in both configured Surefire executions: 10 new recovery tests, 1 new Master integration test, 5 existing success handler tests, and 1 existing forced-success test. The 115-module dependency reactor completed successfully with Spotless checks and JaCoCo instrumentation enabled. spotless:apply was also run before testing.

The integration test uses real H2 persistence with an injected DAO exception; it does not exercise a real database outage, database-specific exception translation, or a deployed cluster. Local testing was limited to the 17 tests listed above; the full project test suite was not run.

Pull Request Notice

Pull Request Notice

[boring-cyborg]

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)

[ruanwenjun]

Other persistent events need to be handled in the same way. A utility method could be created here to handle them uniformly; however, to ensure recovery in the event of a database operation failure, all relevant sections of the program must be checked.

[ThreeFire1]

Other persistent events need to be handled in the same way. A utility method could be created here to handle them uniformly; however, to ensure recovery in the event of a database operation failure, all relevant sections of the program must be checked.

Thanks for the feedback! The local changes now cover the other task lifecycle updates through a shared helper that persists a copy before updating the shared instance.
I’m checking the related recovery paths and test coverage before pushing the update here.

[ThreeFire1]

@ruanwenjun Updated this PR to use a shared helper across the nine task lifecycle update paths. The tests cover failed database updates followed by retry and recovery, repeated reports, ACK failures, output variables, and task-group resource release. The Master/H2 tests also verify that workflows complete or reach the expected stopped/failed state after the injected update failure. These tests pass on JDK 8 and 11.

The broader review also found existing transaction-related recovery issues in workflow completion and Retry/Failover instance creation. These need changes beyond the current helper. Would you prefer to address them in separate follow-up PRs?

[ruanwenjun]

LGTM

[ruanwenjun]

@ruanwenjun Updated this PR to use a shared helper across the nine task lifecycle update paths. The tests cover failed database updates followed by retry and recovery, repeated reports, ACK failures, output variables, and task-group resource release. The Master/H2 tests also verify that workflows complete or reach the expected stopped/failed state after the injected update failure. These tests pass on JDK 8 and 11.

The broader review also found existing transaction-related recovery issues in workflow completion and Retry/Failover instance creation. These need changes beyond the current helper. Would you prefer to address them in separate follow-up PRs?

@ThreeFire1 Thanks for finding out this., use a separate PR LGTM.

[SbloodyS]

+1

[boring-cyborg]

Awesome work, congrats on your first merged pull request!