QuicSentPacketManager::OnConnectionMigration() shouldn't mark packet for retransmission if the packet doesn't have transmittable frames. If such packet is still in flight, mark it not to contribute to congestion control.

PiperOrigin-RevId: 353659204
Change-Id: I3bb5f9b89f9c415349ba52614c7522209bd5d047

quic/core/quic_sent_packet_manager.cc

diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index 73ee59e..4f2b200 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -661,7 +661,9 @@
   QUIC_BUG_IF(transmission_type != LOSS_RETRANSMISSION &&
               transmission_type != RTO_RETRANSMISSION &&
               !unacked_packets_.HasRetransmittableFrames(*transmission_info))
-      << "transmission_type: " << transmission_type;
+      << "packet number " << packet_number
+      << " transmission_type: " << transmission_type << " transmission_info "
+      << transmission_info->DebugString();
   // Handshake packets should never be sent as probing retransmissions.
   DCHECK(!transmission_info->has_crypto_handshake ||
          transmission_type != PROBING_RETRANSMISSION);
@@ -1610,10 +1612,12 @@
       unacked_packets_.RemoveFromInFlight(packet_number);
       // Retransmitting these packets with PATH_CHANGE_RETRANSMISSION will mark
       // them as useless, thus not contributing to RTT stats.
-      MarkForRetransmission(packet_number, PATH_RETRANSMISSION);
-    } else {
-      it->state = NOT_CONTRIBUTING_RTT;
+      if (unacked_packets_.HasRetransmittableFrames(packet_number)) {
+        MarkForRetransmission(packet_number, PATH_RETRANSMISSION);
+        DCHECK_EQ(it->state, NOT_CONTRIBUTING_RTT);
+      }
     }
+    it->state = NOT_CONTRIBUTING_RTT;
   }
   return old_send_algorithm;
 }

quic/core/quic_sent_packet_manager_test.cc

diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index ad30c41..f286de8 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2333,7 +2333,7 @@
             manager_.GetRttStats()->latest_rtt());
 
   SendDataPacket(3, ENCRYPTION_FORWARD_SECURE);
-  // Trigger loss timeout and makr packet3 for retransmission.
+  // Trigger loss timeout and mark packet3 for retransmission.
   EXPECT_CALL(*loss_algorithm, GetLossTimeout())
       .WillOnce(Return(clock_.Now() + QuicTime::Delta::FromMilliseconds(10)));
   EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _))
@@ -2349,8 +2349,8 @@
   EXPECT_EQ(0u, BytesInFlight());
 
   // Migrate again with unACK'ed but not in-flight packet.
-  //  Packet3 shouldn't be marked for retransmission again as it is not in
-  //  flight.
+  // Packet3 shouldn't be marked for retransmission again as it is not in
+  // flight.
   old_send_algorithm =
       manager_.OnConnectionMigration(/*reset_send_algorithm=*/true);
 
@@ -2382,6 +2382,79 @@
                                    ENCRYPTION_FORWARD_SECURE));
   EXPECT_EQ(0u, BytesInFlight());
   EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero());
+
+  SendDataPacket(4, ENCRYPTION_FORWARD_SECURE);
+  // Trigger loss timeout and mark packet4 for retransmission.
+  EXPECT_CALL(*loss_algorithm, GetLossTimeout())
+      .WillOnce(Return(clock_.Now() + QuicTime::Delta::FromMilliseconds(10)));
+  EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _))
+      .WillOnce(WithArgs<5>(Invoke([](LostPacketVector* packet_lost) {
+        packet_lost->emplace_back(QuicPacketNumber(4u), kDefaultLength);
+        return LossDetectionInterface::DetectionStats();
+      })));
+  EXPECT_CALL(notifier_, OnFrameLost(_));
+  EXPECT_CALL(*send_algorithm_,
+              OnCongestionEvent(false, kDefaultLength, _, _, _));
+  EXPECT_CALL(*network_change_visitor_, OnCongestionChange());
+  manager_.OnRetransmissionTimeout();
+  EXPECT_EQ(0u, BytesInFlight());
+
+  // Application retransmit the data as LOSS_RETRANSMISSION.
+  RetransmitDataPacket(5, LOSS_RETRANSMISSION, ENCRYPTION_FORWARD_SECURE);
+  EXPECT_EQ(kDefaultLength, BytesInFlight());
+
+  clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(30));
+  // Receiving an ACK for packet4 should update RTT, but not bytes in flight.
+  // This spurious retransmission should be reported to the loss algorithm.
+  manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(),
+                           clock_.Now());
+  manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(5));
+  EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _));
+  EXPECT_CALL(*loss_algorithm, SpuriousLossDetected(_, _, _, _, _));
+  EXPECT_CALL(*send_algorithm_,
+              OnCongestionEvent(/*rtt_updated=*/true, kDefaultLength, _, _, _));
+  EXPECT_CALL(*network_change_visitor_, OnCongestionChange());
+  EXPECT_CALL(notifier_, OnFrameAcked(_, _, _));
+  EXPECT_EQ(PACKETS_NEWLY_ACKED,
+            manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3),
+                                   ENCRYPTION_FORWARD_SECURE));
+  EXPECT_EQ(kDefaultLength, BytesInFlight());
+  EXPECT_EQ(QuicTime::Delta::FromMilliseconds(30),
+            manager_.GetRttStats()->latest_rtt());
+
+  // Migrate again with in-flight packet5 whose retransmittable frames are all
+  // ACKed. Packet5 should be marked for retransmission but nothing to
+  // retransmit.
+  EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillOnce(Return(false));
+  EXPECT_CALL(notifier_, OnFrameLost(_)).Times(0u);
+  old_send_algorithm =
+      manager_.OnConnectionMigration(/*reset_send_algorithm=*/true);
+  EXPECT_EQ(default_init_rtt, rtt_stats->initial_rtt());
+  EXPECT_EQ(0u, manager_.GetConsecutiveRtoCount());
+  EXPECT_EQ(0u, manager_.GetConsecutiveTlpCount());
+  EXPECT_EQ(0u, BytesInFlight());
+  EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero());
+
+  manager_.SetSendAlgorithm(old_send_algorithm.release());
+
+  clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+  // Receiving an ACK for packet5 shouldn't update RTT. Though packet 5 was
+  // marked for retransmission, this spurious retransmission shouldn't be
+  // reported to the loss algorithm.
+  manager_.OnAckFrameStart(QuicPacketNumber(5), QuicTime::Delta::Infinite(),
+                           clock_.Now());
+  manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(6));
+  EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _));
+  EXPECT_CALL(*loss_algorithm, SpuriousLossDetected(_, _, _, _, _)).Times(0u);
+  EXPECT_CALL(*send_algorithm_,
+              OnCongestionEvent(/*rtt_updated=*/false, 0, _, _, _));
+  EXPECT_CALL(*network_change_visitor_, OnCongestionChange());
+  EXPECT_CALL(notifier_, OnFrameAcked(_, _, _));
+  EXPECT_EQ(PACKETS_NEWLY_ACKED,
+            manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3),
+                                   ENCRYPTION_FORWARD_SECURE));
+  EXPECT_EQ(0u, BytesInFlight());
+  EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero());
 }
 
 TEST_F(QuicSentPacketManagerTest, PathMtuIncreased) {