Skip to content

fix: don't advance lastDate() for ticks skipped by waitForCompletion#1068

Open
chatman-media wants to merge 1 commit into
kelektiv:mainfrom
chatman-media:fix/last-execution-skipped-ticks
Open

fix: don't advance lastDate() for ticks skipped by waitForCompletion#1068
chatman-media wants to merge 1 commit into
kelektiv:mainfrom
chatman-media:fix/last-execution-skipped-ticks

Conversation

@chatman-media

Copy link
Copy Markdown

Description

When waitForCompletion is enabled and a callback is still running, the scheduled tick is skipped early in fireOnTick():

async fireOnTick() {
  // skip job if previous callback is still running
  if (this.waitForCompletion && this._isCallbackRunning) return;
  ...
}

However, this.lastExecution = new Date() was assigned at the call sites before invoking fireOnTick() (the normal timer path, plus the runOnInit and threshold-"execute immediately" paths). As a result lastExecution advanced on every tick check, even when the callback was skipped because the previous one was still running.

This means lastDate() / lastExecution reported the moment of the last tick check rather than the last real execution, which contradicts their documented meaning (lastDate: Provides the last execution date.) and is inconsistent with nextDate()/nextDates() which describe when an onTick actually activates.

Related Issue

Closes #1048

Motivation and Context

The fix moves the lastExecution assignment into fireOnTick(), placed right after the already-running guard, so it is only updated when a callback truly fires. The four call-site assignments are removed (the runOnInit, normal-tick, and threshold paths all call fireOnTick() immediately after, so firing behaviour is unchanged; the skipped path no longer advances the timestamp).

This keeps the existing documented semantics intact — confirmed by the pre-existing tests "should give the correct last execution date" and the #710 regression test, both of which still pass.

How Has This Been Tested?

Added a regression test under the waitForCompletion and job status tracking suite. With a 1.5s callback on a * * * * * * schedule and waitForCompletion: true:

The test fails on main (Expected: 1000, Received: 2000) and passes with the fix. Full suite: 162 passed. ESLint + Prettier clean, tsc build clean. Ran with TZ='Europe/Paris' as configured.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • If my change introduces a breaking change, I have added a ! after the type/scope in the title (n/a — not a breaking change).

When `waitForCompletion` is enabled and a callback is still running, the
scheduled tick is skipped in `fireOnTick()`. However `lastExecution` was
assigned right before calling `fireOnTick()` (and in `runOnInit`/threshold
paths), so it advanced even when no callback actually ran. This made
`lastDate()`/`lastExecution` report the time of the last tick check rather
than the last real execution, contradicting their documented meaning.

Move the `lastExecution` assignment into `fireOnTick()`, after the
already-running guard, so it only updates when the callback truly fires.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lastDate()/lastExecution does not reflect the last execution of the registered callback(s)

2 participants