Skip to content

fix(core): preserve calendar anchors after DST gaps - #5354

Merged
Astro-Han merged 3 commits into
apache:mainfrom
seekskyworld:fix/calendar-dst-anchor
Oct 10, 2026
Merged

Astro-Han merged 3 commits into
apache:mainfrom
seekskyworld:fix/calendar-dst-anchor

Conversation

@seekskyworld

@seekskyworld seekskyworld commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Daily and weekly calendar schedules rebuild each occurrence from the intended date and the anchor's clock time. Computing a 02:30 schedule on a spring DST transition day no longer moves later occurrences to 03:30.

Calendar recurrences preserve JavaScript Date's existing policy: shift nonexistent local times forward and choose the first occurrence in a fold. Cron retains its separate skip-gap policy. The contract and consecutive-fire coverage now make this distinction explicit for daily, weekly and monthly recurrences.

Fixes #5353

Verification

  • Full repository build, typecheck, lint, format, Desktop/UI knip, ASF headers and diff checks passed on Node 24.18.1.
  • All 845 Core tests passed. Timezone subprocesses cover New York, Lord Howe, seconds/milliseconds, folds, and consecutive daily/weekly/monthly fires across a gap.
  • The original regression fails against the old implementation. Gap-day normalization is retained; subsequent occurrences return to the anchor wall time.
  • Full application suites and native UI E2E were not run for this Core change.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex investigated the issue and review feedback, authored the changes and tests, and ran validation. This is an automated PR update. Commits include Signed-off-by and Generated-by trailers; final review and merge remain human decisions.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Generated-by: OpenAI Codex
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
@seekskyworld
seekskyworld force-pushed the fix/calendar-dst-anchor branch from 02a282f to 059d063 Compare September 15, 2026 16:59

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — I re-derived the behavior locally (Node 26, TZ set per zone) instead of relying on the PR narrative. No blocking issues found; one policy question and a couple of nits below.

What the fix does (confirmed)

The root cause is exactly as described: the old daily/weekly branches mutated a copy of after with setHours(anchor…) and then setDate(+1), so on a spring-gap day the Date normalization of the nonexistent wall time (02:30 → 03:30 in America/New_York) leaked into the following occurrence. Old vs. new, side by side:

zone anchor after old new
America/New_York 2026-03-01T02:30:15.125-05:00 (Sun) 2026-03-08T04:00-04:00 daily → Mar 9 03:30, weekly → Mar 15 03:30 daily → Mar 9 02:30, weekly → Mar 15 02:30
Australia/Lord_Howe 2026-09-27T02:15:15.125+10:30 (Sun) 2026-10-04T04:00+11:00 daily → Oct 5 02:45, weekly → Oct 11 02:45 daily → Oct 5 02:15, weekly → Oct 11 02:15

Rebuilding every candidate from the anchor's wall time (scheduled-task.ts:631-640) is the right shape, and it makes daily/weekly consistent with the monthly path, which already built each candidate fresh via addMonthsClamped (scheduled-task.ts:644-649, :652). All three calendar recurrences now share one policy.

Verified

  • Test expectations are correct, and the test really regresses. All four scenarios in scheduled-task.test.ts:185-236 match the patched implementation epoch-exactly (ms included), and three of them (NY gap ×2, Lord Howe ×2) produce different values on the pre-fix code — so the "fails without the change" checkbox holds.
  • Gap-day normalization is preserved, as the PR states. 02:30 still fires 03:30 on 2026-03-08 (scenario 2). I confirmed the same for Europe/Berlin (Mar 29), Australia/Sydney (Oct 4), Pacific/Auckland (Sep 27) and the 2-hour gap in Antarctica/Troll (Mar 29 → 04:30), so half-hour/large gaps behave the same way.
  • Fold policy unchanged. Old and new agree for America/New_York 2026-11-01 with a 01:30 anchor: only the first 01:30 (EDT) fires and the next occurrence is Nov 2 — i.e. no double-fire, same as before.
  • Anchors stay stable over time. Iterating fire → recompute for a full year in New York, Berlin, Lord Howe, Chatham, Kolkata, Santiago, Sydney, Auckland and Troll: strictly increasing at every step, weekday preserved for weekly, wall clock preserved on every day except the single gap day, and no drift afterwards. The fallback occurrence(dayOffset + periodDays) is 1–13 days ahead, so it can never be ≤ after — the next > after ? … : … form is safe.
  • Non-DST days are value-identical to the old code in every case I swept (diff count 0), and month-end overflow (Jan 31 + 1 → Feb 1) is handled by Date normalization rather than skipped.
  • Test approach matches repo convention: spawn process.execPath with TZ in env and import the built module by URL, same as cron-expression.test.ts:136-166. That works here because core tests run from dist, and the core CI job is ubuntu-24.04 only, so the TZ-env dependency isn't a portability risk.

Questions / nits (non-blocking)

  1. The gap-day policy is inconsistent with the cron path in the same package — worth confirming this is intended. cron-expression.ts:240-242 deliberately advances in epoch minutes so a nonexistent local time is never emitted, and cron-expression.test.ts:155 asserts 30 2 * * * evaluated on 2026-03-08 fires 2026-03-09 02:30 (the gap day is skipped). A daily calendar task anchored at 02:30 instead fires at 03:30 that day. Since both mechanisms can express the same user intent, it would help to spell the calendar policy out in the doc comment on nextCalendarFireAt (or align the two) so the retained shift doesn't read as the same bug class as #5353.
  2. nit: the regression test asserts only one step per scenario. Looping fire → recompute across the transition (Mar 8 → Mar 9 for NY) would lock in the "a gap-normalized hour must not carry into the next date" invariant this PR is actually about, and would catch a re-introduced setDate mutation.
  3. nit: the monthly path shares the gap-normalization policy but has no DST assertion; one extra case (e.g. a Jan-31 anchor across Mar 8) would document the shared behavior cheaply.

Generated-by: OpenAI Codex
Signed-off-by: seekskyworld <djh1813553759@gmail.com>

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result

I found no correctness or scope issues at exact head 66ca1a3cdba55b7f27602dce9ac20a133b21cafa.

The defect is reproducible in the calendar scheduler. With an America/New_York anchor at 02:30, the previous daily and weekly branches first normalized the spring-gap date to 03:30 and then carried that adjusted hour into March 9 and March 15. Reconstructing each candidate directly from its intended date and the anchor's clock fields returns those ordinary dates to 02:30 while preserving JavaScript Date behavior on the gap date itself.

The implementation is a small repair at the scheduling authority. Daily and weekly now share one occurrence builder, while monthly retains its existing fresh-candidate path. The new function comment also makes the policy explicit: calendar recurrences shift a nonexistent local time forward and choose the first occurrence in a fold, whereas cron skips a nonexistent local time.

The follow-up test covers two consecutive fires for daily, weekly, and monthly schedules. Replacing the built implementation with the previous production branches made both DST-focused tests fail: daily and weekly retained the one-hour adjustment after the gap, while the monthly control remained stable. Restoring the change made both tests pass.

Exact-head Core passed 845/845 tests. I also tested the clean synthetic merge with current main (cd93f13da78d3f8aa7f480f509bf80b8bd024cec): Core passed 854/854 tests, and Biome, ASF headers, the Windows test inventory, and git diff --check passed. The synthetic merge tree is c7e681786863f590c4f00b1dfa8aa21277c95078. The hosted exact-head test is complete and successful.


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 18, 2026

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.

Summary

Fixes calendar recurrence across DST gaps by rebuilding each daily/weekly occurrence from the anchor's wall-clock fields on every iteration (nextCalendarFireAt, scheduled-task.ts:628-648), so a spring-forward normalization on one date no longer shifts subsequent cycles' wall time. The issue is real (previously the normalized occurrence time propagated forward), the fix is minimal and at the right layer, and the monthly path (addMonthsClamped) correctly clamps to month end. Tests run computeNextFireAt in child processes under TZ=America/New_York and TZ=Australia/Lord_Howe via execFileSync, covering both hemispheres' transitions plus fall-back, and the "returns to anchor wall time after firing on a DST gap day" case pins the non-propagation property directly. CI green (test 12m53s).

Findings

  1. [P3] packages/core/src/scheduled-task.ts:619-622 — the new explanatory comment block is written in Chinese ("日历重复保留 Date 的本地时间规则…"); repo convention keeps explanatory comments in English (CJK appears only in locale string literals). Same class as #5373; translate for community reviewability.
  2. [P3] The DST tests depend on the host tz database via TZ env in child processes — fine on Linux CI, but worth noting that Windows runners have no TZ support, so these tests would silently test UTC semantics if ever run on Windows. Currently the suite runs on ubuntu, so no action needed.

Verdict

merge-ready — correct DST semantics restored with genuine cross-timezone regression coverage; two P3 notes only.

Generated-by: OpenAI Codex
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
@seekskyworld

Copy link
Copy Markdown
Contributor Author

Translated the calendar DST rationale into English in ad387e3. This is comment-only; diff check passes. The existing timezone regression evidence remains scoped to the tested Node/macOS/Linux environments; no new Windows claim is made.

Automated update by OpenAI Codex under the contributor’s direction.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review: 059d0636 (our previous review) to ad387e3a. Same merge-base (dd15b63c), no rebase or main merge: git range-diff shows the fix commit unchanged plus two author commits (66ca1a3c test + doc comment, ad387e3a comment wording). The increment adds 33 test lines and a 5-line doc comment, with no logic change.

Prior findings

  1. Addressed - gap-day policy differs from cron. nextCalendarFireAt now documents it (scheduled-task.ts:619-623): calendar recurrence follows Date local-time rules (shift gaps forward, take the first repeated time) and rebuilds from the anchor, while cron skips nonexistent times. This matches what I verified previously (fold: first occurrence only; gap: shift forward on the gap day only).
  2. Fixed - single-step assertions. The new test (scheduled-task.test.ts:241-272) fires twice in a row from 2026-03-08T00:00-05:00 in America/New_York. It asserts the gap-day fire at 03:30 EDT and then a return to 02:30 on the next cycle. This is the exact invariant a reintroduced setDate mutation would break.
  3. Fixed - monthly DST case. The same test covers a monthly Jan-8 02:30 anchor: Mar 8 03:30 EDT, then Apr 8 02:30 EDT.

I re-derived the expected epochs by hand (weekly anchor Sun Mar 1, so Mar 8 and then Mar 15; monthly Jan 8, so Mar 8 and then Apr 8). They are correct, and on the pre-fix code the daily/weekly second fire would be 03:30, so the test regresses.

New findings

None. Locally (core built at head): scheduled-task.test.js 18/18 pass.

No protocol files touched, so no epoch impact.

CI / mergeability: hosted test check passes on ad387e3a. The branch is well behind main but git merge-tree against current main is clean.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at @Astro-Han's explicit request: the automated review of this exact head found no blocking (P0–P2) issues, and CI is green.

@Astro-Han
Astro-Han merged commit 45525d7 into apache:main Oct 10, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): calendar schedules carry DST gap adjustments into later occurrences

4 participants