Skip to content

[management] fix ephemeral peers hadn't been added to ephemeral list on login #5165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
crn4 merged 2 commits into from
Jan 23, 2026
Merged

[management] fix ephemeral peers hadn't been added to ephemeral list on login #5165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43b6e96
ephemeral peers track on login
crn4 Jan 23, 2026
433fcd5
Merge branch 'main' into fix/ephemeral-peers
crn4 Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions management/internals/controllers/network_map/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,3 +856,7 @@ func (c *Controller) GetNetworkMap(ctx context.Context, peerID string) (*types.N
func (c *Controller) DisconnectPeers(ctx context.Context, accountId string, peerIDs []string) {
c.peersUpdateManager.CloseChannels(ctx, peerIDs)
}

Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Renaming TrackEphemeralPeer to something more descriptive like TrackNewEphemeralPeer or MarkEphemeralPeerForTracking
  2. Adding the ephemeral manager's method directly to the interface (e.g., exposing a method like TrackUnconnectedEphemeralPeer)
  3. Adding a clarifying comment explaining that this method is used for newly created peers that haven't connected yet
Suggested change
// 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.

Copilot uses AI. Check for mistakes.
func (c *Controller) TrackEphemeralPeer(ctx context.Context, peer *nbpeer.Peer) {
c.EphemeralPeersManager.OnPeerDisconnected(ctx, peer)
}
2 changes: 2 additions & 0 deletions management/internals/controllers/network_map/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,6 @@ type Controller interface {
DisconnectPeers(ctx context.Context, accountId string, peerIDs []string)
OnPeerConnected(ctx context.Context, accountID string, peerID string) (chan *UpdateMessage, error)
OnPeerDisconnected(ctx context.Context, accountID string, peerID string)

TrackEphemeralPeer(ctx context.Context, peer *nbpeer.Peer)
}
16 changes: 14 additions & 2 deletions management/internals/controllers/network_map/interface_mock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions management/server/peer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,11 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, accountID, setupKe
return fmt.Errorf("failed adding peer to All group: %w", err)
}

if temporary {
// we should track ephemeral peers to be able to clean them if the peer don't sync and be marked as connected
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Suggested change
// 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

Copilot uses AI. Check for mistakes.
am.networkMapController.TrackEphemeralPeer(ctx, newPeer)
}
Comment thread
crn4 marked this conversation as resolved.

if addedByUser {
err := transaction.SaveUserLastLogin(ctx, accountID, userID, newPeer.GetLastLogin())
if err != nil {
Expand Down
Loading