fix(s2n-quic-transport): don't let cwnd drop below initial cwnd when MTU decreases (#2308)

Author: WesleyRosenblum

quic/s2n-quic-core/src/recovery/bbr.rs

@@ -532,8 +532,6 @@ impl CongestionController for BbrCongestionController {
     //# window measured in bytes [RFC4821].
 
     //= https://www.rfc-editor.org/rfc/rfc9002#section-7.2
-    //= type=exception
-    //= reason=The maximum datagram size remains at the minimum (1200 bytes) during the handshake
     //# If the maximum datagram size is decreased in order to complete the
     //# handshake, the congestion window SHOULD be set to the new initial
     //# congestion window.
@@ -542,8 +540,11 @@ impl CongestionController for BbrCongestionController {
         let old_max_datagram_size = self.max_datagram_size;
         self.max_datagram_size = max_datagram_size;
 
-        self.cwnd =
+        let cwnd =
             ((self.cwnd as f32 / old_max_datagram_size as f32) * max_datagram_size as f32) as u32;
+        let initial_window = Self::initial_window(max_datagram_size, &Default::default());
+
+        self.cwnd = max(cwnd, initial_window);
     }
 
     #[inline]

…__bbr__tests__events__on_mtu_update.snap …decrease_larger_than_initial_window.snapquic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update.snap renamed to quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update.snap renamed to quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap

@@ -0,0 +1,5 @@
+---
+source: quic/s2n-quic-core/src/recovery/bbr/tests.rs
+expression: ""
+---
+

quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap

@@ -0,0 +1,5 @@
+---
+source: quic/s2n-quic-core/src/recovery/bbr/tests.rs
+expression: ""
+---
+

quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_increase.snap

@@ -1103,7 +1103,7 @@ fn control_update_required() {
 }
 
 #[test]
-fn on_mtu_update() {
+fn on_mtu_update_increase() {
     let mut mtu = 5000;
     let mut bbr = BbrCongestionController::new(mtu, Default::default());
     let mut publisher = event::testing::Publisher::snapshot();
@@ -1118,6 +1118,42 @@ fn on_mtu_update() {
     assert_eq!(bbr.cwnd, 200_000);
 }
 
+#[test]
+fn on_mtu_update_decrease_smaller_than_initial_window() {
+    let mut mtu = 9000;
+    let mut bbr = BbrCongestionController::new(mtu, Default::default());
+    let mut publisher = event::testing::Publisher::snapshot();
+    let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id());
+
+    bbr.cwnd = 18_000;
+
+    mtu = 1350;
+    bbr.on_mtu_update(mtu, &mut publisher);
+
+    assert_eq!(bbr.max_datagram_size, mtu);
+
+    // updated initial window is 10 x MTU = 10 x 1350
+    assert_eq!(bbr.cwnd, 13_500);
+}
+
+#[test]
+fn on_mtu_update_decrease_larger_than_initial_window() {
+    let mut mtu = 9000;
+    let mut bbr = BbrCongestionController::new(mtu, Default::default());
+    let mut publisher = event::testing::Publisher::snapshot();
+    let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id());
+
+    bbr.cwnd = 180_000;
+
+    mtu = 1350;
+    bbr.on_mtu_update(mtu, &mut publisher);
+
+    assert_eq!(bbr.max_datagram_size, mtu);
+
+    // Congestion window is still 20 packets based on the updated MTU
+    assert_eq!(bbr.cwnd, 27_000);
+}
+
 /// Helper method to move the given BBR congestion controller into the
 /// ProbeBW state with the given CyclePhase
 fn enter_probe_bw_state<Pub: Publisher>(

quic/s2n-quic-core/src/recovery/bbr/tests.rs

@@ -451,8 +451,6 @@ impl CongestionController for CubicCongestionController {
     //# limit to compensate for the size of the actual packets.
 
     //= https://www.rfc-editor.org/rfc/rfc9002#section-7.2
-    //= type=exception
-    //= reason=The maximum datagram size remains at the minimum (1200 bytes) during the handshake
     //# If the maximum datagram size is decreased in order to complete the
     //# handshake, the congestion window SHOULD be set to the new initial
     //# congestion window.
@@ -462,8 +460,12 @@ impl CongestionController for CubicCongestionController {
         self.max_datagram_size = max_datagram_size;
         self.cubic.max_datagram_size = max_datagram_size;
 
-        self.congestion_window =
+        let congestion_window =
             (self.congestion_window / old_max_datagram_size as f32) * max_datagram_size as f32;
+        let initial_window =
+            Self::initial_window(&self.cubic, max_datagram_size, &Default::default());
+
+        self.congestion_window = max(congestion_window as u32, initial_window) as f32;
     }
 
     //= https://www.rfc-editor.org/rfc/rfc9002#section-6.4

quic/s2n-quic-core/src/recovery/cubic.rs

@@ -0,0 +1,5 @@
+---
+source: quic/s2n-quic-core/src/recovery/cubic/tests.rs
+expression: ""
+---
+

quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap

@@ -0,0 +1,5 @@
+---
+source: quic/s2n-quic-core/src/recovery/cubic/tests.rs
+expression: ""
+---
+

quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap

@@ -790,25 +790,64 @@ fn on_packet_lost_persistent_congestion() {
 #[test]
 fn on_mtu_update_increase() {
     let mut mtu = 5000;
-    let cwnd_in_packets = 100_000f32;
-    let cwnd_in_bytes = cwnd_in_packets / mtu as f32;
+    let cwnd_in_bytes = 100_000f32;
     let mut cc = CubicCongestionController::new(mtu, Default::default());
     let mut publisher = event::testing::Publisher::snapshot();
     let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id());
-    cc.congestion_window = cwnd_in_packets;
+    cc.congestion_window = cwnd_in_bytes;
 
     mtu = 10000;
     cc.on_mtu_update(mtu, &mut publisher);
     assert_eq!(cc.max_datagram_size, mtu);
     assert_eq!(cc.cubic.max_datagram_size, mtu);
 
     assert_delta!(cc.congestion_window, 200_000.0, 0.001);
+}
 
-    //= https://www.rfc-editor.org/rfc/rfc8899#section-3
-    //= type=test
-    //# An update to the PLPMTU (or MPS) MUST NOT increase the congestion
-    //# window measured in bytes [RFC4821].
-    assert_delta!(cc.congestion_window / mtu as f32, cwnd_in_bytes, 0.001);
+//= https://www.rfc-editor.org/rfc/rfc9002#section-7.2
+//= type=test
+//# If the maximum datagram size changes during the connection, the
+//# initial congestion window SHOULD be recalculated with the new size.
+
+//= https://www.rfc-editor.org/rfc/rfc9002#section-7.2
+//= type=test
+//# If the maximum datagram size is decreased in order to complete the
+//# handshake, the congestion window SHOULD be set to the new initial
+//# congestion window.
+#[test]
+fn on_mtu_update_decrease_smaller_than_initial_window() {
+    let mut mtu = 9000;
+    let cwnd_in_bytes = 18_000f32;
+    let mut cc = CubicCongestionController::new(mtu, Default::default());
+    let mut publisher = event::testing::Publisher::snapshot();
+    let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id());
+    cc.congestion_window = cwnd_in_bytes;
+
+    mtu = 1350;
+    cc.on_mtu_update(mtu, &mut publisher);
+    assert_eq!(cc.max_datagram_size, mtu);
+    assert_eq!(cc.cubic.max_datagram_size, mtu);
+
+    // updated initial window is 10 x MTU = 10 x 1350
+    assert_delta!(cc.congestion_window, 13_500.0, 0.001);
+}
+
+#[test]
+fn on_mtu_update_decrease_larger_than_initial_window() {
+    let mut mtu = 9000;
+    let cwnd_in_bytes = 180_000f32;
+    let mut cc = CubicCongestionController::new(mtu, Default::default());
+    let mut publisher = event::testing::Publisher::snapshot();
+    let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id());
+    cc.congestion_window = cwnd_in_bytes;
+
+    mtu = 1350;
+    cc.on_mtu_update(mtu, &mut publisher);
+    assert_eq!(cc.max_datagram_size, mtu);
+    assert_eq!(cc.cubic.max_datagram_size, mtu);
+
+    // Congestion window is still 20 packets based on the updated MTU
+    assert_delta!(cc.congestion_window, 27_000.0, 0.001);
 }
 
 //= https://www.rfc-editor.org/rfc/rfc9002#section-6.4