-
Notifications
You must be signed in to change notification settings - Fork 5
Timestamp type safety #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jagerman
wants to merge
14
commits into
session-foundation:dev
Choose a base branch
from
jagerman:ts-safety
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7c13dfd
to
a37e00c
Compare
This uses std::chrono::sys_seconds for the new profile_updated timestamp field, so that we can avoid the unfortunate bug that happened in earlier releases where some clients violated the documented restrictions and passed milliseconds or microseconds: sys_seconds is absolutely unambiguous as to what it holds and makes such an error virtually impossible. - Adds `session::to_sys_seconds(int)` that can get a proper sys_seconds from a maybe-s, ms, or µs value by guessing based on the magnitude. - Add methods for getting/setting sys_seconds timestamps from/into configs.
There are a lot of internal calls such as `maybe_int(c, "key").value_or(0)` and `maybe_string(c, "key").value_or("")`. This makes it them slightly less cumbersome by adding `int_or_0` and `string_or_empty` functions that handle the fallback-to-0/empty automatically.
Taking raw integers has led to clients passing invalid values in past releases (milliseconds or microseconds). This is, however, quite damaging as it means there are *three* possible representations of the same config, depending on which of s/ms/us gets stored, and while we have patched around those issues, there is still a type safety concern as it could happen again. This fixes it so that we *always* store timestamps as std::chrono::sys_seconds making it completely unambiguous what it holds, and ensuring that the proper values gets passed in (either through direct construction, or by calling session::to_sys_seconds to explicitly guess based on the magnitude). This should keep any milliseconds or microseconds usage strictly within the API and make it very difficult for such an erroneous value to end up inside the actual serialized config. This also updates volatile last_read in a similar way: but since that one is always unix epoch milliseconds, using a std::chrono::sys_time<std::chrono::milliseconds> (typedefed to session::sys_milliseconds) for the type safety. Unlike the above, this one has never had ambiguous values and so doesn't need the same heuristics. Making it typesafe makes it stay that way.
mpretty-cyro
approved these changes
Jul 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Builds on #54 and #52; only the last commit here is relevant)
Replaces internal timestamp
int64_t
storage withstd::chrono::sys_seconds
.This introduces one breaking change to the API: C++ methods retrieving a timestamp now return a
std::chrono::sys_seconds
instead of anint64_t
. Additionally the loading methods that take an int64_t are now overloaded with a sys_seconds version, and the int64_t versions are marked deprecated.Taking raw integers has led to clients passing invalid values in past releases (milliseconds or microseconds). This is, however, quite damaging as it means there are three possible representations of the same config, depending on which of s/ms/us gets stored, which violates our requirement that serialized config values have unique representations. While we have patched around those issues, there is still a type safety concern as it could happen again.
This fixes it so that we always store timestamps as
std::chrono::sys_seconds
making it completely unambiguous what it holds, and ensuring that the proper values gets passed in (either through direct construction, or by callingsession::to_sys_seconds
to explicitly guess based on the magnitude). For existing values that might have been broken we still apply the heuristic when loading timestamp values from serialized data, but for all new ones we can avoid the need to do so.This should keep any milliseconds or microseconds usage strictly within the API and make it very difficult for such an erroneous value to end up inside the actual serialized config.
(As this breaks API, this bumps the version to 1.6.0, and probably should be merged sometime later than #52/#54 which apply this change to new timestamp fields, but not existing ones).