diff --git a/app/extension/PacketTunnelProvider.swift b/app/extension/PacketTunnelProvider.swift index 67d7b6d3..a40e9f60 100644 --- a/app/extension/PacketTunnelProvider.swift +++ b/app/extension/PacketTunnelProvider.swift @@ -559,6 +559,61 @@ class PacketTunnelProvider: NEPacketTunnelProvider { // Only the explicit logout app message rotates the identity. let keyMaterial: SdkDeviceLocalKeyMaterial? = localState.getDeviceLocalKeyMaterial() + // KNOWN LIMITATION -- the control-plane ip family force is Auto here + // for the FIRST connect after it is set with the tunnel down, and + // that is deliberate. + // + // importNetworkSpace above restores the persisted policy from THIS + // process's container. A force set while the tunnel was down lives + // only in the app's container, so the restore finds nothing; the + // device below starts control-plane work during construction (the + // provider's platform websocket dial and the api jwt refresh both go + // out before this function returns), and the rpc listener that + // carries the app's queued policy is not opened until setRpcServer + // further down. The app's first sync lands roughly a second later -- + // after those dials are already issued, so it governs what follows + // rather than rescuing them. + // + // Not closed, because the cost is one repetition of a cost the + // Automatic path already pays on every single connect. The sdk's + // demotion ledger is in-memory and process-global, so a fresh + // extension process starts with an empty one every time: an + // unforced user re-learns the bad family, at the price of one + // stalled first handshake, on every connect. + // + // That price is bounded and known: ControlFamilyFirstHandshakeTimeout, + // 8s -- the floor connect puts on the first handshake of a control + // dial so the retry over the other family still has budget to run + // (connect/control_family_dial.go). The api jwt refresh is the dial + // that pays it and the dial that learns; the platform websocket + // arrives with gorilla's 5s cap, which is under 8s + + // ControlFamilyRetryReserve, so connect leaves that handshake + // unbounded and it inherits the answer from the process-global ledger + // instead of paying for its own. (An earlier revision of this comment + // said ~3s. That figure came from a budget-halving helper which has + // since been deleted in favour of the floor, and it was wrong from the + // moment the floor landed.) + // + // The force-setter pays that same 8s once and is then strictly better + // off, because the sync persists the policy into this process's own + // container and every later start restores it here. + // + // The regimes that actually hurt are already covered: the app + // process dials pre-login and with the tunnel down, and there the + // force is in effect the moment it is set -- which is the state a + // user is in when they open the Developer menu to reach for it. + // + // Every available fix (a key in providerConfiguration, the + // startTunnel options dict, or the group.network.ur App Group) works + // by copying a preference into a durable store owned by the OS or by + // the filesystem, beside LocalState which already owns it. That is a + // second, staleable source of truth and a third channel for + // preferences into this process, bought for one stalled 8s handshake + // on one connect of a developer-only setting. It also contradicts the + // contract the rest of this block states plainly below: the extension + // seeds from its own local state, and those values hold "until the app + // connects and sets the user values". + let newDevice = SdkNewDeviceLocalWithMemoryTarget( networkSpace, byJwt, diff --git a/app/network/Main/Account/Settings/Developer/DeveloperView.swift b/app/network/Main/Account/Settings/Developer/DeveloperView.swift index fb484017..4b9b4c6f 100644 --- a/app/network/Main/Account/Settings/Developer/DeveloperView.swift +++ b/app/network/Main/Account/Settings/Developer/DeveloperView.swift @@ -48,6 +48,10 @@ struct DeveloperView: View { // not of this screen, and a write is an rpc that must not be abandoned // half-way by navigating back @ObservedObject private var verbosityState = LogVerbosityState.shared + // held outside the view for the same reason, and for one more: the write + // may be an rpc into the extension, and the row it drives is the one a + // user reaches for when the api is unreachable + @ObservedObject private var ipFamilyState = IpFamilyState.shared @State private var showLogPicker = false @State private var selectedLogNames: Set = [] @@ -86,6 +90,23 @@ struct DeveloperView: View { reliabilityStore.setActive(presentationActive) } .task { + // FIRST, ahead of the inventory's disk enumeration: restored by + // the sdk across a relaunch and settable from three different + // places, so what the row shows has to be re-read here rather than + // carried over from the last appearance -- and until it resolves + // the row renders its initial Automatic, which a tap in that + // window would advance from as if it were the value in force, so + // it goes first to close that window as early as possible. + // + // NOT free, unlike the rest of this screen's reads. The learned + // demotion belongs to whichever process dialed, so with a device + // the status half is an rpc into the extension rather than a cgo + // call. Over the live loopback connection the app already holds + // that is a sub-millisecond round trip, but a wedged extension + // makes it the head of this chain -- bounded by the rpc keepalive + // reaping the connection, after which the read falls back to this + // process's own ledger. + await ipFamilyState.refresh(device: deviceManager.device) // the inventory (and with it the total size and any unavailable // source) has to be on screen BEFORE the user commits to an // export, so it is read here rather than when the picker opens @@ -138,6 +159,7 @@ struct DeveloperView: View { if LogVerbosity.revealsDestinations(verbosityState.level) { destinationWarning } + ipFamilyRow if let inventoryLabel = exportState.inventoryLabel { Text(inventoryLabel) .font(themeManager.currentTheme.secondaryBodyFont) @@ -306,6 +328,76 @@ struct DeveloperView: View { .padding(.vertical, 2) } + /** + * Which address family the control plane dials over, as a row that cycles + * Automatic -> Force IPv4 -> Force IPv6 on tap. + * + * ENABLED WITH NO DEVICE, unlike the verbosity row above it -- and that is + * the point of the row, not an oversight. A user reaches for this when the + * api is unreachable: signed out at the login screen, or with the tunnel + * down and no device constructed. Gating it on a device would make it + * inert in exactly the state it exists to rescue. `IpFamilyState.cycle` + * falls back from the device to the network space to the process-global + * setter so that there is always somewhere for the write to land. + * + * Held only while a write is in flight: with a device that write is an rpc + * into the packet tunnel extension, so a second tap is refused rather than + * queued behind it. + */ + private var ipFamilyRow: some View { + // `.disabled` alone is invisible here: `.plain` dims nothing of its + // own and does not override an explicit `foregroundColor`, so the row + // would look fully tappable while every tap is dropped by cycle's + // in-flight guard. Muted the same way `actionRow` mutes a disabled + // action -- one demoted-to-textMutedColor idiom for the whole screen. + let isEnabled = !ipFamilyState.isApplying + return VStack(alignment: .leading, spacing: 4) { + Button(action: cycleIpFamily) { + HStack { + // same words as android's dev_ip_family, the way "Log + // detail" is shared with dev_log_verbosity -- one name for + // one setting across the two support threads + Text("Control connections") + .font(themeManager.currentTheme.bodyFont) + .foregroundColor( + isEnabled + ? themeManager.currentTheme.textColor + : themeManager.currentTheme.textMutedColor) + Spacer() + Text(IpFamily.valueLabel(ipFamilyState.policy)) + .font(themeManager.currentTheme.secondaryBodyFont) + .foregroundColor( + isEnabled + ? themeManager.currentTheme.accentColor + : themeManager.currentTheme.textMutedColor) + } + .contentShape(Rectangle()) + } + .buttonStyle(.plain) + .disabled(!isEnabled) + + Text(IpFamily.detail(ipFamilyState.policy, status: ipFamilyState.status)) + .font(themeManager.currentTheme.secondaryBodyFont) + .foregroundColor(themeManager.currentTheme.textMutedColor) + .fixedSize(horizontal: false, vertical: true) + } + .padding(.vertical, 2) + } + + /** + * Advances the policy. Both the device AND the network space are read + * here, on the main actor, before the hop -- `DeviceManager` is main-actor + * isolated, and the space is an independent published property rather than + * something the state could re-derive from the device off the main actor. + */ + private func cycleIpFamily() { + let device = deviceManager.device + let networkSpace = deviceManager.networkSpace + Task { + await ipFamilyState.cycle(device: device, networkSpace: networkSpace) + } + } + /** * Shown for as long as the level is raised, and deliberately loud. * diff --git a/app/network/Main/Account/Settings/Developer/IpFamily.swift b/app/network/Main/Account/Settings/Developer/IpFamily.swift new file mode 100644 index 00000000..ae682424 --- /dev/null +++ b/app/network/Main/Account/Settings/Developer/IpFamily.swift @@ -0,0 +1,92 @@ +// +// IpFamily.swift +// URnetwork +// +// Which address family the control plane dials over, and what each choice +// costs. +// +// The service publishes both an A and an AAAA record for its api and its +// control websocket, and the AAAA is in a tunnel-brokered range some ISPs +// route badly. Such a path completes the tcp handshake -- small packets pass +// -- and then drops the larger tls handshake, so the platform's own Happy +// Eyeballs race picks it, declares it the winner, and stalls. The sdk demotes +// a family that fails that way on its own; this is the override for when it +// does not, and the way to prove the diagnosis on a device that reproduces it. +// +// Pure so it can be tested without a device: the policy-to-label mapping, the +// clamp and the cycle order are the whole of the logic. +// + +import Foundation +import URnetworkSdk + +enum IpFamily { + + /// Mirrors the sdk's constants. Read the numbers from the sdk rather than + /// redeclaring them, so a rename on the Go side is a compile error here + /// rather than a silently wrong row. + static let auto = Int(SdkIpFamilyPolicyAuto) + static let force4 = Int(SdkIpFamilyPolicyForce4) + static let force6 = Int(SdkIpFamilyPolicyForce6) + + /// Anything the sdk would not recognise is Automatic, matching what the + /// sdk itself does with an out-of-range value. + static func clamp(_ policy: Int) -> Int { + switch policy { + case force4: return force4 + case force6: return force6 + default: return auto + } + } + + /// Tap order. Automatic first so the row returns to the safe default + /// without the user having to know which force is which. + static func next(_ policy: Int) -> Int { + switch clamp(policy) { + case auto: return force4 + case force4: return force6 + default: return auto + } + } + + static func name(_ policy: Int) -> String { + switch clamp(policy) { + case force4: return "Force IPv4" + case force6: return "Force IPv6" + default: return "Automatic" + } + } + + /** + * What this policy means right now, including anything the sdk has + * learned. + * + * `status` is the sdk's demotion description and is empty when nothing is + * demoted. It is reported only under Automatic: a force does not consult + * the ledger, so naming a demotion beside one would describe state that is + * not in effect. + */ + static func detail(_ policy: Int, status: String) -> String { + switch clamp(policy) { + case force4: + return "Control-plane connections use IPv4 only." + + " Turn this off on an IPv6-only network." + case force6: + return "Control-plane connections use IPv6 only." + + " Turn this off if the app cannot reach the server." + default: + if !status.isEmpty { + return "Automatic. \(status)." + } + return "Uses whichever family connects first," + + " and routes around one that fails after connecting." + } + } + + /// Name alone. There is no number worth showing here -- unlike the log + /// level, the policy's integer means nothing to a support thread that the + /// word does not already say. + static func valueLabel(_ policy: Int) -> String { + name(policy) + } +} diff --git a/app/network/Main/Account/Settings/Developer/IpFamilyState.swift b/app/network/Main/Account/Settings/Developer/IpFamilyState.swift new file mode 100644 index 00000000..24653859 --- /dev/null +++ b/app/network/Main/Account/Settings/Developer/IpFamilyState.swift @@ -0,0 +1,124 @@ +// +// IpFamilyState.swift +// URnetwork +// +// The control-plane address family policy in force, and the one write path +// that changes it. +// +// DELIBERATELY UNLIKE LogVerbosityState, which routes everything through the +// device and goes inert when there is none. This setting has to work with no +// device and with the tunnel down, because those are the states a user is in +// when the api is unreachable -- which is the whole reason to reach for it. +// So the write is a THREE-way fallback, the same one android's +// DeveloperViewModel uses: the device when there is one (on ios that is what +// carries the policy into the packet tunnel extension, the process that dials +// while the tunnel is up); else the network space (which sets this process +// and records the choice for the next launch); else the process-global +// SdkSetControlIpFamilyPolicy, which records nothing but at least puts the +// choice in force for the session. +// +// Held outside the view so an in-flight change is not abandoned by navigating +// away mid-rpc. +// + +import Foundation +import URnetworkSdk + +final class IpFamilyState: ObservableObject { + + static let shared = IpFamilyState() + + /// The policy this process reports. Never a learned demotion -- that is + /// `status` -- so the row round-trips exactly what was set and Automatic + /// always reads back as Automatic. + @Published private(set) var policy: Int = IpFamily.auto + + /// What the sdk has learned on its own, empty when nothing is demoted. + /// Rendered in the detail line so Automatic does not look identical + /// whether the heuristic has fired or not. + /// + /// Read from the DEVICE when there is one, unlike `policy`, which either + /// process can answer. A demotion is not set, it is LEARNED in whichever + /// process made the dial that failed, and while the tunnel is up that is + /// the network extension. Asking this process there would report an empty + /// string with a demotion actively in force -- exactly the ambiguity the + /// detail line exists to remove. + @Published private(set) var status: String = "" + + /// True across a write AND the read-back that follows it, as one unit. + /// With a device the write is an rpc round trip into the extension, so the + /// row is held rather than allowed to queue a second tap behind it. + @Published private(set) var isApplying = false + + @MainActor + func refresh(device: SdkDeviceRemote?) async { + let read = await Task.detached(priority: .userInitiated) { + ( + policy: device?.getControlIpFamilyPolicy() ?? Int(SdkGetControlIpFamilyPolicy()), + // an rpc round trip into the dialing process. The + // process-global is the right answer only when there is no + // device, because then THIS process is the one dialing -- + // see DeviceRemote.GetControlIpFamilyStatus, which falls + // back the same way and for the same reason + status: device?.getControlIpFamilyStatus() ?? SdkGetControlIpFamilyStatus() + ) + }.value + policy = IpFamily.clamp(read.policy) + status = read.status + } + + /** + * Advances to the next policy and republishes what the sdk then reports. + * + * Three write paths, tried in order, matching android exactly: + * + * 1. the device, when there is one -- it sets this process, records the + * choice, AND carries the policy across to the extension, so it is + * always preferred; + * 2. `networkSpace`, which is what makes this work signed out or with the + * tunnel down: it sets this process and persists the choice; + * 3. the process-global setter, when there is neither -- nothing records + * it, but the session at least dials under what the row shows. + * + * `networkSpace` comes from `DeviceManager.networkSpace` + * (`DeviceManager.swift:60`), which is an independent `@Published` + * property and is NOT derived from `device`. Android's is + * (`DeviceManager.kt:54` is `device?.networkSpace`), which is why that + * platform fetches the space from `NetworkSpaceManagerProvider` instead. + * The two reach the same three-way behaviour by different routes. + */ + @MainActor + func cycle(device: SdkDeviceRemote?, networkSpace: SdkNetworkSpace?) async { + guard !isApplying else { return } + let next = IpFamily.next(policy) + + // Held across the write AND the read-back as one unit. Clearing it + // between the two would leave the guard open while `refresh` is + // suspended on its detached read, and the main actor is free there: + // a second tap queued behind this one would run, compute + // `IpFamily.next(policy)` from the PRE-refresh policy this call has + // already superseded, write that same policy again, and be silently + // lost -- auto -> force4, tap again -> force4 instead of force6. + // + // `defer` rather than an assignment after the read-back so it clears + // on every exit path, including any early return added later: a stuck + // flag would wedge the row permanently, which is worse than the lost + // tap this guards against. + isApplying = true + defer { isApplying = false } + + await Task.detached(priority: .userInitiated) { + if let device { + device.setControlIpFamilyPolicy(next) + } else if let networkSpace { + networkSpace.setControlIpFamilyPolicy(next) + } else { + // no device and no space: set this process so the choice is at + // least in force for the session, even though nothing records it + SdkSetControlIpFamilyPolicy(next) + } + }.value + + await refresh(device: device) + } +} diff --git a/app/networkTests/IpFamilyStateTests.swift b/app/networkTests/IpFamilyStateTests.swift new file mode 100644 index 00000000..a792b184 --- /dev/null +++ b/app/networkTests/IpFamilyStateTests.swift @@ -0,0 +1,143 @@ +// +// IpFamilyStateTests.swift +// networkTests +// +// Covers the write path behind the "Control connections" row: that a tap is +// never silently lost to a second tap landing inside the read-back, and that +// the in-flight guard always reopens afterwards. +// +// Driven through the no-device / no-space fallback, which is the +// process-global setter -- so this needs neither a device, a tunnel, nor the +// simulator ui. That global is shared across the test process, hence +// `.serialized` and an explicit reset in every test. +// +// The rounds are not padding. Which suspensions actually hand the main actor +// over is the runtime's business, so a single pass can miss the read-back +// window and prove nothing; the test therefore requires that at least one +// round genuinely caught the cycle mid-flight. +// +// What is asserted is the GUARD, not the end state, because the end state +// cannot tell the two behaviours apart: a second tap that is correctly +// refused and a second tap that is accepted-but-lost both leave the policy on +// Force IPv4, since the lost one recomputes `next` from the same pre-refresh +// Automatic and writes Force IPv4 a second time. The observable difference is +// that the lost one was admitted at all -- an extra write, which with a +// device is a real rpc into the extension -- so that is what is measured +// here. (Checked: a test asserting only the final policy passes against the +// defect, so it is not written.) +// + +import Testing +import URnetworkSdk +@testable import URnetwork + +/// Set by the cycling task the instant `cycle` returns, so an observing loop +/// can tell "still in flight" from "finished" without a timeout. +@MainActor +private final class CycleCompletion { + var finished = false +} + +private let rounds = 8 + +@Suite(.serialized) +@MainActor +struct IpFamilyStateTests { + + /// A fresh state reading Automatic, with the process global it writes to + /// reset to match. + private func automaticState() async -> IpFamilyState { + SdkSetControlIpFamilyPolicy(IpFamily.auto) + let state = IpFamilyState() + await state.refresh(device: nil) + return state + } + + /** + * The in-flight guard must stay closed for the WHOLE of `cycle` -- the + * write and the read-back as one unit. + * + * `cycle` suspends twice, once on the write and once inside `refresh`, and + * both suspensions free the main actor. A second tap queued behind the + * first runs at exactly those points. If the guard has already reopened by + * the second one, that tap computes `IpFamily.next(policy)` from the + * pre-refresh policy the first tap already superseded, writes the same + * value back, and is lost: auto -> force4, tap again -> force4. + * + * So this observes `isApplying` from the main actor at every scheduling + * point the in-flight cycle yields -- the same points a queued tap would + * get -- and requires it true at all of them. + */ + @Test func inFlightGuardStaysClosedAcrossTheReadBack() async { + var roundsThatCaughtItInFlight = 0 + + for _ in 0..