From a61b13a005141c14af347df55bc45695b4a382e0 Mon Sep 17 00:00:00 2001
From: Jolene Tan <lene@meta.com>
Date: Wed, 15 Jan 2025 14:08:52 -0800
Subject: [PATCH] Add option to clone all CRYPTO packets at most once

Summary: Restore the buggy version of `cloneAllPacketsWithCryptoFrame`, which did allow up to two packets to be cloned in the same PTO flight but would only clone them at most once, as an optional mode of operation.

Reviewed By: mjoras

Differential Revision: D67994014

fbshipit-source-id: 16f66fd48a9a4410427ba21dea6e7f6d1eee48e6
---
 quic/api/QuicPacketScheduler.cpp          |  3 ++
 quic/api/test/QuicPacketSchedulerTest.cpp | 34 +++++++++++++++--------
 quic/state/TransportSettings.h            |  1 +
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp
index 039e5ddb7..92e3404d5 100644
--- a/quic/api/QuicPacketScheduler.cpp
+++ b/quic/api/QuicPacketScheduler.cpp
@@ -938,6 +938,9 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket(
         if (std::find_if(frames.begin(), frames.end(), [](const auto& frame) {
               return frame.type() == QuicWriteFrame::Type::WriteCryptoFrame;
             }) != frames.end()) {
+          if (conn_.transportSettings.cloneCryptoPacketsAtMostOnce) {
+            continue;
+          }
           auto mostRecentOutstandingPacketIdentifier =
               conn_.outstandings.packets.back().maybeClonedPacketIdentifier;
           if (mostRecentOutstandingPacketIdentifier ==
diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp
index e193bda5a..a658e0821 100644
--- a/quic/api/test/QuicPacketSchedulerTest.cpp
+++ b/quic/api/test/QuicPacketSchedulerTest.cpp
@@ -630,14 +630,18 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) {
   EXPECT_EQ(expected, result.clonedPacketIdentifier->packetNumber);
 }
 
-class CloneAllPacketsWithCryptoFrameTest : public QuicPacketSchedulerTest,
-                                           public WithParamInterface<bool> {};
+class CloneAllPacketsWithCryptoFrameTest
+    : public QuicPacketSchedulerTest,
+      public WithParamInterface<std::tuple<bool, bool>> {};
 TEST_P(
     CloneAllPacketsWithCryptoFrameTest,
     TestCloneAllPacketsWithCryptoFrameTrueFalse) {
   QuicClientConnectionState conn(
       FizzClientQuicHandshakeContext::Builder().build());
-  conn.transportSettings.cloneAllPacketsWithCryptoFrame = GetParam();
+  auto testParams = GetParam();
+  conn.transportSettings.cloneAllPacketsWithCryptoFrame =
+      std::get<0>(testParams);
+  conn.transportSettings.cloneCryptoPacketsAtMostOnce = std::get<1>(testParams);
   FrameScheduler noopScheduler("frame", conn);
   CloningScheduler cloningScheduler(noopScheduler, conn, "cryptoClone", 0);
 
@@ -671,7 +675,7 @@ TEST_P(
         MaxDataFrame(conn.flowControlState.advertisedMaxOffset));
   }
 
-  // Add a third outstanding packet, which is a clone of the first packet
+  // Add a third outstanding packet, which is a re-clone of the first packet
   {
     addInitialOutstandingPacket(conn);
     conn.outstandings.packets.back().packet.frames.push_back(
@@ -687,6 +691,7 @@ TEST_P(
         MaxDataFrame(conn.flowControlState.advertisedMaxOffset));
   }
 
+  // Schedule a fourth packet
   std::vector<uint8_t> zeroConnIdData(quic::kDefaultConnectionIdSize, 0);
   ConnectionId srcConnId(zeroConnIdData);
   LongHeader header(
@@ -701,18 +706,25 @@ TEST_P(
       conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
   auto result = cloningScheduler.scheduleFramesForPacket(
       std::move(builder), kDefaultUDPSendPacketLen);
-  EXPECT_TRUE(
-      result.clonedPacketIdentifier.has_value() && result.packet.has_value());
-  EXPECT_EQ(
-      conn.transportSettings.cloneAllPacketsWithCryptoFrame ? secondPacketNum
-                                                            : firstPacketNum,
-      result.clonedPacketIdentifier->packetNumber);
+  if (conn.transportSettings.cloneAllPacketsWithCryptoFrame &&
+      conn.transportSettings.cloneCryptoPacketsAtMostOnce) {
+    // First and second packets already cloned, skip all and schedule no packet
+    EXPECT_FALSE(result.clonedPacketIdentifier.has_value());
+    EXPECT_FALSE(result.packet.has_value());
+  } else {
+    EXPECT_TRUE(
+        result.clonedPacketIdentifier.has_value() && result.packet.has_value());
+    EXPECT_EQ(
+        conn.transportSettings.cloneAllPacketsWithCryptoFrame ? secondPacketNum
+                                                              : firstPacketNum,
+        result.clonedPacketIdentifier->packetNumber);
+  }
 }
 
 INSTANTIATE_TEST_SUITE_P(
     CloneAllPacketsWithCryptoFrameTest,
     CloneAllPacketsWithCryptoFrameTest,
-    Bool());
+    Combine(Bool(), Bool()));
 
 TEST_F(QuicPacketSchedulerTest, DoNotSkipUnclonedCryptoPacket) {
   QuicClientConnectionState conn(
diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h
index 5a81966b2..8d226db21 100644
--- a/quic/state/TransportSettings.h
+++ b/quic/state/TransportSettings.h
@@ -413,6 +413,7 @@ struct TransportSettings {
   // receiving and then processing.
   bool networkDataPerSocketRead{false};
   bool cloneAllPacketsWithCryptoFrame{false};
+  bool cloneCryptoPacketsAtMostOnce{false};
   // Use a reordering threshold heuristic of inflight / 2.
   bool useInflightReorderingThreshold{false};
   // Raise read callbacks for all unidirectional streams first on data