From e14801232d286dc1ba8c800ef22ebfc4b556c1f5 Mon Sep 17 00:00:00 2001 From: Ryanmello07 <67509637+Ryanmello07@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:46:25 -0700 Subject: [PATCH] persist split rules in the app's own local state The split rules live in the network extension's container, which the app process cannot read. BlockActionsStore held no copy of its own, so with the tunnel down the app had no source of rules at all: the sheet showed the never-configured empty state over rules the extension still had on disk, and a rule added in that state lived only in DeviceRemote's memory and went away with the process. Worse than not saving, it destroyed. DeviceRemote's offline mutators rebuild an authoritative full list from whatever base they can see, and with nothing seeded that base is empty -- so the truncated list crossed on the next connect and replaced the extension's saved rules. Give split rules the two halves transport settings already has here: the store mirrors every list it can vouch for as the whole list, and initDevice seeds the next device from that mirror. The app is the sole author of these rules -- DeviceLocal changes its list only through the rpc setters the app drives, plus its own startup seed -- so the mirror replacing the extension's copy on connect is the one author replaying its own record, not two authors racing. Two distinctions the fix rests on, both covered by the new tests: a store that was never written reads back nil and seeds nothing, so rules left by an older build survive the first launch after this ships; and an empty list is written as an empty list rather than as nil, because nil deletes the store and a deleted store reads back as "never edited", which would resurrect the rules the user just removed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AWV4MaF9JBcQppSX9UxATa --- app/network/NetworkApp.swift | 10 +- .../Shared/ViewModels/BlockActionsStore.swift | 89 +++++++++- .../Shared/ViewModels/DeviceManager.swift | 14 ++ .../BlockActionOverridesMirrorTests.swift | 164 ++++++++++++++++++ 4 files changed, 268 insertions(+), 9 deletions(-) create mode 100644 app/networkTests/BlockActionOverridesMirrorTests.swift diff --git a/app/network/NetworkApp.swift b/app/network/NetworkApp.swift index 637465da..c97d7c00 100644 --- a/app/network/NetworkApp.swift +++ b/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) } diff --git a/app/network/Shared/ViewModels/BlockActionsStore.swift b/app/network/Shared/ViewModels/BlockActionsStore.swift index e4c72319..331472d0 100644 --- a/app/network/Shared/ViewModels/BlockActionsStore.swift +++ b/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] { diff --git a/app/network/Shared/ViewModels/DeviceManager.swift b/app/network/Shared/ViewModels/DeviceManager.swift index 94ac8408..0936ef89 100644 --- a/app/network/Shared/ViewModels/DeviceManager.swift +++ b/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 } diff --git a/app/networkTests/BlockActionOverridesMirrorTests.swift b/app/networkTests/BlockActionOverridesMirrorTests.swift new file mode 100644 index 00000000..411d3f8a --- /dev/null +++ b/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 + /// `/.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..