[management] fix ephemeral peers hadn't been added to ephemeral list on login #5165
[management] fix ephemeral peers hadn't been added to ephemeral list on login #5165
Conversation
📝 WalkthroughWalkthroughIntroduces ephemeral peer tracking by adding a new Changes
Sequence DiagramsequenceDiagram
participant Client as Server/Client
participant PM as PeerManager
participant Ctrl as Network Map Controller
participant EPM as EphemeralPeersManager
Client->>PM: AddPeer(ctx, accountID, setupKey, userID, peer, temporary=true)
PM->>PM: Add peer to All group
alt temporary flag is true
PM->>Ctrl: TrackEphemeralPeer(ctx, peer)
Ctrl->>EPM: OnPeerDisconnected(ctx, peer)
EPM->>EPM: Register peer for cleanup tracking
end
PM-->>Client: Return peer, networkMap, posture checks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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
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 |
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where ephemeral peers weren't being added to the ephemeral tracking list when they were first created during login/registration. Without this fix, ephemeral peers that were created but never connected wouldn't be scheduled for automatic cleanup.
Changes:
- Added tracking of ephemeral peers immediately upon creation in
AddPeermethod - Introduced new
TrackEphemeralPeermethod to the network map controller interface to handle ephemeral peer tracking - Updated mock interface to include the new method
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| management/server/peer.go | Added call to track ephemeral peers when they are created, ensuring they are scheduled for cleanup if never connected |
| management/internals/controllers/network_map/interface.go | Added new TrackEphemeralPeer method to the Controller interface |
| management/internals/controllers/network_map/controller/controller.go | Implemented TrackEphemeralPeer method that delegates to the ephemeral manager |
| management/internals/controllers/network_map/interface_mock.go | Updated generated mock to include the new interface method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *Controller) DisconnectPeers(ctx context.Context, accountId string, peerIDs []string) { | ||
| c.peersUpdateManager.CloseChannels(ctx, peerIDs) | ||
| } | ||
|
|
There was a problem hiding this comment.
The method TrackEphemeralPeer is calling OnPeerDisconnected, which is semantically misleading. The intent here is to track a newly created ephemeral peer that hasn't connected yet, but calling OnPeerDisconnected suggests the peer was previously connected and then disconnected. While the functionality works correctly (adding the peer to the tracking list for cleanup), the naming creates confusion about the actual behavior.
Consider either:
- Renaming
TrackEphemeralPeerto something more descriptive likeTrackNewEphemeralPeerorMarkEphemeralPeerForTracking - Adding the ephemeral manager's method directly to the interface (e.g., exposing a method like
TrackUnconnectedEphemeralPeer) - Adding a clarifying comment explaining that this method is used for newly created peers that haven't connected yet
| // TrackEphemeralPeer registers a newly created ephemeral peer for tracking/cleanup. | |
| // Note: the peer may not have connected yet; we intentionally reuse OnPeerDisconnected | |
| // to add it to the ephemeral peers manager's tracking list. |
| } | ||
|
|
||
| if temporary { | ||
| // we should track ephemeral peers to be able to clean them if the peer don't sync and be marked as connected |
There was a problem hiding this comment.
The comment states "if the peer don't sync and be marked as connected" which has a grammatical issue. Consider rephrasing to "if the peer doesn't sync and get marked as connected" or "if the peer doesn't connect and sync".
| // we should track ephemeral peers to be able to clean them if the peer don't sync and be marked as connected | |
| // we should track ephemeral peers to be able to clean them if the peer doesn't sync and get marked as connected |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/peer.go (1)
547-547: AllAddPeercall sites correctly updated with thetemporaryargument; however, moveTrackEphemeralPeeroutside the transaction.The
TrackEphemeralPeercall at line 731 is invoked inside the database transaction block. If the transaction fails or rolls back after this call, the tracking state will be inconsistent with the database. Move this call outside the transaction to execute only after successful commit.Current code structure (lines 710–740)
err = am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { // ... transaction operations ... if temporary { am.networkMapController.TrackEphemeralPeer(ctx, newPeer) // ← side-effect inside transaction } // ... more operations ... })
🤖 Fix all issues with AI agents
In `@management/server/peer.go`:
- Around line 731-734: The TrackEphemeralPeer call is being invoked inside the
DB transaction, causing side effects if the transaction rolls back and extending
lock time; move the am.networkMapController.TrackEphemeralPeer(ctx, newPeer)
invocation out of the transaction commit path so it runs only after the
transaction successfully commits—locate the block that checks the temporary
boolean and the transaction scope in peer.go around where newPeer is created and
call TrackEphemeralPeer(ctx, newPeer) after the transaction completes (using the
same temporary/newPeer variables and ctx).
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
bug fix
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
✏️ Tip: You can customize this high-level summary in your review settings.