Improve logic for setting next run time in `Job::reschedule()`.

[peterwilsoncc]

Job::reschedule() uses the value nextrun + interval when calculating the new time to run the job.

https://github.com/humanmade/Cavalcade-Runner/blob/master/inc/class-job.php#L84

This causes the jobs to run constantly if the task is originally scheduled before the current timestamp until nextrun + interval is after the current time stamp.

To reproduce schedule an event with wp_schedule_event( 0, 'hourly', 'pwcc_every_hour' ) as a shorthand way of setting the task to start immediately and then once every hour. The outcome is:

• the action pwcc_every_hour fires 426,388 consecutively
• the action pwcc_every_hour continues firing once each hour

A better approach would be to record the start time when the lock is obtained and add the interval to that when rescheduling.

[edubacco]

I have the same problem. I've stopped my cavalcade runner for a while (some days) to do maintenance.
Now I have lots of cron jobs running everytime, they are scheduled hourly.

To fix the problem, i've changed this line with
$this->nextrun = date( MYSQL_DATE_FORMAT, time() + $this->interval );

There is this PR opened about this problem too

[archon810]

The patch has been here for 1.5 years. Is anyone going to merge it?

[roborourke]

Fixed in #64 and tag 1.0.1

[maciejmackowiak]

@roborourke How forcing the nextrun calculation to use UTC solves this issue?
Correct me if I'm wrong, but I believe the case created by @peterwilsoncc will still be an issue with forced UTC.

[roborourke]

@maciejmackowiak quite right, I misread the issue as it was relating to the same section of code #64 dealt with.

[r-a-y]

We're running into a similar situation on a multisite install.

Our install has a lot of cron jobs scheduled (mostly because of plugins needing syncing functionality). Meaning the number of workers cannot adequately handle the number of jobs. This causes a backlog to occur and this backlog will keep increasing until we purge these older waiting jobs.

We could increase the number of workers used, but that's somewhat of a workaround. It would be nice if a hook was added to the rescheduling portion of the runner so we could override the nextrun + interval behavior if needed.

Perhaps after this block:

Cavalcade-Runner/inc/class-job.php

Lines 86 to 91 in 0dfb42d

	public function reschedule() {
	$date = new DateTime( $this->nextrun, new DateTimeZone( 'UTC' ) );
	$date->add( new DateInterval( "PT{$this->interval}S" ) );
	$this->nextrun = $date->format( MYSQL_DATE_FORMAT );
	$this->status = 'waiting';

We could add the following:

$this->nextrun = Runner::instance()->hooks->run( 'Job.reschedule.nextrun', $this->nextrun, $this );

[boonebgorges]

I'd like to chime in and connect this issue to the thread at humanmade/Cavalcade#74 (comment). Ryan states in his follow-up that this rescheduling behavior is intentional because it provides resiliency in case the Runner crashes and then later restarts, and adds:

So long as everything is in UTC, there should be no issue with this, as the clock (generally) monotonically increases.

But the problem described in this ticket is not with monotonicity. The scheduled events in the backlog being described here have timestamps that are correctly ordered. The problem is that the timestamps are all in the past, so that the nextrun + interval calculation never catches up to the current timestamp.

Changing to time() + interval would make the problem go away, but I understand there may be concern that certain tasks would be run less frequently than the scheduling "client" (whatever code is calling wp_schedule_event()) expects it to be. In my experience, this change would not matter for the vast majority of WP scheduled tasks. For example, if a plugin checks an RSS feed every hour, and it misses three checks due to a lack of workers, it is not important that those three checks be "made up".

That being said, a more conservative option might be for the runner to refuse to reschedule a task if an identical task (same hook + site + args) is already scheduled with a past timestamp (ie, it's due and waiting for available runners).

[joehoyle]

I believe we should at a minimum add a filter for the "catchup / reschedule behavior". I think there are 3 options:

1. Guaranteed reschedule (current behavior): jobs are always run at last_schedule_date + interval. This is best suited when you need jobs to always have 1 run per interval. E.g. an hourly job will always run 24 times for each day, regardless of pauses / lag etc.
2. Skip ahead: Any past-due job is run, and then rescheduled at "current_date + interval" rounded up to the nearest interval to prevent schedule drift.
3. Skip ahead guaranteed: Any past-due job is run, and then rescheduled at "current_date - interval" rounded up to the nearest interval to prevent schedule drift. This would mean if you had a daily job, and it's 3 days past due, it would be run once, and the reschedule for "today" which may be already due, and therefor run a maximum of 1 additional time.

I think we could also support specifying any of these options in the job's args (merging them with hook args) to set the behavior on a per-job basis.

For Altis Local Server, where you stop the cavalcade-runner for large periods of time, I'd expect us to default this to one of the latter two options.

@rmccue what do you think?

[peterwilsoncc]

For Altis Local Server, where you stop the cavalcade-runner for large periods of time, I'd expect us to default this to one of the latter two options.

@joehoyle Do you think it's worth changing the default to time() + interval in the runner when WP_ENVIRONMENT_TYPE is set to local? Possibly something more complex if you wish to avoid drift.

I usually hack it in my local as I've hit API limits more than once when I've not booted my local environment for an extended period.