Support schedule jitter

[epicoxymoron]

Closes #88.

Worth noting: I develop on a Mac, and 12 of the test cases fail on main (without any changes), mainly around DST handling. I did not fix them. I assume they pass when run on Windows.

1. GetNextOccurrence_HandleDifficultDSTCases_WhenTheClockJumpsForwardOnFriday
2. GetNextOccurrence_HandleDifficultDSTCases_WhenTheClockJumpsBackwardOnFriday
3. GetNextOccurrence_HandleDST_WhenTheClockJumpsForwardAndFromIsAroundDST
4. GetNextOccurrence_ReturnsAGreaterValue_EvenWhenMillisecondTruncationRuleIsAppliedDueToDST

I cleaned up a few typos around the codebase while I was there, and took a stab at writing the README docs. I'll call out some specific non-jitter related changes that you may want me to revert (I don't mind):

1. I removed the [Flags] attribute on CronFormat since you can't use both formats at the same time. I assume that was put there intentionally but it didn't make sense to me.
2. In the README, Rider decided it wanted to build out flesh out the tables with all the extra padding. I didn't want to fight it, but I figure that could be controversial.

I did run the benchmarks locally, and while the hash benchmark is noticeably slower than the others and allocates more, I think it's forgivable for what it provides.

One last thing: I'm fairly new to the .NET ecosystem, having really only developed on .NET 9. If there are conventions I've used that don't work on older versions of .NET that you'd like to support, I'll just need a little coaching on how to adjust.

Feedback welcome. I'm looking forward to seeing this capability integrated into HangFire at some point.

[epicoxymoron]

Now that this is out there, I'm thinking that the caller passing in a pre-seeded Random instance is a cleaner and more intuitive API, especially considering the macro use case...

[odinserj]

Thanks a lot, this is a really great addition to Cronos 😊 And many thanks for the correction of the README.md file! Let's go through the general questions first, and then we will dig into the details:

Worth noting: I develop on a Mac, and 12 of the test cases fail on main (without any changes), mainly around DST handling. I did not fix them. I assume they pass when run on Windows.

Yes, this is a big topic related to a few time zones, and Cronos is "lucky" enough to use some of them. The problem is the source of time zone information, which is different on different platforms. Since .NET is working with Window-based source since the very beginning, it does everything right. However, on Linux and macOS IANA's tzdb is used, and the code that reads it is relatively new (appeared in .NET 5.0). And there are problems with reading it and converting it into .NET's internal data structures.

NodaTime does everything right, but .NET's own code needs improvements. The corresponding issues are already filed as a GitHub issue, but not implemented yet. I have a list of issues with time zones somewhere, but didn't have time to add more information to those GitHub issues yet.

I cleaned up a few typos around the codebase while I was there, and took a stab at writing the README docs. I'll call out some specific non-jitter related changes that you may want me to revert (I don't mind):

This is really awesome, thank you again 😍

I removed the [Flags] attribute on CronFormat since you can't use both formats at the same time. I assume that was put there intentionally but it didn't make sense to me.

Not sure about this change, since IncludeYears option can be added in the future. For two-valued enum it doesn't make sense, but with the Flags attribute specified we will be able to add years support without introducing breaking changes.

In the README, Rider decided it wanted to build out flesh out the tables with all the extra padding. I didn't want to fight it, but I figure that could be controversial.

The original intention was to add minimal number of characters to convert this into a table and make it easy to add new rows. Since this table is mostly immutable and since the new format have better look when viewing raw file, it may be worth using the new format, leaving this on you.

I did run the benchmarks locally, and while the hash benchmark is noticeably slower than the others and allocates more, I think it's forgivable for what it provides.

I think this is fully expected, since Random instance is being allocated, so no problems here.

One last thing: I'm fairly new to the .NET ecosystem, having really only developed on .NET 9. If there are conventions I've used that don't work on older versions of .NET that you'd like to support, I'll just need a little coaching on how to adjust.

Cronos has a limited scope, so there are no features specific to versions. One day it might be converted to using modern Span type to avoid unsafe methods, but now everything is fine.

Feedback welcome. I'm looking forward to seeing this capability integrated into HangFire at some point.

Yes, this would be a great feature!

[odinserj]

This line is interesting, and should be covered by a dedicated unit test, and the Interval flag relates to Daylight Saving Time transitions, please see https://github.com/HangfireIO/Cronos?tab=readme-ov-file#interval-expression.

The point is that the jitter feature is sensitive to DST transitions, since it can easily schedule occurrences to "dangerous" time like 01:00 and 02:00 AM. Since Cronos handles this automatically, no problems are expected, but we should ensure that both interval-based and non-interval-based expressions work as expected with H characters.

[epicoxymoron]

I misunderstood the intent of that flag.

H on its own is a stand-in for a single number and is not an interval expression on its own, but it can be combined with steps. I need to think about this a little more, thanks for pointing it out.

[odinserj]

Could you explain this, please?

[epicoxymoron]

I'll try 😄 . I spent a while trying to implement this in a way that didn't break existing unit tests. The reversed ranges were particularly problematic.

So this method gets called in these scenarios:

1. A low-high range, e.g. 2-4
2. A high-low range, implemented as two separate low-high ranges, e.g. hours 22-2 become 22-23 and 0-2.
3. A non-hash step, e.g. */8
4. A hash with step, e.g. H/8

Taking them backwards:

A hash with step requires us to go through the entire field of numbers starting at the offset and wrap our way around, e.g. H/8 as an hour field might start us out at 17, causing us to set 0+17, (8+17)%24=1, and (16+17)%24=9. Simple, intuitive.

A non-hash step can start with the field's low value and never have to worry about modulus. To apply one in a unified formula, though, we must know the full range of the field, and in this case, the low and high parameters correctly define the range.

A range, whether high-low or low-high, doesn't have enough information for us to be able to infer the full range of the field to be used as a modulus, it needs to be passed in as another argument to the function, e.g. 22-23 being the low/high does NOT mean that the range for modulus is 1. That issue is easy to overcome. However, the ranges by definition can't have hashes, so attempting to use the modulus is unnecessary anyways.

I kept trying to find ways to unify the formula but ultimately decided that doing that check inside the for loop was probably less performant than only invoking modules when it was actually needed anyway.

---

And I think I have a bug on fields where the low value is non-zero, e.g. if a day-of-month has an expression of H/15 and our offset is 12: we'd set 1+12=13 and (16+12)%28=0, but 0 is out of range. I will fix that and add a test.

The unified formula probably looks something like this, if we really wanted to use a modulus each time:

for (var i = 0; i < intervalRangeOrFieldRange; i += step)
{
    SetBit(ref bits, low + ((i + (jitter ?? 0)) % fieldRange));
}

I can keep digging in that direction if you think it's important.

[epicoxymoron]

Some fresher eyes have allowed me to find a way to avoid the modulus issue entirely 🥳

[epicoxymoron]

And finally:

However, on Linux and macOS IANA's tzdb is used, and the code that reads it is relatively new (appeared in .NET 5.0). And there are problems with reading it and converting it into .NET's internal data structures.

👍 , and I appreciate the back-story. I just wanted to call 'em out as preexisting failures.

Not sure about this change, since IncludeYears option can be added in the future. For two-valued enum it doesn't make sense, but with the Flags attribute specified we will be able to add years support without introducing breaking changes.

Makes sense. I saw that GH issue after submitting this PR. I'll revert that specific change.

The original intention was to add minimal number of characters to convert this into a table and make it easy to add new rows. Since this table is mostly immutable and since the new format have better look when viewing raw file, it may be worth using the new format, leaving this on you.

I'll leave the edits, since they're already done.

---

I appreciate the thorough review.

[epicoxymoron]

(Nearly) all feedback should be implemented now:

1. I've reverted to the int jitterSeed based implementation instead of Random.
2. I've restored [Flags] on CronFormat.
3. I've added example API usage to the README.
4. I've implemented Parse as overloads to maintain ABI compatibility.
5. I've cleaned up the GetRangeBits method to have unified math and avoiding modulus by generating a random offset within the step range instead of the field range.
6. I've implemented intervals correctly this time and added unit tests covering jumping forward and backward.
7. I've fixed the bug that occurs when the jitter offset is calculated as the last element in the field range.

The only thing (I think) I didn't address is the comment on nullable reference types. I need a little more guidance understanding what you'd prefer to see there, since it's valid for the caller to use a non-hashed expression. The parsing of each field needs to be passed a reference to a shared, stateful Random in case the expression involves a hash; the expression as a whole is only problematic when a hash is used and no seed is provided.

[odinserj]

Sorry for the delayed response, quite a busy week. I will try my best to continue our conversation next week!

[odinserj]

Hi @epicoxymoron! Thanks a lot for working on this, I appreciate your time and efforts in making this done. No worries regarding the nullable reference type, it was a misunderstanding from my side, now with a dedicated exception, the intention is perfectly clear. Thank you also for the description regarding wrap-arounds when parsing steps, it looks like I understand the problem now. I think your original implementation was better, but please let me clutter your head with unnecessary details first.

The full form of steps is based on range, so 15-45/5 will be a full example of a step, where the low, high and step values will be defined. Other representations are special cases, for example */5 means 0-59/5 (depending on a field), 15/5 means 15-59/5, according to Wikipedia (https://en.wikipedia.org/wiki/Cron#Cron_expression) with no wrap-arounds, because first number is lower than the second one. However, Cronos also supports reverse ranges, where wrap-arounds happen, e.g. 45-15/5 expressions.

When we talk about expressions like H/5, they look like 15/5, 27/5 or 45/5 expressions, where H is substituted by a random number. However, that's wrong, because the given expressions are expanded to 15-59/5, 27-59/5 or 45-59/5, and using this semantics with H will limit its power. Instead, expressions like H/5 mean a new thing – any random 5-minute range, so they behave more like 15-14/5, 27-26/5 or 45-44/5 reverse ranges.

Since the ParseStep already knows how to work with reverse ranges, we could just pass these ranges explicitly based on a random number and covering the full field range, avoiding any custom wrap-around implementation, So, the num1 number will be generated as a random number within field.First and field.Last (limited in the DoM field) values, and num2 will be calculated as (num1 + range - 1) % range (can be simplified):

[SuppressMessage("Security", "CA5394:Do not use insecure randomness")]
private static unsafe ulong ParseHash(CronField field, ref char* pointer, ref CronExpressionFlag flags, Random? rng)
{
    // prior to this point in parsing, it has been valid to not pass in a jitter seed.
    if (rng == null) throw new MissingSeedException();

    // Prevent against calculating the 31st of February
    var maxValueInclusive = field == CronField.DaysOfMonth
        ? CronField.LastCommonDayOfMonth
        : field.Last;
    
    var jitter = rng.Next(field.First, maxValueInclusive + 1);

    if (Accept(ref pointer, '/'))
    {
        if (field.CanDefineInterval) flags |= CronExpressionFlag.Interval;

        var range = maxValueInclusive - field.First + 1;
        return ParseStep(field, ref pointer, jitter, (jitter + range - 1) % range);
    }

    return GetBit(jitter);
}

In this case, no jitter is required to be passed to ParseStep and other methods.

[odinserj]

What do you think about such an implementation? I can make the changes on my own, but need to re-consider the tests first, since random numbers in this case are different, because higher bound is different.

[epicoxymoron]

At first glance, I'm not so sure that both approaches don't take us to the same place.

The idea of treating H/N as a reverse range, e.g. 15-14/5, 27-26/5 or 45-44/5 isn't something I had considered. For some reason, I assumed that ranges like 27-26/5 would have still followed a % N == 0 rule and thus evaluated to 30,35,40,...,20,25, but Cronos certainly behaves as you describe and it looks like even Vixie does, too. I learned something new 😄.

That said, my current implementation treats it more akin to (*/N)+K where K < N. Considering the 27-26/5 example the corollary is K=2 and we end up with 2,7,12,...,47,52,57. The resultant range is identical.

I think it comes down to just which fits your mental model better and what you'd rather maintain longer-term. I'm happy to leave that decision to you, but I certainly grant your version is less intrusive to the codebase.

From a testing standpoint, I tried out a few seeds and tried to stay away from values that would generate either 0, 1, or the last value in the range. IIRC 3 was a decent seed when the range was 0-field.Last, too, but some time has passed and I've slept since then, so YMMV 😄.

Whichever way you'd like to go is fine with me. This has been fun.