Proposed fix to mozilla#585

As well as tracking largest acked sent time, track oldest lost packet
time. This is reset if largest acked sent time changes.

neqo-transport/src/cc.rs

@@ -6,7 +6,7 @@
 
 // Congestion control
 
-use std::cmp::max;
+use std::cmp::{max, min};
 use std::fmt::{self, Display};
 use std::time::{Duration, Instant};
 
@@ -29,6 +29,8 @@ pub struct CongestionControl {
     bytes_in_flight: usize,
     congestion_recovery_start_time: Option<Instant>,
     ssthresh: usize,
+
+    oldest_lost_pkt_time: Option<Instant>,
 }
 
 impl Default for CongestionControl {
@@ -38,6 +40,7 @@ impl Default for CongestionControl {
             bytes_in_flight: 0,
             congestion_recovery_start_time: None,
             ssthresh: std::usize::MAX,
+            oldest_lost_pkt_time: None,
         }
     }
 }
@@ -97,10 +100,14 @@ impl CongestionControl {
         }
     }
 
+    pub fn clear_oldest_lost_pkt_time(&mut self) {
+        self.oldest_lost_pkt_time = None;
+    }
+
     pub fn on_packets_lost(
         &mut self,
         now: Instant,
-        largest_acked_sent: Option<Instant>,
+        prev_largest_acked_sent: Option<Instant>,
         pto: Duration,
         lost_packets: &[SentPacket],
     ) {
@@ -111,6 +118,16 @@ impl CongestionControl {
         for pkt in lost_packets.iter().filter(|pkt| pkt.cc_in_flight()) {
             assert!(self.bytes_in_flight >= pkt.size);
             self.bytes_in_flight -= pkt.size;
+
+            // Find the send time of the first lost packet since the last acked
+            // packet. Further below will compare this with the last lost
+            // packet, to see if persistent congestion is present.
+            if Some(pkt.time_sent) > prev_largest_acked_sent {
+                self.oldest_lost_pkt_time = match self.oldest_lost_pkt_time {
+                    None => Some(pkt.time_sent),
+                    Some(prev_oldest) => Some(min(prev_oldest, pkt.time_sent)),
+                }
+            }
         }
 
         qdebug!([self], "Pkts lost {}", lost_packets.len());
@@ -121,12 +138,11 @@ impl CongestionControl {
         let in_persistent_congestion = {
             let congestion_period = pto * PERSISTENT_CONG_THRESH;
 
-            match largest_acked_sent {
-                Some(las) => las < last_lost_pkt.time_sent - congestion_period,
+            match self.oldest_lost_pkt_time {
+                Some(olpt) => olpt < last_lost_pkt.time_sent - congestion_period,
                 None => {
-                    // Nothing has ever been acked. Could still be PC.
-                    let first_lost_pkt_sent = lost_packets.first().unwrap().time_sent;
-                    last_lost_pkt.time_sent - first_lost_pkt_sent > congestion_period
+                    // No previous lost packets, it's not PC.
+                    false
                 }
             }
         };

neqo-transport/src/recovery.rs

@@ -491,6 +491,7 @@ impl LossRecovery {
 
         // Track largest PN acked per space
         let prev_largest_acked_sent_time = space.largest_acked_sent_time;
+        let mut curr_largest_acked_sent_time = None;
         if Some(largest_acked) > space.largest_acked {
             space.largest_acked = Some(largest_acked);
 
@@ -502,6 +503,9 @@ impl LossRecovery {
                 let latest_rtt = now - largest_acked_pkt.time_sent;
                 self.rtt_vals.update_rtt(latest_rtt, ack_delay);
             }
+
+            curr_largest_acked_sent_time =
+                max(curr_largest_acked_sent_time, space.largest_acked_sent_time);
         }
         self.cc.on_packets_acked(&acked_packets);
 
@@ -520,6 +524,12 @@ impl LossRecovery {
             &lost_packets,
         );
 
+        // If a more recent packet has just been acked, start over in tracking
+        // for persistent congestion detection
+        if curr_largest_acked_sent_time != prev_largest_acked_sent_time {
+            self.cc.clear_oldest_lost_pkt_time();
+        }
+
         self.pto_state = None;
 
         (acked_packets, lost_packets)