From 9d9e027a61ccc0ef8c21c944eb7171b4e3dfde37 Mon Sep 17 00:00:00 2001
From: Rick Newton-Rogers <rnro@apple.com>
Date: Tue, 14 Jul 2026 09:43:12 +0100
Subject: [PATCH] Don't gate PTO packets on address validation

At the moment the code will not send an ack-eliciting ping once the
peer's address is validated. This appears to be linked to RFC 9000
Section 8.1, which states
> To prevent this deadlock, clients MUST send a packet on a Probe
> Timeout (PTO) [...] the client MUST send an Initial packet in a UDP
> datagram that contains at least 1200 bytes if it does not have
> Handshake keys, and otherwise send a Handshake packet.
However I think this is a slight mis-read, and that RFC 9002 Section
6.2.4
> When a PTO timer expires, a sender MUST send at least one ack-eliciting
> packet [...] When there is no data to send, the sender SHOULD send a
> PING or other ack-eliciting frame in a single packet, rearming the PTO
> timer.
shows that whilst validation is an important inflection point for the two
behaviors, PTO pings should still be sent after it. The pseudocode in A9
I think also indicates this.

A new test with the current code results in outstanding frames which will
never have a PTO fire and the current code will end up in a tight loop.

This change removes the validation distinction in `sendPTO`, along with
the now-unused validation local and the timer-clear branch it fed; the
difference in behavior required by the RFC will be enacted by the timer.
---
 Sources/SwiftNetwork/QUIC/Recovery.swift | 79 ++++++++++++------------
 Tests/QUICTests/RecoveryTests.swift      | 61 ++++++++++++++++++
 2 files changed, 102 insertions(+), 38 deletions(-)

diff --git a/Sources/SwiftNetwork/QUIC/Recovery.swift b/Sources/SwiftNetwork/QUIC/Recovery.swift
index acda37d..93eb7fb 100644
--- a/Sources/SwiftNetwork/QUIC/Recovery.swift
+++ b/Sources/SwiftNetwork/QUIC/Recovery.swift
@@ -656,14 +656,16 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
             _ packets: consuming NetworkUniqueDeque<SentPacketRecord>,
             connection: QUICConnection
         ) -> Bool {
-            var packets = packets
-            guard !packets.isEmpty else {
+            if packets.isEmpty {
                 return false
             }
+
+            var packets = packets
             while !packets.isEmpty {
-                let packet = packets.remove(at: 0)
+                let packet = packets.removeFirst()
                 sentPacket(packet, time: connection.now, connection: connection)
             }
+
             return true
         }
     }
@@ -754,7 +756,7 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
     ) {
         var sentPackets = sentPackets
         while !sentPackets.isEmpty {
-            let packet = sentPackets.remove(at: 0)
+            let packet = sentPackets.removeFirst()
             sentPacket(packet, time: connection.now, connection: connection)
         }
         if inBatch {
@@ -1115,25 +1117,27 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
 
     mutating func sendPTO(connection: QUICConnection, path: QUICPath) {
         var sentPTO = false
-        let (_, pnSpace) = getEarliestTime(
+
+        let (_, packetNumberSpace) = getEarliestTime(
             earliestTimeType: EarliestTimeType.lastSentAckElicitingTime,
             connection: connection
         )
-        var hasAckEliciting = false
-        connection.withPendingItems(for: pnSpace) { pendingItems in
-            hasAckEliciting = pendingItems.hasAckElicitingPendingItems
-        }
-        let peerCompletedValidation = peerCompletedValidation(connection: connection)
-        var shouldClearTimer = false
+
+        let hasAckEliciting = connection.withPendingItems(for: packetNumberSpace) { $0.hasAckElicitingPendingItems }
         var discardInitialRecoveryState = false
         applyToAllInnerStatesMutable { innerState, packetNumberSpace in
             let ackElicitingPacketsInFlight = innerState.ackElicitingPacketsInFlight
-            if ackElicitingPacketsInFlight == 0 {
+            guard ackElicitingPacketsInFlight > 0 else {
+                if _slowPath(ackElicitingPacketsInFlight < 0) {
+                    connection.log.fault("ackElicitingPacketsInFlight negative: \(ackElicitingPacketsInFlight)")
+                }
                 return
             }
+
             connection.log.datapath(
                 "PTO \(path.recoveryState.PTOCount) (\(packetNumberSpace)) fired on path \(path.identifier) with \(ackElicitingPacketsInFlight) ack-eliciting packets in flight"
             )
+
             if hasAckEliciting {
                 connection.log.datapath("Sending next frames with new data as PTOs")
                 sentPTO = true
@@ -1147,8 +1151,10 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
                         "Unable to force send PTOs, likely flow-controlled or unavailable"
                     )
                 }
-            } else if ackElicitingPacketsInFlight > 0 {
+
+            } else {
                 connection.log.datapath("Retransmitting two tail-packets as PTO")
+
                 var addedPackets = 0
                 let packetCount = innerState.outstandingPackets.count
                 for i in 0..<packetCount {
@@ -1174,6 +1180,7 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
                             discardInitialRecoveryState: &discardInitialRecoveryState
                         )
                     }
+
                     if let packets, innerState.recordSentPackets(packets, connection: connection) {
                         sentPTO = true
                         addedPackets += 1
@@ -1188,37 +1195,33 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
             }
         }
 
+        // Anti deadlock PING frame (i.e PADDED PING). The PING will be padded when we send an initial packet.
+        // Issue whether or not handshake has completed, the timer will make the distinction.
         if !sentPTO {
             connection.log.datapath("Sending a PING as PTO")
-            if peerCompletedValidation {
-                connection.log.fault("PTO fired after validation")
-                shouldClearTimer = true
-            } else {
-                // Anti deadlock PING frame (i.e PADDED PING). The PING will be padded when we send an initial packet.
-                let pnSpace =
-                    !connection.receivedHandshakePacket
-                    ? PacketNumberSpace.initial : PacketNumberSpace.applicationData
-                connection.withPendingItems(for: pnSpace) { item in
-                    item.ping = true
-                }
-                sentPTO = true
-                withMutableInnerState(packetNumberSpace: pnSpace) { innerState in
-                    let packets = connection.sendFramesFromRecovery(
-                        on: path,
-                        ignoreCongestionWindow: true,
-                        discardInitialRecoveryState: &discardInitialRecoveryState
+            let packetNumberSpace =
+                !connection.receivedHandshakePacket
+                ? PacketNumberSpace.initial : PacketNumberSpace.applicationData
+            connection.withPendingItems(for: packetNumberSpace) { item in
+                item.ping = true
+            }
+
+            sentPTO = true
+
+            withMutableInnerState(packetNumberSpace: packetNumberSpace) { innerState in
+                let packets = connection.sendFramesFromRecovery(
+                    on: path,
+                    ignoreCongestionWindow: true,
+                    discardInitialRecoveryState: &discardInitialRecoveryState
+                )
+                if !innerState.recordSentPackets(packets, connection: connection) {
+                    connection.log.datapath(
+                        "Unable to force send PTOs, likely flow-controlled or unavailable"
                     )
-                    if !innerState.recordSentPackets(packets, connection: connection) {
-                        connection.log.datapath(
-                            "Unable to force send PTOs, likely flow-controlled or unavailable"
-                        )
-                    }
                 }
             }
         }
-        if shouldClearTimer {
-            setTimer(delay: .zero, connection: connection)
-        }
+
         if discardInitialRecoveryState {
             self.resetPNSpace(packetNumberSpace: .initial, connection: connection)
             connection.withCurrentPath {
diff --git a/Tests/QUICTests/RecoveryTests.swift b/Tests/QUICTests/RecoveryTests.swift
index 81031f9..961a706 100644
--- a/Tests/QUICTests/RecoveryTests.swift
+++ b/Tests/QUICTests/RecoveryTests.swift
@@ -653,6 +653,67 @@ final class RecoveryTests: XCTestCase {
         XCTAssertEqual(path.recoveryState.PTOCount, 3)
         XCTAssertGreaterThan(connection.recovery.computedTimeout, .milliseconds(7900))
     }
+
+    // A PTO with ack-eliciting data in flight must emit a probe, even when the only outstanding
+    // packet carries STREAM data for a now-closed flow (so it can't be rebuilt) and the connection
+    // is validated; otherwise sendPTO sends nothing and the connection spins to idle timeout.
+    func testPTOWithClosedFlowStreamPacketStillProbes() {
+        // Register a flow and close it, so its STREAM data can never be rebuilt for retransmission.
+        let stream = QUICStreamInstance(parent: connection, inbound: true)
+        stream.setup(streamID: QUICStreamID(0), logPrefixer: recoveryTestsLogPrefixer)
+        connection.multiplexedFlows[stream.identifier] = stream
+        stream.closed = true
+        XCTAssertFalse(stream.isOpen)
+
+        // A single ack-eliciting application-data packet is outstanding, carrying only that flow's
+        // STREAM data. This keeps ackElicitingPacketsInFlight > 0 (so the PTO stays armed) while
+        // being unrebuildable once the flow is closed.
+        var packet = SentPacketRecord()
+        packet.identifier = .init(space: .applicationData, number: 0)
+        packet.isInFlightEligible = true
+        packet.isAckEliciting = true
+        packet.totalLength = 20 + 96
+        packet.sentPath = connection.currentPath?.identifier ?? .none
+        packet.transmittedItems.sentStreams.append(
+            TransmittedItems.SentStream(
+                flowID: stream.identifier,
+                streamID: QUICStreamID(0),
+                offset: 0,
+                length: 32,
+                isFinal: true
+            )
+        )
+        XCTAssertTrue(packet.transmittedItems.hasRetransmissibleItems)
+        sentPacket(packet, connection: connection)
+
+        connection.recovery.withImmutableInnerState(packetNumberSpace: .applicationData) { innerState in
+            XCTAssertEqual(innerState.ackElicitingPacketsInFlight, 1)
+        }
+
+        // Establish (address-validated) connection: peerCompletedValidation must be true. This is
+        // the condition under which the anti-deadlock PING is incorrectly skipped.
+        connection.recovery.received1RTTAck = true
+        XCTAssertTrue(connection.recovery.peerCompletedValidation(connection: connection))
+        XCTAssertEqual(path.recoveryState.PTOCount, 0)
+
+        // Fire the PTO with no new ack-eliciting data pending.
+        let expectation = XCTestExpectation()
+        self.connection.context.async {
+            self.connection.withCurrentPath { path in
+                self.connection.recovery.sendPTO(connection: self.connection, path: path)
+            }
+            expectation.fulfill()
+        }
+        wait(for: [expectation], timeout: 5.0)
+
+        // The PTO must have sent a probe and advanced the PTO count. On the buggy code no probe is
+        // sent and PTOCount stays 0, so the connection would spin until idle timeout.
+        XCTAssertEqual(
+            path.recoveryState.PTOCount,
+            1,
+            "PTO produced no probe for a closed-flow tail packet; connection would spin to idle timeout"
+        )
+    }
 }
 
 #endif