Creating Time with Time.utc or Time.local should wrap 60 seconds around if not a valid leap second

[b-n]

I've been trying to clean up ruby spec for Time, and this is one of the failures in this specific suite: https://github.com/artichoke/artichoke/blob/trunk/spec-runner/vendor/spec/core/time/shared/gm.rb#L59-L69

⚠️ Important note about Timezone support for Artichoke vs. MRI - MRI relies on setting the environment variable TZ and then retrieving this information from the operating system. MRI then uses a bit of this information for parsing valid leap seconds etc. Artichoke only uses the environments local timezone for getting the offset, and thus the information about leap seconds is lost/not used.

artichoke/spinoso-time/src/time/tzrs/offset.rs

Lines 147 to 155 in af5fcd2

	pub fn local() -> Self {
	// Per the docs, it is suggested to cache the result of fetching the
	// local timezone: https://docs.rs/tzdb/latest/tzdb/fn.local_tz.html.
	static LOCAL_TZ: Lazy<TimeZoneRef<'static>> = Lazy::new(local_time_zone);
	Self {
	inner: OffsetType::Tz(*LOCAL_TZ),
	}
	}

The below

MRI:

3.1.2 > Time.utc(2022, 1, 1, 23, 59, 59)
=> 2022-01-01 23:59:59 UTC
3.1.2 > Time.utc(2022, 1, 1, 23, 59, 60)
=> 2022-01-02 00:00:00 UTC
3.1.2 > Time.utc(2022, 1, 1, 23, 59, 59) + 1
=> 2022-01-02 00:00:00 UTC

Artichoke: (after #2425 is merged)

>>> Time.utc(2022, 1, 1, 23, 59, 59)
=> 2022-01-01 23:59:59 UTC
>>> Time.utc(2022, 1, 1, 23, 59, 60)
=> 2022-01-01 23:59:60 UTC
>>> Time.utc(2022, 1, 1, 23, 59, 59) + 1
=> 2022-01-02 00:00:00 UTC

I believe this is a result of this: https://docs.rs/tz-rs/latest/tz/struct.DateTime.html#method.find which will return the same information it is provided, even if the leapsecond isn't valid.

[b-n]

It is entirely possible we could raise this issue with tz-rs, however I'm still contemplating if this is just a Ruby interpretation opinion, or whether tz-rs should actually provide a Datetime with the seconds value added/rounded into the date, and thus modifying the Datetime accessors respectively. Raising the issue here at least so it's tracked.

[b-n]

This is the reason #2223 Is still open - specifically most specs for Time,utc and Time.local aren't passing because of this 60 second wrap.

I've also just finished doing a bunch of research as to how this is handled in POSIX systems.

Specifically, https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap04.html#tag_21_04_16 gives a bunch of information as to what can be expected from POSIX itself. e.g. Leap seconds don't "exist". This can be seen by using date:

$ date --date='TZ="UTC" 2016-12-31T23:59:59+00:00' +%s   
1483228799
$ date --date='TZ="UTC" 2017-01-01T00:00:00+00:00' +%s    
1483228800

☝️ There should be an extra second in there. Note, date throws errors when trying to create any time with 23:59:60, regardless of whether that is a valid leap second or not.

The somewhat annoying part from Ruby docs, is that leap seconds is only mentioned here: https://ruby-doc.org/3.1.2/Time.html#method-c-utc with respects to "seconds is 0..=60" essentially.

This leads me to thinking that we should impement this in spinoso-time instead of in tz-rs, since it feels like more of a Ruby problem. IMO, providing :60 should not normally wrap around and would ideally throw an error (unless it was a valid leap second) - but my opinion is irrelevant to the MRI standard etc. In other words, I would be likely to implement an error in tz-rs, rather than wrapping it around - and as such, I think spinoso-time being a representation of Ruby Time is more fit for purpose.

[lopopolo]

I wonder if this is the sort of thing where we can use an MRI repl to generate times for all of 2015 and 2016 with 60 seconds in a time stamp, see what it spits out, and add a functional test for it.

If it's a simple as always rolling over or hardcoding when the leap seconds are (there aren't that many of them), let's do that.

Probably makes sense to have tests for all other leap second time stamps as well.

[lopopolo]

Thanks for digging on this @b-n 🙇

[b-n]

Ah, you triggered a thought in me just now. The reason I started looking at POSIX time, was due to how ruby spec uses the with_timezone function - which to my knowledge, isn't part of core or stdlib funnily enough. This was the only place I could find with_timezone being declared, which is of mild concern consider the test under spec (not mspec) is using a function also called with_timezone.

And now I just realised what is going on. UTC in POSIX is not UTC - right/UTC is the real one:

$ date --date='TZ="right/UTC" 2017-01-01T00:00:00+00:00' +%s   
1483228827
$ date --date='TZ="right/UTC" 2016-12-31T23:59:59+00:00' +%s  
1483228825
$ date --date='TZ="right/UTC" 2016-12-31T23:59:60+00:00' +%s   
1483228826

Ref: https://www.ucolick.org/~sla/leapsecs/right+gps.html which TL;DR: some design decisions in the past meant POSIX needed a new timezone to be able to support proper UTC.

So, the scary part with the spec test, is that under the hood, it's setting the timezone by temporarily changing the TZ env var of the system, which leads to the native C code finding the leapsecond.

I haven't look at it yet, but I suspect we just have to hard code it as you say. Part of me wonders whether we can upstream it into tzdb by getting a RIGHT_UCT to match https://docs.rs/tzdb/0.5.7/src/tzdb/generated/mod.rs.html#27391-27402 but with the leap seconds.

Funnily enough, that spec test will always set the ENV var though. Makes me mildly concerned that core/time for us also needs to check the ENV var and match on that value to find the correct timezone to pass into spinoso_time 🤔

[b-n]

Slight oof. It appears there is a right/ for all timezone's, and they are all adjusted for leap seconds. e.g. tzdb to support right/ timezones = double the generated size. This issue needs more thoughts, and deep ones haha.

Ref: https://github.com/Kijewski/tzdb/blob/v0.5.x/make-tzdb/src/main.rs#L92-L94

And specifically compare the /usr/share/zoneinfo/posix and /usr/share/zoneinfo/right directories on your favourite flavour of linux.

[b-n]

@lopopolo So, I think we're a bit blocked on the idea of getting leapseconds working (for now).

If we want to get leap seconds working, then we've got a couple of options:

• Find a crate that is able to at runtime peak into zoneinfo directly (instead of tzdb which is statically compiled). This comes with disadvantages of how cruby handles this - messing with env vars, + also multiplatform support will be "fun".
• Upstream to tzdb and/or design our own crate which statically compiles all zoneinfo for both unix time + right/ time. I imagine we could put features for specific zones, include/exclude right time etc.
  • If we did our own one, I'd be tempted to build based on the iana database directly - my understanding from a quick look into tzdb is that it's using the local files in /usr/share/zoneinfo for compilation.
  • regardless of solution, if we want to be compliant with MRI, we would still be intercepting ENV values, and likely parsing in the trampoline, or passing them through to spinoso-time with a with_timezone string.

For the interim however, it feels like we should turn off the leapseconds test. I don't know of a way to do this apart from patching over the test in spec to just turn it off, and implementing something in spinoso-time which just rolls the second over.

[lopopolo]

@b-n I marked this as blocked. We can leave it open for now, but I think there's probably some other work we can take on before getting back to this. The reliance on ENV for setting timezone here isn't something I think we should feel required to tie ourselves to.