From 9d9e027a61ccc0ef8c21c944eb7171b4e3dfde37 Mon Sep 17 00:00:00 2001 From: Rick Newton-Rogers Date: Tue, 14 Jul 2026 09:43:12 +0100 Subject: [PATCH 1/2] 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, 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.. 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 From e26bce3cd7b7d5899cf3ebc6e0e98efdc10f4080 Mon Sep 17 00:00:00 2001 From: Rick Newton-Rogers Date: Thu, 6 Aug 2026 16:25:06 -0400 Subject: [PATCH 2/2] Base PTO probe decisions on the state being probed `sendPTO` chose how to probe using state that did not match what it was probing. `hasAckElicitingPendingItems` was read once for the space returned by `getEarliestTime` and then applied to *every space*. I don't think this should be the case because pending items are held per space. An error in the previous commit meant that the fallback probed unconditionally, so a PTO that fired with nothing in flight still sent a PING. This happens when the handshake completes and clears the in-flight packets after the timer was armed. `ackElicitingPacketsInFlight` no longer decrements below zero, so the read side does not need to detect negative values. The all-spaces sum that `sendPTO` and `resetTimer` both consult is now a shared property rather than duplicated. The two PTO tests now assert that a probe was recorded rather than that the PTO was counted, since the count advances on the attempt. They cover the two branches that can produce no packet: a tail retransmit that cannot be rebuilt, and new data that writes no payload. --- Sources/SwiftNetwork/QUIC/Recovery.swift | 63 ++++++++++++++-------- Tests/QUICTests/RecoveryTests.swift | 67 +++++++++++++++++++++--- 2 files changed, 102 insertions(+), 28 deletions(-) diff --git a/Sources/SwiftNetwork/QUIC/Recovery.swift b/Sources/SwiftNetwork/QUIC/Recovery.swift index 93eb7fb..6c605d4 100644 --- a/Sources/SwiftNetwork/QUIC/Recovery.swift +++ b/Sources/SwiftNetwork/QUIC/Recovery.swift @@ -363,7 +363,11 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { ) { if sentEntry.lostTime == .zero && sentEntry.packet.isInFlightEligible { if sentEntry.packet.isAckEliciting { - ackElicitingPacketsInFlight -= 1 + if ackElicitingPacketsInFlight > 0 { + ackElicitingPacketsInFlight -= 1 + } else { + log.fault("Cannot decrement ackElicitingPacketsInFlight below zero") + } let number = sentEntry.packet.number log.datapath( "Ack eliciting packet \(number) acked, decrementing ackElicitingPacketsInFlight to: \(ackElicitingPacketsInFlight)" @@ -913,7 +917,11 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { // and set the lost time. entry.lostTime = timeNow if entry.packet.isAckEliciting { - ackElicitingPacketsInFlight -= 1 + if ackElicitingPacketsInFlight > 0 { + ackElicitingPacketsInFlight -= 1 + } else { + connection.log.fault("Cannot decrement ackElicitingPacketsInFlight below zero") + } } lostPackets.append(entry.packet.identifier) Recovery.logAckElicitingPacketsInFlight( @@ -1118,19 +1126,10 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { mutating func sendPTO(connection: QUICConnection, path: QUICPath) { var sentPTO = false - let (_, packetNumberSpace) = getEarliestTime( - earliestTimeType: EarliestTimeType.lastSentAckElicitingTime, - connection: connection - ) - - let hasAckEliciting = connection.withPendingItems(for: packetNumberSpace) { $0.hasAckElicitingPendingItems } var discardInitialRecoveryState = false applyToAllInnerStatesMutable { innerState, packetNumberSpace in let ackElicitingPacketsInFlight = innerState.ackElicitingPacketsInFlight guard ackElicitingPacketsInFlight > 0 else { - if _slowPath(ackElicitingPacketsInFlight < 0) { - connection.log.fault("ackElicitingPacketsInFlight negative: \(ackElicitingPacketsInFlight)") - } return } @@ -1138,15 +1137,21 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { "PTO \(path.recoveryState.PTOCount) (\(packetNumberSpace)) fired on path \(path.identifier) with \(ackElicitingPacketsInFlight) ack-eliciting packets in flight" ) + let hasAckEliciting = connection.withPendingItems(for: packetNumberSpace) { + $0.hasAckElicitingPendingItems + } + if hasAckEliciting { connection.log.datapath("Sending next frames with new data as PTOs") - sentPTO = true let packets = connection.sendFramesFromRecovery( on: path, ignoreCongestionWindow: true, discardInitialRecoveryState: &discardInitialRecoveryState ) - if !innerState.recordSentPackets(packets, connection: connection) { + // Only a recorded packet counts as a probe; the pending items may write no payload. + if innerState.recordSentPackets(packets, connection: connection) { + sentPTO = true + } else { connection.log.datapath( "Unable to force send PTOs, likely flow-controlled or unavailable" ) @@ -1195,9 +1200,20 @@ 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 { + if totalAckElicitingPacketsInFlight == 0, peerCompletedValidation(connection: connection) { + // Nothing is in flight to probe for, so the state must have changed between arming the + // timer and it firing (e.g. the handshake completed and cleared the in-flight packets). + // `resetTimer` cancels the timer for this state once we return. + connection.log.fault("PTO fired after validation") + return + } + + // Send an ack-eliciting probe because either: + // - packets are in flight, RFC 9002 Section 6.2.4 requires a probe + // - nothing is in flight and the peer has not validated our address, RFC 9002 Section 6.2.2.1 + // requires an anti-deadlock packet to unblock the server. + // The PING is padded when it goes out in an initial packet. connection.log.datapath("Sending a PING as PTO") let packetNumberSpace = !connection.receivedHandshakePacket @@ -1320,13 +1336,8 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { } mutating func resetTimer(connection: QUICConnection) { - var ackElicitingPacketsInFlight = 0 // if there are ack eliciting packets on any of the innerStates, the L4S error should not be emitted - applyToAllInnerStatesImmutable { innerState, _ in - ackElicitingPacketsInFlight += innerState.ackElicitingPacketsInFlight - } - - if ackElicitingPacketsInFlight == 0 && peerCompletedValidation(connection: connection) { + if totalAckElicitingPacketsInFlight == 0 && peerCompletedValidation(connection: connection) { log.datapath("No ack eliciting packets in flight, cancelling timer") setTimer(delay: .zero, connection: connection) connection.withCurrentPath { path in @@ -1408,6 +1419,16 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { return hasOutstandingPackets } + // The PTO timer is shared across packet number spaces, so decisions about whether there is + // anything left to probe for consider every space. + var totalAckElicitingPacketsInFlight: Int { + var totalAckElicitingPacketsInFlight = 0 + applyToAllInnerStatesImmutable { innerState, _ in + totalAckElicitingPacketsInFlight += innerState.ackElicitingPacketsInFlight + } + return totalAckElicitingPacketsInFlight + } + mutating func resetPNSpace( packetNumberSpace: PacketNumberSpace, connection: QUICConnection diff --git a/Tests/QUICTests/RecoveryTests.swift b/Tests/QUICTests/RecoveryTests.swift index 961a706..1452ecc 100644 --- a/Tests/QUICTests/RecoveryTests.swift +++ b/Tests/QUICTests/RecoveryTests.swift @@ -656,8 +656,9 @@ final class RecoveryTests: XCTestCase { // 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() { + // is validated; otherwise `sendPTO` sends nothing and the connection makes no progress until the + // idle timeout closes it. + func testValidatedPTOProbesWhenTailRetransmitProducesNothing() { // 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) @@ -706,13 +707,65 @@ final class RecoveryTests: XCTestCase { } 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. + // A probe must have been recorded, taking the packets in flight to two, and the PTO counted. XCTAssertEqual( - path.recoveryState.PTOCount, - 1, - "PTO produced no probe for a closed-flow tail packet; connection would spin to idle timeout" + connection.recovery.totalAckElicitingPacketsInFlight, + 2, + "PTO produced no probe for a closed-flow tail packet" ) + XCTAssertEqual(path.recoveryState.PTOCount, 1) + } + + // `sendPTO` must emit a probe; otherwise the PTO makes no progress. A pending item whose flow was + // torn down writes no payload, so ensure the probe only counts once the packet is recorded. + func testPTOProbesWhenNewDataProducesNothing() { + // A stream queued for service whose flow has since been torn down: it is absent from + // `multiplexedFlows`, so writing it produces no payload. + let unregisteredStream = QUICStreamInstance(parent: connection, inbound: true) + unregisteredStream.setup(streamID: QUICStreamID(0), logPrefixer: recoveryTestsLogPrefixer) + XCTAssertNil(connection.flow(for: unregisteredStream.identifier)) + connection.withPendingItems(for: .initial) { pendingItems in + pendingItems.streamsToService.append(unregisteredStream.identifier) + pendingItems.stream = true + } + + // Recovery still sees new ack-eliciting data, so the PTO sends that rather than retransmitting. + let hasPendingAckEliciting = connection.withPendingItems(for: .initial) { + $0.hasAckElicitingPendingItems + } + XCTAssertTrue(hasPendingAckEliciting) + + // One ack-eliciting packet outstanding, so the PTO is armed and the per-space loop runs. + var packet = SentPacketRecord() + packet.identifier = .init(space: .initial, number: 0) + packet.isInFlightEligible = true + packet.isAckEliciting = true + packet.totalLength = 20 + 96 + packet.sentPath = connection.currentPath?.identifier ?? .none + + sentPacket(packet, connection: connection) + + connection.recovery.withImmutableInnerState(packetNumberSpace: .initial) { innerState in + XCTAssertEqual(innerState.ackElicitingPacketsInFlight, 1) + } + + 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) + + // Ensure a probe has been recorded, taking the packets in flight to two. + connection.recovery.withImmutableInnerState(packetNumberSpace: .initial) { innerState in + XCTAssertEqual( + innerState.ackElicitingPacketsInFlight, + 2, + "PTO reported a probe but no packet was sent" + ) + } } }