KM-16992: make sure to disconnect when user taps disconnect

[kp-mario-nachbaur]

• disconnect() in all three tunnel profiles now stops the tunnel immediately, not only after saveToPreferences succeeds, and stops it again afterward (on-demand rules can resurrect it in between). Also clears a stale pending connect-wait observer and syncs UI status when the tunnel was already dead.
• DefaultVPNProvider.disconnect() no longer blocks on fetching the tunnel log before disconnecting (it's now fire-and-forget, since a wedged tunnel may never reply).
• VPNDaemon and DefaultVPNProvider's reconnect/retry logic now check disconnectedManually so they never auto-reconnect after the user has explicitly disconnected — including mid-flight reconnects and the fallback retry timer.
• connect() clears disconnectedManually since a new connection attempt supersedes the prior manual-disconnect intent.

[claude]

Claude finished @kp-mario-nachbaur's task in 2m 39s —— View job

---

PR Review: KM-16992 — make sure to disconnect when user taps disconnect

• Read the full diff against origin/master
• Analyze disconnect logic in the three tunnel profiles
• Analyze DefaultVPNProvider and VPNDaemon reconnect changes
• Check for bugs, threading/race issues, retain cycles
• Assess test coverage & impacted flows

---

Overall this is a solid, well-reasoned fix. The inline comments explaining why each stopVPNTunnel() call exists (on-demand resurrection, wedged tunnel, stale UI) are excellent and make the tricky NE lifecycle much easier to follow. The disconnectedManually gating across the daemon + provider closes a real class of "user tapped disconnect but we reconnected anyway" bugs. Comments below, ordered by importance.

🐛 Correctness / behavior

1. Direct UI status write bypasses the daemon's isReconnecting suppression
IKEv2Profile.swift:168-173, PIATunnelProfile.swift:176-181, PIAWGTunnelProfile.swift:246-251

The new "tunnel already dead → sync UI" block writes Client.database.transient.vpnStatus = .disconnected directly. VPNDaemon deliberately gates that same write behind if !isReconnecting (VPNDaemon.swift:225) precisely to avoid flashing .disconnected between two .connecting states during a reconnect. disconnect() is also invoked from reconnect(forceDisconnect:) (DefaultVPNProvider.swift:324) and from install()/protocol-switch (DefaultVPNProvider.swift:191) — not only from a user tap. If the tunnel happens to be .disconnected/.invalid at that moment, this bypasses the guard and can momentarily expose .disconnected to the UI mid-reconnect/mid-switch. Edge-casey (during reconnect the status is usually .connecting), but worth confirming it can't flash the dashboard during a protocol change.

Consider guarding the direct write on !Client.configuration.disconnectedManually (or only doing it when we know the caller is a genuine user disconnect), so automated reconnect/switch paths don't touch transient.vpnStatus directly.

2. Error paths stop the tunnel but never disable on-demand
IKEv2Profile.swift:151-158, PIATunnelProfile.swift:160-166, PIAWGTunnelProfile.swift:230-236

When loadFromPreferences/find fails, the code calls stopVPNTunnel() but skips isOnDemandEnabled = false + save. If an on-demand rule is active, it can immediately resurrect the tunnel — the exact scenario the happy path's "stop again after save" defends against. Given prefs failed to load there may be little you can do, but it's asymmetric with the main path and worth a comment noting the residual risk (or a best-effort second stop).

🧵 Threading / races

3. waitObserver accessed without a defined thread contract
IKEv2Profile.swift:146-149 (and the two tunnel profiles)

waitObserver is read/cleared at the top of disconnect() and also mutated inside the .NEVPNStatusDidChange observer (queued on .main) and in waitForDisconnectedThenStart. If disconnect() is ever called off the main thread while the observer fires on main, this is an unsynchronized access to a shared NSObjectProtocol?. In practice these NE callbacks are main-queue, so likely fine today — but it's fragile. Confirm disconnect() is only ever called on main, or hop to main before touching waitObserver.

4. disconnectedManually is a plain Bool read/written from several call sites
Client+Configuration.swift:43, read in VPNDaemon (main), the fallback timer (main), and provider callbacks. Mostly main-thread, so probably safe, but there's no synchronization if any provider callback resolves off-main. Capturing it once at the top of tryUpdateStatus (VPNDaemon.swift:99) is a nice touch that avoids the reset-mid-switch problem.

🧹 Maintainability

5. disconnect() is now duplicated ~40 lines × 3 profiles
IKEv2Profile.swift:142, PIATunnelProfile.swift:150, PIAWGTunnelProfile.swift:220

The three implementations are nearly identical (waitObserver teardown, stop-before-save, UI sync, on-demand disable, stop-after-save). This is easy to drift out of sync — a future fix to one will likely miss the others. Consider extracting the shared body into a NetworkExtensionProfile protocol extension helper (e.g. performDisconnect(on connection:manager:callback:)), leaving each profile to supply its manager/connection. Fix this →

6. vpnLog += on an empty log
DefaultVPNProvider.swift:285 — when vpnLog starts empty, the result is a leading "\n\n<content>". Cosmetic. Also note the log capture is now fire-and-forget, so a debug report submitted immediately after disconnect may not yet include the tunnel's final log lines — an acceptable trade-off given a wedged tunnel may never reply, but callers relying on vpnLog right after disconnect() should be aware.

✅ Tests

No tests were added/changed (VPNTests.swift untouched). This change is squarely in the kind of state-machine logic that regresses silently. Worth adding coverage for:

• connect() clears disconnectedManually (DefaultVPNProvider.swift:261).
• reconnect() aborts when disconnectedManually is set, on both the disconnect-first and updatePreferences branches (DefaultVPNProvider.swift:331,350).
• VPNDaemon fallback timer stops and does not reconnect when disconnectedManually is true (VPNDaemon.swift:309).
• Daemon ignores the last-disconnect-error reconnect when disconnectedManually is true (VPNDaemon.swift:262).

🔎 Impacted flows to test manually

• Tap Disconnect while connected (all 3 protocols) — must stay disconnected, no auto-reconnect.
• Tap Disconnect during a network change / while .connecting (the original bug) — tunnel must die and UI must not stay stuck on "connecting".
• Disconnect with on-demand / "connect on demand" enabled — must not be resurrected.
• Connectivity-failure reconnect and max-attempts give-up paths — confirm no unwanted UI flash to disconnected and that legitimate reconnects still work.
• Protocol switch (IKEv2 ↔ OpenVPN ↔ WireGuard) via install() which calls disconnect() — confirm no spurious .disconnected UI flash.
• Disconnect, then immediately reconnect — disconnectedManually must be cleared so the daemon's retry logic works again.
  • KM-16992-pia-blocks-connection

[kpkb-1f8e9813897fe9831983e89f7143]

KB review — 2ec42d4

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

View in KB

[claude]

Claude finished @kp-mario-nachbaur's task in 2m 14s —— View job

---

PR Review: KM-16992 — make sure to disconnect when user taps disconnect

• Read the current diff against origin/master
• Analyze disconnect logic in the three tunnel profiles
• Analyze DefaultVPNProvider / VPNDaemon changes
• Check for bugs, threading/races, retain cycles
• Assess test coverage & impacted flows

---

This synchronize was a merge of master (bringing in #361, which raises vpnConnectivityRetryDelay from 5s → 20s). The merge touched only Client+Configuration.swift; the five core PR files are byte-for-byte identical to the state I reviewed at 2ec42d4. So the substance of my earlier review stands. Below is the still-open list, plus one note on how the new 20s retry delay interacts with this change.

🔁 Interaction with the new 20s retry delay (#361, merged in)

The longer retry interval makes two parts of this PR more important, not less:

• The disconnectedManually guard on the fallback timer (VPNDaemon.swift:309) now prevents up to a 20-second window in which a resurrected reconnect could fire after the user tapped disconnect. Good that it's there.
• The "tunnel already dead → sync UI" write in each profile matters more too: without it a stale .connecting could linger ~20s before the next event. Also good.

No code change needed — just confirming the merge doesn't undermine the fix.

🐛 Still-open correctness items (from prior review, unchanged in code)

1. Direct UI status write bypasses the daemon's isReconnecting suppression — IKEv2Profile.swift:168-173, PIATunnelProfile.swift:176-181, PIAWGTunnelProfile.swift:246-251
The transient.vpnStatus = .disconnected write is unconditional (only gated on status == .disconnected/.invalid). VPNDaemon deliberately gates that same write behind if !isReconnecting (VPNDaemon.swift:225). Since disconnect() is also reached from reconnect(forceDisconnect:) and install()/protocol-switch — not just a user tap — an already-dead tunnel at that instant can flash .disconnected to the UI mid-reconnect/switch. Consider guarding the direct write on !accessedConfiguration.disconnectedManually so only genuine user disconnects touch transient.vpnStatus directly. Fix this →

2. Error paths stop the tunnel but never disable on-demand — IKEv2Profile.swift:151-158, PIATunnelProfile.swift:160-166, PIAWGTunnelProfile.swift:230-236
On loadFromPreferences/find failure the code calls stopVPNTunnel() but skips isOnDemandEnabled = false + save, so an active on-demand rule can immediately resurrect the tunnel — the exact case the happy path defends against. Given prefs failed to load there may be little to do, but it's asymmetric; at minimum add a comment on the residual risk.

🧵 Threading (low risk, worth confirming)

3. waitObserver is read/cleared at the top of disconnect() and mutated inside the .NEVPNStatusDidChange observer (main queue) and waitForDisconnectedThenStart. Fine if disconnect() is always called on main; otherwise it's an unsynchronized access. Please confirm the main-thread contract or hop to main before touching it.

4. disconnectedManually is a plain Bool read/written from several sites. The capture-once-at-top pattern in tryUpdateStatus (VPNDaemon.swift:99) is the right call and correctly avoids the reset-mid-switch problem for the last-disconnect-error branch (:262).

🧹 Maintainability

5. disconnect() is now ~40 near-identical lines × 3 profiles (IKEv2Profile.swift:142, PIATunnelProfile.swift:150, PIAWGTunnelProfile.swift:220). High drift risk — a future fix to one will likely miss the others. Consider extracting a shared NetworkExtensionProfile protocol-extension helper. Fix this →

6. Fire-and-forget log capture (DefaultVPNProvider.swift:279-289) — good defensive change (a wedged tunnel may never reply). Note the += "\n\n" prefix and that a debug report submitted immediately after disconnect may miss the tunnel's final lines. Acceptable trade-off; just flagging for anyone relying on vpnLog right after disconnect().

✅ Tests — still the biggest gap

VPNTests.swift remains untouched. This is exactly the kind of state-machine logic that regresses silently. Worth covering:

• connect() clears disconnectedManually (DefaultVPNProvider.swift:261).
• reconnect() aborts on both branches when disconnectedManually is set (:331, :350).
• Fallback timer stops and does not reconnect when disconnectedManually is true (VPNDaemon.swift:309).
• Last-disconnect-error reconnect is ignored when disconnectedManually is true, using the value captured at :99 (:262).

🔎 Manual flows to verify

• Disconnect while connected (all 3 protocols) — stays disconnected, no auto-reconnect.
• Disconnect during a network change / while .connecting (the original bug) — tunnel dies, UI doesn't stick on "connecting" (now up to a 20s window if the guard failed).
• Disconnect with on-demand enabled — not resurrected.
• Connectivity-failure reconnect and max-attempts give-up — no spurious .disconnected flash; legitimate reconnects still work.
• Protocol switch (IKEv2 ↔ OpenVPN ↔ WireGuard) via install() → disconnect() — no spurious .disconnected flash (item how to get amount of data which was uploaded and downloaded?? #1).
• Disconnect then immediately reconnect — disconnectedManually cleared so retry logic works again.

Overall: solid fix, well-commented, and the merged retry-delay bump only reinforces its value. Items #1 and the test gap are the two I'd most want addressed before merge.
• KM-16992-pia-blocks-connection