ackhandler: store the last four skipped packets (#5322)

This allows for more accurate packet number difference calculations,
which is especially important once we attempt to detect spurious loss
detection events.

internal/ackhandler/sent_packet_history.go

@@ -3,10 +3,13 @@ package ackhandler
 import (
 	"fmt"
 	"iter"
+	"slices"
 
 	"github.com/quic-go/quic-go/internal/protocol"
 )
 
+const maxSkippedPackets = 4
+
 type sentPacketHistory struct {
 	packets          []*packet
 	pathProbePackets []packetWithPacketNumber
@@ -25,6 +28,7 @@ func newSentPacketHistory(isAppData bool) *sentPacketHistory {
 	}
 	if isAppData {
 		h.packets = make([]*packet, 0, 32)
+		h.skippedPackets = make([]protocol.PacketNumber, 0, maxSkippedPackets)
 	} else {
 		h.packets = make([]*packet, 0, 6)
 	}
@@ -48,6 +52,9 @@ func (h *sentPacketHistory) SkippedPacket(pn protocol.PacketNumber) {
 	if len(h.packets) > 0 {
 		h.packets = append(h.packets, nil)
 	}
+	if len(h.skippedPackets) == maxSkippedPackets {
+		h.skippedPackets = slices.Delete(h.skippedPackets, 0, 1)
+	}
 	h.skippedPackets = append(h.skippedPackets, pn)
 }
 
@@ -163,13 +170,6 @@ func (h *sentPacketHistory) Remove(pn protocol.PacketNumber) error {
 	if len(h.packets) > 0 && h.packets[0] == nil {
 		panic("cleanup failed")
 	}
-	if len(h.packets) > 0 && len(h.skippedPackets) > 0 {
-		for _, p := range h.skippedPackets {
-			if p < h.firstPacketNumber {
-				h.skippedPackets = h.skippedPackets[1:]
-			}
-		}
-	}
 	return nil
 }
 

internal/ackhandler/sent_packet_history_test.go

@@ -98,7 +98,8 @@ func TestSentPacketHistoryRemovePackets(t *testing.T) {
 	require.Equal(t, []protocol.PacketNumber{2, 3, 5}, slices.Collect(hist.SkippedPackets()))
 	require.NoError(t, hist.Remove(1))
 	require.Equal(t, []protocol.PacketNumber{4, 6}, hist.getPacketNumbers())
-	require.Equal(t, []protocol.PacketNumber{5}, slices.Collect(hist.SkippedPackets()))
+	// skipped packets should be preserved
+	require.Equal(t, []protocol.PacketNumber{2, 3, 5}, slices.Collect(hist.SkippedPackets()))
 
 	// add one more packet
 	hist.SentAckElicitingPacket(7, &packet{})
@@ -114,12 +115,20 @@ func TestSentPacketHistoryRemovePackets(t *testing.T) {
 	require.Error(t, err)
 	require.EqualError(t, err, "packet 9 not found in sent packet history")
 
+	// only the last 4 skipped packets should be preserved
+	hist.SkippedPacket(9)
+	hist.SkippedPacket(10)
+	hist.SentAckElicitingPacket(11, &packet{})
+	hist.SkippedPacket(12)
+	require.Equal(t, []protocol.PacketNumber{5, 9, 10, 12}, slices.Collect(hist.SkippedPackets()))
+
 	// Remove all packets
 	require.NoError(t, hist.Remove(4))
 	require.NoError(t, hist.Remove(6))
 	require.NoError(t, hist.Remove(8))
+	require.NoError(t, hist.Remove(11))
 	require.Empty(t, hist.getPacketNumbers())
-	require.Empty(t, slices.Collect(hist.SkippedPackets()))
+	require.Len(t, slices.Collect(hist.SkippedPackets()), 4)
 	require.False(t, hist.HasOutstandingPackets())
 }
 
@@ -171,7 +180,7 @@ func TestSentPacketHistoryIterating(t *testing.T) {
 	}
 
 	require.Equal(t, []protocol.PacketNumber{1, 2, 6}, packets)
-	require.Equal(t, []protocol.PacketNumber{4, 5}, slices.Collect(hist.SkippedPackets()))
+	require.Equal(t, []protocol.PacketNumber{0, 4, 5}, slices.Collect(hist.SkippedPackets()))
 }
 
 func TestSentPacketHistoryDeleteWhileIterating(t *testing.T) {