test: Make Drop50 message drop deterministic in LedgerReplayer test - #7964
test: Make Drop50 message drop deterministic in LedgerReplayer test#7964pratikmankawde wants to merge 2 commits into
Conversation
The 'network drops 50% messages' testcase dropped each request with an independent random 50% probability. A subtask permanently fails after 1 + kSubTaskMaxTimeouts (11) consecutive dropped sends, so each run had a ~1/512 chance of failure, causing intermittent CI failures at LedgerReplay_test.cpp(1340) and (1342). Drop every other message via a counter instead. This still exercises the timeout/retry path but guarantees every subtask eventually gets a reply.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Straightforward fix: replaces the random 50% drop with a deterministic alternating counter, which correctly maintains an invariant (each subtask's send sequence starts at an even/dropped position, since prior subtasks consume an even number of total sends) so no subtask should hit 11 consecutive drops. The only point worth double-checking is why the counter was made atomic, since that implies concurrent callers, which could undermine the new determinism guarantee.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
There was a problem hiding this comment.
Straightforward, well-targeted fix: replaces the random randInt-based 50% drop with a deterministic atomic counter that drops every other Drop50 message, removing the ~1/512 flaky failure mode while still exercising the timeout/retry path. The std::atomic<int> counter is correctly used (single read-modify-write via post-increment), the unused random.h include was removed, and <atomic> was added. No correctness, security, or style issues found in the changed lines.
There was a problem hiding this comment.
Pull request overview
This PR fixes a flaky xrpl.app.LedgerReplayer unit test by replacing probabilistic message dropping (via RNG) with deterministic alternating drops, preserving coverage of the retry/timeout behavior while eliminating the ~1/512 failure mode from long consecutive drops.
Changes:
- Remove RNG-based 50% drop simulation and implement deterministic “drop every other message” logic for
PeerSetBehavior::Drop50. - Add a per-
TestPeerSetstd::atomic<int>counter to support deterministic dropping under potential concurrency. - Remove the now-unused
random.hinclude and add the required<atomic>include.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mathbunnyru
left a comment
mathbunnyru
left a comment
There was a problem hiding this comment.
I think we should rename it to DropEverySecond.
Also, as an alternative, maybe we should increase number of messages and fail after more drops, practically making probability zero?
High Level Overview of Change
Fix flaky
xrpl.app.LedgerReplayertest.Context of Change
The "network drops 50% messages" testcase used
randIntto drop each request independently. A subtask fails after 11 consecutive drops, creating a ~1/512 failure rate per run.Changed to drop every other message via counter. Still exercises timeout/retry but guarantees eventual progress.
API Impact
None.