Address review findings on relay failover (PR #688) - #692
Closed
kasnder wants to merge 2 commits into
Closed
Conversation
…s 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
- 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.
Member
Author
|
Closing — this was opened as a standalone follow-up before realizing the fix commit should just land on #688's own branch ( Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds on #688 ("Fail over to a different Mullvad/IVPN relay when the current one stays dead") with a follow-up commit addressing review findings on that change:
FAILOVER_TIMEOUT_MS): previouslyforceRestartPendingblocked every other recovery path while the unbounded network call ran, so a hang could stall recovery longer than not having failover at all.failoverInFlightanAtomicBooleanto close a check-then-set race between the monitor and rebind threads.WgRelayFailovergenerates/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.WgProfileManager#updateProfileConfigIfActive), so a profile switch mid-fetch can't overwrite a different profile's stored config.ApiRejectedException/CaptchaRequiredExceptionfrom other failures in the failover log.WgRelayFailoverTestand extendedWgProfileManagerTestfor the new guard clauses.Test plan
No Android SDK is available in this sandbox, so the change could not be compiled/tested locally; please run CI.
./gradlew :app:compileGithubDebugJavaWithJavac./gradlew :app:testGithubDebugUnitTestGenerated by Claude Code