The job stops if the time in the system changes at an unfortunate moment

[psixdev]

Hi, I have been watching a server for quite some time, where sometimes the system time changes by a few seconds. Several times I came across a situation where one of the jobs stopped just at the moment of time change. I researched the library and found the problem:
If during the execution of the getTimeout function (https://github.com/kelektiv/node-cron/blob/master/lib/time.js#L201) between the calculation of the sendAt time and the current time, the system time moves a few seconds forward, then it can it turns out that timeout is calculated negative and the job will stop with this line (https://github.com/kelektiv/node-cron/blob/master/lib/job.js#L190).

I created a pr in my repository fork, in which, using the synon, I simulated the time change at the right moment. My job starts every 5 seconds and I move the time 7 seconds forward, after which the job stops.
https://github.com/psixdev/node-cron/pull/1/files

I understand that an accidental change in the time on the server is an unlikely event, but I believe that the problem really exists and it is serious. I suppose a similar bug can occur during the transition to daylight saving time (or vice versa), if this transition accidentally coincides with the call to the getTImeout function.

What do you think about this problem?

[intcreator]

hey I think this might be fixed by #639 because there's some code there that checks if the time changed unexpectedly and it runs the job if so. want to try running your example on that branch? if it still fails I think that branch adds some utility functions we can use to address this

[GijsvandenHoven]

Hey, I noticed my PR was mentioned so I had a cursory glance at this issue.

I'm a bit pessimistic that it wouldn't fix this problem. The main detection method is in _getNextDateFrom is spotting gaps in DateTime's representation of time units. e.g. a Sudden jump from 2:00 to 3:00.

While sendAt() might call _getNextDateFrom() in this code block:

node-cron/lib/time.js

Line 183 in d07333f

if (isNaN(i) || i < 0) {

, These are system corrections instead of real gaps in DateTime itself. I don't think they would be picked up by adding / subtracting time to DateTime objects, since they would still behave.

Additionally, because my PR primarily thought about daylight savings time, it focuses on minute and hour changes. Adjustments on the order of seconds aren't really detected or scanned. (Though this is easier to adjust than the fundamental problem with the detection method, would definitely take a bit of extra changes though).

[GijsvandenHoven]

One way to somehow evade the root cause is using a monotonic clock (https://stackoverflow.com/questions/46964779/monotonically-increasing-time-in-node-js) , but that's probably going to be very difficult to work with in combination with DateTime, because

The time returned from process.hrtime() is] relative to an arbitrary time in the past, and not related to the time of day and therefore not subject to clock drift.

Meaning it can't be used interchangeably with DateTime.