Conversation
Networks() called GetFullStatus() once per network to find the peer that serves it, copying every peer state and taking eight recorder locks each time. With 100+ peers and the UI calling Networks() from every peer list change, this queued hundreds of callers on the status recorder lock. Take one snapshot per call and index it by route.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Android client now builds a route-owner map from resolved peer states and uses it during network construction. Route ownership tests cover multiple routes, idle peers, missing owners, and duplicate routes. ChangesRoute owner lookup
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The route-owner tests run successfully, and the new lookup preserves existing network peer selection behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the performance change and its scope, but it omits the required issue ticket or approved discussion link for a behavior change. It also leaves the test and local-validation checklist items unchecked even though tests were added, and does not explain why documentation is not needed.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/android/route_owners.go">
<violation number="1" location="client/android/route_owners.go:7">
P2: routeOwners picks whichever peer appears first in the states slice, but GetPeerStates builds that slice from map iteration (d.peers), so the order is nondeterministic. For a route served by an HA group, the chosen owner can be a random, disconnected sibling, and findBestRoutePeer returns that state without checking ConnStatus, so the UI can show a stale Disconnected status for a network a connected peer actively serves. Prefer a connected peer (StatusConnected) when several peers advertise the same route, or at least document that selection is arbitrary.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "github.com/netbirdio/netbird/client/internal/peer" | ||
| ) | ||
|
|
||
| func routeOwners(states []peer.State) map[string]peer.State { |
There was a problem hiding this comment.
P2: routeOwners picks whichever peer appears first in the states slice, but GetPeerStates builds that slice from map iteration (d.peers), so the order is nondeterministic. For a route served by an HA group, the chosen owner can be a random, disconnected sibling, and findBestRoutePeer returns that state without checking ConnStatus, so the UI can show a stale Disconnected status for a network a connected peer actively serves. Prefer a connected peer (StatusConnected) when several peers advertise the same route, or at least document that selection is arbitrary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/android/route_owners.go, line 7:
<comment>routeOwners picks whichever peer appears first in the states slice, but GetPeerStates builds that slice from map iteration (d.peers), so the order is nondeterministic. For a route served by an HA group, the chosen owner can be a random, disconnected sibling, and findBestRoutePeer returns that state without checking ConnStatus, so the UI can show a stale Disconnected status for a network a connected peer actively serves. Prefer a connected peer (StatusConnected) when several peers advertise the same route, or at least document that selection is arbitrary.</comment>
<file context>
@@ -0,0 +1,17 @@
+ "github.com/netbirdio/netbird/client/internal/peer"
+)
+
+func routeOwners(states []peer.State) map[string]peer.State {
+ owners := make(map[string]peer.State)
+ for _, state := range states {
</file context>
There was a problem hiding this comment.
A route is added to a peer state only by the route watcher, for the currently chosen route peer (routemanager/client/client.go addAllowedIPs), and removed when the choice changes. So at most one peer carries a given route key at a time; HA siblings that are not selected do not have it. The first-wins branch is only a guard. Same map order as the old GetFullStatus() scan, so behavior is unchanged.
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
1 participant
Describe your changes
The Android app asks the engine for the list of networks with
Networks().For every network in that list, the engine called
GetFullStatus()to findthe peer that serves the route. One
GetFullStatus()call takes eight lockson the status recorder and copies the state of every peer. With N networks
this ran N times per
Networks()call.This is cheap with a few peers. With 100+ peers it is not: an Android ANR
report showed 283 threads waiting inside
Networks()at the same time, allqueued on the status recorder lock, and the UI thread stuck behind them.
The peer list callback storm that starts those calls is fixed separately in
#7546; this PR removes the cost per call.
Now
Networks()takes one snapshot of the peer states at the start, builds aroute -> peer map from it, and looks up each network in that map. The two
cheap
GetPeerfallbacks stay as they were, so the result does not change.Only the Android binding has this pattern, the iOS SDK already builds its
route status from a single snapshot.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Tests