Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schedule next_run does not take daylight savings time into account #4025

Closed
3 tasks done
m0uka opened this issue Mar 27, 2022 · 6 comments
Closed
3 tasks done

Schedule next_run does not take daylight savings time into account #4025

m0uka opened this issue Mar 27, 2022 · 6 comments
Labels
not confirmed Report seems plausible but requires additional testing or 3rd part confirmation.

Comments

@m0uka
Copy link
Contributor

m0uka commented Mar 27, 2022

Current Behavior

Currently, due to the change to daylight savings time, every schedule that was previously executed yesterday was executed an hour later.
I am guessing it happens because next_run_at is being set when the previous schedule executes, causing the resulting date to not include DST.

Expected Behavior

The schedule should be executed at the same time every time.
Maybe make next_run_at use UTC, so there won't be any problems with DST (make getNextRunDate return UTC, and instead of NOW() use UTC_TIMESTAMP())?

Steps to Reproduce

  1. Create a schedule that executes every day at 12:00
  2. Wait until daylight savings time starts
  3. The day DST starts, the schedule will execute an hour later

Panel Version

1.7.0

Wings Version

1.6.1

Games and/or Eggs Affected

No response

Docker Image

No response

Error Logs

No response

Is there an existing issue for this?

  • I have searched the existing issues before opening this issue.
  • I have provided all relevant details, including the specific game and Docker images I am using if this issue is related to running a server.
  • I have checked in the Discord server and believe this is a bug with the software, and not a configuration issue with my specific system.
@m0uka m0uka added the not confirmed Report seems plausible but requires additional testing or 3rd part confirmation. label Mar 27, 2022
@parkervcp
Copy link
Member

This should depend on your system time.

@m0uka
Copy link
Contributor Author

m0uka commented Mar 27, 2022

The system time is correct, now the schedules are running normally, but if the previous schedule ran without DST, then the next would run +1 hour later, after that everything is normal

@DaneEveritt
Copy link
Member

DaneEveritt commented Mar 28, 2022

Seems more like a bug with the cron library. I don't actually have any particularly special logic that would be the culprit.

public function getNextRunDate()
{
$formatted = sprintf('%s %s %s %s %s', $this->cron_minute, $this->cron_hour, $this->cron_day_of_month, $this->cron_month, $this->cron_day_of_week);
return CarbonImmutable::createFromTimestamp(
(new CronExpression($formatted))->getNextRunDate()->getTimestamp()
);
}

@m0uka
Copy link
Contributor Author

m0uka commented Mar 28, 2022

Apparently not a bug - mtdowling/cron-expression#116
What about refactoring it to use UTC? I know this is a really minor issue, but it can be annoying and confusing to some people.

@DaneEveritt
Copy link
Member

DaneEveritt commented Mar 28, 2022

Guessing it might be a bug that was long fixed in https://github.com/dragonmantank/cron-expression which is the actual library we use, in which case it will likely end up fixed in coming weeks as we update dependencies and push out those changes.

@jchambo
Copy link

jchambo commented May 19, 2023

Guessing it might be a bug that was long fixed in https://github.com/dragonmantank/cron-expression which is the actual library we use, in which case it will likely end up fixed in coming weeks as we update dependencies and push out those changes.

Was this ever looked into / resolved? I'm still seeing this myself in Schedules.

image

You can see these are actually offset by an hour, and have been since Daylight Savings changed on 03/12/2023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not confirmed Report seems plausible but requires additional testing or 3rd part confirmation.
Projects
None yet
Development

No branches or pull requests

4 participants