Skip to content

fix: parsing end dates on systems with 32-bit time_t #24

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.

Sign up for GitHub

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

Closed
greghuels wants to merge 1 commit into from
Closed

fix: parsing end dates on systems with 32-bit time_t #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,27 @@ jobs:

- name: Run memory tests
run: make test-memory

test-32bit-time-t:
runs-on: ubuntu-latest
name: Test with 32-bit time_t

steps:
- name: Checkout code
uses: actions/checkout@v5
with:
repository: ${{ github.event.pull_request.head.repo.full_name || 'Eppo-exp/cpp-sdk' }}
ref: ${{ env.SDK_BRANCH_NAME }}

- name: Set up QEMU for multi-arch
uses: docker/setup-qemu-action@v3

- name: Test with 32-bit time_t
run: |
docker run --rm --platform linux/386 -v $PWD:/workspace -w /workspace debian:bookworm bash -c "
Comment on lines +136 to +138
Copy link

@maciej-perplexity maciej-perplexity Nov 26, 2025

Choose a reason for hiding this comment

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

Not sure if this represents my case, this might be a windows vs linux problem rather than 32 vs 64. I've had this problem with x64 Windows that has 64-bit time_t - the problem is caused by the use _mkgmtime. Would be good to test on windows too, but I understand it could be a logistical problem

apt-get update &&
apt-get install -y build-essential g++ git make &&
git clone -b ${{env.TEST_DATA_BRANCH_NAME}} --depth 1 --single-branch https://github.com/Eppo-exp/sdk-test-data.git test/data/ &&
make clean &&
make test
"
9 changes: 4 additions & 5 deletions src/config_response.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,13 +312,12 @@ void to_json(nlohmann::json& j, const Allocation& a) {
j = nlohmann::json{{"key", a.key}, {"rules", a.rules}, {"splits", a.splits}};

if (a.startAt.has_value()) {
auto time = std::chrono::system_clock::to_time_t(a.startAt.value());
j["startAt"] = time;
j["startAt"] = formatISOTimestamp(a.startAt.value());
}

if (a.endAt.has_value()) {
auto time = std::chrono::system_clock::to_time_t(a.endAt.value());
j["endAt"] = time;
// Use special formatting for endAt to handle year 9999 sentinel value
j["endAt"] = formatISOTimestampForConfigEndTime(a.endAt.value());
}

if (a.doLog.has_value()) {
Expand All @@ -344,7 +343,7 @@ void from_json(const nlohmann::json& j, Allocation& a) {

if (j.contains("endAt")) {
auto timeStr = j.at("endAt").get<std::string>();
a.endAt = parseISOTimestamp(timeStr);
a.endAt = parseISOTimestampForConfigEndTime(timeStr);
}

if (j.contains("doLog")) {
Expand Down
38 changes: 35 additions & 3 deletions src/time_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,22 @@

namespace eppoclient {

std::chrono::system_clock::time_point parseISOTimestamp(const std::string& timestamp) {
std::chrono::system_clock::time_point parseISOTimestampForConfigEndTime(
const std::string& timestamp) {
// Check for the sentinel value that represents "no end date"
if (timestamp == "9999-12-31T00:00:00.000Z") {
return std::chrono::system_clock::time_point::max();
}
return parseISOTimestamp(timestamp, std::chrono::system_clock::time_point::max());
Comment on lines +10 to +14
Copy link

@maciej-perplexity maciej-perplexity Nov 26, 2025

Choose a reason for hiding this comment

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

this still feels arbitrary, I'll echo the concern @dd-oleksii had. What if someone sets (explicitly) that an assignment ends in the year 5000? Or, more realistically, 2039.

I think it might be better to make sure parseISOTimestamp is overflow-safe on its own, and "naturally" saturates to time_point::max() if timestamp is bigger than you can represent in a time_point on this platform. And I think your changes already (almost) do it, so no need for this check.

The platform I've tested on that had the problem was Windows 11 x64:

  • std::get_time works as expected for 9999 and returns a valid std::tm with tm_year 8099, tm_mon 11, tm_mday 31
  • time_c = _mkgmtime(&tm); returns -1 even though std::time_t is 64 bit! Same with _mkgmtime64
  • after your change with return errorValue; it should be enough to handle this case
  • the one thing that can still go wrong is overflow in tp += std::chrono::milliseconds(milliseconds);, if tp is almost on max already. Maybe skip the whole millisecond parsing? 😄

Also, I have another idea:

  • change return type of parseISOTimestamp to optional<time_point> and return a nullopt for when the timestamp cannot be represented on the platform
  • this makes sense because the callsite is a.endAt = parseISOTimestamp and a.endAt is already an optional
  • and the code that matches assignments, shards etc. skips the date check when the timestamp is nullopt
  • behaviorally, this ends up working the same as returning "max" or "min" depending on call context, but without needing parseISOTimestampForConfigStartTime and parseISOTimestampForConfigEndTime

Copy link

@maciej-perplexity maciej-perplexity Nov 26, 2025

Choose a reason for hiding this comment

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

Hah, I found this:

The range of the _mkgmtime32 function is from midnight, January 1, 1970, UTC to 23:59:59 January 18, 2038, UTC. The range of _mkgmtime64 is from midnight, January 1, 1970, UTC to 23:59:59, December 31, 3000, UTC. An out-of-range date results in a return value of -1.

I expected the 64 bit version would have astronomical upper time limit, because why not, but 9999 really is out of range on Windows 🙃

Copy link
Collaborator Author

@greghuels greghuels Dec 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might be better to make sure parseISOTimestamp is overflow-safe on its own, and "naturally" saturates to time_point::max() if timestamp is bigger than you can represent in a time_point on this platform. And I think your changes already (almost) do it, so no need for this check.

Yeah, this is a better approach. I'll update!

}

std::chrono::system_clock::time_point parseISOTimestampForConfigStartTime(
const std::string& timestamp) {
return parseISOTimestamp(timestamp, std::chrono::system_clock::time_point());
}

std::chrono::system_clock::time_point parseISOTimestamp(
const std::string& timestamp, std::chrono::system_clock::time_point errorValue) {
std::tm tm = {};
char dot = '\0';
int milliseconds = 0;
Expand Down Expand Up @@ -52,16 +67,33 @@ std::chrono::system_clock::time_point parseISOTimestamp(const std::string& times
#endif

if (time_c == -1) {
return std::chrono::system_clock::time_point();
// timegm failed - could be out of range for time_t
// Return the supplied error value
return errorValue;
}
auto tp = std::chrono::system_clock::from_time_t(time_c);
tp += std::chrono::milliseconds(milliseconds);
return tp;
}

std::string formatISOTimestampForConfigEndTime(const std::chrono::system_clock::time_point& tp) {
// Special handling for max time_point (year 9999 dates)
if (tp == std::chrono::system_clock::time_point::max()) {
return "9999-12-31T00:00:00.000Z";
}
Comment on lines +80 to +83
Copy link

@maciej-perplexity maciej-perplexity Nov 26, 2025

Choose a reason for hiding this comment

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

Q: does this date stay on the client, or goes back to the backend (for consumption by potentially other clients with different date parsing capabilities)?

If it stays on the client, we could just let formatISOTimestamp do its best at representing the largest date it can - it should get parsed back to max() in parseISOTimestamp, if the implementations are symmetrical. In that case, no need for this special-casing.

Copy link
Collaborator Author

@greghuels greghuels Dec 1, 2025

Choose a reason for hiding this comment

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

It does stay on the client. That's a good idea.

return formatISOTimestamp(tp);
}

std::string formatISOTimestamp(const std::chrono::system_clock::time_point& tp) {
auto tt = std::chrono::system_clock::to_time_t(tp);
std::tm tm = *std::gmtime(&tt);
std::tm* tm_ptr = std::gmtime(&tt);

// Handle gmtime failure (can happen on 32-bit systems with out-of-range dates)
if (!tm_ptr) {
return "1970-01-01T00:00:00.000Z"; // Return epoch as fallback
}
Comment on lines +91 to +94
Copy link

@maciej-perplexity maciej-perplexity Nov 26, 2025

Choose a reason for hiding this comment

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

Epoch is different on Posix vs Windows. It could be fine to use the Posix epoch since it's unlikely that clients will need to represents dates outside of it, but in general maybe it'd be better to not return any string if tp doesn't represent a valid point in time?

Otherwise, with out-of-range dates, was the caller intending to represent a date in the far future, or far past? This formatting function is used to set "updatedAt" in BanditConfiguration. Setting it as epoch means this will always be the "oldest" bandit config possible, is this correct?


std::tm tm = *tm_ptr;

// Add milliseconds
auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(tp.time_since_epoch()) % 1000;
Expand Down
41 changes: 38 additions & 3 deletions src/time_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,34 @@ namespace eppoclient {
* Examples: "2024-06-09T14:23:11", "2024-06-09T14:23:11.123"
*
* @param timestamp The ISO 8601 formatted timestamp string
* @return A time_point representing the parsed timestamp, or a default-constructed
* time_point if parsing fails
* @param errorValue Optional value to return when time_c == -1 (defaults to epoch)
* @return A time_point representing the parsed timestamp, or errorValue if parsing fails
*/
std::chrono::system_clock::time_point parseISOTimestamp(const std::string& timestamp);
std::chrono::system_clock::time_point parseISOTimestamp(
const std::string& timestamp,
std::chrono::system_clock::time_point errorValue = std::chrono::system_clock::time_point());

/**
* Parse an ISO 8601 timestamp string for configuration endAt field.
*
* Returns time_point::max() on parsing failures, which represents "no end date".
*
* @param timestamp The ISO 8601 formatted timestamp string
* @return A time_point representing the parsed timestamp, or time_point::max() on failure
*/
std::chrono::system_clock::time_point parseISOTimestampForConfigEndTime(
const std::string& timestamp);

/**
* Parse an ISO 8601 timestamp string for configuration startAt field.
*
* Returns epoch (default time_point) on parsing failures.
*
* @param timestamp The ISO 8601 formatted timestamp string
* @return A time_point representing the parsed timestamp, or epoch on failure
*/
std::chrono::system_clock::time_point parseISOTimestampForConfigStartTime(
const std::string& timestamp);

/**
* Format a system_clock::time_point into an ISO 8601 timestamp string.
Expand All @@ -28,6 +52,17 @@ std::chrono::system_clock::time_point parseISOTimestamp(const std::string& times
*/
std::string formatISOTimestamp(const std::chrono::system_clock::time_point& tp);

/**
* Format a system_clock::time_point for configuration endAt field.
*
* Handles the special case of time_point::max() which represents "no end date"
* and formats it as "9999-12-31T00:00:00.000Z" for configuration consistency.
*
* @param tp The time_point to format
* @return An ISO 8601 formatted timestamp string, or "9999-12-31T00:00:00.000Z" for max
*/
std::string formatISOTimestampForConfigEndTime(const std::chrono::system_clock::time_point& tp);

} // namespace eppoclient

#endif // EPPOCLIENT_TIME_UTILS_HPP_
39 changes: 39 additions & 0 deletions test/test_time_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,3 +180,42 @@ TEST_CASE("parseISOTimestamp and formatISOTimestamp - round trip preserves milli
std::chrono::duration_cast<std::chrono::milliseconds>(parsed.time_since_epoch()) - seconds;
REQUIRE(milliseconds.count() == 123);
}

TEST_CASE("parseISOTimestampForConfigEndTime - year 9999 sentinel returns max time_point",
"[time_utils]") {
std::string timestamp = "9999-12-31T00:00:00.000Z";
auto result = parseISOTimestampForConfigEndTime(timestamp);

// The exact sentinel value should be treated as "no end date" and return time_point::max()
REQUIRE(result == std::chrono::system_clock::time_point::max());

// Verify that current time is always less than max
auto now = std::chrono::system_clock::now();
REQUIRE(now < result);
}

TEST_CASE("formatISOTimestampForConfigEndTime - time_point::max() formats to year 9999",
"[time_utils]") {
auto maxTime = std::chrono::system_clock::time_point::max();
std::string result = formatISOTimestampForConfigEndTime(maxTime);

// time_point::max() should format to the sentinel year 9999 date
REQUIRE(result == "9999-12-31T00:00:00.000Z");
}

TEST_CASE(
"parseISOTimestampForConfigEndTime and formatISOTimestampForConfigEndTime - year 9999 "
"round trip",
"[time_utils]") {
std::string original = "9999-12-31T00:00:00.000Z";

// Parse year 9999 sentinel date
auto parsed = parseISOTimestampForConfigEndTime(original);
REQUIRE(parsed == std::chrono::system_clock::time_point::max());

// Format back to string
std::string formatted = formatISOTimestampForConfigEndTime(parsed);

// Should format to the exact same sentinel value
REQUIRE(formatted == "9999-12-31T00:00:00.000Z");
}
Loading