hellodml opened a new pull request, #18573:
URL: https://github.com/apache/dolphinscheduler/pull/18573
## Was this PR generated or assisted by AI?
YES. The investigation, the one-line change and the unit test were produced
with the help of Claude (Anthropic), working from production logs of my own
3.4.2 deployment. Everything was compiled and tested locally before submitting,
and the fix direction was proposed by @ruanwenjun in #18570.
## Purpose of the pull request
Closes #18570.
`CommandDuplicateHandleException` is raised inside a `CompletableFuture`
chain and therefore reaches `CommandEngine.bootstrapError` wrapped in
`CompletionException`. The direct `instanceof` check cannot see through the
wrapper, so the duplicate branch is never taken and the workflow instance is
force-failed instead.
Why this matters beyond a wrong log line:
`forceUpdateWorkflowInstanceState(..., FAILURE)` only updates the DB row. It
does not remove the execution from `workflowRepository` nor deregister its
event bus. The still-running **first** execution then has a FAILURE row
underneath it:
```
UnsupportedOperationException: The WorkflowInstance: 516982 state is
FAILURE, no need to notify
```
so it can never complete and never leaves the in-memory repository. Since
`MasterServerLoadProtection` reads
```java
int currentWorkflowInstanceCount = workflowRepository.getAll().size();
```
the count can no longer fall and `CommandEngine` keeps refusing to consume
commands. In our production incident the logged count sat at exactly 25 against
a limit of 20 for two hours (`25 x 7194` log lines in a single hour, no other
value), 553 of the 573 workflow instances created in that window never got a
single task instance row, and only a restart recovered it.
## Brief change log
- `CommandEngine.bootstrapError`: use `ExceptionUtils.throwableOfType(...)`
instead of `instanceof` so the exception is still recognised when wrapped.
`ExceptionUtils` was already imported in this file, so no new import is
required.
- Add `CommandEngineTest` covering the wrapped, doubly-nested and unwrapped
cases.
## Verify this pull request
Covered by the new unit test. Reverting only the production line makes
exactly the two wrapped cases fail while the unwrapped case still passes:
```
with fix: Tests run: 3, Failures: 0, Errors: 0 BUILD SUCCESS
without fix: Tests run: 3, Failures: 2, Errors: 0 BUILD FAILURE
CommandEngineTest.bootstrapErrorShouldNotFailWorkflowWhenDuplicateExceptionIsWrapped
CommandEngineTest.bootstrapErrorShouldNotFailWorkflowWhenDuplicateExceptionIsNestedTwice
```
Verified against the `dev` branch with Maven 3.9.6 / JDK 8u492 (`mvn -pl
dolphinscheduler-master -am install -DskipTests`, then `mvn -pl
dolphinscheduler-master test -Dtest=CommandEngineTest`).
The failure is a Mockito verification failure on
`workflowInstanceDao.forceUpdateWorkflowInstanceState`, i.e. it points directly
at the workflow being wrongly force-failed.
## Notes for reviewers
- `bootstrapError` is private, so the test invokes it through
`ReflectionTestUtils`. If that is not acceptable here, dropping `private` and
marking it visible-for-testing works equally well and needs no test change.
- `transactionTemplate` is mocked to run its callback inline. Without that
the non-duplicate branch throws NPE instead of producing a clear verification
failure; the test still distinguishes fixed from unfixed either way.
- This change only addresses the duplicate command being misclassified. I
have **not** verified whether it also resolves `IllegalStateException:
WorkflowExecuteRunnable(...) already registered` (reproducible by re-running
individual tasks of a finished workflow instance in quick succession), nor
whether executions already wedged in `workflowRepository` get cleaned up.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
