From 93edc56c98615d618794efe8b0d7326cd3e199f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:24:08 +0000 Subject: [PATCH 1/2] Fail over to a different Mullvad/IVPN relay when the current one stays dead WgEgress already retries a broken tunnel with backoff, but every retry re-resolved and restarted against the same fixed [Peer] endpoint baked into the profile. If the relay server itself is down (not just a local network blip), that loop never recovers. After a few consecutive full-restart cycles keep failing to produce any traffic, WgEgress now asks a new provider-aware hook (WgRelayFailover) to move the active Mullvad/IVPN profile to a different relay, reusing the existing key material, before continuing the restart loop. Self-hosted or manually imported profiles are left untouched, since there is only one server to fail over to. Fixes #679 --- .../eu/faircode/netguard/ServiceSinkhole.java | 3 +- .../net/kollnig/missioncontrol/wg/WgEgress.kt | 80 +++++++++++++++++-- .../missioncontrol/wg/WgRelayFailover.java | 64 +++++++++++++++ 3 files changed, 141 insertions(+), 6 deletions(-) create mode 100644 app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index a449bcf8..13b470a5 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -1807,7 +1807,8 @@ private boolean startNative(final ParcelFileDescriptor vpn, List listAllow // It also handles the disable case internally — no need to gate. net.kollnig.missioncontrol.wg.WgEgress.INSTANCE.setRecoveryCallbacks( () -> ServiceSinkhole.reload("wireguard connectivity repair", ServiceSinkhole.this, false), - () -> showWireGuardErrorNotification(getString(R.string.msg_wg_recovery_failed))); + () -> showWireGuardErrorNotification(getString(R.string.msg_wg_recovery_failed)), + () -> net.kollnig.missioncontrol.wg.WgRelayFailover.attemptFailover(ServiceSinkhole.this)); jni_wireguard_required(prefs.getBoolean("wg_enabled", false) && !TextUtils.isEmpty(prefs.getString("wg_config", ""))); boolean wgOk = net.kollnig.missioncontrol.wg.WgEgress.INSTANCE.startOrUpdate( diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt b/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt index 3f0bb117..be1c01a4 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt @@ -42,6 +42,13 @@ object WgEgress { // indefinitely, each doing DNS + handshake + monitor polling under a wakelock. private const val RESTART_BACKOFF_BASE_MS = 20_000L private const val RESTART_BACKOFF_MAX_MS = 5 * 60_000L + // A dead relay server looks identical to any other broken-tunnel cause + // from in here (no rx, handshake stalls). After this many full-restart + // cycles against the SAME endpoint have all failed to recover, ask the + // provider-aware callback to move the active profile to a different + // relay before continuing the restart loop, instead of retrying the + // one dead server forever. + private const val RELAY_FAILOVER_AFTER_ATTEMPTS = 3 @Volatile private var tunnel: WgTunnel? = null // Monotonically identifies the tunnel instance published in [tunnel]. @@ -112,6 +119,16 @@ object WgEgress { @Volatile private var requestReloadCb: Runnable? = null @Volatile private var notifyBrokenCb: Runnable? = null + // Provider-aware hook: tries to move the active profile to a different + // relay server (same provider/account/country, reused key material) and + // returns whether it actually switched. Null, or a profile with no + // provider (self-hosted / manually imported single-server configs), means + // there is nothing to fail over to. + @Volatile private var regenerateEndpointCb: (() -> Boolean)? = null + @Volatile private var failoverInFlight: Boolean = false + private val failoverExecutor = java.util.concurrent.Executors.newSingleThreadExecutor { + Thread(it, "wg-failover").apply { isDaemon = true } + } // Guarded by monitorLock: started/stopped from the vpn handler thread, // the wg-rebind thread, and the dying monitor thread itself. Unsynchronized // access could leak a second polling thread (double prods, double restarts). @@ -134,9 +151,14 @@ object WgEgress { * [notifyBroken] surfaces a user-facing notification. Registered once * when the service starts the egress. */ - fun setRecoveryCallbacks(requestReload: Runnable, notifyBroken: Runnable) { + fun setRecoveryCallbacks( + requestReload: Runnable, + notifyBroken: Runnable, + regenerateEndpoint: (() -> Boolean)? = null + ) { requestReloadCb = requestReload notifyBrokenCb = notifyBroken + regenerateEndpointCb = regenerateEndpoint } fun addStateListener(l: Runnable) { listeners.add(l) } @@ -340,7 +362,6 @@ object WgEgress { private fun requestFullRestart(reason: String, notify: Boolean, expected: TunnelSnapshot) { val attempt: Int - val delay: Long synchronized(tunnelLifecycleLock) { // This check and installation of pending state must be atomic with // tunnel replacement. Otherwise a replacement can land between @@ -348,14 +369,24 @@ object WgEgress { if (!isCurrentLocked(expected)) return clearEndpointCache() attempt = restartAttempts++ - delay = if (attempt == 0) 0L - else minOf(RESTART_BACKOFF_MAX_MS, RESTART_BACKOFF_BASE_MS shl (attempt - 1).coerceAtMost(10)) forceRestartPending = true pendingRestartTunnel = expected.tunnel pendingRestartTunnelGeneration = expected.generation } - Log.w(TAG, "$reason; forcing restart (attempt=${attempt + 1}, delay=${delay}ms)") + Log.w(TAG, "$reason; forcing restart (attempt=${attempt + 1})") if (notify) scheduleRecoveryNotificationCheck() + + val regenerate = regenerateEndpointCb + if (attempt > 0 && attempt % RELAY_FAILOVER_AFTER_ATTEMPTS == 0 && regenerate != null) { + tryRelayFailover(regenerate, expected, attempt) + return + } + scheduleRestart(attempt) + } + + private fun scheduleRestart(attempt: Int) { + val delay = if (attempt == 0) 0L + else minOf(RESTART_BACKOFF_MAX_MS, RESTART_BACKOFF_BASE_MS shl (attempt - 1).coerceAtMost(10)) verifyHandler.removeCallbacks(pendingRestartRunnable) if (delay <= 0L) { restartScheduled = false @@ -366,6 +397,45 @@ object WgEgress { } } + /** + * The same relay has now failed to recover across several full-restart + * cycles — likely the server itself is down, not a local network blip. + * Runs the (network-bound) [regenerate] callback off-thread; on success + * it has already rewritten the active profile's config, so the queued + * restart picks up the new peer. On failure or timeout, falls back to + * the normal same-config backoff so we never restart less often than + * before this existed. + */ + private fun tryRelayFailover(regenerate: () -> Boolean, expected: TunnelSnapshot, attempt: Int) { + if (failoverInFlight) { + scheduleRestart(attempt) + return + } + failoverInFlight = true + try { + failoverExecutor.execute { + val switched = try { + regenerate() + } catch (e: Throwable) { + Log.w(TAG, "relay failover callback threw", e) + false + } finally { + failoverInFlight = false + } + if (!isCurrent(expected)) return@execute + if (switched) { + Log.w(TAG, "relay failover: switched the active profile to a different server") + scheduleRestart(0) + } else { + scheduleRestart(attempt) + } + } + } catch (e: java.util.concurrent.RejectedExecutionException) { + failoverInFlight = false + scheduleRestart(attempt) + } + } + private fun scheduleRecoveryNotificationCheck() { val gen = ++recoveryNotificationGeneration verifyHandler.postDelayed({ diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java b/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java new file mode 100644 index 00000000..14bd4ed7 --- /dev/null +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java @@ -0,0 +1,64 @@ +package net.kollnig.missioncontrol.wg; + +import android.content.Context; +import android.text.TextUtils; +import android.util.Log; + +/** + * Moves the active WireGuard profile to a different relay server for the + * same provider/account/country, reusing the existing key material, when + * {@link WgEgress} has given up trying to recover the current relay. + * + *

Mullvad and IVPN each expose many relay servers, but a profile bakes in + * one fixed {@code [Peer]} at creation time (see {@link + * MullvadProfileGenerator} / {@link IvpnProfileGenerator}). If that specific + * server goes down, re-resolving and restarting against the same endpoint + * forever never recovers — this picks a fresh relay instead. + */ +public class WgRelayFailover { + private static final String TAG = "TrackerControl.WgFailover"; + + private WgRelayFailover() { + } + + /** + * Performs blocking network I/O (a relay-list fetch); call off the main + * thread. Returns true only if the active profile's config was actually + * rewritten to a different server. + */ + public static boolean attemptFailover(Context context) { + WgProfileManager manager = new WgProfileManager(context); + WgProfileManager.Profile active = manager.getActiveProfile(); + if (active == null || TextUtils.isEmpty(active.provider) || TextUtils.isEmpty(active.config)) { + // Self-hosted or manually imported configs have a single, + // user-chosen server — there is nothing to fail over to. + return false; + } + + try { + String newConfig; + if ("mullvad".equals(active.provider)) { + MullvadProfileGenerator.GeneratedProfile generated = new MullvadProfileGenerator() + .generate(active.account, active.countryCode, active.config); + newConfig = generated.config; + } else if ("ivpn".equals(active.provider)) { + IvpnProfileGenerator.GeneratedProfile generated = new IvpnProfileGenerator() + .generate(active.account, active.countryCode, + manager.getIvpnSession(active.account)); + newConfig = generated.config; + } else { + return false; + } + + if (TextUtils.isEmpty(newConfig) || newConfig.equals(active.config)) + return false; + + manager.updateActiveProfileConfig(newConfig); + Log.w(TAG, "Switched " + active.provider + " profile to a different relay after repeated failures"); + return true; + } catch (Throwable ex) { + Log.w(TAG, "Relay failover for " + active.provider + " failed: " + ex.getMessage()); + return false; + } + } +} From c003087ceb3f2c99787c737c5d07dac972825711 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 08:15:10 +0000 Subject: [PATCH 2/2] Address review findings on relay failover (PR #688) - Bound the relay-list fetch with a real timeout (FAILOVER_TIMEOUT_MS): previously forceRestartPending blocked every other recovery path while the unbounded network call ran, so a hang could stall recovery longer than not having failover at all. - Make failoverInFlight an AtomicBoolean to close a check-then-set race between the monitor and rebind threads. - Only trigger failover from monitor-observed breaks, not from rebind failures after a network change (those aren't evidence the relay is dead). - Reset the restart backoff after a successful switch so a fresh relay doesn't inherit the old one's penalty. - Persist the IVPN session WgRelayFailover generates/reuses; it was minting sessions that never got saved, orphaning them against the account's device limit and desyncing stored session state from the live config. - Scope the config write to the profile ID resolved before the network call (WgProfileManager#updateProfileConfigIfActive), so a profile switch mid-fetch can't overwrite a different profile's stored config. - Exclude the relay that just failed from the next pick when another candidate exists, and reject a pick that silently widens the exit country, for both Mullvad and IVPN. - Distinguish ApiRejectedException/CaptchaRequiredException from other failures in the failover log. - Add WgRelayFailoverTest and extend WgProfileManagerTest for the new guard clauses. --- .../wg/IvpnProfileGenerator.java | 30 +++++++- .../wg/MullvadProfileGenerator.java | 27 ++++++- .../net/kollnig/missioncontrol/wg/WgEgress.kt | 74 ++++++++++++++----- .../missioncontrol/wg/WgProfileManager.java | 23 +++++- .../missioncontrol/wg/WgRelayFailover.java | 63 +++++++++++++++- .../wg/WgProfileManagerTest.java | 18 +++++ .../wg/WgRelayFailoverTest.java | 63 ++++++++++++++++ 7 files changed, 273 insertions(+), 25 deletions(-) create mode 100644 app/src/test/java/net/kollnig/missioncontrol/wg/WgRelayFailoverTest.java diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java b/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java index 5ef0526a..6dfec972 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java @@ -115,6 +115,19 @@ public GeneratedProfile generate(String accountNumber, String requestedCountryCo WgProfileManager.IvpnSession reusableSession, String captchaId, String captchaValue) throws Exception { + return generate(accountNumber, requestedCountryCode, reusableSession, captchaId, captchaValue, null); + } + + /** + * @param excludeHostname a relay hostname to avoid re-picking when another + * candidate is available in the chosen pool, e.g. a + * relay a caller just failed over away from. + */ + public GeneratedProfile generate(String accountNumber, String requestedCountryCode, + WgProfileManager.IvpnSession reusableSession, + String captchaId, String captchaValue, + String excludeHostname) + throws Exception { String account = accountNumber == null ? "" : accountNumber.trim(); if (account.isEmpty()) throw new IllegalArgumentException("IVPN account number is required"); @@ -126,7 +139,7 @@ public GeneratedProfile generate(String accountNumber, String requestedCountryCo session = createSession(account, privateKey, publicKey, captchaId, captchaValue); } - Relay relay = chooseRelay(fetchRelays(), requestedCountryCode); + Relay relay = chooseRelay(fetchRelays(), requestedCountryCode, excludeHostname); String config = buildConfig(session.privateKey, session.address, relay); return new GeneratedProfile("IVPN - " + relay.countryName, config, account, relay.countryCode, relay.countryName, relay.hostname, session); @@ -245,6 +258,10 @@ private List fetchRelays() throws Exception { } private Relay chooseRelay(List relays, String requestedCountryCode) { + return chooseRelay(relays, requestedCountryCode, null); + } + + private Relay chooseRelay(List relays, String requestedCountryCode, String excludeHostname) { if (relays.isEmpty()) throw new IllegalStateException("No IVPN WireGuard relays found"); @@ -254,6 +271,17 @@ private Relay chooseRelay(List relays, String requestedCountryCode) { if (candidates.isEmpty()) candidates = new ArrayList<>(relays); + if (!TextUtils.isEmpty(excludeHostname)) { + List withoutExcluded = new ArrayList<>(); + for (Relay relay : candidates) + if (!excludeHostname.equals(relay.hostname)) + withoutExcluded.add(relay); + // Only apply the exclusion if it leaves a choice — a single-relay + // country should still return that relay rather than throw. + if (!withoutExcluded.isEmpty()) + candidates = withoutExcluded; + } + Collections.sort(candidates, Comparator.comparingDouble(relay -> relay.load)); int bestPool = Math.min(3, candidates.size()); return candidates.get(random.nextInt(Math.max(1, bestPool))); diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java b/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java index 674351a1..48341f04 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java @@ -110,6 +110,16 @@ public GeneratedProfile generate(String accountNumber, String requestedCountryCo } public GeneratedProfile generate(String accountNumber, String requestedCountryCode, String reusableConfig) throws Exception { + return generate(accountNumber, requestedCountryCode, reusableConfig, null); + } + + /** + * @param excludeHostname a relay hostname to avoid re-picking when another + * candidate is available in the chosen pool, e.g. a + * relay a caller just failed over away from. + */ + public GeneratedProfile generate(String accountNumber, String requestedCountryCode, String reusableConfig, + String excludeHostname) throws Exception { String account = accountNumber == null ? "" : accountNumber.trim(); if (account.isEmpty()) throw new IllegalArgumentException("Mullvad account number is required"); @@ -126,7 +136,7 @@ public GeneratedProfile generate(String accountNumber, String requestedCountryCo privateKey = reusable.getPrivateKey(); device = deviceFromConfig(reusable); } - Relay relay = chooseRelay(fetchRelays(), requestedCountryCode); + Relay relay = chooseRelay(fetchRelays(), requestedCountryCode, excludeHostname); String config = buildConfig(privateKey, device, relay); return new GeneratedProfile("Mullvad - " + relay.countryName, config, account, @@ -278,6 +288,10 @@ private List fetchRelays() throws Exception { } private Relay chooseRelay(List relays, String requestedCountryCode) { + return chooseRelay(relays, requestedCountryCode, null); + } + + private Relay chooseRelay(List relays, String requestedCountryCode, String excludeHostname) { if (relays.isEmpty()) throw new IllegalStateException("No active Mullvad WireGuard relays found"); @@ -287,6 +301,17 @@ private Relay chooseRelay(List relays, String requestedCountryCode) { if (candidates.isEmpty()) candidates = new ArrayList<>(relays); + if (!TextUtils.isEmpty(excludeHostname)) { + List withoutExcluded = new ArrayList<>(); + for (Relay relay : candidates) + if (!excludeHostname.equals(relay.hostname)) + withoutExcluded.add(relay); + // Only apply the exclusion if it leaves a choice — a single-relay + // country should still return that relay rather than throw. + if (!withoutExcluded.isEmpty()) + candidates = withoutExcluded; + } + List stboot = new ArrayList<>(); for (Relay relay : candidates) if (relay.stboot) diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt b/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt index be1c01a4..d6ce48dc 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/WgEgress.kt @@ -44,11 +44,18 @@ object WgEgress { private const val RESTART_BACKOFF_MAX_MS = 5 * 60_000L // A dead relay server looks identical to any other broken-tunnel cause // from in here (no rx, handshake stalls). After this many full-restart - // cycles against the SAME endpoint have all failed to recover, ask the - // provider-aware callback to move the active profile to a different - // relay before continuing the restart loop, instead of retrying the - // one dead server forever. + // cycles against the SAME endpoint have all failed to recover, and on + // every further multiple of it, ask the provider-aware callback to move + // the active profile to a different relay before continuing the restart + // loop, instead of retrying the one dead server forever. private const val RELAY_FAILOVER_AFTER_ATTEMPTS = 3 + // Bounds the relay-list fetch + config rewrite so a stalled network call + // can never withhold a restart for longer than the caller would have + // waited anyway. forceRestartPending is already set by the time this + // runs, which makes onMonitorBroken/onUnderlyingNetworkChanged no-op + // until a restart is scheduled — without a bound, a hung call would + // silently stall recovery instead of merely skipping the relay switch. + private const val FAILOVER_TIMEOUT_MS = 15_000L @Volatile private var tunnel: WgTunnel? = null // Monotonically identifies the tunnel instance published in [tunnel]. @@ -125,7 +132,7 @@ object WgEgress { // provider (self-hosted / manually imported single-server configs), means // there is nothing to fail over to. @Volatile private var regenerateEndpointCb: (() -> Boolean)? = null - @Volatile private var failoverInFlight: Boolean = false + private val failoverInFlight = java.util.concurrent.atomic.AtomicBoolean(false) private val failoverExecutor = java.util.concurrent.Executors.newSingleThreadExecutor { Thread(it, "wg-failover").apply { isDaemon = true } } @@ -356,11 +363,19 @@ object WgEgress { requestFullRestart( "connectivity monitor: tunnel still broken after cheap recovery", notify = true, - expected = expected + expected = expected, + // Only a monitor-observed break (no rx/handshake) is evidence the + // relay itself is unreachable; a rebind failure below is not. + eligibleForFailover = true ) } - private fun requestFullRestart(reason: String, notify: Boolean, expected: TunnelSnapshot) { + private fun requestFullRestart( + reason: String, + notify: Boolean, + expected: TunnelSnapshot, + eligibleForFailover: Boolean + ) { val attempt: Int synchronized(tunnelLifecycleLock) { // This check and installation of pending state must be atomic with @@ -377,7 +392,7 @@ object WgEgress { if (notify) scheduleRecoveryNotificationCheck() val regenerate = regenerateEndpointCb - if (attempt > 0 && attempt % RELAY_FAILOVER_AFTER_ATTEMPTS == 0 && regenerate != null) { + if (eligibleForFailover && attempt > 0 && attempt % RELAY_FAILOVER_AFTER_ATTEMPTS == 0 && regenerate != null) { tryRelayFailover(regenerate, expected, attempt) return } @@ -402,16 +417,32 @@ object WgEgress { * cycles — likely the server itself is down, not a local network blip. * Runs the (network-bound) [regenerate] callback off-thread; on success * it has already rewritten the active profile's config, so the queued - * restart picks up the new peer. On failure or timeout, falls back to - * the normal same-config backoff so we never restart less often than - * before this existed. + * restart picks up the new peer and the backoff resets, since the new + * relay hasn't earned any of the old one's penalty. On failure or + * [FAILOVER_TIMEOUT_MS] timeout, falls back to the normal same-config + * backoff so we never restart less often than before this existed. + * + * [resolved] guards against the timeout and the completed callback both + * trying to schedule a restart for the same attempt: whichever of the + * two runs first (main thread for the timeout, [failoverExecutor] for + * the callback) wins the race, the other is a no-op. */ private fun tryRelayFailover(regenerate: () -> Boolean, expected: TunnelSnapshot, attempt: Int) { - if (failoverInFlight) { + if (!failoverInFlight.compareAndSet(false, true)) { scheduleRestart(attempt) return } - failoverInFlight = true + val resolved = java.util.concurrent.atomic.AtomicBoolean(false) + val timeoutRunnable = Runnable { + if (resolved.compareAndSet(false, true)) { + failoverInFlight.set(false) + if (isCurrent(expected)) { + Log.w(TAG, "relay failover timed out after ${FAILOVER_TIMEOUT_MS}ms; falling back to backoff") + scheduleRestart(attempt) + } + } + } + verifyHandler.postDelayed(timeoutRunnable, FAILOVER_TIMEOUT_MS) try { failoverExecutor.execute { val switched = try { @@ -420,19 +451,23 @@ object WgEgress { Log.w(TAG, "relay failover callback threw", e) false } finally { - failoverInFlight = false + failoverInFlight.set(false) } + verifyHandler.removeCallbacks(timeoutRunnable) + if (!resolved.compareAndSet(false, true)) return@execute if (!isCurrent(expected)) return@execute if (switched) { Log.w(TAG, "relay failover: switched the active profile to a different server") + restartAttempts = 0 scheduleRestart(0) } else { scheduleRestart(attempt) } } } catch (e: java.util.concurrent.RejectedExecutionException) { - failoverInFlight = false - scheduleRestart(attempt) + verifyHandler.removeCallbacks(timeoutRunnable) + failoverInFlight.set(false) + if (resolved.compareAndSet(false, true)) scheduleRestart(attempt) } } @@ -587,7 +622,12 @@ object WgEgress { requestFullRestart( "WG rebind failed after network change", notify = false, - expected = expected + expected = expected, + // A rebind failure means the local network + // changed under us, not that the relay is + // dead — don't let it count toward + // switching relays. + eligibleForFailover = false ) } } diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/WgProfileManager.java b/app/src/main/java/net/kollnig/missioncontrol/wg/WgProfileManager.java index d41ef663..e19db66e 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/WgProfileManager.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/WgProfileManager.java @@ -229,9 +229,27 @@ public void updateActiveProfileConfig(String config) { String active = getActiveProfileId(); if (TextUtils.isEmpty(active)) return; + updateProfileConfig(active, config); + } + + /** + * Like {@link #updateActiveProfileConfig}, but only writes if {@code profileId} + * is still the active profile. For callers (e.g. background relay failover) + * that resolved which profile to update before doing slow work: without this + * check, a profile switch that lands during that work would silently write + * the stale profile's regenerated config over whatever the user switched to. + * Returns whether the write happened. + */ + public boolean updateProfileConfigIfActive(String profileId, String config) { + if (TextUtils.isEmpty(profileId) || !profileId.equals(getActiveProfileId())) + return false; + updateProfileConfig(profileId, config); + return true; + } + private void updateProfileConfig(String profileId, String config) { JSONArray profiles = readProfilesJson(); - JSONObject profile = findJsonProfile(profiles, active); + JSONObject profile = findJsonProfile(profiles, profileId); if (profile == null) return; @@ -239,7 +257,8 @@ public void updateActiveProfileConfig(String config) { profile.put("config", config == null ? "" : config); SharedPreferences.Editor editor = prefs.edit(); writeProfilesJson(editor, profiles); - editor.putString(PREF_WG_CONFIG, config == null ? "" : config); + if (profileId.equals(getActiveProfileId())) + editor.putString(PREF_WG_CONFIG, config == null ? "" : config); editor.apply(); } catch (JSONException ex) { Log.w(TAG, "Update WireGuard profile failed: " + ex.getMessage()); diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java b/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java index 14bd4ed7..da59f3ed 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/WgRelayFailover.java @@ -4,6 +4,9 @@ import android.text.TextUtils; import android.util.Log; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + /** * Moves the active WireGuard profile to a different relay server for the * same provider/account/country, reusing the existing key material, when @@ -18,6 +21,16 @@ public class WgRelayFailover { private static final String TAG = "TrackerControl.WgFailover"; + // Both generators stamp the chosen relay's hostname into the generated + // config as a comment (see MullvadProfileGenerator#buildConfig / + // IvpnProfileGenerator#buildConfig) — that comment is the only place the + // hostname survives once the config is saved, so it's how a repeat + // failover recognizes and excludes the relay that just failed. + private static final Pattern MULLVAD_RELAY_PATTERN = + Pattern.compile("(?m)^#\\s*Mullvad relay\\s*=\\s*(\\S+)"); + private static final Pattern IVPN_RELAY_PATTERN = + Pattern.compile("(?m)^#\\s*IVPN relay\\s*=\\s*(\\S+)"); + private WgRelayFailover() { } @@ -37,15 +50,26 @@ public static boolean attemptFailover(Context context) { try { String newConfig; + String newCountryCode; if ("mullvad".equals(active.provider)) { + String excludeHostname = currentRelayHostname(active.config, MULLVAD_RELAY_PATTERN); MullvadProfileGenerator.GeneratedProfile generated = new MullvadProfileGenerator() - .generate(active.account, active.countryCode, active.config); + .generate(active.account, active.countryCode, active.config, excludeHostname); newConfig = generated.config; + newCountryCode = generated.countryCode; } else if ("ivpn".equals(active.provider)) { + String excludeHostname = currentRelayHostname(active.config, IVPN_RELAY_PATTERN); + WgProfileManager.IvpnSession session = manager.getIvpnSession(active.account); IvpnProfileGenerator.GeneratedProfile generated = new IvpnProfileGenerator() - .generate(active.account, active.countryCode, - manager.getIvpnSession(active.account)); + .generate(active.account, active.countryCode, session, "", "", excludeHostname); + // generate() may have had to mint a fresh session (no reusable + // one, or the reusable one was stale); persist it regardless of + // whether the switch below goes through, or the device/session + // this config now authenticates as is lost and never saved. + if (generated.session != null) + manager.saveIvpnSession(generated.session); newConfig = generated.config; + newCountryCode = generated.countryCode; } else { return false; } @@ -53,12 +77,43 @@ public static boolean attemptFailover(Context context) { if (TextUtils.isEmpty(newConfig) || newConfig.equals(active.config)) return false; - manager.updateActiveProfileConfig(newConfig); + // Both generators try the profile's own country first and only + // widen the pool if that country currently has no relays. A + // mismatch here means a widened pick — surfacing that silently as + // a "still connected" recovery would relocate the user's exit + // country without telling them, which defeats the point of + // choosing a country in the first place. + if (!TextUtils.isEmpty(active.countryCode) && !active.countryCode.equals(newCountryCode)) { + Log.w(TAG, "Relay failover for " + active.provider + " found no relay left in " + + active.countryCode + "; not silently switching country to " + newCountryCode); + return false; + } + + if (!manager.updateProfileConfigIfActive(active.id, newConfig)) { + Log.w(TAG, "Relay failover for " + active.provider + + " found a new relay, but the active profile changed meanwhile; discarding"); + return false; + } Log.w(TAG, "Switched " + active.provider + " profile to a different relay after repeated failures"); return true; + } catch (MullvadProfileGenerator.ApiRejectedException | IvpnProfileGenerator.ApiRejectedException ex) { + Log.w(TAG, "Relay failover for " + active.provider + " was rejected by the provider API: " + + ex.getMessage()); + return false; + } catch (IvpnProfileGenerator.CaptchaRequiredException ex) { + Log.w(TAG, "Relay failover for " + active.provider + + " requires solving a captcha; cannot proceed automatically"); + return false; } catch (Throwable ex) { Log.w(TAG, "Relay failover for " + active.provider + " failed: " + ex.getMessage()); return false; } } + + private static String currentRelayHostname(String config, Pattern pattern) { + if (TextUtils.isEmpty(config)) + return null; + Matcher matcher = pattern.matcher(config); + return matcher.find() ? matcher.group(1) : null; + } } diff --git a/app/src/test/java/net/kollnig/missioncontrol/wg/WgProfileManagerTest.java b/app/src/test/java/net/kollnig/missioncontrol/wg/WgProfileManagerTest.java index 85addaa5..86ad533f 100644 --- a/app/src/test/java/net/kollnig/missioncontrol/wg/WgProfileManagerTest.java +++ b/app/src/test/java/net/kollnig/missioncontrol/wg/WgProfileManagerTest.java @@ -90,6 +90,24 @@ public void saveUpdateAndSelectProfilesKeepLegacyConfigInSync() throws Exception assertEquals(second, manager.getActiveProfileId()); } + @Test + public void updateProfileConfigIfActiveOnlyWritesWhenStillActive() throws Exception { + manager.saveProfile("", "One", "config-1"); + String first = manager.getActiveProfileId(); + manager.saveProfile("", "Two", "config-2"); + String second = manager.getActiveProfileId(); + + assertTrue(manager.updateProfileConfigIfActive(second, "config-2b")); + assertEquals("config-2b", manager.getProfile(second).config); + assertEquals("config-2b", prefs.getString(WgProfileManager.PREF_WG_CONFIG, "")); + + // "first" is no longer active: the write must not clobber the live + // PREF_WG_CONFIG pointer, which now belongs to "second". + assertFalse(manager.updateProfileConfigIfActive(first, "stale-write")); + assertEquals("config-1", manager.getProfile(first).config); + assertEquals("config-2b", prefs.getString(WgProfileManager.PREF_WG_CONFIG, "")); + } + @Test public void deletingInactiveProfilePreservesActiveSelection() throws Exception { manager.saveProfile("", "One", "config-1"); diff --git a/app/src/test/java/net/kollnig/missioncontrol/wg/WgRelayFailoverTest.java b/app/src/test/java/net/kollnig/missioncontrol/wg/WgRelayFailoverTest.java new file mode 100644 index 00000000..b31212c8 --- /dev/null +++ b/app/src/test/java/net/kollnig/missioncontrol/wg/WgRelayFailoverTest.java @@ -0,0 +1,63 @@ +package net.kollnig.missioncontrol.wg; + +import static org.junit.Assert.assertFalse; + +import android.content.Context; +import android.content.SharedPreferences; + +import androidx.preference.PreferenceManager; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; +import org.robolectric.annotation.Config; + +/** + * Covers the guard clauses in {@link WgRelayFailover#attemptFailover} that + * don't require network access: no active profile, no provider (self-hosted + * or manually imported config), and an unrecognized provider string. The + * provider-specific success paths need a live relay-list fetch and aren't + * covered here. + */ +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 36, qualifiers = "en") +public class WgRelayFailoverTest { + private Context context; + private WgProfileManager manager; + + @Before + public void setUp() { + context = RuntimeEnvironment.getApplication(); + SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); + prefs.edit().clear().commit(); + manager = new WgProfileManager(context); + } + + @Test + public void noopWhenNoActiveProfile() { + assertFalse(WgRelayFailover.attemptFailover(context)); + } + + @Test + public void noopForSelfHostedProfileWithoutProvider() throws Exception { + manager.saveProfile("", "Home server", "[Interface]\nPrivateKey = k\n[Peer]\nPublicKey = p\n"); + + assertFalse(WgRelayFailover.attemptFailover(context)); + } + + @Test + public void noopForUnrecognizedProvider() throws Exception { + manager.saveProfile("", "Other", "config", "wireguard-other", "account"); + + assertFalse(WgRelayFailover.attemptFailover(context)); + } + + @Test + public void noopWhenActiveConfigIsEmpty() throws Exception { + manager.saveProfile("", "Mullvad", "", "mullvad", "account"); + + assertFalse(WgRelayFailover.attemptFailover(context)); + } +}