Skip to content

[Fix-18538][Master] Schedule task retry at endTime + retryInterval - #18539

Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
apache:devfrom
SEPURI-SAI-KRISHNA:Fix-18538
Open

[Fix-18538][Master] Schedule task retry at endTime + retryInterval#18539
SEPURI-SAI-KRISHNA wants to merge 1 commit into
apache:devfrom
SEPURI-SAI-KRISHNA:Fix-18538

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown

Was this PR generated or assisted by AI?

YES. The faulty expression was located and the fix and unit tests were drafted with AI
assistance (Claude Code); the reasoning, the failover impact analysis and the final code
were reviewed and verified by me.

Purpose of the pull request

Closes #18538.

TaskRetryLifecycleEvent#of computes the delay before a failed task is retried. The
intent is to retry at endTime + retryInterval, so the remaining delay is
retryInterval - (now - endTime). The expression instead evaluated to
retryInterval + (now - endTime) — the elapsed time was added rather than subtracted:

final long remainingTime =
        TimeUnit.MINUTES.toMillis(delayTime) + System.currentTimeMillis() - taskInstance.getEndTime().getTime();

In the common path now - endTime is a few milliseconds, so the error is invisible. It
matters when the retry event is created long after the task actually ended — most
notably on master failover: WorkflowFailoverCommandHandler rebuilds the execution
graph from the existing task instances, so a task left in FAILURE with retries
remaining keeps its old endTime; TaskFailureStateAction#onStartEvent then republishes
the failure event with that stale endTime, and the retry gets postponed by the whole
outage duration on top of the configured interval instead of firing immediately.

Brief change log

  • TaskRetryLifecycleEvent#of: subtract the elapsed time since endTime from the retry
    interval, and clamp the result to 0 so an already-overdue retry is triggered
    immediately.
  • Renamed the local delayTime to retryInterval — it holds the configured retry
    interval and shadowed the inherited AbstractDelayEvent#delayTime field, which is what
    made the wrong expression easy to miss.
  • Added TaskRetryLifecycleEventTest.

Verify this pull request

This change added tests and can be verified as follows:

  • Added TaskRetryLifecycleEventTest with three cases:
    • retry interval not elapsed → the event is delayed for the remaining interval;
    • retry interval already elapsed (the failover case, endTime two hours in the
      past) → the event is ready immediately. This case fails on dev with a delay of
      ~2h05m;
    • retry times exhausted → IllegalStateException, guarding the existing checkState.
./mvnw -pl dolphinscheduler-master -am clean test \
    -Dtest=TaskRetryLifecycleEventTest \
    -Dsurefire.failIfNoSpecifiedTests=false

Verified locally:

  • The new tests fail on dev and pass with this change. On dev the failover case reports
    a delay of 7499999 ms (~2h05m) where 0 is expected, and the normal case reports
    330016 ms where at most 270000 ms is expected.
  • The full dolphinscheduler-master suite was run (98 tests). The only non-passing test was
    WorkflowStartTimeoutTestCase#testStartWorkflow_withTimeoutWarnFailedTask, which is
    unrelated timing flakiness under the 4-way parallel forks: its fixture defines no
    failRetryTimes, so maxRetryTimes is 0, isTaskInstanceCanRetry() is false and
    TaskRetryLifecycleEvent#of is never reached. The class passes 5/5 when re-run in
    isolation.

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

🤖 Generated with Claude Code

- Subtract the elapsed time since the task ended instead of adding it
  - Retry immediately when the retry interval has already passed
  - Add TaskRetryLifecycleEventTest covering both cases

  Closes apache#18538

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@boring-cyborg

boring-cyborg Bot commented Aug 7, 2026

Copy link
Copy Markdown

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [Master] Task retry is delayed by the elapsed time instead of being scheduled at (endTime + retryInterval)

1 participant