HangfireIO / Cronos

A fully-featured .NET library for working with Cron expressions. Built with time zones in mind and intuitively handles daylight saving time transitions
MIT License
974 stars 114 forks source link

GetNextOccurrence calculated incorrect when switching from DST #71

Open redstarty opened 3 weeks ago

redstarty commented 3 weeks ago

Hello there!

This issue was initially spotted using Hangfire jobs scheduler, but I managed to investigate it to the following point:

So, imagine we have a cron 0 0 2 * * 0 (run at 2:00 each Sunday) and a specific time zone W. Europe Standard Time (supports DST, UTC+1 when non-DST, UTC+2 when DST). And Sunday 27th of October is the day when we move from DST to non-DST which leads to time 02:00 in this timezone happen twice (at 00:00 UTC and 01:00 UTC).

We request next occurrence exactly at 00:30 UTC and expect it to be 01:00 UTC time, but it is actually skipped.

Here, I draw a picture: dst drawio

And here is a code to reproduce it:

using Cronos;
using TimeZoneConverter;

var cron = CronExpression.Parse("0 0 2 * * 0", CronFormat.Standard | CronFormat.IncludeSeconds);
var requestedAt = new DateTime(2024, 10, 27, 0, 30, 0, DateTimeKind.Utc);
var nextOccurrence = cron.GetNextOccurrence(requestedAt, TZConvert.GetTimeZoneInfo("W. Europe Standard Time"), inclusive: false);

Console.WriteLine(nextOccurrence?.ToString("o"));

The code output is 2024-11-03T01:00:00.0000000Z - next week. And I would expect it to be 2024-10-27T01:00:00.0000000Z

odinserj commented 3 weeks ago

Hello @redstarty

Thank you for the description and examples. Let me explain the logic behind handling DST transitions in Cronos. To avoid messing things up, Cronos splits the timeline into Daylight Time → Ambiguous Time → Standard Time during transitions in Autumn, and handles the timeline differently for different types of cron expressions.

Interval-based expressions, like */30 * * * 0

When second, minute or hour field do contain stars, steps or ranges, then everything works as usually, since we expect that our cron expression will be triggered each 30 minutes, without any differences for each interval.

image

Non-interval-based expressions, like 0 2 * * 0

However, when second, minute or hour field don't contain stars, steps or ranges, we usually don't expect there will be multiple executions during daylight saving time transitions, since for the remaining part of the year they are executing only once per week. So, these expressions are mapped only to the Daylight Time part of the Ambiguous time, and aren't mapped to the Standard Time part of the Ambiguous time, so the whole range from 1:00 to 2:00 AM UTC is ignored to avoid having duplicate executions for these expressions, and this is by design.

image

But if you run the following lines, where requestedAt is changed to Oct 26, 23:30 UTC, e.g. is not in the ambiguous time, then next execution will be on Oct 27 00:00 UTC, and the subsequent execution will be on Nov 3, 01:00 UTC.

var cron = CronExpression.Parse("0 0 2 * * 0", CronFormat.Standard | CronFormat.IncludeSeconds);
var requestedAt = new DateTime(2024, 10, 26, 23, 30, 0, DateTimeKind.Utc);
var timeZone = TimeZoneInfo.FindSystemTimeZoneById("W. Europe Standard Time");
var nextOccurrence = cron.GetNextOccurrence(requestedAt, timeZone, inclusive: false);

Assert.NotNull(nextOccurrence);
Assert.Equal(requestedAt.AddMinutes(30), nextOccurrence.Value);
redstarty commented 3 weeks ago

Thanks a lot for the clear explanation and examples.

As I understand, a problem with my use case could happen only when the next occurrence is requested during the Ambiguous time. The probability of which... well, not that high.

Would you recommend any way to clearly understand if now is the Ambiguous time?

In any case, this issue can be closed. Thanks again.

odinserj commented 3 weeks ago

As I understand, a problem with my use case could happen only when the next occurrence is requested during the Ambiguous time. The probability of which... well, not that high.

On the contrary, this behavior will help you to avoid having unwanted second recurring job invocation during the DST transition in Hangfire. So there will be the following invocations around that date, once per week:

Oct 20, 2024, 00:00 UTC
Oct 27, 2024, 00:00 UTC
Nov 3, 2024, 01:00 UTC
Nov 10, 2024 01:00 UTC

However, if we disable such behavior, we'll get two invocations on Oct 27, and usually such a behavior leads to unexpected errors.

Oct 20, 2024, 00:00 UTC
Oct 27, 2024, 00:00 UTC // In Daylight Time
Oct 27, 2024, 01:00 UTC // In Standard Time
Nov 3, 2024, 01:00 UTC
Nov 10, 2024 01:00 UTC
redstarty commented 3 weeks ago

Alas, for my use case it would be better to actually have this extra invocation during the DST transition in Hangfire - to make sure that if user wants something to happen at 2:00 in their time zone, than it will actually happen at 2:00, no matter if he requested it beforehand or at 00:30 UTC that day. And extra invocation won't be a problem for me.

odinserj commented 3 weeks ago

That's why I used the word usually 🤷‍♂️ Let's wait and see if there are any upvotes to this feature before starting to implement it.