Skip to content

Don't gate PTO packets on address validation #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rnro wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −38
Draft

Don't gate PTO packets on address validation #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 41 additions & 38 deletions Sources/SwiftNetwork/QUIC/Recovery.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)")

@agnosticdev agnosticdev Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make sure we are not underflowing ackElicitingPacketsInFlight, possibly adding an underflow check to -= 1 since this is an integer. That way we do not have to log out these negative values.

}
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
Expand All @@ -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 {
Expand All @@ -1174,6 +1180,7 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
discardInitialRecoveryState: &discardInitialRecoveryState
)
}

if let packets, innerState.recordSentPackets(packets, connection: connection) {
sentPTO = true
addedPackets += 1
Expand All @@ -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")

@agnosticdev agnosticdev Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically this log has been a warning that something in Recovery may not be working as expected, or you may have run into a race condition in the logic. For example, I am looking at a case now where ackElicitingPacketsInFlight is 1, but then the handshake completes, clears the ack-eliciting packets, resetPTOCount set the count back 0, and then sendPTO falls into this path as a matter of timing. When this happens sendPTO tries to reset the timer and realizes that there are now no ackElicitingPacketsInFlight and then cancels the timer and continues on as normal.
So in this path we have now removed the resetTimer functionality, what is the rationale there?

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 {
Expand Down
61 changes: 61 additions & 0 deletions Tests/QUICTests/RecoveryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading