humanmade / Cavalcade

A better wp-cron. Horizontally scalable, works perfectly with multisite.
https://engineering.hmn.md/projects/cavalcade/
Other
515 stars 46 forks source link

Improved support for local MySQL time #55

Closed boonebgorges closed 6 years ago

boonebgorges commented 6 years ago

I'm setting up Cavalcade in a hosting environment where MySQL is running in local time - UTC-5, in my case. This is causing a mismatch: WP and mu-plugins/cavalcade are recording the job (properly) in UTC, while the runner's nextrun < NOW() query is offset by 5 hours, causing jobs to be run 5 hours late.

I realize that the ideal situation is to use UTC everywhere, but I don't control the environment, and certain parts of the infrastructure are shared with a couple other resources.

I've fixed this in my local codebase by putting some offset logic in the /connector/namespace.php methods. I'm wondering whether you'd be interested in an enhancement that makes it possible to set a flag (a constant or something) so that Cavalcade will perform this offset automatically if the flag is set.

Feel free to close if you feel like this would be reinforcing a bad practice! And thanks for the cool tool 😎

rmccue commented 6 years ago

Is your database server's time offset 5 hours, or is the timezone itself just set to UTC-5? From what I can see, datetime columns (as used in Cavalcade) are always stored as UTC, so it's only when we cast it in order to compare to now() that it's an issue.

What I think you might be able to do is use per-connection timezones in both WordPress and Cavalcade-Runner to enforce the use of UTC. For Cavalcade, you should be able to do that with:

HM\Cavalcade\Runner::instance()->hooks->register( 'Runner.connect_to_db.connected', function ( $db ) {
    $db->exec( 'SET time_zone = "+00:00"' );
} );

(You may want to do this on the WordPress side; the code for that is left as an exercise for the reader.)

Does that solve the issue for you? We could potentially look at building this in to Cavalcade, or changing NOW() to use date( 'Y-m-d H:i:s' ) (although I'm unsure if the latter would affect query caching).

rmccue commented 6 years ago

I did some local testing.

So it appears that indeed the time_zone variable does affect NOW() but does not affect the datetime column, so setting the time_zone variable on connect should work correctly here.

boonebgorges commented 6 years ago

@rmccue Thanks for this - you're right that time_zone is the culprit. From what you've suggested here (which aligns with my tests), setting time_zone in the runner seems to be the crux of the issue.

discoinfiltrator commented 6 years ago

I've just run into this issue as well.

The proposed fix, adding:

HM\Cavalcade\Runner::instance()->hooks->register( 'Runner.connect_to_db.connected', function ( $db ) {
    $db->exec( 'SET time_zone = "+00:00"' );
} );

didn't work for me. Adding:

$this->db->exec( 'SET time_zone = "+00:00"' );

after we create the PDO instance works though.

rmccue commented 6 years ago

@discoinfiltrator These should be equivalent, so not sure what's happening there. You may need to check that your callback is actually being executed; might be that where you have that code is not being run?

discoinfiltrator commented 6 years ago

Ah, my bad. I had added the code snippet incorrectly. Both are solving the problem for me.

rmccue commented 6 years ago

Fantastic to hear.

I'd be happy to add this to the Cavalcade Runner code itself since it simply makes things consistent with no downsides for others, if anyone wants to open a PR :)

discoinfiltrator commented 6 years ago

Sounds great. I've created: https://github.com/humanmade/Cavalcade-Runner/pull/44