Skip to content

fix: use floor() instead of trunc() for pre-epoch fractional seconds - #743

Merged
thomasvl merged 4 commits into
google:mainfrom
rootkiller6788:fix-gtlrdatetime-negative-milliseconds
Sep 7, 2026
Merged

thomasvl merged 4 commits into
google:mainfrom
rootkiller6788:fix-gtlrdatetime-negative-milliseconds

Conversation

@rootkiller6788

Copy link
Copy Markdown
Contributor

Problem

GTLRDateTime drops the fractional-second component for dates before the Unix epoch. In setFromDate:, the fraction is computed as asTimeInterval - trunc(asTimeInterval). For a pre-1970 date timeIntervalSince1970 is negative and trunc() rounds toward zero, leaving a negative fraction. Both -date and -RFC3339String only apply a positive milliseconds value, so the fraction is silently dropped and the result is off by up to ~1 second, violating the documented 0–999 milliseconds invariant.

Change

Use floor() instead of trunc() so the fraction always stays in [0, 1).

Test

Added testFractionalSecondsBeforeEpoch, which round-trips pre-epoch dates with fractional seconds (-0.5 and -1.25 seconds since the epoch) through -RFC3339String and -date and asserts the milliseconds are preserved.

For dates before 1970, timeIntervalSince1970 is negative and trunc() rounds toward zero, leaving a negative fraction that -date and -RFC3339String silently drop (both only apply positive milliseconds). Use floor() so the fraction stays in [0, 1).
@thomasvl

Copy link
Copy Markdown
Member

@rootkiller6788 - Are you still working on this? It was opened as a draft, so I wasn't sure if you wanted anyone to look at it yet or not.

@rootkiller6788

Copy link
Copy Markdown
Contributor Author

Thanks for checking in, and sorry for the quiet. The fix is complete and CI is green. setFromDate: now uses floor() instead of trunc() so the fractional-second component is preserved for dates before the epoch — with trunc() the fraction went negative and both -date and -RFC3339String dropped it. I added testFractionalSecondsBeforeEpoch, which round-trips 1969-12-31T23:59:59.500Z and 1969-12-31T23:59:58.750Z through -date/-RFC3339String and asserts the milliseconds survive. I'd left it as a draft to sanity-check the approach first, but it's ready for review now — could you please take a look?

@rootkiller6788
rootkiller6788 marked this pull request as ready for review August 29, 2026 14:05
round() on the fraction can yield 1000 when the fraction is within half
a millisecond of the next second, which would render a four-digit
".1000" fraction and break the 0-999 milliseconds invariant (both
before and after the epoch). Clamp to 999 so the value always matches
the calendar's floored whole-second components.

Also rework the pre-epoch test to be table-driven and cover whole
seconds, the rounding clamp boundary, and an offset.
@rootkiller6788

Copy link
Copy Markdown
Contributor Author

Fixes a bug where GTLRDateTime silently dropped fractional seconds for dates before the Unix epoch.

In setFromDate: the fractional part was computed as asTimeInterval - trunc(asTimeInterval). For pre-1970 dates
timeIntervalSince1970 is negative, so trunc() rounds toward zero and leaves a negative fraction. Both -date and
-RFC3339String only apply a positive milliseconds value, so that fraction got thrown away — pre-epoch dates with
sub-second precision came back off by up to a second.

Switching to floor() keeps the fraction in [0, 1), matching what the calendar components already do.

Also fixed while I was in there

round(worker * 1000) returns 1000 when the fraction is within half a millisecond of the next second, which would
render a four-digit .1000 fraction and break the documented 0–999 milliseconds invariant (this happens after the epoch
too, it's not new). Added a clamp to 999.

Test

Reworked testFractionalSecondsBeforeEpoch to be table-driven, matching the style of testFractionalSeconds. It now
covers:

  • a fractional second (-0.5)
  • a whole second plus a fraction (-1.25)
  • a whole second with no fraction (-1.0)
  • the rounding clamp boundary (-0.0001 → .999)
  • a pre-epoch date with an offset (-01:00)

Each case also round-trips through -date to confirm the milliseconds survive.

@thomasvl

Copy link
Copy Markdown
Member

The rounding pushing to 1000 is interesting, why should we always pull that back to 999 instead of making the fraction 000 and adding 1 more second? i.e. - .9999 -> why should that be .999 vs. 1.000?

Haven't really thought about it, but the code change made me think of it.

@thomasvl

Copy link
Copy Markdown
Member

Expanding on my thoughts a little - since we round in general, seems like we should still round vs. force things dow of that one case. Looks like the old code had this same issue and we hadn't realized it, so it's been a bug from that pov for a while.

round() can give 1000ms when the fraction is within half a millisecond
of the next second. Clamping that back to 999ms was forcing the value
down; roll the extra second into the date components instead so it
rounds the same way every other fraction does.
-0.0001 is 1969-12-31T23:59:59.9999, which rounds up to the epoch rather
than clamping to .999.
@rootkiller6788

Copy link
Copy Markdown
Contributor Author

Thank you for the review, and good point. You're right that clamping to 999 forced the value down when it should have rounded up like every other fraction. I've updated setFromDate: so that when round() carries the fraction to 1000ms, the extra second is folded back into the date components (re-extracting through the calendar so the carry normalizes across minute/hour/day boundaries) and the milliseconds reset to 0. So -0.0001 (1969-12-31T23:59:59.9999) now renders as 1970-01-01T00:00:00Z rather than clamping to .999 — and this also covers the latent case you noticed in the old code for dates after the epoch. I updated the boundary test case in testFractionalSecondsBeforeEpoch to expect the roll-over. Could you please take another look?

@thomasvl thomasvl 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.

Thank you for the report and fix!

@thomasvl
thomasvl merged commit 7a6df46 into google:main Sep 7, 2026
38 of 39 checks passed
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.

2 participants