quic: don't re-lose packets when discarding keys

When discarding keys for a packet number space, we remove any
in-flight packets in that space from our count of bytes in flight.
Skip any packets in the space that were already acked or declared lost.

Also skip over acked/lost packets when discarding state in response
to a Reset message. This change shouldn't have any effect, since we
never declare packets acked/lost until we receive an ACK, and we
never handle a Reset after receiving an ACK.

Change-Id: I8842f03aa06e57a834a211f1284a48fe5d469db3
Reviewed-on: https://go-review.googlesource.com/c/net/+/664335
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
Auto-Submit: Damien Neil <[email protected]>

Author: neild

quic/loss.go

@@ -315,6 +315,11 @@ func (c *lossState) receiveAckEnd(now time.Time, log *slog.Logger, space numberS
 func (c *lossState) discardPackets(space numberSpace, log *slog.Logger, lossf func(numberSpace, *sentPacket, packetFate)) {
 	for i := 0; i < c.spaces[space].size; i++ {
 		sent := c.spaces[space].nth(i)
+		if sent.acked || sent.lost {
+			// This should not be possible, since we only discard packets
+			// in spaces which have never received an ack, but check anyway.
+			continue
+		}
 		sent.lost = true
 		c.cc.packetDiscarded(sent)
 		lossf(numberSpace(space), sent, packetLost)
@@ -330,6 +335,9 @@ func (c *lossState) discardKeys(now time.Time, log *slog.Logger, space numberSpa
 	// https://www.rfc-editor.org/rfc/rfc9002.html#section-6.4
 	for i := 0; i < c.spaces[space].size; i++ {
 		sent := c.spaces[space].nth(i)
+		if sent.acked || sent.lost {
+			continue
+		}
 		c.cc.packetDiscarded(sent)
 	}
 	c.spaces[space].discard()

quic/loss_test.go

@@ -795,16 +795,38 @@ func TestLossKeysDiscarded(t *testing.T) {
 	// https://www.rfc-editor.org/rfc/rfc9002.html#section-6.4-1
 	test := newLossTest(t, clientSide, lossTestOpts{})
 	test.send(initialSpace, 0, testSentPacketSize(1200))
+	test.send(initialSpace, 1, testSentPacketSize(1200)) // will be acked
+	test.send(initialSpace, 2, testSentPacketSize(1200))
+	test.ack(initialSpace, 0*time.Millisecond, i64range[packetNumber]{1, 2})
+	test.wantAck(initialSpace, 1)
 	test.send(handshakeSpace, 0, testSentPacketSize(600))
-	test.wantVar("bytes_in_flight", 1800)
+	test.send(handshakeSpace, 1, testSentPacketSize(600)) // will be acked
+	test.send(handshakeSpace, 2, testSentPacketSize(600))
+	test.ack(handshakeSpace, 0*time.Millisecond, i64range[packetNumber]{1, 2})
+	test.wantAck(handshakeSpace, 1)
+	test.wantVar("bytes_in_flight", 1200+1200+600+600) // 3600
 
 	test.discardKeys(initialSpace)
-	test.wantVar("bytes_in_flight", 600)
+	test.wantVar("bytes_in_flight", 600+600) // 1200
 
 	test.discardKeys(handshakeSpace)
 	test.wantVar("bytes_in_flight", 0)
 }
 
+func TestLossDiscardPackets(t *testing.T) {
+	test := newLossTest(t, clientSide, lossTestOpts{})
+	test.send(initialSpace, 0, testSentPacketSize(1200))
+	test.send(initialSpace, 1, testSentPacketSize(1200)) // will be acked
+	test.send(initialSpace, 2, testSentPacketSize(1200))
+	test.ack(initialSpace, 0*time.Millisecond, i64range[packetNumber]{1, 2})
+	test.wantAck(initialSpace, 1)
+
+	test.discardPackets(initialSpace)
+	test.wantLoss(initialSpace, 0)
+	test.wantLoss(initialSpace, 2)
+	test.wantVar("bytes_in_flight", 0)
+}
+
 func TestLossInitialCongestionWindow(t *testing.T) {
 	// "Endpoints SHOULD use an initial congestion window of [...]"
 	// https://www.rfc-editor.org/rfc/rfc9002.html#section-7.2-1
@@ -1492,6 +1514,13 @@ func (c *lossTest) discardKeys(spaceID numberSpace) {
 	c.c.discardKeys(c.now, nil, spaceID)
 }
 
+func (c *lossTest) discardPackets(spaceID numberSpace) {
+	c.t.Helper()
+	c.checkUnexpectedEvents()
+	c.t.Logf("discard packets in %s", spaceID)
+	c.c.discardPackets(spaceID, nil, c.onAckOrLoss)
+}
+
 func (c *lossTest) setMaxAckDelay(d time.Duration) {
 	c.t.Helper()
 	c.checkUnexpectedEvents()