You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org> on 2026/08/08 03:57:43 UTC
[PR] [Fix-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
SEPURI-SAI-KRISHNA opened a new pull request, #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541
<!--Thanks very much for contributing to Apache DolphinScheduler, we are happy that you want to help us improve DolphinScheduler! -->
## Was this PR generated or assisted by AI?
YES. The missing field resets were found and the fix and unit tests were drafted with AI
assistance (Claude Code); the behaviour, the comparison against the sibling factories and
the final code were reviewed and verified by me.
## Purpose of the pull request
Closes #18540.
`FailedRecoverTaskInstanceFactory` recreates a `FAILURE` / `KILL` task instance when a
workflow is recovered with "Recover failed tasks". It clones the old instance, clears part
of the runtime state and inserts it as a new row — but it never resets `retryTimes`,
`startTime`, `endTime`, `pid` or `alertFlag`, all of which `cloneTaskInstance` copies.
Because a task that failed after exhausting its retries has `retryTimes == maxRetryTimes`,
and `TaskExecution#isTaskInstanceCanRetry()` is `retryTimes < maxRetryTimes`, the
recreated instance starts with a fully consumed retry budget and is never retried on the
recovery run, however many retries the task definition asks for.
The two sibling factories already do this correctly — `FirstRunTaskInstanceFactory` sets
`retryTimes(0)`, `startTime(null)`, `endTime(null)`, `alertFlag(NO)`, and
`RetryTaskInstanceFactory` sets `startTime(null)`, `endTime(null)`, `pid(0)` while
deliberately incrementing `retryTimes`. This change brings the recover factory in line
with them.
The stale `startTime` / `endTime` are user visible too: until the task actually starts, the
new instance is shown with the previous attempt's timestamps, and
`TaskInstanceServiceImpl#queryTaskListPaging` derives its duration from them.
## Brief change log
- `FailedRecoverTaskInstanceFactory#createTaskInstance`: reset `retryTimes` to `0` and
clear `startTime`, `endTime`, `pid` and `alertFlag` on the recreated task instance.
- Added `FailedRecoverTaskInstanceFactoryTest`.
## Verify this pull request
This change added tests and can be verified as follows:
- Added `FailedRecoverTaskInstanceFactoryTest` with three cases: the retry budget is
restored, the runtime state of the failed attempt is cleared, and the origin instance is
still marked invalid while the new one is inserted.
```bash
./mvnw -pl dolphinscheduler-master -am clean test \
-Dtest=FailedRecoverTaskInstanceFactoryTest \
-Dsurefire.failIfNoSpecifiedTests=false
```
Verified locally:
- The new tests fail on `dev` and pass with this change. On `dev` the recreated instance
reports `retryTimes = 3` where `0` is expected, and keeps the `startTime` of the failed
attempt where `null` is expected.
- The full `dolphinscheduler-master` suite passes: 98 tests, 0 failures, 0 errors.
- `./mvnw -pl dolphinscheduler-master spotless:check` passes.
## Pull Request Notice
[Pull Request Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
If your pull request contains incompatible change, you should also add it to `docs/docs/en/guide/upgrade/incompatible.md`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541#issuecomment-5237892853
CI is green now — both dolphinscheduler-master shards pass on Java 8 and 11 after the re-run (the earlier failure was Failed to load ApplicationContext, unrelated flake). The only red check is dead-link, which fails on dev nightly too.
Could you take another look when you have a moment? The appLink reset you asked for is reverted with the reasoning above — happy to open a separate issue for the AbstractRemoteTask resubmit problem if you'd like it tracked.
--
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-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541#issuecomment-5237536662
The Unit-Test (dolphinscheduler-master | Java 11) failure on this run is WorkflowInstancePauseTestCase#testPauseWorkflow_with_subWorkflowTask_success » IllegalState Failed to load ApplicationContext— a Spring context boot failure, not an assertion, and a different test from the previous run.
I verified the appLink revert is correct with a controlled local run of WorkflowInstanceRecoverStopTestCase: with setAppLink(null) applied it fails deterministically in isolation; with it removed it passes 2/2. So the current branch state is right.
Could you re-run the failed job? I believe the master integration suite is flaky under the
4-way parallel forks.
--
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-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541#issuecomment-5236515749
Good catch, thanks — confirmed and fixed in the follow-up commit.
I traced the chain to be sure:
- `AbstractTaskInstanceFactory#cloneTaskInstance` copies `appLink`
- `TaskExecutionContextBuilder#buildAppIds` sets `taskExecutionContext.setAppIds(taskInstance.getAppLink())`
- `AbstractRemoteTask#handle` returns early when `appIds` is not empty:
```java
if (StringUtils.isNotEmpty(taskRequest.getAppIds())) {
setAppIds(taskRequest.getAppIds());
trackApplicationStatus();
return;
}
submitApplication();
```
So recovering a failed Spark/Flink/EMR-style task would never resubmit — it would attach to
the already-dead application from the previous attempt. Same bug class as the other fields
in this PR, so it belongs here.
`FailedRecoverTaskInstanceFactory#createTaskInstance` now clears `appLink`, and
`FailedRecoverTaskInstanceFactoryTest#testCreateTaskInstance_clearPreviousRuntimeState`
asserts it. I verified the assertion fails without the reset
(`expected: null but was: application_1717430400000_0001`) and passes with it.
## One related question
While checking this I noticed the same gap in `RetryTaskInstanceFactory#createTaskInstance` —
it resets `pid`, `startTime`, `endTime` and increments `retryTimes`, but does not clear
`appLink` either. A retry is also a fresh attempt, so it looks like it would attach to the
dead application in the same way.
`FailoverTaskInstanceFactory` keeping `appLink` is clearly deliberate — the remote
application is still alive there and the new attempt should take it over rather than
resubmit — so the intent seems to be "failover keeps `appLink`, every other new attempt
clears it".
Would you like me to include the `RetryTaskInstanceFactory` fix in this PR, or should I
open a separate issue/PR for it to keep this one scoped?
--
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-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541#issuecomment-5238233439
Awesome work, congrats on your first merged pull request!
--
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-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541#issuecomment-5237031598
I had to revert the `appLink` reset — it regresses sub-workflow recovery, and CI caught it.
`Unit-Test (dolphinscheduler-master | Java 11)` failed on
`WorkflowInstanceRecoverStopTestCase#testRecoverStoppedWorkflow_with_subWorkflowTask_success`
with the commit that cleared `appLink`
([job](https://github.com/apache/dolphinscheduler/actions/runs/31360840016/job/93369977204)).
The reason is that `appLink` is overloaded — it is not only a remote application id:
- For `AbstractRemoteTask` (Spark/Flink/EMR) it holds the remote application id, which is the
case you pointed at.
- For `SubWorkflowLogicTask` it holds the serialized `SubWorkflowLogicTaskRuntimeContext`,
i.e. the **sub workflow instance id**:
```java
// SubWorkflowLogicTask constructor
this.subWorkflowLogicTaskRuntimeContext = JSONUtils.parseObject(
taskExecutionContext.getAppIds(),
SubWorkflowLogicTaskRuntimeContext.class);
```
```java
private SubWorkflowLogicTaskRuntimeContext initializeSubWorkflowInstance() {
if (subWorkflowLogicTaskRuntimeContext == null) {
return triggerNewSubWorkflow();
}
switch (...getCommand().getCommandType()) {
case RECOVER_TOLERANCE_FAULT_PROCESS: return recoverFromFaultTolerantTasks();
case RECOVER_SUSPENDED_PROCESS: return recoverFromSuspendTasks();
case START_FAILURE_TASK_PROCESS: return recoverFromFailedTasks();
default: return triggerNewSubWorkflow();
}
}
```
Clearing `appLink` makes `subWorkflowLogicTaskRuntimeContext` null, so the recreated task
short-circuits to `triggerNewSubWorkflow()` and spawns a **duplicate** sub workflow instead
of recovering the existing one. Note this hits `START_FAILURE_TASK_PROCESS` too — the very
command this PR is about — not just `RECOVER_SUSPENDED_PROCESS`.
`RecoverSuspendWorkflowCommandHandler extends RecoverFailureTaskCommandHandler`, so both
recovery commands go through `FailedRecoverTaskInstanceFactory` and both are affected.
So this PR now keeps `appLink` as before, and I added a comment on the test assertion
recording *why* it must be preserved, so the next person doesn't remove it for the same
reason I did. `WorkflowInstanceRecoverStopTestCase` (2 tests) and
`FailedRecoverTaskInstanceFactoryTest` (3 tests) both pass locally after the revert.
## On the underlying remote-task problem
Your analysis of `AbstractRemoteTask#handle` is still correct — a recovered Spark/Flink task
should not attach to the dead application of the previous attempt. But it can't be fixed by
blanket-clearing `appLink` in the factory, because the same field carries the sub workflow
runtime context. It needs something that distinguishes "resume this attempt" from "this is a
new attempt", e.g. only clearing `appIds` for `AbstractRemoteTask` when the context is built,
or giving the logic-task runtime context its own field instead of reusing `appLink`.
That looks like its own issue rather than something to fold in here — happy to open one and
work it if you agree. Would you prefer that, or should I keep trying to solve it inside this
PR?
--
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-18540][Master] Reset the runtime state when recreating a failed task instance [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS merged PR #18541:
URL: https://github.com/apache/dolphinscheduler/pull/18541
--
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