cron: interpret Bun.cron.parse() and in-process schedules in local time; add { tz } option - #35122
Conversation
Bun.cron.parse() and the in-process Bun.cron(schedule, handler) now interpret cron expressions in the system's local time zone instead of UTC, matching the OS-level Bun.cron(path, schedule, title) overload (crontab/launchd/schtasks are all local-time). Reinstates the DST handling that was dropped when the parser switched to UTC: spring-forward times shift forward by the gap; in the fall-back duplicated hour, fixed-time schedules fire once (first occurrence) while schedules with a wildcard minute or hour fire through both occurrences (cronie semantics). Part of #28792.
WalkthroughChangesCron parsing and in-process scheduling now support local and named time zones, including DST-aware occurrence resolution. Public typings, documentation, conversion helpers, API wiring, and regression tests are updated accordingly. Local-time Cron Scheduling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: Reproduced All cron tests green in CI across builds 77813/77855/77865/77907. All review feedback addressed, including a correctness fix for chaining wildcard schedules past a fall-back transition. Remaining CI red is unrelated: |
|
Updated 11:16 AM PT - Jul 22nd, 2026
❌ @robobun, your commit 8520c37 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 35122That installs a local version of the PR into your bun-35122 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/bun/cron/cron-parse.test.ts (1)
26-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest iterates non-UTC zones inside a "pinned TZ=UTC" describe block — and duplicates
cron-local-time.test.ts.This test spawns with varying
TZ(America/Los_Angeles,Asia/Tokyo,UTC), which contradicts the enclosing describe's name ("Bun.cron.parse — local time (pinned TZ=UTC)") and this file's own top-of-file comment stating zone-sensitive cases belong incron-local-time.test.ts. The exact same three TZ/expected-value pairs are already asserted incron-local-time.test.tslines 43-59. See consolidated comment below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/bun/cron/cron-parse.test.ts` around lines 26 - 44, Remove the duplicated TZ-iteration test from the pinned-UTC describe block in the cron parse tests. Keep zone-sensitive coverage consolidated in cron-local-time.test.ts, and preserve this file’s scope for tests that run under the pinned UTC environment.src/jsc/JSGlobalObject.rs (1)
124-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate wrapper bodies — extract a shared helper.
gregorian_date_time_to_ms/gregorian_date_time_to_ms_utcandms_to_gregorian_date_time/ms_to_gregorian_date_time_utcare identical except for thelocalTimebool literal passed to the FFI call. Factoring out a private helper takinglocal: boolwould remove the duplication and prevent the two variants from drifting apart later.♻️ Proposed refactor
- pub fn gregorian_date_time_to_ms_utc( - &self, - year: i32, - month: i32, - day: i32, - hour: i32, - minute: i32, - second: i32, - millisecond: i32, - ) -> JsResult<f64> { - crate::mark_binding(); - crate::cpp::Bun__gregorianDateTimeToMS( - self, year, month, day, hour, minute, second, millisecond, false, - ) - } - - pub fn gregorian_date_time_to_ms( - &self, - year: i32, - month: i32, - day: i32, - hour: i32, - minute: i32, - second: i32, - millisecond: i32, - ) -> JsResult<f64> { - crate::mark_binding(); - crate::cpp::Bun__gregorianDateTimeToMS( - self, year, month, day, hour, minute, second, millisecond, true, - ) - } + fn gregorian_date_time_to_ms_impl( + &self, + year: i32, + month: i32, + day: i32, + hour: i32, + minute: i32, + second: i32, + millisecond: i32, + local: bool, + ) -> JsResult<f64> { + crate::mark_binding(); + crate::cpp::Bun__gregorianDateTimeToMS( + self, year, month, day, hour, minute, second, millisecond, local, + ) + }(similarly for the
ms_to_gregorian_date_time*pair.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/JSGlobalObject.rs` around lines 124 - 215, Extract private helpers for the two Gregorian conversion pairs, each accepting a local-time boolean, and move the shared `mark_binding` and FFI invocation logic into those helpers. Update `gregorian_date_time_to_ms_utc` and `gregorian_date_time_to_ms` to delegate with the appropriate boolean, and do the same for `ms_to_gregorian_date_time_utc` and `ms_to_gregorian_date_time`, preserving their existing return values and FFI arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/cron/cron-local-time.test.ts`:
- Around line 70-88: Remove the duplicate weekday-7 test suite named
“Bun.cron.parse — weekday 7 = Sunday in ranges” from cron-local-time.test.ts,
keeping the identical coverage in cron-parse.test.ts as the single source of
these assertions.
In `@test/js/bun/cron/cron-parse.test.ts`:
- Around line 83-99: Remove the duplicate weekday-7 test suite identified by
describe.concurrent("Bun.cron.parse — weekday 7 = Sunday in ranges"), retaining
the canonical identical coverage in cron-local-time.test.ts. Do not alter the
cron parsing implementation or other distinct tests.
---
Outside diff comments:
In `@src/jsc/JSGlobalObject.rs`:
- Around line 124-215: Extract private helpers for the two Gregorian conversion
pairs, each accepting a local-time boolean, and move the shared `mark_binding`
and FFI invocation logic into those helpers. Update
`gregorian_date_time_to_ms_utc` and `gregorian_date_time_to_ms` to delegate with
the appropriate boolean, and do the same for `ms_to_gregorian_date_time_utc` and
`ms_to_gregorian_date_time`, preserving their existing return values and FFI
arguments.
In `@test/js/bun/cron/cron-parse.test.ts`:
- Around line 26-44: Remove the duplicated TZ-iteration test from the pinned-UTC
describe block in the cron parse tests. Keep zone-sensitive coverage
consolidated in cron-local-time.test.ts, and preserve this file’s scope for
tests that run under the pinned UTC environment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26c527f5-78e2-489f-af6a-32f783c89f7a
📒 Files selected for processing (6)
docs/runtime/cron.mdxpackages/bun-types/bun.d.tssrc/jsc/JSGlobalObject.rssrc/runtime/api/cron_parser.rstest/js/bun/cron/cron-local-time.test.tstest/js/bun/cron/cron-parse.test.ts
…eb-30 timing bound
- JSGlobalObject: factor gregorian_date_time_to_ms{,_utc} and
ms_to_gregorian_date_time{,_utc} into single impls taking a local-time
flag.
- cron-parse.test.ts: drop the TZ-iteration test (covered by
cron-local-time.test.ts) and rename the describe block; drop the
sub-50ms timing assertion on the Feb-30 null case since the local-time
walk does more FFI round-trips than the UTC one and the test timeout
already guards against unbounded search.
- cron-local-time.test.ts: drop the weekday-7 suite (covered by
cron-parse.test.ts).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/JSGlobalObject.rs:194-214— Rewiringcron_parser.rsto the new local-time variant leavesms_to_gregorian_date_time_utc(immediately above, lines 172-192) with zero callers repo-wide. It's apub fnso the dead-code lint won't flag it — per REVIEW.md, delete helpers whose last caller you rewired in the same PR. (gregorian_date_time_to_ms_utcis still used by the SQL date types, so only the ms→gregorian direction is dead.)Extended reasoning...
What
This PR adds
JSGlobalObject::ms_to_gregorian_date_time(local-time) and rewiresCronExpression::next()insrc/runtime/api/cron_parser.rsto use it. Before this PR,cron_parser.rswas the only caller ofJSGlobalObject::ms_to_gregorian_date_time_utc— the diff removes both call sites (global_object.ms_to_gregorian_date_time_utc(from_ms)and the round-trip normalization). After the rewire,ms_to_gregorian_date_time_utc(defined atsrc/jsc/JSGlobalObject.rs:172-192, immediately above the newly-added function) has zero callers.Proof
Repo-wide grep on the PR checkout:
$ rg -n ms_to_gregorian_date_time_utc src/jsc/JSGlobalObject.rs:172: pub fn ms_to_gregorian_date_time_utc(&self, ms: f64) -> GregorianDateTime {Only the definition remains. It's a
pub fn, so#[warn(dead_code)]will not flag it — REVIEW.md explicitly calls this out: "Public items escape dead-code lints — grep for callers manually."Scope check — sibling function
The pair function
gregorian_date_time_to_ms_utcis not dead — it's still called bysrc/sql_jsc/postgres/types/date.rsandsrc/sql_jsc/mysql/MySQLValue.rsfor UTC timestamp conversion. Only the ms→gregorian UTC direction lost its last caller in this PR, so onlyms_to_gregorian_date_time_utcshould be deleted.Why the lint doesn't catch it
pubitems are reachable from outside the crate as far as rustc knows, so the dead-code lint stays silent. This is exactly the case REVIEW.md's "Delete dead code in the same PR that makes it dead" rule targets: "superseded implementations, helpers whose last caller you rewired… Public items escape dead-code lints — grep for callers manually."Impact
None at runtime — the function simply sits unused. This is a code-hygiene finding, not a correctness bug: leaving it in place adds a second near-identical 20-line FFI wrapper next to the new one, differing only in the
false/truelocalTimeflag, with nothing to distinguish which one callers should reach for.Fix
Delete
ms_to_gregorian_date_time_utc(lines 172-192 ofsrc/jsc/JSGlobalObject.rs) and mention the deletion in the PR description per REVIEW.md ("required scope — name the deletions in the description").
…est helpers - JSGlobalObject: cron_parser.rs was the only caller of ms_to_gregorian_date_time_utc; the local-time rewire removed both call sites, so delete it (gregorian_date_time_to_ms_utc stays, used by the SQL date types). - cron-local-time.test.ts / cron-parse.test.ts: pipe stderr and assert it empty before the exit-code check so a child crash surfaces the error message instead of a bare 'expected 0, got 1'.
|
Lets add a {tz: string} option, so users can keep the existing behaviour by forcing to UTC if they want it |
|
Will do. Adding (Also pushing the |
Bun.cron.parse(expr, from?, { tz?: string }) and the in-process
Bun.cron(schedule, handler, { tz?: string }) now accept an IANA
time-zone name. Default is the system's local zone; { tz: 'UTC' }
preserves the pre-1.4 behaviour.
- bindings.cpp: Bun__resolveTimeZoneID / Bun__msToGregorianDateTimeInZone
/ Bun__gregorianDateTimeToMSInZone wrap JSC's Temporal ICU bridge
(intlResolveTimeZoneID, getOffsetNanosecondsFor,
exactTimeToLocalDateAndTime, getEpochNanosecondsFor with Compatible
disambiguation), so the per-TimeZone UCalendar LRU cache and DST
gap/fold handling are inherited rather than reimplemented.
- cron_parser.rs: CronTz::{Local,Named(u32)} parameterises the
zone-dependent calls (initial breakdown, resolve_local_match,
matches_instant). The noon normalization round-trip in next() now uses
UTC, since day overflow + weekday are pure calendar math and the
named-zone path can't accept overflowed fields.
- cron.rs: resolve_cron_tz() reads { tz } from the third argument of
both Bun.cron.parse and the in-process Bun.cron callback overload;
CronJob stores the resolved tz.
Also:
- cron.test.ts: pin the ~40-test describe('Bun.cron.parse') block to
Etc/UTC via beforeAll/afterAll. It calls parse() in-process and
asserts Date.UTC(...) values, which would fail on non-UTC hosts now
that parse() is local by default (test/preload.ts deliberately skips
TZ when syncing bunEnv into process.env).
- cron-parse.test.ts: pipe stderr and assert combined
{ stdout, stderr, exitCode } in the two remaining inline spawns.
- Types and docs updated for the new options argument.
9 new tests in cron-local-time.test.ts cover the tz override, DST under
the override, error handling, and the in-process callback path with tz
under fake timers.
robobun
changed the title
…e fixture
- test/leaksan.supp: JSC::TemporalCore::withTimeZone holds an 8-entry
per-TimeZone UCalendar LRU for the process lifetime. The ICU
OlsonTimeZone transition rules it lazily loads are never freed; the
same suppression class as the existing IntlDateTimeFormat entries.
- test/integration/bun-types/fixture/cron.ts: cover the new 3-arg
Bun.cron.parse(expr, from, { tz }) / Bun.cron(schedule, handler,
{ tz }) forms and the exported Bun.CronOptions type.
…rlap
- resolve_cron_tz: use opts.get() + is_undefined_or_null() instead of
get_truthy() so { tz: '' } reaches the validator and throws 'unknown
time zone' instead of silently falling back to local. Also reject
non-ASCII up front so the Latin-1 StringView cast in
Bun__resolveTimeZoneID stays sound for its UTF-8 input (all valid
IANA names are ASCII).
- resolve_local_match: the real-time-minute probe can only find a match
when a fall-back transition lies in (from_ms, result] (more real
minutes elapsed than wall-clock minutes). Compare the UTC distance of
the two local breakdowns against result - from_ms and skip the probe
when they're equal, so @hourly chaining outside DST windows no longer
does ~59 TZ lookups per call.
- cron-local-time.test.ts: cover empty-string and non-ASCII tz, and the
fall-back wildcard-hour chain via the tz option (exercises the gate).
The UTC normalization round-trip plus the tz option make the impossible-date search host-TZ-independent and fast enough to keep the sub-50ms guard (measured ~2ms in debug ASAN).
…tore
- resolve_local_match: when the wall-clock candidate is in a fall-back's
repeated window and maps to its earlier instant (<= from_ms), the
later instant may still match. Add result <= from_ms as a second
probe trigger and don't cap at result in that branch, so chaining
'* * * * *' or '*/N * * * *' from the transition instant walks the
whole repeated hour instead of jumping to the next unambiguous
minute. Regression-tested with a NY fall-back every-minute chain and
a */15 chain.
- cron.test.ts: restore process.env.TZ via assignment ('' when unset)
instead of delete, since deleting the CustomAccessor removes it
without clearing the WTF::setTimeZoneOverride and would leak Etc/UTC
to later test files on hosts that don't export TZ.
There was a problem hiding this comment.
The two findings from the last pass are addressed in 8520c37 — the result <= from_ms probe branch now recovers the repeated fall-back window (verified against the new * * * * * and */15 chain tests), and cron.test.ts restores TZ via assignment instead of delete. No further issues found, but this is a breaking semantic change (UTC → local) with hand-rolled DST resolution, new Temporal-ICU FFI, and new API surface — worth a human sign-off on the DST semantics choices and the leaksan suppression.
What was reviewed:
resolve_local_matchfall-back probe: traced* * * * *,0 * * * *,30 * * * *, and*/15from both sides of the 2025-11-02 NY transition; the new gate is only reachable during an actual overlap.Bun__msToGregorianDateTimeInZone/Bun__gregorianDateTimeToMSInZonemonth (1-based) and weekday (ISO8601::dayOfWeek % 7→ 0=Sun) match the existing WTF-backed bindings.resolve_cron_tznow gates on presence (get+is_undefined_or_null) and rejects empty/non-ASCII before the Latin-1 cast.
Extended reasoning...
Overview
Switches Bun.cron.parse() and the in-process Bun.cron(schedule, handler) from UTC to local-time interpretation, and adds a { tz?: string } option to both. Touches: cron_parser.rs (new CronTz enum, wall-clock walk with manual hour/minute carry, resolve_local_match DST resolver), cron.rs (option parsing, CronJob.tz field), JSGlobalObject.rs + bindings.cpp (three new Temporal-ICU FFI wrappers), bun.d.ts + docs/runtime/cron.mdx (API surface + DST semantics docs), four test files, and a new leaksan.supp entry for JSC's per-TimeZone UCalendar LRU.
Security risks
None identified. The only untrusted input on the new path is the tz string, which is now ASCII-gated on the Rust side before the Latin1Character* reinterpret in Bun__resolveTimeZoneID, and unrecognised names throw. The probe loop is bounded (≤120 iterations) and only reachable via * minute or * hour schedules.
Level of scrutiny
High. This is a breaking behaviour change to a shipped API (any user relying on Bun.cron.parse returning UTC-interpreted times will see different results), and the DST resolver encodes specific policy choices (croner semantics for spring-forward multi-minute patterns; cronie/Vixie semantics for fall-back wildcard schedules) that are now committed to in public docs. The algorithm went through four review rounds here — each found a real defect (fall-back skip past the transition instant, unconditional probe on non-DST days, empty-tz silent fallback, Latin-1/UTF-8 boundary, cross-file TZ leak, dropped perf assertion). All are now addressed, but the density of edge cases argues for a maintainer pass on the design rather than bot-only approval.
Other factors
- alii requested the
{ tz }option mid-thread but hasn't reviewed the implementation. - New
leak:JSC::TemporalCore::withTimeZonesuppression — the PR description attributes it to JSC's per-TimeZone UCalendar LRU; a maintainer should confirm that's the intended long-term stance vs. a targeted free. - Test coverage is good (25 new DST/tz tests including Lord Howe 30-min shifts and Santiago midnight gap; fall-back chain tests added for the last fix), and the Feb-30 termination-speed guard was restored in-process.
3 participants
Part of #28792.
What
Bun.cron.parse()and the in-processBun.cron(schedule, handler)now interpret cron expressions in the system's local time zone instead of UTC, matching the OS-levelBun.cron(path, schedule, title)overload (crontab, launchd, and Windows Task Scheduler are all local-time).Both also accept a new
{ tz?: string }options argument with any IANA time-zone name.{ tz: "UTC" }preserves the pre-1.4 behaviour;{ tz: "America/New_York" }fires at that zone's wall-clock time regardless of the server'sTZ.Repro
$ TZ=America/Los_Angeles bun -e 'console.log(Bun.cron.parse("0 9 * * *", new Date("2026-06-15T00:00:00Z")).toISOString())'Before:
2026-06-15T09:00:00.000Z(9:00 UTC, regardless of TZ)After:
2026-06-15T16:00:00.000Z(9:00 AM PDT)Implementation
Local-time walk
JSGlobalObject::{ms_to_gregorian_date_time, gregorian_date_time_to_ms}local-time wrappers (the underlyingBun__msToGregorianDateTime/Bun__gregorianDateTimeToMSC++ bindings already take alocalTimeflag).CronExpression::next()walks wall-clock local time: minute/hour are carried manually so the candidate is checked against the bitfields before DST shifts it, and date+weekday are normalized via a UTC round-trip (pure calendar math, avoids TZ DB lookups in the inner loop).resolve_local_match()converts a matching wall-clock minute to a real instant, handling DST:30 2 * * *runs at 3:30 on the spring-forward day). For multi-minute patterns inside the gap, only the first match fires (croner semantics).*minute or*hour fires through both occurrences (once per real-time minute), matching cronie/Vixie.{ tz }optionbindings.cpp:Bun__resolveTimeZoneID/Bun__msToGregorianDateTimeInZone/Bun__gregorianDateTimeToMSInZonewrap JSC's Temporal ICU bridge (intlResolveTimeZoneID,getOffsetNanosecondsFor,exactTimeToLocalDateAndTime,getEpochNanosecondsForwithCompatibledisambiguation). This reuses JSC's per-TimeZoneUCalendarLRU cache and its DST gap/fold handling rather than reimplementing either.cron_parser.rs:CronTz::{Local, Named(u32)}parameterises the zone-dependent calls.cron.rs:resolve_cron_tz()reads{ tz }from the third argument of bothBun.cron.parseand the in-process callback overload;CronJobstores the resolved id.Test fixtures
cron.test.ts: thedescribe("Bun.cron.parse")block (~40 tests) callsparse()in-process and assertsDate.UTC(...)values; pinned toEtc/UTCviabeforeAll/afterAllso it stays host-independent (test/preload.tsdeliberately skipsTZwhen syncingbunEnvintoprocess.env).ms_to_gregorian_date_time_utcwrapper;gregorian_date_time_to_ms_utcstays (used by the SQL date types).Tests
test/js/bun/cron/cron-local-time.test.ts(25 tests): TZ-sensitive parsing across LA/Tokyo/Auckland, the full DST transition matrix (US 1h shifts, Lord Howe 30-min shifts, Santiago midnight gap), and a{ tz }option block (override vs. process TZ, DST under the override, error handling, in-process callback with tz under fake timers).test/js/bun/cron/cron-parse.test.ts: algorithm tests spawn underTZ=UTCso assertions are independent of the host's zone; the boundary/overflow tests are kept.in-process-cron.test.tsandcron.test.tspass unchanged underTZ=Asia/TokyoandTZ=America/Los_Angeles.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/cron/cron.test.ts