Reconnect the VPNs that were up after an update

.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

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

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<Void, Never>?
 
@@ -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<String>, 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) } }
     }

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 }

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)

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)]),
     ]
 )

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-<version>.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

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";

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 [] }), [])
+    }
+}

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)) }

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<u32>,
     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")