fix: network bug where a message could be dropped - #624
Conversation
Consolidated Tests Results 2026-06-01 - 16:48:34Test ResultsDetails
test-reporter: Run #2485
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 |
There was a problem hiding this comment.
Pull request overview
Fixes a corner case in NetworkSession::receive where, after a sender is marked as completed, a legitimate message arriving during the timeout window could be consumed but effectively discarded.
Changes:
- Adjusted the
tick_interval.tick()branch in thereceiveloop so that if the sender is incompleted_parties, an in-flight message received during the “grace period” is returned instead of being dropped. - Renamed the post-loop packet variable (
local_packet→returned_packet) to better reflect its role after the receive loop completes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jot2re
left a comment
There was a problem hiding this comment.
LGTM! Although this is super subtle. Maybe it would be good to add a few more code comments?
Also as Copilot mentions, maybe adding a test validating the fix would also be good
|
Will work on a test. // The sender has said the session is complete, wait for the timeout time to ensure there is no more messages lingeringTo me this implicitly say that we should consider incoming message for the next |
20c2fa7 to
f88ee1e
Compare
Consolidated Tests Results 2026-06-05 - 11:29:40Test ResultsDetails
test-reporter: Run #2601
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 |
Consolidated Tests Results 2026-06-05 - 14:16:24Test ResultsDetails
test-reporter: Run #2614
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 |
jot2re
left a comment
There was a problem hiding this comment.
Still looks good to me!
BTW I don't think I was clear previously; the comments I thought that might make sense was on the specific branches of the production code; since it is a bit tricky to follow what is returned at which depth given the nested select!'s
|
Added a quite verbose comment if the code wasn't clear enough |
dvdplm
left a comment
There was a problem hiding this comment.
Excellent comments both in the code and for the test. :)
Description of changes
This fixes a corner case of the network where:
rcv(B)completed_parties' settick_intervalselectbetween asleepandrcvEffectively A drops a legitimate message from B.
Issue ticket number and link
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md