Hopefully fix flaky overlay test. - #5365
Conversation
There was a problem hiding this comment.
Pull request overview
Re-enables and stabilizes the overlay flow-control test by measuring deterministic throttle and recovery states.
Changes:
- Re-enables the test.
- Verifies 100 throttle/recovery cycles using receive and capacity metrics.
- Captures cycle-specific diagnostics.
| REQUIRE(predicate()); | ||
| }; | ||
| auto waitForReceiverThrottle = [&]() { | ||
| auto waitForThrottle = [&](size_t cycle) { |
There was a problem hiding this comment.
Is std::this_thread::sleep_for(std::chrono::milliseconds(1)); correct here? Does it work with Simulation class, which cranks virtual time separately?
It's not a common pattern across our test suite, so I wanted to confirm that we're confident this is the right fix.
There was a problem hiding this comment.
@graydon I don't want to block this change since CI is unstable - we can merge as-is if you want. However, I'm still not sure if we should sleep at all. Wouldn't anything time-based like this continue to cause flakiness depending on how fast messages get processed/tasks are scheduled on main?
One way around that is to make the test simpler: we can just let it crank for a while, then check in metrics that core throttled connections the number of times we expect and that peers are still connected. Of course, we can't assert concrete checks like "reading capacity is N", but presumably the invariance in the code already does the heavy lifting for us (e.g. capacity can't go negative, capacity can't go over the limit, etc). I would only add an assert in the test that reading capacity is higher than flood capacity, to make sure that if we there's a bug and flood capacity is touched for some reason, we trigger an error of sorts. Up to you though.
| waitForThrottle(cycle); | ||
| simulation->crankUntil( | ||
| [&]() { | ||
| return getRecvCount() == initialRecvCount + MESSAGES_TO_FLOOR; |
There was a problem hiding this comment.
Isn't this a race? The metric is updated before msg destructor is called, which releases capacity.
| REQUIRE(predicate()); | ||
| }; | ||
| auto waitForReceiverThrottle = [&]() { | ||
| auto waitForThrottle = [&](size_t cycle) { |
There was a problem hiding this comment.
@graydon I don't want to block this change since CI is unstable - we can merge as-is if you want. However, I'm still not sure if we should sleep at all. Wouldn't anything time-based like this continue to cause flakiness depending on how fast messages get processed/tasks are scheduled on main?
One way around that is to make the test simpler: we can just let it crank for a while, then check in metrics that core throttled connections the number of times we expect and that peers are still connected. Of course, we can't assert concrete checks like "reading capacity is N", but presumably the invariance in the code already does the heavy lifting for us (e.g. capacity can't go negative, capacity can't go over the limit, etc). I would only add an assert in the test that reading capacity is higher than flood capacity, to make sure that if we there's a bug and flood capacity is touched for some reason, we trigger an error of sorts. Up to you though.
There was a small race in checking a predicate in this test and it was disabled as a result. This change rewrites the test a bit to measure slightly different and hopefully less-racy things (and re-enables the test). I've run it a few hundred times on my workstation without problem.