KM-17276: Stabilize VPN reconnect fallback

[kp-diego-trevisan]

Here's the full picture of everything this branch does, in plain language.

The problem it fixes

The original bug was a "reconnect storm": when connecting the VPN (especially WireGuard on slower/older devices), the app could get stuck rapidly retrying — killing connections that were just slow, hammering servers, and sometimes freezing in "Connecting…" or a stuck state. Along the way, a few related failure modes were fixed too.

What changed, area by area

1. More patient timing (Client+Configuration.swift)

• First connection attempt now gets 20 seconds instead of 5, so a slow-but-healthy connect isn't killed.
• Retries slow down instead of repeating fast: 20s → 40s → 60s (capped), max 3 attempts for a fresh connect.
• Added a 10-second limit for waiting on a clean shutdown.

2. Bounded retries instead of an endless loop (VPNDaemon.swift)

• The old "fire every 5 seconds forever" timer is gone. Each attempt now schedules a single timer with the next, longer delay.
• For a cold connect that never succeeds (dead region, unreachable endpoint), it stops cleanly after 3 attempts.

3. Never abandon a connection you actually had — the key safety behavior

• This is the important distinction. If you were Connected and the server dies while you still have internet, the app now keeps trying forever (rotating servers, with the capped backoff) rather than giving up. Giving up would disconnect you and turn off the kill switch, silently exposing you to leaks after you chose to be protected.
• It tracks this with a "you had an established connection" flag (didReachConnected) that's set once you reach Connected and cleared only when you manually disconnect or it truly gives up.
• The retry-vs-give-up decision is now in one place (VPNFallbackPolicy.decision): no internet → give up (nothing to leak, avoids the kill-switch deadlock); internet + you were connected → keep trying; internet + cold connect → bounded by the cap.

4. Clean "stop, wait, then start" handoff (NetworkExtensionProfile.swift + the WireGuard/OpenVPN/IKEv2 profiles)

• New shared helper TunnelRestartCoordinator with one rule: only start a new tunnel once the old one is fully shut down. Otherwise stop it, wait for it to settle, then start.
• If the old tunnel doesn't finish within 10 seconds, it reports an error (vpnDisconnectTimedOut, in ClientError.swift) instead of stacking a new tunnel on a dying one. Same safe logic now for all three protocols.

5. A real disconnect when giving up, with a safety net (VPNDaemon.swift)

• When it does give up, it does a genuine disconnect and disables auto-reconnect, ending in a real Disconnected state — instead of hanging forever waiting on a signal the kill switch can suppress.
• Added a watchdog so a give-up can never get permanently stuck: if the disconnect confirmation never arrives within 10s, it force-resets so the app always recovers.

6. The "stuck in Connecting forever" fix (found in device testing)

• The reconnect watchdog was being armed before the app's status was actually set to "Connecting", so it silently failed to start on the first try. When a reconnect hung, nothing rotated servers → stuck forever. Now the watchdog is armed after the status is committed, so it reliably fires and rotates.

Code-quality cleanup (no behavior change)

• The giant ~285-line status-handling method was slimmed by pulling out three focused pieces: handleDisconnectError (classifies the disconnect reason), isConnectivityCheckFailure (the WireGuard/OpenVPN/IKEv2 error-matching), and startReconnect (dedupes the two identical reconnect blocks).
• The core decisions are now small, testable units (VPNFallbackPolicy, VPNRetryDecision, TerminalDisconnectPolicy, VPNGiveUpState, TunnelRestartPolicy).
• Added // MARK: - section separators throughout for navigation.
• Unit tests (VPNFallbackPolicyTests.swift) cover the backoff math, the retry/give-up decision (including "keep trying forever while connected" and "cold connect is bounded"), the restart-safety rule, the give-up state machine, and the reconnect-teardown guard.

Bottom line

A connection attempt gets a realistic amount of time; a cold connect that can't succeed stops after 3 tries; a connection that drops while you still have internet keeps recovering indefinitely and never leaves you unprotected; server switches never stack tunnels; give-up can't hang the app; and the watchdog now reliably rotates servers so it can't sit in "Connecting…" forever.

One caveat worth remembering: during a sustained outage the retry cadence settles at ~60s between attempts (kill switch protects you throughout) — snappier than the old storm, but a genuinely-desired reconnect could take up to a minute once a good server is reachable. That's a tuning knob if you want it faster.

[claude]

Claude finished @kp-diego-trevisan's task in 1m 24s —— View job

---

Claude Code Review — KM-17276 (re-review after 1ae3b0ae)

• Read latest diff vs origin/master
• Re-check High finding: reconnect teardown vs terminal give-up
• Re-check Medium finding: give-up watchdog/timeout
• Review new changes & tests
• Post review

The new commit "Guard reconnect teardown from terminal give-up" addresses both findings from the previous review, and does so cleanly. Verified against the current file.

✅ High — resolved

The clean-disconnect and generic-error branches are now gated on !isReconnecting, factored into a pure TerminalDisconnectPolicy (VPNDaemon.swift:48-72) and wired in at VPNDaemon.swift:402-418. This matches the connectivityCheckFailed branch's guard, so the intermediate .disconnected of a forced reconnect (disconnect { connect }) is no longer misclassified as a failed connect. The 20/40/60 backoff can now reach attempts 2–3 deterministically instead of relying on the status race. The regression is locked in by reconnectTeardownDoesNotGiveUpOnCleanDisconnect / reconnectTeardownDoesNotGiveUpOnGenericError, plus positive/manual/non-connecting cases. Good.

✅ Medium — resolved

giveUp() now arms a giveUpWatchdog (VPNDaemon.swift:520, 563-574) that force-resets giveUpState/reset() after vpnDisconnectWaitTimeout if the terminal disconnect never settles. finishGivingUpIfPossible() invalidates it on the happy path (552), and the watchdog re-checks isGivingUp before acting, so a normal completion can't be clobbered. This removes the permanent-wedge path where isGivingUp would silently kill every future connect. The watchdog is armed only on the first giveUp() (the isGivingUp early-return at 509 short-circuits re-entry), which is the correct semantics.

🟢 Minor / notes (non-blocking)

• Stale disconnect completion after a watchdog reset. If the watchdog force-resets and the original disconnect completion fires afterward, its closure still runs recordDisconnectCompletion + posts .PIAVPNDidFail + finishGivingUpIfPossible(). On the already-reset state that's a harmless no-op today. The only theoretical concern is a new give-up cycle starting within the same window, where the stale callback could mark the new cycle's disconnect complete early — but that requires a full connect→fail inside vpnDisconnectWaitTimeout (10s) and only shortens, never wedges. Not worth a code change; flagging for awareness.
• No deinit for the two timers. Both fallbackTimer and giveUpWatchdog use [weak self], and VPNDaemon is a long-lived singleton, so there's no retain cycle or practical leak. Consistent with the pre-existing pattern.

Test coverage

TerminalDisconnectPolicy is now covered by six focused cases including the two regression guards. As noted last round, the extracted pure types are well-tested; the remaining gap is full VPNDaemon orchestration against a MockVPNProvider (intermediate .disconnected during isReconnecting, watchdog firing vs. a real completion). Not blocking, but a daemon-level test would be the highest-value follow-up.

On-device priorities before merge

These two fixes are best confirmed on the affected flows:

• Unreachable endpoint → back off 20→40→60 across the 3 configured attempts, then a real Disconnected (validates the High fix end-to-end).
• Tunnel stuck Disconnecting > 10s / force-quit during teardown → daemon recovers and a later manual Connect succeeds (validates the watchdog).
• WireGuard/OpenVPN/IKEv2 cold connect, connected + rapid server switches, network-loss during connecting, manual cancel mid-connecting.

Both previously-raised issues are addressed correctly — no new blocking concerns. 👍
· branch KM-17276-wireguard-connection-storm

[kpkb-1f8e9813897fe9831983e89f7143]

KB review — 8d2198d

No findings changes on this PR relative to the default branch. ✅

View in KB

[claude]

Claude finished @kp-diego-trevisan's task in 4m 16s —— View job

---

Claude Code Review — KM-17276 (re-review after 8d2198d8)

• Read latest diff vs origin/master
• Review new commit 8d2198d8 (keep-reconnecting + watchdog arming + refactor)
• Check VPNDaemon orchestration & timers
• Check profile terminal-disconnect changes
• Review test coverage
• Post review

The new commit does three things: adds a didReachConnected latch so a dropped established connection keeps reconnecting while online, fixes the fallback-watchdog arming so a hung connect is actually retried, and refactors VPNDaemon into smaller pure/typed helpers. All three are good changes. The watchdog-arming fix and the refactor are clean, and the new VPNFallbackPolicy.decision(...) is well covered by the added tests. Below is one behavioral concern plus minor notes.

🟠 Medium — didReachConnected latch can outlive the session it represents

didReachConnected (VPNDaemon.swift:158) is cleared in exactly two places: a manual disconnect (VPNDaemon.swift:349) and giveUp() (VPNDaemon.swift:621). It is intentionally not cleared in reset().

The gap is a clean, non-manual disconnect from an established session — OS reclaims the extension, the VPN config is removed/invalidated in Settings, another VPN app takes over, etc. Trace it:

• .connected → .disconnected, previousStatus == .connected, wasDisconnectedManually == false, internet reachable.
• Line 327 (previousStatus != .connecting) → invalidateTimer() + reset(), but reset() does not touch didReachConnected.
• handleDisconnectError with a clean disconnect: shouldGiveUpOnCleanDisconnect requires previousStatus == .connecting, so it's false → no giveUp(), and no connectivity-reconnect is triggered either.

Net: the daemon settles at .disconnected, unprotected, with nothing retrying, yet didReachConnected stays true. The next connection then inherits the latch:

• On the following genuine cold connect (.disconnected → .connecting, !isReconnecting, VPNDaemon.swift:273), numberOfAttempts is reset but didReachConnected is not.
• If that fresh connect keeps hitting connectivity-check failures against an unreachable endpoint, decision(hasEstablishedConnection: true) returns .reconnect forever (60s-capped) instead of the bounded 3-attempt → terminal-disconnect path the PR promises for a cold connect.

The release-blocking "unreachable endpoint → bounded retries + terminal cleanup" check still passes on a first-boot cold connect (latch is false), so this is not a first-order regression — but the guarantee is silently lost for a connect that follows a prior established-then-clean-dropped session.

Suggested fix: clear the latch at the start of a genuine fresh connect, in the branch already guarded by !isReconnecting so it can't clobber a recovery reconnect:

if accessedDatabase.transient.vpnStatus == .disconnected, !isReconnecting {
    didReachConnected = false          // fresh cold connect starts un-latched
    if numberOfAttempts > 0 {
        numberOfAttempts = 0
        updateUIWithAttemptNumber(0)
    }
}

During a recovery reconnect the new .connecting arrives with isReconnecting == true, so this branch is skipped and the latch is preserved across the teardown — exactly the intended "keep reconnecting the dropped established connection" semantics. Fix this →

🟢 Minor / notes (non-blocking)

• Watchdog arming fix is correct. Moving scheduleFallbackTimerIfNeeded() to after the transient.vpnStatus commit and gating it on nextStatus == .connecting (VPNDaemon.swift:375) resolves the silent no-op the old placement had. The single-shot-plus-reschedule-on-reconnect design means the nextStatus != previousStatus early-return at line 360 (e.g. IKEv2 .connecting ⇄ .reasserting) doesn't lose the timer — it's re-armed from the startReconnect callback. Good.
• TunnelRestartCoordinator looks solid. Generation counter + main-queue confinement + timeout DispatchWorkItem correctly prevent a superseded wait from starting a stale config, and cancelPendingWait()/deinit remove the observer. One theoretical edge: if the coordinator (or owning profile) is deallocated mid-wait, finish's guard let self short-circuits and neither onReady nor onTimeout fires, so the reconnect callback never completes. The profile is long-lived (transient.activeVPNProfile), so it's not a practical leak — flagging for awareness only.
• Unbounded numberOfAttempts growth while latched. In the keep-reconnecting-forever path numberOfAttempts increments indefinitely; delay() stays safe because min(20 * pow(2, n), 60) saturates to maximumDelay (and pow → .infinity still clamps). No overflow/crash — just noting the counter grows without bound during a sustained established-connection outage.
• Refactor quality. Extracting handleDisconnectError / isConnectivityCheckFailure / startReconnect and the VPNRetryDecision enum makes the state machine much easier to follow, and the guard-based early returns read cleanly.

Test coverage

VPNFallbackPolicy.decision(...) is now covered for offline-gives-up, established-never-gives-up, and cold-connect-bounded. As with prior rounds, the remaining gap is daemon-level orchestration against a MockVPNProvider — in particular the didReachConnected lifecycle (set on .connected, preserved across recovery reconnects, and — if the fix above lands — cleared on a fresh connect). That's the highest-value follow-up test.

On-device priorities before merge

• Establish a connection, then drop it non-manually (kill the extension / toggle airplane briefly / remove-readd network) and confirm recovery reconnect behaves, then manually reconnect to an unreachable endpoint and verify it still terminates in a real Disconnected state (validates the latch concern above).
• Unreachable endpoint on a first-boot cold connect → 20→40→60 backoff, 3 attempts, terminal Disconnected.
• Tunnel stuck Disconnecting > 10s / force-quit during teardown → give-up watchdog recovers and a later manual Connect succeeds.
• WireGuard/OpenVPN/IKEv2 cold connect, connected + rapid server switches, network-loss during connecting, manual cancel mid-connecting.

No blocking bugs found in the new commit; the Medium above is a behavioral edge worth confirming on-device (and ideally the one-line guard). 👍
· branch KM-17276-wireguard-connection-storm