Skip to content

Commit c866d86

Browse files
kp-diego-trevisankp-diego-trevisan
committed
KM-17276: Stabilize VPN reconnect fallback
1 parent 0b04d3c commit c866d86

8 files changed

Lines changed: 584 additions & 221 deletions

File tree

‎LocalPackages/PIALibrary/Sources/PIALibrary/Client+Configuration.swift‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,12 +110,19 @@ extension Client {
110110
/// Sets the maximum number of failed connectivity checks before giving up.
111111
public var connectivityMaxAttempts: Int
112112

113-
/// Sets the delay after which to retry VPN connectivity checks.
113+
/// Sets the initial connection-attempt budget. Later retries back off from this value.
114114
public var vpnConnectivityRetryDelay: TimeInterval
115115

116116
/// Sets the maximum number of failed VPN connectivity attempts before giving up.
117117
public var vpnConnectivityMaxAttempts: Int
118118

119+
/// The maximum delay between VPN reconnection attempts when backing off.
120+
public var vpnConnectivityMaximumRetryDelay: TimeInterval
121+
122+
/// The maximum time to wait for a clean `.disconnected` state before
123+
/// (re)starting a tunnel. Guards against hanging if the status never arrives.
124+
public var vpnDisconnectWaitTimeout: TimeInterval
125+
119126
/// Sets the rsa certificate to use for pinning puposes.
120127
public var rsa4096Certificate: String?
121128

@@ -208,8 +215,10 @@ extension Client {
208215
connectivityRetryDelay = 10000
209216
connectivityMaxAttempts = 3
210217

211-
vpnConnectivityRetryDelay = 5.0
218+
vpnConnectivityRetryDelay = 20.0
212219
vpnConnectivityMaxAttempts = 3
220+
vpnConnectivityMaximumRetryDelay = 60.0
221+
vpnDisconnectWaitTimeout = 10.0
213222

214223
rsa4096Certificate = nil
215224

‎LocalPackages/PIALibrary/Sources/PIALibrary/ClientError.swift‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,9 @@ public enum ClientError: Error, Equatable {
5555
/// VPN client configuration could not be built (e.g. missing password reference).
5656
case vpnClientConfigurationUnavailable
5757

58+
/// The previous VPN session did not finish disconnecting before the restart deadline.
59+
case vpnDisconnectTimedOut
60+
5861
/// Error while checking the dip token renewal.
5962
case dipTokenRenewalError
6063

‎LocalPackages/PIALibrary/Sources/PIALibrary/Daemons/VPNDaemon.swift‎

Lines changed: 265 additions & 80 deletions
Large diffs are not rendered by default.

‎LocalPackages/PIALibrary/Sources/PIALibrary/VPN/IKEv2Profile.swift‎

Lines changed: 38 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ private let log = PIALogger.logger(for: IKEv2Profile.self)
2828
/// Implementation of `VPNProfile` providing IKEv2 connectivity.
2929
public final class IKEv2Profile: NetworkExtensionProfile {
3030

31-
private var waitObserver: NSObjectProtocol?
31+
private let restartCoordinator = TunnelRestartCoordinator()
3232

3333
private var currentVPN: NEVPNManager {
3434
return NEVPNManager.shared()
@@ -83,59 +83,52 @@ public final class IKEv2Profile: NetworkExtensionProfile {
8383
let currentStatus = self.currentVPN.connection.status
8484
log.debug("[IKEv2] connect — current status: \(currentStatus.descriptionForLog)")
8585

86-
// If the tunnel is already active, stop it before starting the new one.
87-
// Calling startVPNTunnel() on a connected IKEv2 tunnel may silently retain
88-
// the existing connection rather than switching to the new server, resulting
89-
// in the app believing it is connected when it is not.
90-
if currentStatus == .connected || currentStatus == .connecting || currentStatus == .reasserting {
91-
log.debug("[IKEv2] connect — stopping active tunnel before restart")
92-
self.currentVPN.connection.stopVPNTunnel()
93-
}
94-
95-
if currentStatus == .disconnecting {
96-
log.debug("[IKEv2] connect — waiting for .disconnected before start")
97-
self.waitForDisconnectedThenStart(callback: callback)
86+
// Only start immediately from a fully settled state. From any other state we
87+
// stop the current tunnel and wait for a clean .disconnected before starting.
88+
// Calling startVPNTunnel() on a tunnel that is still active or tearing down may
89+
// silently retain the existing connection (wrong server) or fail the new
90+
// attempt — the latter being the source of the reconnect storm when the
91+
// fallback timer forces reconnects.
92+
if TunnelRestartPolicy.canStartTunnel(from: currentStatus) {
93+
self.startTunnel(context: "connect", callback: callback)
9894
} else {
99-
do {
100-
try self.currentVPN.connection.startVPNTunnel()
101-
log.debug("[IKEv2] connect — startVPNTunnel issued")
102-
callback?(nil)
103-
} catch let e {
104-
log.error("[IKEv2] connect — startVPNTunnel threw: \(e)")
105-
callback?(e)
95+
if currentStatus != .disconnecting {
96+
log.debug("[IKEv2] connect — stopping active tunnel before restart")
97+
self.currentVPN.connection.stopVPNTunnel()
10698
}
99+
log.debug("[IKEv2] connect — waiting for .disconnected before start")
100+
self.waitForDisconnectedThenStart(callback: callback)
107101
}
108102
}
109103
}
110104

111-
private func waitForDisconnectedThenStart(callback: SuccessLibraryCallback?) {
112-
if let existing = waitObserver {
113-
NotificationCenter.default.removeObserver(existing)
114-
waitObserver = nil
105+
private func startTunnel(context: String, callback: SuccessLibraryCallback?) {
106+
do {
107+
try currentVPN.connection.startVPNTunnel()
108+
log.debug("[IKEv2] \(context) — startVPNTunnel issued")
109+
callback?(nil)
110+
} catch let e {
111+
log.error("[IKEv2] \(context) — startVPNTunnel threw: \(e)")
112+
callback?(e)
115113
}
114+
}
116115

117-
var token: NSObjectProtocol?
118-
token = NotificationCenter.default.addObserver(forName: .NEVPNStatusDidChange, object: currentVPN.connection, queue: .main) { [weak self, currentVPN] _ in
119-
guard currentVPN.connection.status == .disconnected else {
120-
return
121-
}
122-
123-
defer {
124-
token.map { NotificationCenter.default.removeObserver($0) }
125-
self?.waitObserver = nil
126-
}
127-
128-
log.debug("[IKEv2] waitForDisconnectedThenStart — disconnected, starting")
129-
do {
130-
try currentVPN.connection.startVPNTunnel()
131-
log.debug("[IKEv2] waitForDisconnectedThenStart — startVPNTunnel issued")
132-
callback?(nil)
133-
} catch let e {
134-
log.error("[IKEv2] waitForDisconnectedThenStart — startVPNTunnel threw: \(e)")
135-
callback?(e)
116+
/// Waits for the previous session to settle before starting the replacement tunnel.
117+
/// A timeout completes with an error instead of starting against a session that is
118+
/// still tearing down.
119+
private func waitForDisconnectedThenStart(callback: SuccessLibraryCallback?) {
120+
restartCoordinator.wait(
121+
for: currentVPN.connection,
122+
timeout: Client.configuration.vpnDisconnectWaitTimeout,
123+
onReady: { [weak self] in
124+
log.debug("[IKEv2] waitForDisconnectedThenStart — disconnected, starting")
125+
self?.startTunnel(context: "waitForDisconnectedThenStart", callback: callback)
126+
},
127+
onTimeout: {
128+
log.error("[IKEv2] waitForDisconnectedThenStart — timed out while tunnel was still disconnecting")
129+
callback?(ClientError.vpnDisconnectTimedOut)
136130
}
137-
}
138-
waitObserver = token
131+
)
139132
}
140133

141134
/// :nodoc:

‎LocalPackages/PIALibrary/Sources/PIALibrary/VPN/NetworkExtensionProfile.swift‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,95 @@ import NetworkExtension
2525

2626
private let log = PIALogger.logger(for: NetworkExtensionProfile.self)
2727

28+
enum TunnelRestartPolicy {
29+
static func canStartTunnel(from status: NEVPNStatus) -> Bool {
30+
status == .disconnected || status == .invalid
31+
}
32+
}
33+
34+
/// Serializes the stop/wait/start handoff for a tunnel profile. All observer and timeout
35+
/// state is confined to the main queue, and a generation prevents a superseded wait from
36+
/// starting an older configuration.
37+
final class TunnelRestartCoordinator {
38+
private var generation: UInt = 0
39+
private var observer: NSObjectProtocol?
40+
private var timeoutItem: DispatchWorkItem?
41+
42+
func wait(
43+
for connection: NEVPNConnection,
44+
timeout: TimeInterval,
45+
onReady: @escaping () -> Void,
46+
onTimeout: @escaping () -> Void
47+
) {
48+
DispatchQueue.main.async { [weak self] in
49+
self?.beginWait(
50+
for: connection,
51+
timeout: timeout,
52+
onReady: onReady,
53+
onTimeout: onTimeout
54+
)
55+
}
56+
}
57+
58+
private func beginWait(
59+
for connection: NEVPNConnection,
60+
timeout: TimeInterval,
61+
onReady: @escaping () -> Void,
62+
onTimeout: @escaping () -> Void
63+
) {
64+
cancelPendingWait()
65+
generation &+= 1
66+
let waitGeneration = generation
67+
68+
guard TunnelRestartPolicy.canStartTunnel(from: connection.status) == false else {
69+
onReady()
70+
return
71+
}
72+
73+
let finish: (Bool) -> Void = { [weak self, connection] didTimeOut in
74+
guard let self, generation == waitGeneration else { return }
75+
cancelPendingWait()
76+
77+
if TunnelRestartPolicy.canStartTunnel(from: connection.status) {
78+
onReady()
79+
} else if didTimeOut {
80+
onTimeout()
81+
}
82+
}
83+
84+
observer = NotificationCenter.default.addObserver(
85+
forName: .NEVPNStatusDidChange,
86+
object: connection,
87+
queue: .main
88+
) { [connection] _ in
89+
guard TunnelRestartPolicy.canStartTunnel(from: connection.status) else {
90+
return
91+
}
92+
finish(false)
93+
}
94+
95+
let item = DispatchWorkItem { finish(true) }
96+
timeoutItem = item
97+
DispatchQueue.main.asyncAfter(deadline: .now() + timeout, execute: item)
98+
}
99+
100+
private func cancelPendingWait() {
101+
if let observer {
102+
NotificationCenter.default.removeObserver(observer)
103+
self.observer = nil
104+
}
105+
timeoutItem?.cancel()
106+
timeoutItem = nil
107+
}
108+
109+
deinit {
110+
if let observer {
111+
NotificationCenter.default.removeObserver(observer)
112+
}
113+
timeoutItem?.cancel()
114+
}
115+
}
116+
28117
/// Specific protocol bridging a `VPNProfile` to a native `NEVPNProtocol` from Apple's NetworkExtension framwork.
29118
public protocol NetworkExtensionProfile: VPNProfile {
30119

‎LocalPackages/PIALibrary/Sources/PIALibrary/VPN/PIATunnelProfile.swift‎

Lines changed: 42 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
/// Implementation of `VPNProfile` providing OpenVPN connectivity.
3030
public final class PIATunnelProfile: NetworkExtensionProfile {
3131
private let bundleIdentifier: String
32-
private var waitObserver: NSObjectProtocol?
32+
private let restartCoordinator = TunnelRestartCoordinator()
3333

3434
/**
3535
Default initializer.
@@ -88,62 +88,57 @@
8888
let currentStatus = vpn.connection.status
8989
log.debug("[OpenVPN] connect — current status: \(currentStatus.descriptionForLog)")
9090

91-
// If the tunnel is already active, stop it before starting the new one.
92-
// Calling startTunnel() on a live session may silently retain the existing
93-
// connection rather than switching to the new server, leaving the app in a
94-
// state where it believes it is connected when it is not.
95-
if currentStatus == .connected || currentStatus == .connecting || currentStatus == .reasserting {
96-
log.debug("[OpenVPN] connect — stopping active tunnel before restart")
97-
vpn.connection.stopVPNTunnel()
98-
}
99-
100-
if currentStatus == .disconnecting {
101-
log.debug("[OpenVPN] connect — waiting for .disconnected before start")
102-
self.waitForDisconnectedThenStart(vpn: vpn, callback: callback)
91+
// Only start immediately from a fully settled state. From any other
92+
// state we stop the current session and wait for a clean .disconnected
93+
// before starting. Calling startTunnel() on a session that is still
94+
// active or tearing down may silently retain the existing connection
95+
// (wrong server) or fail the new attempt — the latter being the source
96+
// of the reconnect storm when the fallback timer forces reconnects.
97+
if TunnelRestartPolicy.canStartTunnel(from: currentStatus) {
98+
self.startTunnel(vpn: vpn, context: "connect", callback: callback)
10399
} else {
104-
do {
105-
let session = vpn.connection as? NETunnelProviderSession
106-
try session?.startTunnel(options: nil)
107-
log.debug("[OpenVPN] connect — startTunnel issued")
108-
callback?(nil)
109-
} catch let e {
110-
log.error("[OpenVPN] connect — startTunnel threw: \(e)")
111-
callback?(e)
100+
if currentStatus != .disconnecting {
101+
log.debug("[OpenVPN] connect — stopping active tunnel before restart")
102+
vpn.connection.stopVPNTunnel()
112103
}
104+
log.debug("[OpenVPN] connect — waiting for .disconnected before start")
105+
self.waitForDisconnectedThenStart(vpn: vpn, callback: callback)
113106
}
114107
}
115108
}
116109
}
117110

118-
private func waitForDisconnectedThenStart(vpn: NETunnelProviderManager, callback: SuccessLibraryCallback?) {
119-
if let existing = waitObserver {
120-
NotificationCenter.default.removeObserver(existing)
121-
waitObserver = nil
111+
private func startTunnel(vpn: NETunnelProviderManager, context: String, callback: SuccessLibraryCallback?) {
112+
guard let session = vpn.connection as? NETunnelProviderSession else {
113+
callback?(ClientError.vpnProfileUnavailable)
114+
return
122115
}
116+
do {
117+
try session.startTunnel(options: nil)
118+
log.debug("[OpenVPN] \(context) — startTunnel issued")
119+
callback?(nil)
120+
} catch let e {
121+
log.error("[OpenVPN] \(context) — startTunnel threw: \(e)")
122+
callback?(e)
123+
}
124+
}
123125

124-
var token: NSObjectProtocol?
125-
token = NotificationCenter.default.addObserver(forName: .NEVPNStatusDidChange, object: vpn.connection, queue: .main) { [weak self, vpn] _ in
126-
guard vpn.connection.status == .disconnected else {
127-
return
128-
}
129-
130-
defer {
131-
token.map { NotificationCenter.default.removeObserver($0) }
132-
self?.waitObserver = nil
133-
}
134-
135-
log.debug("[OpenVPN] waitForDisconnectedThenStart — disconnected, starting")
136-
do {
137-
let session = vpn.connection as? NETunnelProviderSession
138-
try session?.startTunnel(options: nil)
139-
log.debug("[OpenVPN] waitForDisconnectedThenStart — startTunnel issued")
140-
callback?(nil)
141-
} catch let e {
142-
log.error("[OpenVPN] waitForDisconnectedThenStart — startTunnel threw: \(e)")
143-
callback?(e)
126+
/// Waits for the previous session to settle before starting the replacement tunnel.
127+
/// A timeout completes with an error instead of starting against a session that is
128+
/// still tearing down.
129+
private func waitForDisconnectedThenStart(vpn: NETunnelProviderManager, callback: SuccessLibraryCallback?) {
130+
restartCoordinator.wait(
131+
for: vpn.connection,
132+
timeout: Client.configuration.vpnDisconnectWaitTimeout,
133+
onReady: { [weak self, vpn] in
134+
log.debug("[OpenVPN] waitForDisconnectedThenStart — disconnected, starting")
135+
self?.startTunnel(vpn: vpn, context: "waitForDisconnectedThenStart", callback: callback)
136+
},
137+
onTimeout: {
138+
log.error("[OpenVPN] waitForDisconnectedThenStart — timed out while tunnel was still disconnecting")
139+
callback?(ClientError.vpnDisconnectTimedOut)
144140
}
145-
}
146-
waitObserver = token
141+
)
147142
}
148143

149144
/// :nodoc:

0 commit comments

Comments
 (0)