From 69998542d519740b78f5e6b2540d25f31caf2fd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Raphae=CC=88l=20Enrici?= Date: Fri, 30 Oct 2026 14:57:49 +0100 Subject: [PATCH] Reconnect the VPNs that were up after an update The installer stops the service, which closes every tunnel, and nothing brought them back. The app now records which VPNs are up and how they were connected (ids and options only, never a secret) and, at the first launch of a new version shortly after, connects them again through the usual path. The service tells its watchers it is stopping (new `stopping` event and feature), so the sessions it closes then are not taken for disconnections by their users. Sparkle's willInstallUpdate does the same before the app is quit for the installer, and keeps that quit from disconnecting. Settings > General: "Reconnect VPNs after an update", on by default. Adds the app's first unit tests (swift test, also in CI). Refs #49 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 1 + apple/Omnitun/App.swift | 26 ++++++ apple/Omnitun/Model.swift | 86 ++++++++++++++++++++ apple/Omnitun/SettingsView.swift | 3 + apple/Omnitun/Updates.swift | 11 +++ apple/Package.swift | 1 + apple/README.md | 9 ++ apple/Resources/fr.lproj/Localizable.strings | 2 + apple/Tests/ReconnectTests.swift | 43 ++++++++++ crates/daemon/src/main.rs | 3 +- crates/daemon/src/manager.rs | 29 ++++++- crates/vpn-client/src/session.rs | 6 +- crates/vpn-core/src/ipc.rs | 3 + crates/vpn-ffi/src/lib.rs | 7 ++ 14 files changed, 224 insertions(+), 6 deletions(-) create mode 100644 apple/Tests/ReconnectTests.swift diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e024b95..6562c98 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,5 +72,6 @@ jobs: - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2 - run: apple/build-bindings.sh - run: swift build --package-path apple --product Omnitun + - run: swift test --package-path apple # The bindings, without the app or a running service. - run: swift run --package-path apple Smoke diff --git a/apple/Omnitun/App.swift b/apple/Omnitun/App.swift index fa11487..0c5b8ee 100644 --- a/apple/Omnitun/App.swift +++ b/apple/Omnitun/App.swift @@ -81,6 +81,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate { } statusMenu = StatusMenu(model: model) model.start() + Updates.shared.willInstall = { [weak model] in model?.holdRecord() } Updates.shared.start() for url in pendingFiles { open(url, model: model) } pendingFiles = [] @@ -131,6 +132,17 @@ enum Settings { set { defaults.set(newValue, forKey: "disconnectOnQuit") } } + static var reconnectAfterUpdate: Bool { + get { defaults.object(forKey: "reconnectAfterUpdate") as? Bool ?? true } + set { defaults.set(newValue, forKey: "reconnectAfterUpdate") } + } + + /// The VPNs up at the last look, to connect again after an update. + static var upSessions: UpSessions? { + get { defaults.data(forKey: "upSessions").flatMap { try? JSONDecoder().decode(UpSessions.self, from: $0) } } + set { defaults.set(newValue.flatMap { try? JSONEncoder().encode($0) }, forKey: "upSessions") } + } + static var notifyConnected: Bool { get { defaults.bool(forKey: "notifyConnected") } set { defaults.set(newValue, forKey: "notifyConnected") } @@ -185,6 +197,20 @@ enum Settings { @MainActor static var relaunching = false } +/// The VPNs that were up, how they were connected, and which version of +/// the app saw them up and when: only ids and options, never a secret. +struct UpSessions: Codable, Equatable { + struct Entry: Codable, Equatable { + var id: String + var direct: Bool + var split: Bool + } + + var version: String + var at: Date + var sessions: [Entry] +} + /// The routing a gateway announced, and when. struct GatewayAnnouncement: Codable, Equatable { var fullTunnel: Bool diff --git a/apple/Omnitun/Model.swift b/apple/Omnitun/Model.swift index 39af478..3bf4f09 100644 --- a/apple/Omnitun/Model.swift +++ b/apple/Omnitun/Model.swift @@ -35,6 +35,13 @@ final class AppModel: ObservableObject { func secretsDidChange() { secretsChanged += 1 } + /// The VPNs to connect again once the service answers: those up when + /// the previous version stopped for an update. + private var resuming: [UpSessions.Entry]? + /// The service is stopping or the app is being updated: the sessions + /// closing now are to come back, so what is up is no longer recorded. + private var stopping = false + private var subscription: VPNClientCore.Subscription? private var watchRetry: Task? @@ -54,6 +61,14 @@ final class AppModel: ObservableObject { func start() { Notifier.shared.setUp() reloadProfiles() + resuming = Self.toResume(Settings.upSessions, version: Self.version, now: Date()) + if resuming != nil { + // A service that never answers: nothing comes back later. + Task { + try? await Task.sleep(for: .seconds(120)) + resuming = nil + } + } watch() } @@ -121,6 +136,8 @@ final class AppModel: ObservableObject { private func watch() { watchRetry?.cancel() + // A new service: what it reports is recorded again. + stopping = false subscription = client.watch(observer: Observer(model: self)) refreshService() } @@ -145,6 +162,11 @@ final class AppModel: ObservableObject { retries[id] = nil } if case .running = service {} else { refreshService() } + if let entries = resuming { + resuming = nil + resume(entries) + } + recordUp() } fileprivate func changed(_ change: StateChange) { @@ -185,6 +207,66 @@ final class AppModel: ObservableObject { } } + // MARK: Reconnecting after an update + + private static let version = Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String + + /// What to connect again at launch: the VPNs a previous version saw up + /// shortly before it stopped. A crash or a quit without an update + /// connects nothing. + nonisolated static func toResume(_ up: UpSessions?, version: String?, now: Date) -> [UpSessions.Entry]? { + guard let up, let version, up.version != version, !up.sessions.isEmpty, + now.timeIntervalSince(up.at) < 15 * 60, now >= up.at else { return nil } + return up.sessions + } + + /// Records the VPNs up now, unless the service is stopping. + private func recordUp() { + guard !stopping else { return } + guard Settings.reconnectAfterUpdate, let version = Self.version else { + Settings.upSessions = nil + return + } + let up = sessions.values + .filter { [.connected, .reconnecting].contains($0.state) } + .map { session in + let options = lastOptions[session.id] + ?? (session.via == nil && !(profile(session.id)?.via.isEmpty ?? true), isSplit(session.id)) + return UpSessions.Entry(id: session.id, direct: options.direct, split: options.split) + } + .sorted { $0.id < $1.id } + let old = Settings.upSessions + guard old?.sessions != up || old?.version != version else { return } + Settings.upSessions = UpSessions(version: version, at: Date(), sessions: up) + } + + /// Keeps the record as it is while the sessions close, dated now. + func holdRecord() { + stopping = true + if var up = Settings.upSessions { + up.at = Date() + Settings.upSessions = up + } + } + + private func resume(_ entries: [UpSessions.Entry]) { + let todo = Self.resumable(entries, known: Set(profiles.map(\.id)), via: { [weak self] in self?.profile($0)?.via ?? [] }) + .filter { !isOn($0.id) } + guard !todo.isEmpty else { return } + Notifier.shared.post( + title: String(localized: "Reconnecting after the update"), + body: todo.map { displayName($0.id) }.formatted(.list(type: .and))) + for entry in todo { connect(entry.id, direct: entry.direct, split: entry.split) } + } + + /// The entries to connect, profiles that still exist only: a VPN that + /// another one goes through comes back with it. + nonisolated static func resumable(_ entries: [UpSessions.Entry], known: Set, via: (String) -> [String]) -> [UpSessions.Entry] { + let present = entries.filter { known.contains($0.id) } + let chained = Set(present.filter { !$0.direct }.flatMap { via($0.id) }) + return present.filter { !chained.contains($0.id) } + } + // MARK: Actions func toggle(_ id: String, on: Bool) { @@ -433,6 +515,10 @@ private final class Observer: SessionObserver, @unchecked Sendable { DispatchQueue.main.async { MainActor.assumeIsolated { self.model?.changed(change) } } } + func stopping() { + DispatchQueue.main.async { MainActor.assumeIsolated { self.model?.holdRecord() } } + } + func stopped(error: ClientError?) { DispatchQueue.main.async { MainActor.assumeIsolated { self.model?.stopped(error) } } } diff --git a/apple/Omnitun/SettingsView.swift b/apple/Omnitun/SettingsView.swift index 9c00145..e2fafbe 100644 --- a/apple/Omnitun/SettingsView.swift +++ b/apple/Omnitun/SettingsView.swift @@ -67,6 +67,7 @@ private struct AboutView: View { private struct GeneralSettings: View { @State private var openAtLogin = LoginItem.isEnabled @State private var disconnectOnQuit = Settings.disconnectOnQuit + @State private var reconnectAfterUpdate = Settings.reconnectAfterUpdate @State private var notifyConnected = Settings.notifyConnected @State private var notifyFailures = Settings.notifyFailures @State private var language = Settings.language @@ -84,6 +85,8 @@ private struct GeneralSettings: View { .onChange(of: disconnectOnQuit) { _, on in Settings.disconnectOnQuit = on } Text("Without the app, a code or a browser login cannot be asked again when a VPN reconnects.") .font(.caption).foregroundStyle(.secondary) + Toggle("Reconnect VPNs after an update", isOn: $reconnectAfterUpdate) + .onChange(of: reconnectAfterUpdate) { _, on in Settings.reconnectAfterUpdate = on } if Updates.shared.available { Toggle("Check for updates every day", isOn: $checkUpdates) .onChange(of: checkUpdates) { _, on in Updates.shared.checksAutomatically = on } diff --git a/apple/Omnitun/Updates.swift b/apple/Omnitun/Updates.swift index 9f7fd89..8860112 100644 --- a/apple/Omnitun/Updates.swift +++ b/apple/Omnitun/Updates.swift @@ -10,6 +10,8 @@ import Sparkle final class Updates: NSObject, SPUStandardUserDriverDelegate, SPUUpdaterDelegate { static let shared = Updates() private var controller: SPUStandardUpdaterController? + /// Called just before the installer stops the service and the app. + var willInstall: (() -> Void)? /// A build run from the package's .app: `swift run` has no bundle, no /// feed and no key to check against. @@ -45,6 +47,15 @@ final class Updates: NSObject, SPUStandardUserDriverDelegate, SPUUpdaterDelegate // active before, never to a menu bar app, and refuses it to us: the // window that then offers to install and relaunch would stay behind. // It floats above the other apps' windows until it closes. + // The VPNs up now come back after the update: quitting for it must not + // disconnect them first. + nonisolated func updater(_ updater: SPUUpdater, willInstallUpdate item: SUAppcastItem) { + MainActor.assumeIsolated { + Settings.relaunching = true + willInstall?() + } + } + nonisolated func updater(_ updater: SPUUpdater, didExtractUpdate item: SUAppcastItem) { MainActor.assumeIsolated { let sparkle = Bundle(for: SPUStandardUpdaterController.self) diff --git a/apple/Package.swift b/apple/Package.swift index aec9a42..39ae776 100644 --- a/apple/Package.swift +++ b/apple/Package.swift @@ -28,5 +28,6 @@ let package = Package( linkerSettings: [.unsafeFlags(["-Xlinker", "-rpath", "-Xlinker", "@executable_path/../Frameworks"])] ), .executableTarget(name: "Smoke", dependencies: ["VPNClientCore"], path: "Smoke"), + .testTarget(name: "OmnitunTests", dependencies: ["Omnitun"], path: "Tests", swiftSettings: [.swiftLanguageMode(.v5)]), ] ) diff --git a/apple/README.md b/apple/README.md index 1fe9833..cac52d4 100644 --- a/apple/README.md +++ b/apple/README.md @@ -27,6 +27,7 @@ override the system language. ```sh apple/build-bindings.sh # Swift bindings + universal Rust library swift run --package-path apple Smoke # checks the bindings +swift test --package-path apple # the app's unit tests swift build --package-path apple # the app alone apple/package.sh # apple/dist/Omnitun.app and Omnitun-.pkg ``` @@ -48,6 +49,14 @@ builds for another repository) and offers the new version; the package is installed only if the user accepts, with an administrator password. Menu “Check for Updates…”, setting in Settings › General. +Installing stops the service, which closes the tunnels. The app then +connects again the VPNs that were up, the same way (directly or through +their chain, in split mode or not), unless “Reconnect VPNs after an update” +is off. It keeps only their ids and options in its preferences, never a +secret; the service's `stopping` event tells it the sessions that close +next were not ended by their users. A quit or a crash without an update +connects nothing. + ```sh apple/release.sh # apple/dist/release/: notarized package, signed appcast.xml; publishes nothing apple/release.sh --publish # also creates the vVERSION Release with those two files diff --git a/apple/Resources/fr.lproj/Localizable.strings b/apple/Resources/fr.lproj/Localizable.strings index bc115a5..3d9d742 100644 --- a/apple/Resources/fr.lproj/Localizable.strings +++ b/apple/Resources/fr.lproj/Localizable.strings @@ -111,6 +111,8 @@ "General" = "Général"; "Service" = "Service"; "Disconnect VPNs when I quit %@" = "Déconnecter les VPN quand je quitte %@"; +"Reconnect VPNs after an update" = "Reconnecter les VPN après une mise à jour"; +"Reconnecting after the update" = "Reconnexion après la mise à jour"; "Without the app, a code or a browser login cannot be asked again when a VPN reconnects." = "Sans l'app, un code ou une connexion par navigateur ne peut pas être redemandé lors d'une reconnexion."; "Language" = "Langue"; "Same as macOS" = "Comme macOS"; diff --git a/apple/Tests/ReconnectTests.swift b/apple/Tests/ReconnectTests.swift new file mode 100644 index 0000000..9268d1a --- /dev/null +++ b/apple/Tests/ReconnectTests.swift @@ -0,0 +1,43 @@ +import XCTest +@testable import Omnitun + +final class ReconnectTests: XCTestCase { + private let now = Date(timeIntervalSince1970: 1_800_000_000) + private let paris = UpSessions.Entry(id: "paris", direct: false, split: true) + + private func up(_ version: String, minutesAgo: Double, _ sessions: [UpSessions.Entry]) -> UpSessions { + UpSessions(version: version, at: now.addingTimeInterval(-minutesAgo * 60), sessions: sessions) + } + + func testReconnectsAfterAnUpdate() { + XCTAssertEqual(AppModel.toResume(up("0.4.3", minutesAgo: 1, [paris]), version: "0.4.4", now: now), [paris]) + } + + func testNothingWithoutAnUpdate() { + XCTAssertNil(AppModel.toResume(up("0.4.4", minutesAgo: 1, [paris]), version: "0.4.4", now: now)) + } + + func testNothingFromLongAgo() { + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: 20, [paris]), version: "0.4.4", now: now)) + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: -5, [paris]), version: "0.4.4", now: now), "dated in the future") + } + + func testNothingRecorded() { + XCTAssertNil(AppModel.toResume(nil, version: "0.4.4", now: now)) + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: 1, []), version: "0.4.4", now: now)) + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: 1, [paris]), version: nil, now: now), "a build without a version") + } + + func testAChainBringsBackWhatItGoesThrough() { + let office = UpSessions.Entry(id: "office", direct: false, split: false) + let home = UpSessions.Entry(id: "home", direct: false, split: false) + let via = { (id: String) in id == "office" ? ["home"] : [] } + XCTAssertEqual(AppModel.resumable([home, office], known: ["home", "office"], via: via), [office]) + let alone = UpSessions.Entry(id: "office", direct: true, split: false) + XCTAssertEqual(AppModel.resumable([home, alone], known: ["home", "office"], via: via), [home, alone], "connected directly") + } + + func testDeletedProfilesAreSkipped() { + XCTAssertEqual(AppModel.resumable([paris], known: [], via: { _ in [] }), []) + } +} diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index 766fb99..ee2a381 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -181,6 +181,7 @@ async fn main() -> anyhow::Result<()> { } } tracing::info!("shutting down, closing all sessions"); + manager.stopping(); // Each engine gets a few seconds to say goodbye; past the deadline the // daemon exits anyway, well before launchd or systemd would kill it, // and the next start removes what is left. @@ -248,7 +249,7 @@ impl Drop for Slot { } /// Optional capabilities announced in `Hello`. -const FEATURES: &[&str] = &["watch", "shutdown", "trust_certificate", "failure_kinds", "prompt_socket"]; +const FEATURES: &[&str] = &["watch", "shutdown", "trust_certificate", "failure_kinds", "prompt_socket", "stopping"]; fn failed(e: anyhow::Error) -> Event { Event::Error { message: format!("{e:#}"), failure: Some(vpn_core::failure::kind_of(&e)) } diff --git a/crates/daemon/src/manager.rs b/crates/daemon/src/manager.rs index b49bf7f..c516c7a 100644 --- a/crates/daemon/src/manager.rs +++ b/crates/daemon/src/manager.rs @@ -100,7 +100,8 @@ impl Default for Manager { #[derive(Clone)] struct Watched { - owner: u32, + /// `None`: for every watcher. + owner: Option, event: Event, } @@ -261,6 +262,12 @@ impl Manager { /// Closes every session at once: the service manager kills the daemon /// if stopping takes too long, so teardowns must not queue up. + /// Tells every watcher that the sessions are about to close because the + /// daemon stops: clients bring them back after an update. + pub fn stopping(&self) { + let _ = self.events.send(Watched { owner: None, event: Event::Stopping }); + } + pub async fn disconnect_all(&self) { let tasks: Vec<_> = { let mut sessions = self.sessions.lock().unwrap(); @@ -295,7 +302,7 @@ impl Manager { _ = link.closed() => return, }; match watched { - Ok(w) if uid == 0 || w.owner == uid => { + Ok(w) if uid == 0 || w.owner.is_none_or(|owner| owner == uid) => { if !link.send(w.event) { return; } @@ -436,7 +443,7 @@ impl Runner { None => tracing::info!(session = self.profile.name, "{state:?}"), } let event = Event::State { id: self.profile.id.clone(), state, detail, failure, retry }; - let _ = self.manager.events.send(Watched { owner: self.owner, event: event.clone() }); + let _ = self.manager.events.send(Watched { owner: Some(self.owner), event: event.clone() }); self.client.notify(event); } @@ -1224,6 +1231,20 @@ mod tests { assert!(timeout(Duration::from_millis(200), theirs_rx.recv()).await.is_err(), "another user's session leaked"); } + #[tokio::test] + async fn every_watcher_hears_that_the_daemon_stops() { + let m = Arc::new(Manager::default()); + let (mine, mut mine_rx) = client(); + let (theirs, mut theirs_rx) = client(); + m.watch(1000, mine); + m.watch(1001, theirs); + assert!(matches!(next_event(&mut mine_rx).await, Event::Status { .. })); + assert!(matches!(next_event(&mut theirs_rx).await, Event::Status { .. })); + m.stopping(); + assert!(matches!(next_event(&mut mine_rx).await, Event::Stopping)); + assert!(matches!(next_event(&mut theirs_rx).await, Event::Stopping)); + } + #[test] fn the_prompt_directory_must_be_private_to_root() { use std::os::unix::fs::PermissionsExt; @@ -1596,7 +1617,7 @@ mod tests { fake_session(&m, "paris", 1000); for i in 0..WATCH_QUEUE + 10 { let event = Event::Warning { message: i.to_string() }; - let _ = m.events.send(Watched { owner: 1000, event }); + let _ = m.events.send(Watched { owner: Some(1000), event }); } let Some(Event::Status { sessions }) = timeout(Duration::from_secs(2), rx.recv()).await.unwrap() else { panic!("expected the status first") diff --git a/crates/vpn-client/src/session.rs b/crates/vpn-client/src/session.rs index c369366..13858bf 100644 --- a/crates/vpn-client/src/session.rs +++ b/crates/vpn-client/src/session.rs @@ -262,7 +262,11 @@ async fn connect_one( }); } Event::Error { message, failure } => return Err(crate::daemon::error(message, failure)), - Event::Prompt { .. } | Event::OpenUrl { .. } | Event::Status { .. } | Event::Hello { .. } => {} + Event::Prompt { .. } + | Event::OpenUrl { .. } + | Event::Status { .. } + | Event::Hello { .. } + | Event::Stopping => {} } } } diff --git a/crates/vpn-core/src/ipc.rs b/crates/vpn-core/src/ipc.rs index 8432c22..700f156 100644 --- a/crates/vpn-core/src/ipc.rs +++ b/crates/vpn-core/src/ipc.rs @@ -134,6 +134,9 @@ pub enum Event { sessions: Vec, }, Done, + /// To watchers: the daemon is stopping (an update, a restart). The + /// sessions it closes next were not ended by their users. + Stopping, Error { message: String, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/crates/vpn-ffi/src/lib.rs b/crates/vpn-ffi/src/lib.rs index 15af53d..9c058cd 100644 --- a/crates/vpn-ffi/src/lib.rs +++ b/crates/vpn-ffi/src/lib.rs @@ -468,6 +468,10 @@ impl Client { } sessions.sort_by(|a, b| a.name.cmp(&b.name)); } + Event::Stopping => { + observer.stopping(); + continue; + } Event::Error { message, failure } => { return Err(vpn_core::failure(failure.unwrap_or(FailureKind::Other), message)); } @@ -509,6 +513,9 @@ pub trait ConnectionDelegate: Send + Sync { pub trait SessionObserver: Send + Sync { fn sessions(&self, sessions: Vec); fn changed(&self, change: StateChange); + /// The daemon is stopping (an update, a restart): the sessions that end + /// next were not ended by their users. + fn stopping(&self); /// The daemon went away or refused; subscribe again later. fn stopped(&self, error: Option); }