Repository navigation
Conversation
…oundary with { tz }
Bun.cron.parse("* * * * *", 8.64e15, { tz: "UTC" }) walked the wall-clock
past year 275760, at which point Bun__gregorianDateTimeToMSInZone constructs
a JSC::ISO8601::PlainDate whose constructor asserts the year. In release the
NaN it produced was swallowed by resolve_local_match as 'keep walking',
eventually surfacing as Invalid Date instead of null.
- Guard the C++ binding: out-of-ISO8601-range year -> NaN, matching what
getEpochNanosecondsFor already returns for out-of-range epochs.
- Surface that NaN from resolve_local_match so next() stops immediately
instead of treating it like a DST-skipped minute.
- Clamp next()'s return to MAX_ECMASCRIPT_TIME so both callers (cron.parse
and the in-process scheduler) see None, not an out-of-range instant. The
per-caller clamp in cron_parse is now subsumed.
Also warms ICU before the Feb-30 timing assertion so debug+ASAN's first-open
cost isn't charged to the walk the test bounds.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix moving the Date-range clamp into CronExpression::next() so both the local-time and { tz } paths agree on returning null past ±8.64e15.
What was reviewed:
- Traced NaN through
resolve_local_match's fall-back DST scan: NaN comparisons skip the scan block and fall through toSome(NaN), whichnext()maps toNoneviaNaN <= MAXbeing false. - Checked the second caller of
next()(compute_next_timespecin cron.rs:1735) — it already treatsNoneas "stop scheduling", so the moved clamp is safe there. - Verified
gregorian_date_time_to_ms_utc(used for the year-275761 date-normalization round-trip) goes through WTF'sGregorianDateTime, notISO8601::PlainDate, so it doesn't hit the assert. - Test spawns subprocesses so the debug assert surfaces as an exit code; covers both the NaN-from-range and out-of-range-year paths across three zones.
Extended reasoning...
Overview
This PR fixes Bun.cron.parse(expr, from, { tz }) at the upper ECMAScript Date boundary (±8.64e15 ms). The local-time path already returned null when the next match fell past the range; the named-tz path (added in #35122) instead hit a debug assert in JSC::ISO8601::PlainDate's constructor (or returned Invalid Date in release). Four files change: a 3-line guard in bindings.cpp, two small edits in cron_parser.rs (surface NaN from resolve_local_match; clamp in next()), removal of the now-redundant per-caller clamp in cron.rs, and new subprocess tests plus an ICU warm-up before a timing assertion.
Security risks
None. This is pure date arithmetic at astronomical boundary values; no user-controlled paths, allocations, or trust boundaries are affected.
Level of scrutiny
Low-to-medium. The change is small and mechanical: guard a C++ constructor precondition, propagate NaN one frame up, and hoist an existing clamp from one caller into the shared next() so both callers benefit. I traced both callers of next() — cron_parse (which had the old clamp) and compute_next_timespec (the in-process scheduler) — and confirmed the latter already handles Ok(None) by returning None and not re-arming, so the behavior change there is only reachable at year 275760 and is more correct than before.
Other factors
The NaN handling composes correctly with the DST fall-back scan: when result is NaN, both result <= from_ms and result - from_ms > ... are false, so the scan is skipped and control reaches the new is_nan() check. The (r <= MAX_ECMASCRIPT_TIME).then_some(r) idiom correctly maps both NaN and >MAX to None. The C++ guard uses isYearWithinLimits — the exact predicate the PlainDate constructor asserts — so it's not a redundant heuristic. Tests spawn subprocesses (so a debug assert manifests as a non-zero exit), cover UTC plus two offset-bearing zones, exercise both the NaN-from-getEpochNanosecondsFor case and the year-275761 case, and pin the exact-boundary and lower-boundary results. The ICU warm-up before the 50 ms timing assertion is a reasonable de-flake for debug+ASAN.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused boundary fix with the clamp moved to the right layer.
What was reviewed:
- Traced both callers of
CronExpression::next()(cron.rs:1735 scheduler, cron.rs:2167 parse) — both handle the newNone-on-overflow correctly; the removed per-caller clamp is fully subsumed. - Checked NaN flow through
resolve_local_match's DST fall-back scan — NaN skips the scan (both branch conditions are false on NaN) and falls through toSome(NaN)→next()returnsNone. - Verified the
"0 0 1 1 *"test case exercises the year-275761 path that hits the newisYearWithinLimitsguard in the C++ binding, distinct from the NaN-from-getEpochNanosecondsForpath.
Extended reasoning...
Overview
Fixes a debug assert / Invalid Date return in Bun.cron.parse(expr, from, { tz }) when from is at the upper ECMAScript Date boundary (±8.64e15 ms). Touches four files: a 2-line guard in bindings.cpp (Bun__gregorianDateTimeToMSInZone), the NaN-handling disjunct in cron_parser.rs::resolve_local_match, moving the MAX_ECMASCRIPT_TIME clamp from the cron_parse caller into CronExpression::next() itself, and new subprocess-spawned regression tests across three timezones.
Security risks
None. This is date-arithmetic boundary handling in a pure computation; no untrusted input parsing changes, no allocation, no I/O.
Level of scrutiny
Low-to-moderate. The change is ~15 lines of production code in a self-contained module. The root-cause analysis in the PR description is precise (NaN compared with > yields false → misinterpreted as DST-skipped → walk continues past year 275760 → PlainDate constructor asserts), and each of the three code changes maps directly to a step in that chain. The clamp is moved from one caller into the shared helper, which is the correct layer per the repo's "fix at the layer that owns the invariant" rule — I confirmed the other caller (the in-process scheduler at cron.rs:1735) already treats None as "no future match", so it inherits the fix rather than regressing.
Other factors
- The comment-cop bot's two flags were addressed in f5125aa (both threads resolved).
- Tests spawn subprocesses so the debug assert manifests as a nonzero exit rather than being masked; they cover both failure modes (NaN from
getEpochNanosecondsForand out-of-limits year reachingPlainDate), plus the just-inside-boundary and lower-boundary cases, across UTC/negative-offset/positive-offset zones. - The ICU warm-up before the 50 ms timing assertion is a reasonable de-flake for debug+ASAN and doesn't weaken what the test asserts.
NaN <= MAX_ECMASCRIPT_TIMEisfalse, so(r <= MAX_ECMASCRIPT_TIME).then_some(r)correctly maps both NaN and over-range toNonein one expression.
|
CI status: the diff is green on every lane that ran. Across builds #88823 and #88870 the only red is the Ready for review. |
|
Heads up: #40402 adds gap handling to |
|
#42297 changes the same function and now covers everything in this PR.
If #42297 merges, close this PR. |
|
Closing in favor of #42297. It bounds the day walk in Checked on
|
### Problem - `CronExpression::next()` (`src/runtime/api/cron_parser.rs:250`) converts each candidate day to a calendar date through JSC. Nothing stops the walk at the end of the Date range. JSC converts nothing more than one day past it. - Before #42319, a debug build of `main` aborted: `Bun.cron.parse("0 0 1 1 *", 8.64e15)` gave `ASSERTION FAILED: year >= minYear && year <= maxYear` at `PlainGregorianDateTime.h(91)`. A release build returned `null`. - #42319 (merged, 6a92015) moved `main` to a WebKit where the conversion returns an all-zero date. The year goes back to 0, the 8-year bound never trips, and `Bun.cron.parse("0 0 30 2 *", new Date(8.64e15 - 86400000 * 400))` never returns. `main` and the canary hang on this call today. ### Fix - `next()` returns `None` when the candidate day is past +275760-09-13. Every zone offset is smaller than one day, so no instant on a later wall-clock day fits in a Date. - `next()` also returns `None` for a `from` outside the Date range (fake timers can set the scheduler's clock to any number). It skips a candidate that resolves past 8.64e15, so `cron_parse` needs no clamp. - Verified: `test/js/bun/cron/cron-parse.test.ts`. The 2 new tests fail without the fix and pass with it, on `main` before and after #42319 (6b394bf and 158ff6c). Also ran the rest of `test/js/bun/cron/`. - Self-reviewed: 7 points raised, 4 addressed. 3 are about #42177, #42267 and #36894 and are comments there. ### Background - `next()` walks wall-clock time forward from `from`, day by day, until all five fields match. - 8.64e15 ms is +275760-09-13T00:00:00Z, the largest time value a `Date` holds. - #42319 merged before this PR. `main` has the endless loop until this PR merges. <details><summary>Notes</summary> **Upstream change.** WebKit 320788@main (d5ba0ecec9, bug 323443) changed `DateCache::computeGregorianDateTime` in `JSDateMath.cpp` from `if (!std::isfinite(ms)) return { };` to `if (!canNarrowToInt64Milliseconds(ms)) return { };`, where `canNarrowToInt64Milliseconds(ms)` is `isfinite(ms) && abs(ms) <= maxECMAScriptTime + msPerDay`. `Bun__msToGregorianDateTime` passes the empty value through as year 0, month 1, day 0, weekday 0. With year 0 the walk climbs about 100 million day steps back to year 275760, gets year 0 again, and repeats. **On `main` before #42319 (WebKit dfd696443b, debug+ASAN build).** These calls abort with the `PlainGregorianDateTime` assertion: `Bun.cron.parse("0 0 1 1 *", 8.64e15)`, `Bun.cron.parse("0 0 30 2 *", new Date(8.64e15 - 86400000 * 400))` with and without `{ tz: "UTC" }`, and `Bun.cron.parse("* * * * *", 8.64e15, { tz: "UTC" })`. The assertion fires inside the `ms_to_gregorian_date_time_utc` call that this PR bounds, when the walk reaches year 275761. Bun 1.4.3 (release) returns `null` for all of them. `* * * * *` from 8.64e15 with a named zone takes 1 to 4.6 s there, because it walks 8 years one minute at a time. With this PR the walk ends after at most two days. **Proof on both WebKit versions.** `bun bd test test/js/bun/cron/cron-parse.test.ts`, with `src/` at the base and then at this commit. - `main` (6b394bf): 23 pass and 2 fail, then 25 pass. The parse test fails with exit code 134 and the assertion text. The scheduler test prints `scheduled` for both clock values. - Head of #42177 (676b168, WebKit 4b9ff9959a) with this commit on top: 23 pass and 2 fail, then 25 pass. Both tests time out and bun:test kills the child. - `main` after #42319 (158ff6c, WebKit cf1b36ec8703) with this branch rebased on it: 23 pass and 2 fail, then 25 pass. Both tests time out and bun:test kills the child. **Probe.** 29 calls, each in a child process with a 20 s limit, on the debug build of 676b168 with and without the day bound, and on bun 1.4.3. Inputs: `* * * * *`, `0 0 1 1 *`, `0 0 30 2 *`, `0 0 13 9 *`, `0 0 14 9 *`, `0 12 13 9 *` from 8.64e15, 8.64e15 - 1 minute, 8.64e15 - 10 days, 8.64e15 - 400 days and -8.64e15, in local time (TZ=UTC), `UTC`, `America/New_York` and `Asia/Tokyo`. Without the bound 10 calls hang. With the bound every result is equal to the 1.4.3 result. **Every conversion in `next()` is now inside the window that JSC converts.** `from` is inside the Date range. The noon of each day that the walk can reach is at most 8.64e15 + 12 h. `resolve_local_match` probes at most 121 minutes past `from`. The lower end needs no bound, because the walk only moves forward. The bound keeps +275760-09-13 itself, so the last minute of the range still matches in `Pacific/Kiritimati` (UTC+14, test row `0 14 13 9 *`). **A candidate on the last day can resolve past 8.64e15.** The local path returns a value above 8.64e15. The named-zone path returns NaN. `next()` skips both and continues to the end of that day (at most 1440 more steps). So `next()` never returns a value outside the Date range. Before, `cron_parse` mapped such a value to `null`, but the scheduler armed a timer for it: with the clock at exactly 8.64e15, `Bun.cron("* * * * *", ...)` scheduled a job. Now it throws `has no future occurrences`, the same as a named zone. **Test design.** Both tests spawn a child, because a walk that does not stop spins in native code and cannot be interrupted. They are not concurrent: bun:test kills the children of a timed-out test only when the test is not concurrent. The `timeout` and `killSignal` on the child are the kill switch for CI, where the per-test timeout is 90 s or more. **Timing test.** "impossible day/month (Feb 30) returns null quickly" asserts under 50 ms. It fails on a debug+ASAN build without this PR (159 ms). The first date conversion in a process costs about 150 ms there, and a warm call takes 2 ms. The test now makes one call before it starts the clock. #36894 has the same edit. **Relation to #36894.** #36894 is closed in favor of this PR. It maps a match past 8.64e15 to `None` inside `next()`, removes the clamp in `cron_parse`, and guards the year in `Bun__gregorianDateTimeToMSInZone`. This PR now does the first two. The year guard is not needed, because the walk cannot reach year 275761. #36894 does not bound the day walk, so it does not stop the loop on the new WebKit. Its `{ tz }` boundary rows are part of the new parse test here, and they pass. **Not changed.** `Cookie.cpp:261` also calls `msToGregorianDateTime`. On the new WebKit `new Bun.Cookie("a", "b", { expires: 1e13 }).toString()` prints `Expires=Sun, 00 Jan 0000 00:00:00 GMT`. With the old WebKit it printed `Sun, 20 May 318857 17:46:40 GMT`. Both are outside the Date range, and neither hangs. `toISOString` in `wtf-bindings.cpp` only receives the time value of a `Date` or the current time. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/cron/cron-parse.test.ts bun test v1.4.3 (4ff9193) test/js/bun/cron/cron-parse.test.ts: (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > impossible day/month (Feb 30) returns null quickly [236.60ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > Feb 29 finds next leap year [1659.45ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > weekday matching uses local day-of-week [1741.19ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > strictly-after: from = exact match returns the next occurrence [1887.91ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 1-7 means Mon-Sun (every day) [1758.25ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > DOM/DOW OR semantics when both restricted [1844.78ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 6-7 means Sat-Sun [1565.94ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > scalar 7 still means Sunday [1374.75ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 5-7 means Fri-Sun [1793.66ms] (pass) Bun.cron.parse — w ... (truncated) release without fix: 2 FAILED bun test v1.4.3-canary.1 (4ff9193) test/js/bun/cron/cron-parse.test.ts: (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > impossible day/month (Feb 30) returns null quickly [5.77ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > weekday matching uses local day-of-week [25.21ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > Feb 29 finds next leap year [23.54ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 1-7 means Mon-Sun (every day) [24.17ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > DOM/DOW OR semantics when both restricted [25.06ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > strictly-after: from = exact match returns the next occurrence [35.13ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 5-7 means Fri-Sun [61.49ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 0-7 means every day [63.62ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > scalar 7 still means Sunday [65.85ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 6-7 means Sat-Sun [70.84ms] (pass) Bun.cron.parse — invalid `from` argument > throws for out-of-range/non-finite ms: 1e+300 [0.20ms] (pass) Bun ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/cron/cron-parse.test.ts bun test v1.4.3 (4ff9193) test/js/bun/cron/cron-parse.test.ts: (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > impossible day/month (Feb 30) returns null quickly [201.36ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > Feb 29 finds next leap year [1469.26ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > strictly-after: from = exact match returns the next occurrence [1652.81ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > DOM/DOW OR semantics when both restricted [1462.10ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 1-7 means Mon-Sun (every day) [1537.11ms] (pass) Bun.cron.parse — algorithm (pinned TZ=UTC) > weekday matching uses local day-of-week [1977.03ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 5-7 means Fri-Sun [1813.74ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > scalar 7 still means Sunday [1615.41ms] (pass) Bun.cron.parse — weekday 7 = Sunday in ranges > 6-7 means Sat-Sun [1781.97ms] (pass) Bun.cron.parse — w ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 840ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/6] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [1/6] cargo bun_runtime → libbun_runtime.a ^[[1m^[[92m Compiling^[[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) ^[[1m^[[92m Compiling^[[0m bun_errno v0.0.0 (/workspace/bun/src/errno) ^[[1m^[[92m Compiling^[[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) ^[[1m^[[92m Compiling^[[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) ^[[1m^[[92m Compiling^[[0m bun_safety v0.0.0 (/workspace/bun/src/safety) ^[[1m^[[92m Compiling^[[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) ^[[1m^[[92m Compiling^[[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) ^[[1m^[[92m Compiling^[[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) ^[[1m^[[92m Compiling^[[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) ^[[1m^[[92m Compiling^[[0m bun_paths v0.0.0 (/workspace/bun/src/paths) ^[[1m^[[92m Compiling^[[0m bun_collections v0.0.0 (/workspace/bun/src/collections) ^[[1m^[[92m Compiling ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/api/cron.rs | 7 +-- src/runtime/api/cron_parser.rs | 25 +++++++--- test/js/bun/cron/cron-parse.test.ts | 99 +++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 12 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/api/cron.rs 3 2 30 src/runtime/api/cron_parser.rs 6 7 30 test/js/bun/cron/cron-parse.test.ts 6 8 29 ``` </details> <!-- robobun:evidence:end -->
1 participant
What
Bun.cron.parse()with a{ tz }option crashed a debug assert (and returnedInvalid Datein release) whenfromwas at or near the upper ECMAScript Date boundary and the next match fell past it. The local-time path already returnednullthere; the{ tz }path introduced in #35122 did not.Repro
$ bun -e 'console.log(Bun.cron.parse("* * * * *", 8.64e15, { tz: "UTC" }))'Debug:
Release:
Invalid Date(expectednull, same asTZ=UTC bun -e 'console.log(Bun.cron.parse("* * * * *", 8.64e15))').Also reproduces with
America/New_York,Asia/Tokyo, and with schedules whose next match is in year 275761 (e.g."0 0 1 1 *"at8.64e15).Cause
CronExpression::next()walks wall-clock time forward fromfrom_ms. Atfrom_ms = 8.64e15(which is+275760-09-13T00:00:00Z), every candidate past that instant is out of the ECMAScript Date range. For a named tz,Bun__gregorianDateTimeToMSInZonecallsgetEpochNanosecondsFor, which returns a range error that the binding maps to NaN.resolve_local_matchcompared that NaN with> from_ms(false) and reportedNone, whichnext()interprets as "DST-skipped minute, keep walking". The walk then advanced until the year rolled past 275760, at which point the binding constructsJSC::ISO8601::PlainDate { 275761, ... }whose constructor assertsisYearWithinLimits.Fix
Bun__gregorianDateTimeToMSInZone: return NaN for years outsideJSC::ISO8601::isYearWithinLimitsinstead of constructing aPlainDatethat asserts;getEpochNanosecondsForwould have produced a range error (NaN) for that date anyway.resolve_local_match: surface NaN asSome(NaN)sonext()sees "out of range" rather than "keep walking".next(): clamp the returned instant toMAX_ECMASCRIPT_TIME(Nonepast it, which also catches NaN). Both callers now seeNonefor an out-of-range match; the per-caller clamp incron_parseis subsumed and removed.Also adds an ICU warm-up before the "Feb 30 returns null quickly" timing assertion; that test's 50 ms bound was being spent on ICU's first-open under debug+ASAN.
Verification
bun bd test test/js/bun/cron/cron-parse.test.ts(andcron-local-time.test.ts,cron.test.ts): 26 / 84 / 63-skipped pass. The new{ tz }boundary cases crash with the PlainDate assertion on main.