test(sampling): make rate_limiter thread-safety test deterministic - #2354
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 2 commits intoAug 17, 2026
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1b780c02a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 64ddac1 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksComparisonBenchmark execution time: 2026-08-17 11:34:21 Comparing candidate commit 64ddac1 in PR branch Found 0 performance improvements and 7 performance regressions! Performance is the same for 41 metrics, 10 unstable metrics.
|
Aaalibaba42
approved these changes
Aug 17, 2026
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
The previous `test_rate_limiter_thread_safety` relied on wall-clock timing and asserted a loose `95..=105` range. On a loaded CI runner the 1s window could roll over or `replenish` could add tokens mid-test, letting the total drift outside the intended 100 while still passing. The ±5 slack also hid real bugs (lost updates, double-spend, token leak). Introduce a `Clock` abstraction (`pub(crate)` enum with `System` and test-only `Mock` variants) so `RateLimiter` reads time through an injectable source. `new` keeps using the real wall-clock; a new `new_with_clock` constructor lets tests drive a frozen, manually advanced clock. The `Mock` variant, `MockClockInner`, `mock()`, and `advance()` are `#[cfg(test)]`-gated so production builds carry no mock machinery or dead code. Replace the flaky test with three deterministic ones: - `test_rate_limiter_thread_safety_no_panic`: shared limiter under contention must not panic (smoke check, no count assertion). - `test_rate_limiter_budget_invariant_concurrent`: four threads race on a frozen clock; total allowed must be exactly 100 and the next request must be denied (no slack). - `test_rate_limiter_window_reset_under_contention`: drain a window, advance the mock clock past the boundary, race on the fresh window; the second budget must be exactly 10. No test in `rate_limiter` depends on wall-clock timing anymore.
Two P2 comments from Codex: 1. Workers were not synchronized, so a scheduler could run each spawned thread to completion before the next starts, reducing the "concurrent" tests to sequential access. Add a `std::sync::Barrier` start gate to `test_rate_limiter_thread_safety_no_panic`, `test_rate_limiter_budget_invariant_concurrent`, and the race portion of `test_rate_limiter_window_reset_under_contention` so all workers reach the start line before any begins issuing requests, maximizing `is_allowed` overlap. 2. `test_rate_limiter_window_reset_under_contention` only verified token replenishment, not the `current_window_start` rollover in `update_rate_counts_locked`. Breaking the rollover logic would still pass because `replenish` alone restores the bucket. Restructure the test to use an asymmetric per-window allow/deny ratio (first window 10/100 = 0.1, second window 10/10 = 1.0) and assert `effective_rate` after the reset: the expected (1.0 + 0.1)/2 = 0.55 can only occur if `prev_window_rate` was set and the counts were reset by the rollover. A mutation test (disabling the rollover branch) confirms the new assertion fails when the rollover is broken.
bantonsson
force-pushed
the
ban/fix-flaky-rate-limiter-test
branch
from
August 17, 2026 11:12
afe07fc to
64ddac1
Compare
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
ban/fix-flaky-rate-limiter-test
branch
August 17, 2026 12:21
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
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
2 participants
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.
What does this PR do?
Replace the flaky
test_rate_limiter_thread_safetytest with three deterministic ones:test_rate_limiter_thread_safety_no_panic: shared limiter under contention must not panic (smoke check, no count assertion).test_rate_limiter_budget_invariant_concurrent: four threads race on a frozen clock; total allowed must be exactly 100 and the next request must be denied (no slack).test_rate_limiter_window_reset_under_contention: drain a window, advance the mock clock past the boundary, race on the fresh window; the second budget must be exactly 10.Motivation
The previous
test_rate_limiter_thread_safetyrelied on wall-clock timing and asserted a loose95..=105range. On a loaded CI runner the 1s window could roll over orreplenishcould add tokens mid-test, letting the total drift outside the intended 100 while still passing. The ±5 slack also hid real bugs (lost updates, double-spend, token leak).Additional Notes
Introduce a
Clockabstraction (pub(crate)enum withSystemand test-onlyMockvariants) soRateLimiterreads time through an injectable source.newkeeps using the real wall-clock; a newnew_with_clockconstructor lets tests drive a frozen, manually advanced clock. TheMockvariant,MockClockInner,mock(), andadvance()are#[cfg(test)]-gated so production builds carry no mock machinery or dead code.No test in
rate_limiterdepends on wall-clock timing anymore.`pHow to test the change?
Run the tests.