[fix] Trigger engine restart on any network type change, not just WiFi#165
[fix] Trigger engine restart on any network type change, not just WiFi#165MichaelUray wants to merge 4 commits intonetbirdio:mainfrom
Conversation
The default was set to true in netbirdio#108 as a workaround for P2P stability issues on Android. Now that the underlying ICE issues are fixed (netbirdio/netbird: guard loop, candidate buffering, anet for Android), P2P connections work reliably and should be enabled by default.
The previous implementation only notified the listener in two cases: - onNetworkAvailable: only when WiFi became available - onNetworkLost: only when WiFi was lost AND mobile was available This missed the case where WiFi is disabled and mobile data takes over as the active connection. The onAvailable(MOBILE) callback was ignored, and if mobile wasn't yet registered when onLost(WIFI) fired, the notification was skipped entirely. This left the Go engine with stale NetworkAddresses (containing the old WiFi subnet), causing posture checks like peer_network_range_check to incorrectly block the peer. The fix notifies the listener on any network type transition: - onNetworkAvailable: notify when a new network type appears - onNetworkLost: notify when a network is lost but others remain This ensures the engine restarts and sends fresh NetworkAddresses to the management server after every WiFi ↔ Mobile switch.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo logic updates: a default preference value in the preferences utility inverted from true to false, and network availability listener notifications refactored to trigger on any network type availability changes rather than WiFi-specific transitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tool/src/main/java/io/netbird/client/tool/networks/ConcreteNetworkAvailabilityListener.java`:
- Around line 34-37: In ConcreteNetworkAvailabilityListener::onNetworkLost,
avoid notifying on spurious/duplicate out-of-order losses by first checking
whether the lost network type was actually tracked/contained/removed from
availableNetworkTypes; only call notifyListener() when the removal changed state
(e.g., removal returned true or the set previously contained the type) and
availableNetworkTypes is non-empty, otherwise do nothing. This ensures
onNetworkLost only triggers a notification on real state changes rather than
duplicate events.
In `@tool/src/main/java/io/netbird/client/tool/Preferences.java`:
- Around line 29-31: The change to isConnectionForceRelayed() silently flips
unset users to P2P; restore explicit migration by detecting when
keyForceRelayConnection is not present and applying an intentional rollout
decision: update isConnectionForceRelayed() (and add a migration helper called
from Preferences init) to use sharedPref.contains(keyForceRelayConnection) — if
absent, set the preference to the old default value (or to a controlled rollout
value) and persist it before returning; reference keyForceRelayConnection,
isConnectionForceRelayed(), and ensure callers EnvVarPackager and
AdvancedFragment keep observing the migrated/stored boolean so the behavior is
explicit and tests can assert the migration path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8573a86-14e9-4f45-ac2e-f9f1ed54eb42
📒 Files selected for processing (3)
netbirdtool/src/main/java/io/netbird/client/tool/Preferences.javatool/src/main/java/io/netbird/client/tool/networks/ConcreteNetworkAvailabilityListener.java
| public boolean isConnectionForceRelayed() { | ||
| return sharedPref.getBoolean(keyForceRelayConnection, true); | ||
| return sharedPref.getBoolean(keyForceRelayConnection, false); | ||
| } |
There was a problem hiding this comment.
Defaulting to false here is an upgrade behavior break for unset prefs.
Line 30 now flips existing “never-set” users from forced relay to P2P because callers in EnvVarPackager and AdvancedFragment consume this value directly. Please add an explicit migration/rollout decision (and align tests) so this change is intentional and observable rather than silent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tool/src/main/java/io/netbird/client/tool/Preferences.java` around lines 29 -
31, The change to isConnectionForceRelayed() silently flips unset users to P2P;
restore explicit migration by detecting when keyForceRelayConnection is not
present and applying an intentional rollout decision: update
isConnectionForceRelayed() (and add a migration helper called from Preferences
init) to use sharedPref.contains(keyForceRelayConnection) — if absent, set the
preference to the old default value (or to a controlled rollout value) and
persist it before returning; reference keyForceRelayConnection,
isConnectionForceRelayed(), and ensure callers EnvVarPackager and
AdvancedFragment keep observing the migrated/stored boolean so the behavior is
explicit and tests can assert the migration path.
There was a problem hiding this comment.
The ForceRelay default change is from a separate PR (#152) and not part of this fix. It appears in the diff because this branch includes that commit. The scope of this PR is only the ConcreteNetworkAvailabilityListener change.
There was a problem hiding this comment.
@MichaelUray, understood — thank you for the context. Since the isConnectionForceRelayed() default flip to false was deliberately introduced in #152 and is merely included here due to branching, my concern doesn't apply to this PR. I'll disregard this comment for the current review.
✏️ Learnings added
Learnt from: MichaelUray
Repo: netbirdio/android-client PR: 165
File: tool/src/main/java/io/netbird/client/tool/Preferences.java:29-31
Timestamp: 2026-04-16T18:57:38.014Z
Learning: In `tool/src/main/java/io/netbird/client/tool/Preferences.java` (netbirdio/android-client), `isConnectionForceRelayed()` intentionally returns `false` as the default (i.e., `sharedPref.getBoolean(keyForceRelayConnection, false)`). This default was changed from `true` to `false` in PR `#152` to re-enable P2P by default now that underlying ICE fixes are in place. Do not flag this default as a silent behavior break or missing migration — it is an intentional rollout decision made in that PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: pappz
Repo: netbirdio/android-client PR: 163
File: app/src/main/res/layout/fragment_troubleshoot.xml:76-80
Timestamp: 2026-04-14T17:59:56.801Z
Learning: In `app/src/main/res/layout/fragment_troubleshoot.xml` (netbirdio/android-client), the `switch_anonymize` SwitchMaterial (`@+id/switch_anonymize`) intentionally defaults to unchecked (disabled). This is the expected UX design: users must explicitly opt-in to anonymizing sensitive data before uploading a debug bundle. Do not flag the unchecked default as a security issue.
Only notify the listener when the lost network type was actually tracked, preventing avoidable engine restart churn from spurious or duplicate onLost events.
Resolves submodule conflict by using current upstream/main netbird commit. The previous submodule pointer (3e020e77) was from an older version of PR #5807 which has since been rebased.
1 participant
Summary
Fixes the
ConcreteNetworkAvailabilityListenerto trigger an engine restart on any network type change, not just WiFi transitions.Problem
The previous implementation only notified the
EngineRestarterin two cases:onNetworkAvailable: only when WiFi became availableonNetworkLost: only when WiFi was lost AND mobile was already registeredThis missed a critical timing scenario during WiFi → Mobile switches:
onLost(WIFI)fires, butisMobileAvailableis stillfalse(mobile not yet registered) → no notificationonAvailable(MOBILE)fires, but the code only checks for WiFi → no notificationNetworkAddresses(containing the old WiFi subnet like192.168.91.0/24) remain on the management serverpeer_network_range_checkincorrectly block the peer because they see the old WiFi subnetFix
Notify the listener on any network type transition:
onNetworkAvailable: notify when a new (previously unavailable) network type appearsonNetworkLost: notify when a network is lost but at least one other network remainsThis ensures the engine restarts and sends fresh
NetworkAddressesto the management server after every WiFi ↔ Mobile switch.Testing
Tested on Samsung Galaxy S26 (SM-S942B, Android 16):
192.168.91.0/24VPN route (device is local)192.168.91.254reachable via NetBird VPN ✓Context
Thanks to @pappz for the review feedback on PR netbirdio/netbird#5807 — your suggestion to investigate whether the existing reconnection flow already covers network changes led me to dig deeper into the Android-side network monitoring. The Go-side
NetworkMonitoris a no-op on Android (monitor_mobile.goreturns an error immediately), so all network change detection relies on the Java-sideConnectivityManager.NetworkCallback. The root cause turned out to be this listener's selective notification logic rather than a missing watcher on the Go side.Related:
Summary by CodeRabbit