[ML] Make CMonotonicTimeTest robust to sleep overshoot on CI - #3201
Merged
Merged
Conversation
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 <cursoragent@cursor.com>
|
Pinging @elastic/ml-core (Team:ML) |
Co-authored-by: Cursor <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the remaining timing assertions, measurement skew, documentation, and formatting issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Updates CMonotonicTimeTest to tolerate sleep overshoot by comparing elapsed time with steady_clock.
Changes:
- Adds millisecond and nanosecond reference-clock measurements.
- Uses a 5% agreement tolerance.
- Updates test rationale and diagnostics.
| File | Summary |
|---|---|
lib/core/unittest/CMonotonicTimeTest.cc |
Updates monotonic timer tests, assertions, and logging. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
valeriy42
self-requested a review
September 21, 2026 06:46
valeriy42
approved these changes
Sep 21, 2026
…here) 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 <cursoragent@cursor.com>
This was referenced Sep 21, 2026
Contributor
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
elastic-vault-github-plugin-prod Bot
added a commit
that referenced
this pull request
Sep 21, 2026
…3205) 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. (cherry picked from commit d6a62eb) Co-authored-by: Ed Savage <ed.savage@elastic.co>
elastic-vault-github-plugin-prod Bot
added a commit
that referenced
this pull request
Sep 21, 2026
…3206) 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. (cherry picked from commit d6a62eb) Co-authored-by: Ed Savage <ed.savage@elastic.co>
elastic-vault-github-plugin-prod Bot
added a commit
that referenced
this pull request
Sep 22, 2026
…3207) 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. (cherry picked from commit d6a62eb) Co-authored-by: Ed Savage <ed.savage@elastic.co>
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
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.


Summary
CMonotonicTimeTest::testMilliseconds/testNanosecondsslept for one second and asserted the monotonic timer advanced by a value inside a fixed[900, 1200]mswindow. That effectively testedstd::this_thread::sleep_foraccuracy rather than the timer itself. On oversubscribed CI machines (notably the macOS aarch64 Orka VMs) the sleep can overshoot well past the 1200ms upper bound, causing intermittent failures.An example failure: ml-cpp-snapshot-builds #7092 (
test_macos-aarch64-RelWithDebInfo, branch 9.5):The timer was behaving correctly; it faithfully reported the ~1293ms that actually elapsed while the thread was descheduled.
Fix
The tests still sleep for a nominal one second, but replace the fragile fixed
[900, 1200]mswindow with two complementary checks:Cross-domain lower bound (
diff > 900ms/900000000ns):sleep_forguarantees it sleeps for at least the requested duration, so a reading well below one second means the monotonic clock is running too slowly or has stalled. This is the 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). A 10% margin absorbs coarse-timer granularity and cross-clock skew.Cross-clock consistency check (agreement with
std::chrono::steady_clockover the same interval, within 5%): validatesCMonotonicTime's own unit-scaling arithmetic. This boundsdifffrom above, but only against the actual elapsed time measured over the same window.There is deliberately no upper bound relative to the nominal sleep duration.
sleep_foronly promises a minimum sleep and can overshoot arbitrarily on loaded/oversubscribed hosts - that's a scheduler property, not a timer defect - which is exactly what made the olddiff < 1200assertion flaky. The rationale is documented in a block comment in the test.Test plan
ml_test_corelocally (macOS aarch64, RelWithDebInfo).CMonotonicTimeTest: both cases pass; the two clocks agree to <1ms (ms case) and <1us (ns case).-Wall -Wextra: zero warnings (fixed a latent-Wsign-compare).