[management] Increment newtork serial on peer update

[bcmmbaga]

Describe your changes

Issue ticket number and link

Stack

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

• I added/updated documentation for this change
• Documentation is not needed for this change (explain why)

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

• Refactor
  • Improved network state consistency when peer configurations are updated.
  • Refined peer expiration handling logic for more predictable system behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes modify network serial increment timing in peer and user update flows. In peer.go, IncrementNetworkSerial is now called during peer updates within a transaction. In user.go, the network serial increment is conditionally executed only when no expired peers require processing, altering the sequencing of state updates.

Changes

Cohort / File(s)	Summary
Network Serial Increment in Peer Updates management/server/peer.go	Added transaction.IncrementNetworkSerial(ctx, accountID) call in UpdatePeer after processing DNS label, SSH, and expiration flag changes; error wrapped with "failed to increment network serial: %w"; executes within the same transaction as peer save operation
Conditional Network State Updates management/server/user.go	Modified conditional in SaveOrAddUsers from unconditional execution to else-if, causing IncrementNetworkSerial and UpdateAccountPeers to run only when len(peersToExpire) == 0; changes control flow to skip these operations when expired peers are processed

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Nmap/fix concurrency #5040: Modifies the same network-serial update and peer-update control flow with overlapping call sites for IncrementNetworkSerial and UpdateAccountPeers.
• Feature/ssh fine grained access nmap #4987: Touches the same files and alters peer update/save control flow with changes to how peer-update behavior is orchestrated.

Suggested reviewers

• pascal-fischer
• pappz

Poem

🐰 Serial increments now dance with care,
In transactions, peer updates pair,
When expired peers take their bow,
The serial waits—conditional, now!
State and state in sync, so fair! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description lacks detail about the specific changes. The 'Describe your changes' section is empty, and no issue ticket link or explanation for why documentation is not needed are provided.	Fill in the 'Describe your changes' section with details about the network serial increment logic and why it prevents double peer updates. Add the issue ticket number and link, and explain why documentation is not needed.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title mentions incrementing network serial on peer update, which aligns with the main change of incrementing network serial during peer updates in management/server/peer.go.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @management/server/peer.go:
- Around line 272-274: UpdatePeer currently always calls
transaction.IncrementNetworkSerial(ctx, accountID) which causes a network serial
bump on every update; change this to only increment when actual peer data
changed by guarding the call with a condition that checks the existing change
flags (peerLabelChanged, sshChanged, loginExpirationChanged,
inactivityExpirationChanged). Locate the IncrementNetworkSerial invocation in
UpdatePeer and wrap it so it runs only if at least one of those boolean flags is
true (or add a combined “anyChanged” variable computed from them) so network map
updates only occur on real changes.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d35b7d6 and 34b12e0.

📒 Files selected for processing (2)

• management/server/peer.go
• management/server/user.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-11-28T12:20:47.254Z

Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.

Applied to files:

• management/server/peer.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Client / Unit
• GitHub Check: release

🔇 Additional comments (1)

management/server/user.go (1)

580-585: LGTM! Correctly prevents double network serial increment.

The conditional logic now ensures that when peers are expired, expireAndUpdatePeers handles both the network serial increment (line 996) and peer updates. When no peers need expiring, the serial is incremented here and UpdateAccountPeers is called. This prevents the double increment and double peer update that would have occurred previously when both conditions were true.