Persist split rules in the app's own local state

app/network/NetworkApp.swift

@@ -167,12 +167,14 @@ struct NetworkApp: App {
     }
 
     private func setupDeviceStores(_ device: SdkDeviceRemote) {
+        let localState = deviceManager.asyncLocalState?.getLocalState()
         throughputStore.setup(device)
-        blockActionsStore.setup(device)
+        // the app local state mirrors the split rule and transport settings
+        // edits so they survive an app relaunch while the tunnel is down
+        // (see the stores)
+        blockActionsStore.setup(device, localState: localState)
         dnsSettingsStore.setup(device)
-        // the app local state mirrors the transport settings edits so they
-        // survive an app relaunch while the tunnel is down (see the store)
-        transportSettingsStore.setup(device, localState: deviceManager.asyncLocalState?.getLocalState())
+        transportSettingsStore.setup(device, localState: localState)
         networkPeersStore.setup(device)
         reliabilityStore.setup(device)
     }

app/network/Shared/ViewModels/BlockActionsStore.swift

@@ -154,6 +154,16 @@ private class BlockActionOverridesListener: NSObject, SdkBlockActionOverridesCha
     }
 }
 
+private class OverridesRemoteListener: NSObject, SdkRemoteChangeListenerProtocol {
+    private let callback: (Bool) -> Void
+    init(callback: @escaping (Bool) -> Void) {
+        self.callback = callback
+    }
+    func remoteChanged(_ remoteConnected: Bool) {
+        callback(remoteConnected)
+    }
+}
+
 /**
  * Publishes the live block action window, block stats, and the
  * block action overrides ("split rules")
@@ -170,11 +180,15 @@ class BlockActionsStore: ObservableObject {
     @Published private(set) var blockedCount: Int = 0
 
     private var device: SdkDeviceRemote?
+    // the app's own store, which the split rules are mirrored into; the
+    // extension keeps its copy in a container this process cannot read
+    private var localState: SdkLocalState?
     private var blockActionViewController: SdkBlockActionViewController?
 
     private var blockActionsSub: SdkSubProtocol?
     private var blockActionStatsSub: SdkSubProtocol?
     private var overridesSub: SdkSubProtocol?
+    private var remoteSub: SdkSubProtocol?
 
     /**
      * the sdk override objects backing `splitRules`, used to rebuild
@@ -223,10 +237,13 @@ class BlockActionsStore: ObservableObject {
         exitAttributionTimer?.invalidate()
     }
 
-    func setup(_ device: SdkDeviceRemote) {
+    func setup(_ device: SdkDeviceRemote, localState: SdkLocalState?) {
         reset()
 
         self.device = device
+        // above the guard below: a device whose block action window will not
+        // open still has split rules to mirror
+        self.localState = localState
 
         guard let blockActionViewController = device.openBlockActionViewController() else {
             return
@@ -251,6 +268,18 @@ class BlockActionsStore: ObservableObject {
                 self?.updateOverrides()
             }
         })
+        // the override change the extension replays on connect arrives over
+        // the reverse sync, which runs before the remote publishes its
+        // service, so that read still comes out of the remote's own memory.
+        // Re-read once the rpc is up so the mirror is written from the
+        // extension's list rather than from this process's guess at it
+        self.remoteSub = device.add(OverridesRemoteListener { [weak self] remoteConnected in
+            DispatchQueue.main.async {
+                if remoteConnected {
+                    self?.updateOverrides()
+                }
+            }
+        })
 
         updateBlockActions()
         updateBlockStats()
@@ -275,6 +304,8 @@ class BlockActionsStore: ObservableObject {
         blockActionStatsSub = nil
         overridesSub?.close()
         overridesSub = nil
+        remoteSub?.close()
+        remoteSub = nil
         if let blockActionViewController {
             if let device {
                 device.close(blockActionViewController)
@@ -284,6 +315,9 @@ class BlockActionsStore: ObservableObject {
         }
         blockActionViewController = nil
         device = nil
+        // nothing is persisted from here: reset runs on every backgrounding
+        // and the mirror is already current by then
+        localState = nil
 
         blockActions = []
         splitRules = []
@@ -504,7 +538,21 @@ class BlockActionsStore: ObservableObject {
         }
     }
 
-    private func updateOverrides() {
+    /**
+     * Re-reads the rules from the device and, when the read is one this
+     * process can vouch for as the WHOLE list, mirrors it.
+     *
+     * Only two reads qualify: one taken off a connected device, which is the
+     * extension's own list, and one taken right after an edit made here,
+     * which is that list plus the edit. With the rpc down and nothing seeded
+     * the device answers out of its own empty memory instead, and mirroring
+     * that would replace the app's only durable copy with a list it never
+     * read -- which the next connect would apply to the extension as a wipe.
+     *
+     * `getConnected()` is GetRemoteConnected; swift's importer strips the
+     * redundant "Remote".
+     */
+    private func updateOverrides(afterEdit: Bool = false) {
         guard let device = self.device else {
             return
         }
@@ -539,6 +587,37 @@ class BlockActionsStore: ObservableObject {
         if items != splitRules {
             splitRules = items
         }
+        if afterEdit || device.getConnected() {
+            persistOverrides()
+        }
+    }
+
+    /**
+     * Writes the rules to the app's own local state.
+     *
+     * The extension persists its own copy, but into a container this process
+     * cannot read, so this mirror is what `DeviceManager.initDevice` seeds
+     * the next device from -- both so the rules render with the tunnel down,
+     * and so an edit made then is queued against the whole list instead of
+     * an empty one.
+     *
+     * An empty list is written as an empty list and never as nil: nil takes
+     * the store's delete branch, and a deleted store reads back as "never
+     * edited", which would leave the extension's rules in place after the
+     * user removed the last one.
+     */
+    private func persistOverrides() {
+        guard let localState = self.localState, let list = SdkBlockActionOverrideList() else {
+            return
+        }
+        for sdkOverride in sdkOverrides {
+            list.add(sdkOverride)
+        }
+        do {
+            try localState.setBlockActionOverrides(list)
+        } catch {
+            print("[BlockActionsStore]failed to persist split rules: \(error.localizedDescription)")
+        }
     }
 
     /**
@@ -564,7 +643,7 @@ class BlockActionsStore: ObservableObject {
         override.hosts = arrayToStringList(hosts)
         override.routeOverride = mode.toSdkRouteOverride()
         device.add(override)
-        updateOverrides()
+        updateOverrides(afterEdit: true)
     }
 
     /**
@@ -588,7 +667,7 @@ class BlockActionsStore: ObservableObject {
             list?.add(sdkOverride)
         }
         device.setBlockActionOverrides(list)
-        updateOverrides()
+        updateOverrides(afterEdit: true)
     }
 
     func removeRule(id: String) {
@@ -599,7 +678,7 @@ class BlockActionsStore: ObservableObject {
             return
         }
         device.removeBlockActionOverride(override.overrideId)
-        updateOverrides()
+        updateOverrides(afterEdit: true)
     }
 
     private func stringListToArray(_ list: SdkStringList?) -> [String] {

app/network/Shared/ViewModels/DeviceManager.swift

@@ -1135,6 +1135,20 @@ extension DeviceManager {
             device.setProviderTransportSettings(providerTransportSettings)
         }
 
+        // split rules ("block action overrides") cross the same way, mirrored
+        // by BlockActionsStore. One consequence beyond the transport settings
+        // above: without the seed the remote rebuilds its "full" list from
+        // nothing, so an edit made while the tunnel was down does not merely
+        // fail to save -- it replaces the extension's saved rules on the next
+        // connect. Nothing stored means never edited, and a store that cannot
+        // be read reports the same way, so both leave the extension's rules
+        // alone; an empty list is a real value -- the last rule was deleted --
+        // and is seeded as one. This must stay above setDevice, which is what
+        // publishes the device and drives BlockActionsStore.setup.
+        if let blockActionOverrides = localState.getBlockActionOverrides() {
+            device.setBlockActionOverrides(blockActionOverrides)
+        }
+
         self.setDevice(device: device)
         return true
     }

app/networkTests/BlockActionOverridesMirrorTests.swift

@@ -0,0 +1,164 @@
+//
+//  BlockActionOverridesMirrorTests.swift
+//  networkTests
+//
+//  Covers the store the split rules are mirrored into.
+//
+//  The rules themselves live in the network extension's container, which the
+//  app process cannot read, so this mirror is the app's only durable copy --
+//  it is what the sheet renders from with the tunnel down and what
+//  DeviceManager.initDevice seeds the next device from.
+//
+//  Two of these cases are the ones the fix would be silently wrong without.
+//  Writing nil DELETES the store, and a deleted store reads back exactly like
+//  one that was never written, so "the user removed their last rule" and "this
+//  install has never mirrored" are the same value unless an empty list is
+//  written as an empty list. The first would resurrect deleted rules on the
+//  next connect; the second would push an empty list over rules the extension
+//  still holds. Everything BlockActionsStore.persistOverrides and the seed's
+//  `if let` do rests on that distinction.
+//
+
+import Testing
+import Foundation
+import URnetworkSdk
+@testable import URnetwork
+
+struct BlockActionOverridesMirrorTests {
+
+    /// Each test gets its own storage home; the SDK roots the store at
+    /// `<home>/.by`, so a fresh temporary directory is a fresh store.
+    private func withLocalState(_ body: (SdkLocalState) throws -> Void) rethrows {
+        let home = FileManager.default.temporaryDirectory
+            .appendingPathComponent("BlockActionOverridesMirrorTests-\(UUID().uuidString)")
+        try? FileManager.default.createDirectory(at: home, withIntermediateDirectories: true)
+        defer { try? FileManager.default.removeItem(at: home) }
+
+        guard let asyncLocalState = SdkAsyncLocalState(home.path()),
+              let localState = asyncLocalState.getLocalState() else {
+            Issue.record("could not open a local state under \(home.path())")
+            return
+        }
+        defer { asyncLocalState.close() }
+        try body(localState)
+    }
+
+    private func override(
+        hosts: [String],
+        local: Bool = false,
+        pin: Bool = false
+    ) -> SdkBlockActionOverride {
+        let override = SdkBlockActionOverride()
+        override.overrideId = SdkNewId()
+        let hostList = SdkStringList()
+        for host in hosts {
+            hostList?.add(host)
+        }
+        override.hosts = hostList
+        let routeOverride = SdkRouteOverride()
+        routeOverride.local = local
+        routeOverride.pin = pin
+        override.routeOverride = routeOverride
+        return override
+    }
+
+    private func list(_ overrides: [SdkBlockActionOverride]) -> SdkBlockActionOverrideList {
+        let list = SdkBlockActionOverrideList()
+        for override in overrides {
+            list?.add(override)
+        }
+        return list!
+    }
+
+    private func hosts(_ override: SdkBlockActionOverride) -> [String] {
+        guard let hosts = override.hosts else {
+            return []
+        }
+        return (0..<hosts.len()).map { hosts.get($0) }
+    }
+
+    /// The fields SplitRuleMode.of reads to decide a rule's chip -- an
+    /// override that round-trips without them renders as the wrong mode.
+    @Test func rulesRoundTripWithTheirIdHostsAndMode() throws {
+        try withLocalState { localState in
+            let excluded = override(hosts: ["*.example.com", "10.0.0.0/8"], local: true)
+            let pinned = override(hosts: ["api.example.org"], pin: true)
+            let excludedId = excluded.overrideId?.idStr
+            let pinnedId = pinned.overrideId?.idStr
+
+            try localState.setBlockActionOverrides(list([excluded, pinned]))
+
+            let read = try #require(localState.getBlockActionOverrides())
+            #expect(read.len() == 2)
+
+            let first = try #require(read.get(0))
+            #expect(first.overrideId?.idStr == excludedId)
+            #expect(hosts(first) == ["*.example.com", "10.0.0.0/8"])
+            #expect(first.routeOverride?.local == true)
+            #expect(first.routeOverride?.pin == false)
+
+            let second = try #require(read.get(1))
+            #expect(second.overrideId?.idStr == pinnedId)
+            #expect(hosts(second) == ["api.example.org"])
+            #expect(second.routeOverride?.local == false)
+            #expect(second.routeOverride?.pin == true)
+        }
+    }
+
+    /// The mirror copies the device's own override objects rather than the
+    /// SplitRuleItem projection the UI renders, so fields the app never models
+    /// have to survive it too.
+    @Test func fieldsTheAppDoesNotModelSurviveTheRoundTrip() throws {
+        try withLocalState { localState in
+            let unrouted = SdkBlockActionOverride()
+            unrouted.overrideId = SdkNewId()
+            let blocked = override(hosts: ["ads.example.com"])
+            let blockOverride = SdkBlockOverride()
+            blockOverride.block = true
+            blocked.blockOverride = blockOverride
+
+            try localState.setBlockActionOverrides(list([unrouted, blocked]))
+
+            let read = try #require(localState.getBlockActionOverrides())
+            #expect(read.len() == 2)
+            #expect(try #require(read.get(0)).routeOverride == nil)
+            #expect(try #require(read.get(1)).blockOverride?.block == true)
+        }
+    }
+
+    /// "I deleted my last rule" -- an empty list is a real value and has to
+    /// come back as one, or the seed skips it and the extension's copy stands.
+    @Test func anEmptyListRoundTripsAsAnEmptyList() throws {
+        try withLocalState { localState in
+            try localState.setBlockActionOverrides(list([override(hosts: ["example.com"])]))
+            try localState.setBlockActionOverrides(list([]))
+
+            let read = localState.getBlockActionOverrides()
+            #expect(read != nil)
+            #expect(read?.len() == 0)
+        }
+    }
+
+    /// Writing nil is the delete branch. persistOverrides must never reach it:
+    /// the result is indistinguishable from an install that never mirrored.
+    @Test func writingNilDeletesTheStore() throws {
+        try withLocalState { localState in
+            try localState.setBlockActionOverrides(list([override(hosts: ["example.com"])]))
+            #expect(localState.getBlockActionOverrides() != nil)
+
+            try localState.setBlockActionOverrides(nil)
+
+            #expect(localState.getBlockActionOverrides() == nil)
+        }
+    }
+
+    /// The upgrade guard. A build that has never mirrored reads nil, so
+    /// DeviceManager.initDevice queues nothing and the rules a pre-fix build
+    /// left in the extension are still there on the first launch after this
+    /// ships -- rather than being replaced by an empty list.
+    @Test func aStoreThatWasNeverWrittenReadsNilRatherThanEmpty() throws {
+        try withLocalState { localState in
+            #expect(localState.getBlockActionOverrides() == nil)
+        }
+    }
+}