Fix flaky pex reactor tests that could miss an evicted peer reconnecting - #4221
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4221 +/- ##
==========================================
- Coverage 66.97% 65.83% -1.14%
==========================================
Files 2182 2061 -121
Lines 167488 155541 -11947
==========================================
- Hits 112171 102402 -9769
+ Misses 55176 52998 -2178
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryLow Risk Overview Test harness: Shared test helper: Reviewed by Cursor Bugbot for commit a17fdeb. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6eb19cd. Configure here.
There was a problem hiding this comment.
The flake diagnosis is correct and the fix is sound — capturing the specific *ConnV2 makes the wait condition monotone, and peerManager.Evict closes every conn for the node ID so either direction's captured conn is a valid target. One reuse issue: the new WaitForConnClosed duplicates the existing WaitForDisconnect helper in the same file.
Findings: 0 blocking | 1 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.

TestReactorSendsRequestsTooOftenhung for the full 30 minute test timeout inlistenForPeerDown. The CI log shows the eviction did happen (evicting ... pex: peer rate limit exceededat 32.842) but the mock peer was dialled and connected again three milliseconds later, because both routers still know each other's address fromconnectAll.WaitForConn(target, false)waits on the peer manager's connection set for a state in which no connection to that node ID exists, and that state only existed inside that three millisecond window, so a waiter that was not scheduled in between never sees it and waits forever. The same tests were only ever passing because the redial usually lost that race.The fix uses the existing
WaitForDisconnect, which waits for one specific*ConnV2to leave the connection set rather than for the node ID to be absent; that is a monotone condition, so a reconnect cannot mask it. The pex suite captures the connection withpeerConnbefore triggering the eviction and hands it tolistenForPeerDown, andTestNode.Disconnectwaits on the connections it closed the same way instead of onWaitForConn(target, false), since it has the same exposure to a redial.Flaked in: https://github.com/sei-protocol/sei-chain/actions/runs/34871012761/job/104066604846