HangfireIO / HangfireIO/Cronos

`GetPreviousOccurrence` returns wrong dates for `*` day-of-month, `W` and `L-n`; plus parser and API edge cases

Open
#99 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
1.2k
Forks
137
Avg merge
5d 11h
Merged PRs (30d)
1

Description

While reviewing the reverse-search code added in #90 I found several cases where `GetPreviousOccurrence` returns a date that `GetNextOccurrence` would never produce, plus a few smaller bugs in the parser and the `GetOccurrences*` API. All of them reproduce on current `main` (0.13.0). A PR with fixes and regression tests follows.

### 1. Reverse search returns a date from a month the expression excludes

```csharp
CronExpression.Parse("0 1 * 3 *")
.GetPreviousOccurrence(new DateTime(2017, 3, 1, 0, 30, 0, DateTimeKind.Utc));
// actual: 2017-02-28 01:00 (February is not in the expression)
// expected: 2016-03-31 01:00
```

When `from` is on the 1st of a matching month and before that day's first match, the hour rollback makes `day = 0`. The `*` day-of-month fast path in `TryGetPreviousDay` accepts day 0, and `CalendarHelper.DateTimeToTicks` silently wraps it into the last day of the previous month. With an unrestricted month field the wrong path happens to yield the right instant, which is why the existing tests pass. The same root cause makes `Parse("0 1 * * *").GetPreviousOccurrence(new DateTime(1, 1, 1, 0, 30, 0, DateTimeKind.Utc))` throw `ArgumentOutOfRangeException` instead of returning `null`.

### 2. Reverse search fires `W` on a day that does not exist in the month

```csharp
CronExpression.Parse("0 0 31W * *")
.GetPreviousOccurrence(new DateTime(2027, 5, 15, 0, 0, 0, DateTimeKind.Utc));
// actual: 2027-04-30 (April has 30 days; the forward search never fires in April)
// expected: 2027-03-31
```

`TryGetPreviousDay` calls `MoveToNearestWeekDay(2027, 4, 31)`; day 31 is interpreted as 1 May (a Saturday), shifted back to the 30th, and accepted. The forward search rejects the month via `day > GetLastDayOfMonth`. Same for `30W` in a leap-year February when 2 March is a Saturday. As a result `GetOccurrencesDescending` is not the reverse of `GetOccurrences`.

### 3. Reverse search with `L-n` (n ≥ 28) returns a phantom day in the previous month

```csharp
CronExpression.Parse("0 0 L-28 * *")
.GetPreviousOccurrence(new DateTime(2017, 3, 1, 0, 0, 0, DateTimeKind.Utc));
// actual: 2017-01-31
// expected: 2017-01-03
```

For February, `GetLastDayOfMonth` yields `28 - 28 = 0`, which again wraps into January. The parser accepts offsets up to 30, and the forward search handles them correctly.

### 4. Time-zone reverse search skips the floored second for non-round, exclusive `from`

```csharp
var from = new DateTimeOffset(2017, 3, 22, 10, 0, 0, 500, TimeSpan.Zero);
CronExpression.EverySecond.GetPreviousOccurrence(from, eastern, inclusive: false);
// actual: 09:59:59Z
// expected: 10:00:00Z (what the UTC overload and the TimeZoneInfo.Utc fast path return)
```

The zoned path floors `from` to whole seconds but keeps `inclusive = false`, so the floored second itself is excluded even though it is strictly earlier than the original `from`. The forward path handles its mirror case correctly.

### 5. Reversed day-of-week range starting at `7` with a step is wrong

```csharp
CronExpression.Parse("* * * * 7-3/2"); // {2} instead of {0, 2, 7}; not equal to "0-3/2"
CronExpression.Parse("* * * * 7-1/2"); // empty day-of-week mask: every search returns null
```

`GetReversedRangeBits` lowers `high` to 6 for day-of-week, so `(high - num1) % step` is negative when `num1 == 7`, and the wrap-around start is computed incorrectly. The README states that `0` and `7` are both Sunday.

### 6. Parsing a long comma-separated list overflows the stack

```csharp
var list = string.Join(",", Enumerable.Repeat("1", 100_000));
CronExpression.TryParse("0 0 1 1 " + list, out _); // StackOverflowException, process crash
```

`ParseList` recurses once per element. `StackOverflowException` cannot be caught, so `TryParse` does not protect callers that parse user-supplied strings.

### 7. `GetOccurrences` / `GetOccurrencesDescending` validate arguments only on first enumeration

All six overloads are iterators, so `ArgumentException` for `from > to` (and the `DateTimeKind` / null-zone checks) is thrown on the first `MoveNext()` instead of at the call site.

### Related

- #92 / #94 (spring-forward gap in reverse search) and #93 / #97 (`DateTime` range boundaries) are independent of the above; the accompanying PR is written so that it applies cleanly alongside both. Note that #94 currently leaves three Jordan gap tests asserting the old behaviour, which fail once it is applied.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at CronExpression.GetPreviousOccurrence, TryGetPreviousDay, GetReversedRangeBits, ParseList, and the GetOccurrences/GetOccurrencesDescending overloads; trace the existing forward-search behavior and time-zone flooring. Add regression coverage for each listed expression and API case, including long-list parsing and eager validation, and confirm reverse results match forward results where stated.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.