Skip to content

fix: keep startOf/endOf below a day inside the current UTC offset - #3176

Open
maximilliangrand wants to merge 1 commit into
iamkun:devfrom
maximilliangrand:fix/startof-endof-dst-fallback
Open

fix: keep startOf/endOf below a day inside the current UTC offset#3176
maximilliangrand wants to merge 1 commit into
iamkun:devfrom
maximilliangrand:fix/startof-endof-dst-fallback

Conversation

@maximilliangrand

Copy link
Copy Markdown

The contract that breaks

startOf(unit) clears the fields below unit and endOf(unit) fills them. Three things follow, and Day.js depends on all three:

  1. t - t.startOf('second') === t.millisecond() — clearing milliseconds cannot move the clock.
  2. startOf(u) <= t <= endOf(u).
  3. isSame, isBefore and isAfter are rule 2 — isSame(that, u) is literally this.startOf(u) <= other && other <= this.endOf(u) (src/index.js).

On any host in a DST timezone, for one hour a year, all three are false.

Reproduction, on 1.11.21, no plugins

// TZ=Europe/Berlin — 2024-10-27T01:30:45.123Z is 02:30:45.123 +01:00,
// the second pass through 02:30 after the 03:00 -> 02:00 fall back.
const d = dayjs('2024-10-27T01:30:45.123Z')

d.startOf('second').toISOString()             // 2024-10-27T00:30:45.000Z
d.endOf('second').toISOString()               // 2024-10-27T00:30:45.999Z
d.startOf('minute').toISOString()             // 2024-10-27T00:30:00.000Z
d.startOf('hour').toISOString()               // 2024-10-27T00:00:00.000Z

d.valueOf() - d.startOf('second').valueOf()   // 3600123   (d.millisecond() is 123)
d.isSame(d, 'second')                         // false
d.isBefore(d, 'second')                       // true      <- a date is before itself

endOf('second') lands before the instant it came from, so anything bucketing, deduplicating or cache-keying by "same minute / same second" is silently wrong for that hour. No error, no invalid date.

Root cause — src/index.js, startOf -> instanceFactorySet

const instanceFactorySet = (method, slice) => {
  const argumentStart = [0, 0, 0, 0]
  const argumentEnd = [23, 59, 59, 999]
  return Utils.w(this.toDate()[method].apply( // eslint-disable-line prefer-spread
    this.toDate('s'),
    (isStartOf ? argumentStart : argumentEnd).slice(slice)
  ), this)
}

hour, minute and second are setMinutes(0,0,0) / setSeconds(0,0) / setMilliseconds(0) on a local Date. Per ECMA-262 every local setter reads LocalTime(t), replaces fields, then maps back with UTC(t) = t - LocalTZA(t, false). That inverse is not injective across a fall back, and the spec resolves the repeated wall clock to the offset before the transition. So for an instant whose wall clock sits in the repeated window the setter hands back an instant one whole offset step earlier — including setMilliseconds(0), which is not supposed to move anything.

Moment, which this library tracks and which these tests already use as the reference, sidesteps it by truncating the timestamp arithmetically — time -= mod(time + utcOffset * MS_PER_MINUTE, MS_PER_HOUR) for hour, and time -= mod(time, unitMs) for minute and second — never touching the ambiguous local->UTC inverse.

startOf('day') is not affected and is deliberately left alone: the local day really does begin at the first midnight, which is exactly what the first-occurrence rule returns, and it matches moment's new Date(y, m, d).

Note on overlap: open PR #3040 restructures this same instanceFactorySet (for the $offset path, and to express endOf as start-of-next minus 1 ms). I checked out its head (e8a7d76) and ran the repro above against it — it still returns 3600123 with isSame(d, 'second') === false — so it does not cover this bug, but the two will conflict textually and I am happy to rebase on whichever lands first.

The fix

instanceFactorySet takes a keepOffset flag, passed only for hour/minute/second. It reads the local offset before and after the setter and, when they differ, restores the original one — but only after confirming that offset is genuinely in force at the corrected instant:

const keep = oldOffset === newOffset ||
  new Date(shifted).getTimezoneOffset() === oldOffset

That last clause is what separates a repeated wall clock (fall back — the corrected instant exists at the original offset, so restore it) from a nonexistent one (spring forward — it does not, so leave the runtime's resolution exactly as it was). UTC-mode instances use setUTC* and have no ambiguity, so they take an early return onto the old path along with day/date — which means startOf('day') does not pay for any of this. oldOffset === newOffset is the fast path: no extra Date is built on a non-DST host, or anywhere away from a transition.

Evidence

Fail-then-pass on a rebuilt origin/dev, then on the built bundle:

BASELINE  TZ=Europe/London      1 failed, 6 passed
BASELINE  TZ=Pacific/Auckland   1 failed, 6 passed
BASELINE  TZ=America/Whitehorse 1 failed, 6 passed
FIXED     all four npm-test legs 7 passed
grep -o getTimezoneOffset dayjs.min.js | wc -l    baseline 1   fixed 4
TZ=Europe/Berlin, built bundle:
  baseline  t - startOf('second') = 3600123   isSame(d,'second') = false
  fixed     t - startOf('second') = 123       isSame(d,'second') = true

Regression surface, differential against moment 2.29.2 plus offset-independent invariants — 17,904 instants per zone (3,000 pseudo-random over 1990-2036 plus a dense sweep around every host transition, at both round and 7,919 ms offsets) x 9 units x {local, utc, utcOffset(330), utcOffset(45), utcOffset(-60)} x {startOf, endOf}, across 18 host timezones:

                          diverging cases
                          base       fixed     fixed(-)   new(+)
TOTAL                  6566759     6206684      360075        0

360,075 cases corrected, zero new divergences. Included: Europe/Berlin, Europe/London, Europe/Dublin, America/New_York, America/Whitehorse, America/St_Johns, America/Havana, America/Santiago, America/Sao_Paulo, Australia/Sydney, Australia/Lord_Howe, Pacific/Auckland, Pacific/Chatham, Asia/Tehran, Asia/Gaza, Asia/Kolkata, Africa/Casablanca, UTC.

Separately, a containment sweep (startOf(u) <= t <= endOf(u) and isSame(self, u)) at 1-minute granularity ±2 h around every host transition 1990-2036 goes to zero violations for second/minute/hour in Europe/Berlin, Europe/London, America/New_York, Pacific/Auckland and Australia/Lord_Howe, with no new violations in any zone.

The timezone-plugin path is untouched: with utc + timezone loaded, isSame(self, u) violation counts under .tz(...) are identical between the baseline and patched builds.

Cost: one extra getTimezoneOffset() on startOf/endOf of hour/minute/second, about 20 ns per call on operations already costing ~330 ns on my machine. I measured this several ways and the percentage is not stable enough to quote precisely — well-controlled isolated runs put it around 6%, but it moves a lot with machine load, so treat it as "one extra getTimezoneOffset()" rather than a firm number. day/date/month and UTC-mode instances take the early return and measure flat. Bundle 2.78 -> 2.86 KB gzipped, against the repo's 2.99 KB size-limit budget.

Test

One case in test/timezone.test.js, the file npm test already re-runs under Pacific/Auckland, Europe/London and America/Whitehorse. It finds the host's own one-hour fall backs from getTimezoneOffset between 2015 and 2030, samples an instant inside each repeated hour, and asserts agreement with moment plus isSame(self, unit) plus the truncation identities. Host-agnostic by construction: it exercises the bug on the DST legs and finds no transitions (so passes trivially) on a UTC runner. It also passes on the other zones I tried by hand, including Pacific/Chatham, Australia/Lord_Howe, America/Havana, Asia/Tehran, Africa/Casablanca and Pacific/Apia.

npm run lint clean. npm test green and exits 0: 93 suites, 774 tests (773 before). src line coverage stays 100% under both a DST host and TZ=UTC, so the lines: 100 threshold holds.

What I could not verify

  • A residual remains, and it is not fully closed. In zones whose DST transition is not on an hour boundary — Pacific/Chatham at :45, historically America/St_Johns and Asia/Gaza at :01 — when the truncated wall clock is one that never existed, startOf('hour') can still land after the instant, so isSame(self, 'hour') can still be false there. Concretely, under TZ=Pacific/Chatham, 2035-09-29T14:00:00Z is 2035-09-30 03:45 +13:45; startOf('hour') returns 04:00 and isSame(self, 'hour') is false — on both the baseline and this patch (moment returns 02:00). This is pre-existing, not introduced: in the containment sweep the patch takes Chatham's hour violations from 1380 to 690, St_Johns' from 4037 to 1298, Gaza's from 2878 to 118, Lord_Howe's to zero, and adds none anywhere. But it does not close that edge — doing so would mean replacing instanceFactorySet with moment's timestamp arithmetic, which also moves the day/date and $offset paths and is a much larger, riskier patch. The new test samples only one-hour fall backs, so it does not assert on this. minute and second do go to zero violations in every zone I measured, including these.
  • Those same zones also keep pre-existing day/date/week/month containment violations (e.g. St_Johns 1239 for day, unchanged at 1239). Those come from the separate instanceFactory midnight path, which this patch does not touch.
  • .utcOffset(n) instances disagree with moment for every unit, including year and month, on any DST host — and also on non-DST hosts. Pre-existing and untouched (this patch happens to fix 630 of those cases and breaks none); it looks like a separate issue in the utc plugin.
  • Branch coverage of src under TZ=UTC dips from 99.51% to 98.60%, because the corrective path is by definition unreachable without DST. Line coverage — the enforced threshold — stays at 100% and npm test exits 0. If a codecov status check is configured on branches rather than lines, this may flag.
  • Running the whole suite under a DST host shows a handful of failures — DST, a time that never existed, DayOfYear set, Should not interpolate characters inside square brackets, and UTC and utcOffset on half-hour-offset hosts. All reproduce identically on untouched dev (Whitehorse: 6 failed on both, same six names), and npm test only runs the full suite under the host default TZ, so CI is unaffected.
  • No browser / SauceLabs run (no credentials). The change is plain ES5: getTimezoneOffset, new Date(number), arithmetic.

The Date setters startOf/endOf use for hour, minute and second convert a
local wall clock back to an instant. At a DST fall back that wall clock
occurs twice and the conversion always picks the first occurrence, so the
result lands a whole offset step before the instant it was derived from:

  TZ=Europe/Berlin
  const d = dayjs('2024-10-27T01:30:45.123Z') // 02:30:45.123 +01:00
  d.startOf('second').toISOString() // 2024-10-27T00:30:45.000Z
  d.valueOf() - d.startOf('second').valueOf() // 3600123, not 123
  d.isSame(d, 'second') // false
  d.isBefore(d, 'second') // true

Restore the offset the instance started in when the setter crossed the
transition and that offset is the one in force at the corrected instant,
which leaves a wall clock that never existed resolved as before. Units of
a day and above keep building their result from local calendar fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant