From a34a09a6f0812a908d89f3b699ad25b941c7db7c Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Mon, 21 Sep 2026 11:14:25 +1200 Subject: [PATCH 1/4] [ML] Make CMonotonicTimeTest robust to sleep overshoot on CI The millisecond and nanosecond timer tests slept for one second and then asserted the monotonic timer had advanced by a value within a fixed [900, 1200]ms window. This really tested the accuracy of sleep_for rather than the timer: on oversubscribed CI machines (e.g. the macOS Orka VMs) the sleep can overshoot substantially, which intermittently pushed the measured interval past the 1200ms upper bound and failed the build. Measure the elapsed interval independently with std::chrono::steady_clock around the same sleep and assert the monotonic timer agrees with that reference to within 5%. This validates what we actually care about - that the timer tracks real elapsed time - and is immune to how long the machine actually slept for. Co-authored-by: Cursor --- lib/core/unittest/CMonotonicTimeTest.cc | 64 ++++++++++++++++++------- 1 file changed, 48 insertions(+), 16 deletions(-) diff --git a/lib/core/unittest/CMonotonicTimeTest.cc b/lib/core/unittest/CMonotonicTimeTest.cc index 7833cb75b5..6018a97d3f 100644 --- a/lib/core/unittest/CMonotonicTimeTest.cc +++ b/lib/core/unittest/CMonotonicTimeTest.cc @@ -15,48 +15,80 @@ #include #include +#include +#include #include BOOST_AUTO_TEST_SUITE(CMonotonicTimeTest) +namespace { +// Tolerance for how closely the monotonic timer must track an independently +// measured reference interval. This is deliberately small because we are +// comparing two measurements of the *same* elapsed period rather than relying +// on the accuracy of sleep_for - the two clocks should agree regardless of how +// long the (possibly oversubscribed CI) machine actually slept for. +const double MONOTONIC_TIMER_TOLERANCE{0.05}; +} + BOOST_AUTO_TEST_CASE(testMilliseconds) { ml::core::CMonotonicTime monoTime; + // Measure the elapsed interval with both the class under test and an + // independent reference clock. sleep_for is unreliable on loaded CI + // machines (it can massively overshoot), so we do not assert against its + // nominal duration - instead we assert that the monotonic timer agrees + // with however much real time actually elapsed. + auto referenceStart = std::chrono::steady_clock::now(); std::uint64_t start(monoTime.milliseconds()); std::this_thread::sleep_for(std::chrono::seconds(1)); std::uint64_t end(monoTime.milliseconds()); + auto referenceEnd = std::chrono::steady_clock::now(); std::uint64_t diff(end - start); - LOG_DEBUG(<< "During 1 second the monotonic millisecond timer advanced by " - << diff << " milliseconds"); - - // Allow 10% margin of error - this is as much for the sleep as the timer - BOOST_TEST_REQUIRE(diff > 900); - // Allow 20% margin of error - sleep seems to sleep too long under Jenkins - // on Apple M1 - BOOST_TEST_REQUIRE(diff < 1200); + std::uint64_t reference(static_cast( + std::chrono::duration_cast(referenceEnd - referenceStart) + .count())); + LOG_DEBUG(<< "The monotonic millisecond timer advanced by " << diff + << " milliseconds; reference clock advanced by " << reference + << " milliseconds"); + + // The monotonic timer must never run backwards or stand still over a real + // elapsed interval. + BOOST_TEST_REQUIRE(diff > 0U); + // Both clocks measured the same real interval, so they must agree closely. + double allowedError{static_cast(reference) * MONOTONIC_TIMER_TOLERANCE}; + BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - static_cast(reference)) < + allowedError); } BOOST_AUTO_TEST_CASE(testNanoseconds) { ml::core::CMonotonicTime monoTime; + auto referenceStart = std::chrono::steady_clock::now(); std::uint64_t start(monoTime.nanoseconds()); std::this_thread::sleep_for(std::chrono::seconds(1)); std::uint64_t end(monoTime.nanoseconds()); + auto referenceEnd = std::chrono::steady_clock::now(); std::uint64_t diff(end - start); - LOG_DEBUG(<< "During 1 second the monotonic nanosecond timer advanced by " - << diff << " nanoseconds"); - - // Allow 10% margin of error - this is as much for the sleep as the timer - BOOST_TEST_REQUIRE(diff > 900000000); - // Allow 20% margin of error - sleep seems to sleep too long under Jenkins - // on Apple M1 - BOOST_TEST_REQUIRE(diff < 1200000000); + std::uint64_t reference(static_cast( + std::chrono::duration_cast(referenceEnd - referenceStart) + .count())); + LOG_DEBUG(<< "The monotonic nanosecond timer advanced by " << diff + << " nanoseconds; reference clock advanced by " << reference + << " nanoseconds"); + + // The monotonic timer must never run backwards or stand still over a real + // elapsed interval. + BOOST_TEST_REQUIRE(diff > 0U); + // Both clocks measured the same real interval, so they must agree closely. + double allowedError{static_cast(reference) * MONOTONIC_TIMER_TOLERANCE}; + BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - static_cast(reference)) < + allowedError); } BOOST_AUTO_TEST_SUITE_END() From 5ac26df7a7cbe8ada8486dc17fe126213ad8a9e2 Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Mon, 21 Sep 2026 12:02:50 +1200 Subject: [PATCH 2/4] [ML] Apply clang-format to CMonotonicTimeTest Co-authored-by: Cursor --- lib/core/unittest/CMonotonicTimeTest.cc | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/core/unittest/CMonotonicTimeTest.cc b/lib/core/unittest/CMonotonicTimeTest.cc index 6018a97d3f..98eb4b998d 100644 --- a/lib/core/unittest/CMonotonicTimeTest.cc +++ b/lib/core/unittest/CMonotonicTimeTest.cc @@ -50,17 +50,16 @@ BOOST_AUTO_TEST_CASE(testMilliseconds) { std::uint64_t reference(static_cast( std::chrono::duration_cast(referenceEnd - referenceStart) .count())); - LOG_DEBUG(<< "The monotonic millisecond timer advanced by " << diff - << " milliseconds; reference clock advanced by " << reference - << " milliseconds"); + LOG_DEBUG(<< "The monotonic millisecond timer advanced by " << diff << " milliseconds; reference clock advanced by " + << reference << " milliseconds"); // The monotonic timer must never run backwards or stand still over a real // elapsed interval. BOOST_TEST_REQUIRE(diff > 0U); // Both clocks measured the same real interval, so they must agree closely. double allowedError{static_cast(reference) * MONOTONIC_TIMER_TOLERANCE}; - BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - static_cast(reference)) < - allowedError); + BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - + static_cast(reference)) < allowedError); } BOOST_AUTO_TEST_CASE(testNanoseconds) { @@ -78,17 +77,16 @@ BOOST_AUTO_TEST_CASE(testNanoseconds) { std::uint64_t reference(static_cast( std::chrono::duration_cast(referenceEnd - referenceStart) .count())); - LOG_DEBUG(<< "The monotonic nanosecond timer advanced by " << diff - << " nanoseconds; reference clock advanced by " << reference - << " nanoseconds"); + LOG_DEBUG(<< "The monotonic nanosecond timer advanced by " << diff << " nanoseconds; reference clock advanced by " + << reference << " nanoseconds"); // The monotonic timer must never run backwards or stand still over a real // elapsed interval. BOOST_TEST_REQUIRE(diff > 0U); // Both clocks measured the same real interval, so they must agree closely. double allowedError{static_cast(reference) * MONOTONIC_TIMER_TOLERANCE}; - BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - static_cast(reference)) < - allowedError); + BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - + static_cast(reference)) < allowedError); } BOOST_AUTO_TEST_SUITE_END() From f7431e82db9c0ffc330c15ec6f7725e3dd4e4f9f Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Mon, 21 Sep 2026 13:04:59 +1200 Subject: [PATCH 3/4] [ML] Add cross-domain lower bound and document absence of upper bound Combine two complementary checks in CMonotonicTimeTest: a cross-domain lower bound (the timer must advance by at least ~the requested sleep, our only independent check that the clock ticks at real-time rate) and the steady_clock agreement check (which validates CMonotonicTime's unit-scaling arithmetic). Document why there is deliberately no upper bound relative to the nominal sleep duration - sleep_for can overshoot arbitrarily on loaded CI hosts, which is a scheduler property, not a timer defect. Co-authored-by: Cursor --- lib/core/unittest/CMonotonicTimeTest.cc | 68 ++++++++++++++++++------- 1 file changed, 50 insertions(+), 18 deletions(-) diff --git a/lib/core/unittest/CMonotonicTimeTest.cc b/lib/core/unittest/CMonotonicTimeTest.cc index 98eb4b998d..d6d21c6531 100644 --- a/lib/core/unittest/CMonotonicTimeTest.cc +++ b/lib/core/unittest/CMonotonicTimeTest.cc @@ -21,23 +21,53 @@ BOOST_AUTO_TEST_SUITE(CMonotonicTimeTest) +// These tests sleep for a nominal one second and then make two independent +// checks on what CMonotonicTime measured over that interval: +// +// 1. A cross-domain lower bound. We assert the timer advanced by at least +// (close to) the requested sleep duration. std::this_thread::sleep_for +// guarantees it sleeps for *at least* the requested time, so a reading +// well below one second means the monotonic clock is running too slowly +// or has stalled. This is our only genuinely independent check that the +// clock ticks at roughly real-time rate, because it compares against a +// different clock domain (the scheduler's sleep timer) rather than +// against another reading of the same underlying counter. +// +// 2. A scaling/consistency check. We measure the same interval with +// std::chrono::steady_clock and assert CMonotonicTime agrees with it to +// within a small tolerance. On every platform steady_clock and +// CMonotonicTime ultimately derive from the same hardware counter, so +// this does not re-check the clock's real-time fidelity (that is covered +// by check 1); what it validates is CMonotonicTime's own unit-scaling +// arithmetic (the mach_timebase / QueryPerformanceFrequency / timespec +// conversions), which is the part of this class we can actually break. +// +// Crucially there is NO upper bound on the elapsed time relative to the +// nominal sleep duration. sleep_for only promises a *minimum* sleep and can +// overshoot arbitrarily when the machine is loaded or oversubscribed - on +// shared CI hosts (notably the macOS Orka VMs) the thread simply is not +// rescheduled promptly. Such overshoot is a property of the scheduler, not a +// timer defect, so any assertion of the form "diff < someConstant" is +// inherently flaky. A previous version of this test asserted diff < 1200ms and +// failed intermittently for exactly this reason (the timer correctly reported +// ~1293ms because that much wall-clock time had genuinely elapsed). Check 2 +// still bounds diff from above, but only against the *actual* elapsed time +// measured over the same window, so overshoot cannot cause a failure. + namespace { -// Tolerance for how closely the monotonic timer must track an independently -// measured reference interval. This is deliberately small because we are -// comparing two measurements of the *same* elapsed period rather than relying -// on the accuracy of sleep_for - the two clocks should agree regardless of how -// long the (possibly oversubscribed CI) machine actually slept for. +// Tolerance for how closely the monotonic timer must track the independently +// measured reference interval (check 2 above). This is deliberately small +// because we are comparing two measurements of the *same* elapsed period; it +// is sized to comfortably exceed the coarsest platform timer's granularity +// (Windows GetTickCount64 is only accurate to ~15ms, i.e. ~1.5% of a one +// second interval), so the nominal one second interval must stay large +// relative to that granularity. const double MONOTONIC_TIMER_TOLERANCE{0.05}; } BOOST_AUTO_TEST_CASE(testMilliseconds) { ml::core::CMonotonicTime monoTime; - // Measure the elapsed interval with both the class under test and an - // independent reference clock. sleep_for is unreliable on loaded CI - // machines (it can massively overshoot), so we do not assert against its - // nominal duration - instead we assert that the monotonic timer agrees - // with however much real time actually elapsed. auto referenceStart = std::chrono::steady_clock::now(); std::uint64_t start(monoTime.milliseconds()); @@ -53,10 +83,11 @@ BOOST_AUTO_TEST_CASE(testMilliseconds) { LOG_DEBUG(<< "The monotonic millisecond timer advanced by " << diff << " milliseconds; reference clock advanced by " << reference << " milliseconds"); - // The monotonic timer must never run backwards or stand still over a real - // elapsed interval. - BOOST_TEST_REQUIRE(diff > 0U); - // Both clocks measured the same real interval, so they must agree closely. + // Check 1: cross-domain lower bound (allow a 10% margin below the one + // second sleep). Deliberately no upper bound - see the note above. + BOOST_TEST_REQUIRE(diff > 900U); + + // Check 2: agreement with the independent reference over the same interval. double allowedError{static_cast(reference) * MONOTONIC_TIMER_TOLERANCE}; BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - static_cast(reference)) < allowedError); @@ -80,10 +111,11 @@ BOOST_AUTO_TEST_CASE(testNanoseconds) { LOG_DEBUG(<< "The monotonic nanosecond timer advanced by " << diff << " nanoseconds; reference clock advanced by " << reference << " nanoseconds"); - // The monotonic timer must never run backwards or stand still over a real - // elapsed interval. - BOOST_TEST_REQUIRE(diff > 0U); - // Both clocks measured the same real interval, so they must agree closely. + // Check 1: cross-domain lower bound (allow a 10% margin below the one + // second sleep). Deliberately no upper bound - see the note above. + BOOST_TEST_REQUIRE(diff > 900000000U); + + // Check 2: agreement with the independent reference over the same interval. double allowedError{static_cast(reference) * MONOTONIC_TIMER_TOLERANCE}; BOOST_TEST_REQUIRE(std::abs(static_cast(diff) - static_cast(reference)) < allowedError); From b79d0c1e3991ee2c9499e6b6699e467fba646b0e Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Tue, 22 Sep 2026 09:21:09 +1200 Subject: [PATCH 4/4] [ML] Clarify cross-clock check comment (not identical counters everywhere) Reword the check-2 comment: steady_clock and CMonotonicTime agree because they observe the same elapsed real time, but they do not share the same hardware counter on every platform - the coarse millisecond paths read a different source (Linux CLOCK_MONOTONIC_COARSE, Windows GetTickCount64). Describe it as a cross-clock consistency check that validates our scaling arithmetic rather than claiming identical-counter coverage. Co-authored-by: Cursor --- lib/core/unittest/CMonotonicTimeTest.cc | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/core/unittest/CMonotonicTimeTest.cc b/lib/core/unittest/CMonotonicTimeTest.cc index d6d21c6531..8686bc465d 100644 --- a/lib/core/unittest/CMonotonicTimeTest.cc +++ b/lib/core/unittest/CMonotonicTimeTest.cc @@ -33,14 +33,18 @@ BOOST_AUTO_TEST_SUITE(CMonotonicTimeTest) // different clock domain (the scheduler's sleep timer) rather than // against another reading of the same underlying counter. // -// 2. A scaling/consistency check. We measure the same interval with +// 2. A cross-clock consistency check. We measure the same interval with // std::chrono::steady_clock and assert CMonotonicTime agrees with it to -// within a small tolerance. On every platform steady_clock and -// CMonotonicTime ultimately derive from the same hardware counter, so -// this does not re-check the clock's real-time fidelity (that is covered -// by check 1); what it validates is CMonotonicTime's own unit-scaling -// arithmetic (the mach_timebase / QueryPerformanceFrequency / timespec -// conversions), which is the part of this class we can actually break. +// within a small tolerance. The two clocks should agree because they +// observe the same elapsed real time - on most platforms via the same +// underlying counter (macOS mach_absolute_time; the CLOCK_MONOTONIC +// nanosecond paths on Linux), though the coarse millisecond paths read a +// different source (Linux CLOCK_MONOTONIC_COARSE, Windows GetTickCount64). +// This is therefore not an independent re-check of the clock's real-time +// fidelity (that is covered by check 1); what it validates is +// CMonotonicTime's own unit-scaling arithmetic (the mach_timebase / +// QueryPerformanceFrequency / timespec conversions), which is the part of +// this class we can actually break. // // Crucially there is NO upper bound on the elapsed time relative to the // nominal sleep duration. sleep_for only promises a *minimum* sleep and can