From 60efe5be2c6d8adcd66f5b012c763c1d7ff1c89b Mon Sep 17 00:00:00 2001 From: Kriti Kukreja Date: Mon, 13 Jul 2026 15:24:33 -0700 Subject: [PATCH 1/2] Tear down old path after migration --- Sources/SwiftNetwork/QUIC/Migration.swift | 46 ++++++-- Sources/SwiftNetwork/QUIC/QUICPath.swift | 4 + Tests/QUICTests/MigrationTests.swift | 103 ++++++++++++++++++ .../SwiftNetworkQUICStackTests.swift | 21 ++++ 4 files changed, 164 insertions(+), 10 deletions(-) create mode 100644 Tests/QUICTests/MigrationTests.swift diff --git a/Sources/SwiftNetwork/QUIC/Migration.swift b/Sources/SwiftNetwork/QUIC/Migration.swift index d5dec2a..578436d 100644 --- a/Sources/SwiftNetwork/QUIC/Migration.swift +++ b/Sources/SwiftNetwork/QUIC/Migration.swift @@ -106,6 +106,7 @@ struct Migration: ~Copyable { return } + let oldPath = connection.currentPath connection.log.notice("Migrating to path \(path.identifier)") connection.currentPath = path path.spinValue = connection.initialSpinValue @@ -128,6 +129,10 @@ struct Migration: ~Copyable { } // TODO: Handle preferred address migration + // Remove the path we just migrated away from. + if let oldPath, oldPath != path { + connection.tearDownMigratedPath(oldPath) + } } func probingPathCount(_ connection: QUICConnection) -> Int { @@ -197,16 +202,7 @@ extension QUICConnection { } break case .unavailable: - if path.isOpenForSending, let dcid = path.dcid, - let sequence = remoteCIDs.retire(connectionID: dcid) - { - withPendingItems( - for: .applicationData, - block: { - $0.addRetireConnectionID(FrameRetireConnectionID(sequence: sequence)) - } - ) - } + retireOutboundCID(forPathGoingAway: path) path.changeState(to: .routeUnavailable) break } @@ -225,5 +221,35 @@ extension QUICConnection { sendFrames(on: path) } } + + // Retires a path's outbound CID and queues a RETIRE_CONNECTION_ID frame for it. + func retireOutboundCID(forPathGoingAway path: QUICPath) { + guard path.isOpenForSending, !path.hasPreAssignedCIDs, let dcid = path.dcid, + let sequence = remoteCIDs.retire(connectionID: dcid) + else { + return + } + withPendingItems(for: .applicationData) { + $0.addRetireConnectionID(FrameRetireConnectionID(sequence: sequence)) + } + } + + // Removes a path we migrated away from. + func tearDownMigratedPath(_ oldPath: QUICPath) { + guard oldPath !== currentPath else { + log.fault("Refusing to tear down the current path \(oldPath.identifier)") + return + } + log.notice("Tearing down old path \(oldPath.identifier) after migration") + + retireOutboundCID(forPathGoingAway: oldPath) + + if oldPath.state.isValidStateChange(to: .routeUnavailable) { + oldPath.changeState(to: .routeUnavailable) + } + oldPath.tearDownLowerStack() + multiplexingPaths.removeValue(forKey: oldPath.identifier) + sendFrames() + } } #endif diff --git a/Sources/SwiftNetwork/QUIC/QUICPath.swift b/Sources/SwiftNetwork/QUIC/QUICPath.swift index 77c6464..f36b470 100644 --- a/Sources/SwiftNetwork/QUIC/QUICPath.swift +++ b/Sources/SwiftNetwork/QUIC/QUICPath.swift @@ -623,6 +623,10 @@ public final class QUICPath: MultiplexingDatagramPath, Equatable parentProtocol.migration.migrate(to: self, connection: parentProtocol) } } + + func tearDownLowerStack() { + try? lower.invokeDetach(self.reference) + } } // Congestion Control access diff --git a/Tests/QUICTests/MigrationTests.swift b/Tests/QUICTests/MigrationTests.swift new file mode 100644 index 0000000..9179ef9 --- /dev/null +++ b/Tests/QUICTests/MigrationTests.swift @@ -0,0 +1,103 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift open source project +// +// Copyright (c) 2026 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +#if !NETWORK_NO_SWIFT_QUIC + +import XCTest + +#if canImport(SwiftNetwork) +@_spi(Essentials) @_spi(ProtocolProvider) @testable import SwiftNetwork +#elseif canImport(Network) +@_spi(Essentials) @_spi(ProtocolProvider) @testable import Network +#endif + +@available(Network 0.1.0, *) +let migrationTestsLogPrefixer: LogPrefixer = LogPrefixer("[MigrationTests]") + +@available(Network 0.1.0, *) +final class MigrationTests: XCTestCase { + var connection = QUICConnection(context: .implicitContext) + + static let oldCID = QUICConnectionID([0xA1, 0xA2, 0xA3, 0xA4])! + static let newCID = QUICConnectionID([0xB1, 0xB2, 0xB3, 0xB4])! + + override func setUp() { + let expectation = XCTestExpectation() + connection.context.async { + try? self.connection.setup(remote: nil, local: nil, parameters: nil, path: nil) + self.connection.recovery = Recovery(logPrefixer: migrationTestsLogPrefixer) + self.connection.recovery.connection = self.connection + expectation.fulfill() + } + wait(for: [expectation], timeout: 5.0) + } + + override func tearDown() { + self.connection.currentPath = nil + } + + // Builds a path that is open for sending, backed by a lower harness, with its DCID + // registered in `remoteCIDs` so it can be retired. `validated` drives it to the + // validated state so `migrate(to:)` will accept it. + private func makePath(dcid: QUICConnectionID, sequenceNumber: UInt64, validated: Bool) -> QUICPath { + let lower = DatagramLowerHarness(identifier: "\(sequenceNumber)", context: .implicitContext) + lower.connect() + var path = QUICPath(parent: connection) + path.set(interface: nil, priority: 1, isInitial: true) // -> .routeEstablished + path.assignDCID(dcid) // -> .cidAssigned (open for sending) + if validated { + path.changeState(to: .probing) + path.changeState(to: .validated) + } + try? path.attachLowerProtocol(lower.reference, remote: nil, local: nil, parameters: nil, path: nil) + try? connection.remoteCIDs.insert( + sequenceNumber: sequenceNumber, + connectionID: dcid, + token: QUICStatelessResetToken(Array(repeating: UInt8(sequenceNumber & 0xff), count: 16))! + ) + return path + } + + func testMigrationRemovesOldPathAndRetiresItsCID() { + let expectation = XCTestExpectation() + connection.context.async { + let oldPath = self.makePath(dcid: Self.oldCID, sequenceNumber: 1, validated: false) + let newPath = self.makePath(dcid: Self.newCID, sequenceNumber: 2, validated: true) + + self.connection.currentPath = oldPath + self.connection.multiplexingPaths[oldPath.identifier] = oldPath + self.connection.multiplexingPaths[newPath.identifier] = newPath + let oldPathID = oldPath.identifier + + self.connection.migration.migrate(to: newPath, connection: self.connection) + + // The path we migrated away from is dropped from the connection and its + // remote CID is retired. + XCTAssertNil(self.connection.multiplexingPaths[oldPathID], "old path should be removed from multiplexingPaths") + XCTAssertNil( + self.connection.remoteCIDs.retire(connectionID: Self.oldCID), + "old path's remote CID should be retired by migration" + ) + + // Only the new path remains, and it is now the current path. + XCTAssertEqual(self.connection.multiplexingPaths.count, 1, "old path should not linger in multiplexingPaths") + XCTAssertEqual(self.connection.currentPath?.identifier, newPath.identifier, "current path should be the new path") + + expectation.fulfill() + } + wait(for: [expectation], timeout: 5.0) + } +} + +#endif diff --git a/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift b/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift index d367726..ae0ad02 100644 --- a/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift +++ b/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift @@ -452,6 +452,16 @@ final class SwiftNetworkQUICStackTests: NetTestCase { } } + // Currently we only keep the current path object and remove all the + // other path objects we are migrating away from, so the number of + // multiplexing paths should be one. + if case .quic(let clientConnection) = clientQUICReference.reference { + XCTAssertEqual( + clientConnection.multiplexingPaths.count, 1, + "Migrated-away path leaked into multiplexingPaths after migrating to path \(pathIndex)" + ) + } + if let dataToSend { Logger.test.info("Writing data to send on path \(pathIndex)") _ = clientUpperHarness?.write(dataToSend) @@ -538,6 +548,17 @@ final class SwiftNetworkQUICStackTests: NetTestCase { dataToSend: Array("Hello World!".utf8) ) } + + func testQUICStackMigrationDoesNotLeakPaths() { + let ipv4Client = Endpoint(address: IPv4Address(SwiftNetworkQUICStackTests.localIPv4Address)!, port: 1234) + let ipv4Server = Endpoint(address: IPv4Address(SwiftNetworkQUICStackTests.localIPv4Address)!, port: 8080) + quicStackHandshake( + clientEndpoint: ipv4Client, + serverEndpoint: ipv4Server, + migrateCount: 4, + dataToSend: Array("Hello World!".utf8) + ) + } } #endif #endif From fd8449354c1c2d000562ce0fcdc5c6b185667aa7 Mon Sep 17 00:00:00 2001 From: Kriti Kukreja Date: Mon, 20 Jul 2026 11:23:35 -0700 Subject: [PATCH 2/2] Formatting and style --- Tests/QUICTests/MigrationTests.swift | 19 +++++++++++++++---- .../SwiftNetworkQUICStackTests.swift | 5 +++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/Tests/QUICTests/MigrationTests.swift b/Tests/QUICTests/MigrationTests.swift index 9179ef9..73b7025 100644 --- a/Tests/QUICTests/MigrationTests.swift +++ b/Tests/QUICTests/MigrationTests.swift @@ -84,15 +84,26 @@ final class MigrationTests: XCTestCase { // The path we migrated away from is dropped from the connection and its // remote CID is retired. - XCTAssertNil(self.connection.multiplexingPaths[oldPathID], "old path should be removed from multiplexingPaths") + XCTAssertNil( + self.connection.multiplexingPaths[oldPathID], + "Old path not removed" + ) XCTAssertNil( self.connection.remoteCIDs.retire(connectionID: Self.oldCID), - "old path's remote CID should be retired by migration" + "Old path CID not retired" ) // Only the new path remains, and it is now the current path. - XCTAssertEqual(self.connection.multiplexingPaths.count, 1, "old path should not linger in multiplexingPaths") - XCTAssertEqual(self.connection.currentPath?.identifier, newPath.identifier, "current path should be the new path") + XCTAssertEqual( + self.connection.multiplexingPaths.count, + 1, + "Unexpected path count" + ) + XCTAssertEqual( + self.connection.currentPath?.identifier, + newPath.identifier, + "Current path not switched" + ) expectation.fulfill() } diff --git a/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift b/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift index ae0ad02..62a6a26 100644 --- a/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift +++ b/Tests/SwiftNetworkTests/SwiftNetworkQUICStackTests.swift @@ -457,8 +457,9 @@ final class SwiftNetworkQUICStackTests: NetTestCase { // multiplexing paths should be one. if case .quic(let clientConnection) = clientQUICReference.reference { XCTAssertEqual( - clientConnection.multiplexingPaths.count, 1, - "Migrated-away path leaked into multiplexingPaths after migrating to path \(pathIndex)" + clientConnection.multiplexingPaths.count, + 1, + "Path leaked after migration \(pathIndex)" ) }