Don't drop retransmitted packets that were ACKed past but not yet delivered#133
Open
JohanG-LAS wants to merge 1 commit intodatarhei:mainfrom
Open
Don't drop retransmitted packets that were ACKed past but not yet delivered#133JohanG-LAS wants to merge 1 commit intodatarhei:mainfrom
JohanG-LAS wants to merge 1 commit intodatarhei:mainfrom
Conversation
When periodicACK advances lastACKSequenceNumber past a gap (e.g. when packets after the gap have ripe PktTsbpdTime), the delivery loop can break before lastDeliveredSequenceNumber catches up. A retransmission arriving in this window for a sequence in the gap was dropped by the Lt(lastACKSequenceNumber) branch as "already acknowledged", which manifests in user reports as PktRecvRetrans approximately equal to PktRecvDrop. Allow retransmissions through this branch; they fall into the out-of-order handling below where they either fill the gap or are detected as duplicates. TLPKTDROP semantics remain enforced by the earlier Lte(lastDeliveredSequenceNumber) check, which still drops anything already delivered or skipped past. Add a focused test that constructs the exact bug state deterministically and verifies the retransmission is reinserted into packetList rather than counted as a drop. Made-with: Cursor
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
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
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.
Problem
receiver.Pushcould drop legitimate retransmissions, even though thepacket had not yet been delivered to the application. This was observed
in production via MediaMTX as a near-1:1 ratio between
packetsReceivedRetransandpacketsReceivedDrop— every retransmitthat the sender sent in response to a NAK was being received and then
silently discarded by the receiver. The same streams over libsrt
(
srt-live-transmit/TSDuck) recovered cleanly with zero drops, whichruled out the network and the sender.
This appears to be the same symptom reported in #91.
Smoking gun (from a debug-instrumented build)
GOSRT_RX rx=0xc00014e4e0 retrans_in seq=691158851 lastACK=691158858 lastDelivered=691158759 maxSeen=691158865 GOSRT_RX rx=0xc00014e4e0 drop reason=lt_lastACK seq=691158851 lastACK=691158858 lastDelivered=691158759 maxSeen=691158865 retrans=trueThe retransmission satisfied
lastDeliveredSequenceNumber < seq < lastACKSequenceNumberand wasdropped by the
Lt(r.lastACKSequenceNumber)branch as "alreadyacknowledged" — even though the gap had not yet been delivered or
skipped past.
Root cause
periodicACKand the delivery loop inreceiver.Tickare two separatecritical sections.
periodicACKcan advancelastACKSequenceNumberpast a gap when the packets after the gap have ripe
PktTsbpdTime,while the delivery loop breaks early on the first packet whose
PktTsbpdTimeis still in the future. A retransmission arriving inthis window for a sequence in the gap landed in the
Lt(lastACKSequenceNumber)drop branch.Fix
Allow retransmissions through the
Lt(lastACKSequenceNumber)branch.They fall into the existing out-of-order handling below, where they
either fill the gap (via
InsertBefore) or are detected as duplicatesby the existing linear-scan check. TLPKTDROP semantics are still
enforced by the earlier
Lte(lastDeliveredSequenceNumber)check, whichremains untouched and continues to drop anything already delivered or
skipped past.
Non-retransmit packets below
lastACK(which would be unusual butpossible under heavy reordering) are still dropped exactly as before.
Test
TestRecvRetransmitPastACKconstructs the bug state deterministically:PktTsbpdTimeso the delivery loopbreaks at the head of the list.
PktTsbpdTimesoperiodicACKadvances past the gap.Tick(50)— assertslastACK == 8andlastDeliveredis stilluninitialized.
RetransmittedPacketFlag = true.PktRetransincremented, and that seq 5 is now inpacketList.Tick(300)— asserts the full[0..8]is delivered in order.Verified that the test fails on
main(withPktDrop == 1exactly asthe production log showed) and passes with the fix applied.
Full
go test ./...suite remains green.Made-with: Cursor