Skip to content

[management] fix skip of ephemeral peers on deletion #5206

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 4 commits into from
Jan 29, 2026
Merged

[management] fix skip of ephemeral peers on deletion #5206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4c7e2a
[management] fix skip of ephemeral peers on deletion
crn4 Jan 28, 2026
e487aed
minor typo
crn4 Jan 28, 2026
6d9b403
Merge branch 'main' into fix/ephemeral-peers
crn4 Jan 29, 2026
f25fd73
minor change on ai review
crn4 Jan 29, 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: 2 additions & 2 deletions management/internals/modules/peers/ephemeral/manager/ephemeral.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,10 @@ func (e *EphemeralManager) cleanup(ctx context.Context) {
}

for accountID, peerIDs := range peerIDsPerAccount {
log.WithContext(ctx).Debugf("delete ephemeral peers for account: %s", accountID)
log.WithContext(ctx).Tracef("cleanup: deleting %d ephemeral peers for account %s", len(peerIDs), accountID)
err := e.peersManager.DeletePeers(ctx, accountID, peerIDs, activity.SystemInitiator, true)
if err != nil {
log.WithContext(ctx).Errorf("failed to delete ephemeral peer: %s", err)
log.WithContext(ctx).Errorf("failed to delete ephemeral peers: %s", err)
}
}
}
Expand Down
12 changes: 11 additions & 1 deletion management/internals/modules/peers/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,19 @@ func (m *managerImpl) DeletePeers(ctx context.Context, accountID string, peerIDs
err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error {
peer, err := transaction.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peerID)
if err != nil {
if e, ok := status.FromError(err); ok && e.Type() == status.NotFound {
log.WithContext(ctx).Tracef("DeletePeers: peer %s not found, skipping", peerID)
return nil
}
return err
}

if checkConnected && (peer.Status.Connected || peer.Status.LastSeen.After(time.Now().Add(-(ephemeral.EphemeralLifeTime - 10*time.Second)))) {
log.WithContext(ctx).Tracef("DeletePeers: peer %s skipped (connected=%t, lastSeen=%s, threshold=%s, ephemeral=%t)",
peerID, peer.Status.Connected,
peer.Status.LastSeen.Format(time.RFC3339),
time.Now().Add(-(ephemeral.EphemeralLifeTime - 10*time.Second)).Format(time.RFC3339),
peer.Ephemeral)
return nil
}

Expand Down Expand Up @@ -150,7 +159,8 @@ func (m *managerImpl) DeletePeers(ctx context.Context, accountID string, peerIDs
return nil
})
if err != nil {
return err
log.WithContext(ctx).Errorf("DeletePeers: failed to delete peer %s: %v", peerID, err)
continue
Comment thread
crn4 marked this conversation as resolved.
}
Comment on lines 161 to 164
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jan 29, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and find the DeletePeers function
cd management/internals/modules/peers
wc -l manager.go

Repository: netbirdio/netbird

Length of output: 75

🏁 Script executed:

# Read the DeletePeers function - lines around 161-179 to see full context
sed -n '140,185p' management/internals/modules/peers/manager.go

Repository: netbirdio/netbird

Length of output: 1304

🏁 Script executed:

# Search for all callers of DeletePeers function
rg -n 'DeletePeers\s*\(' --type=go -g '!*_mock.go' -g '!*_test.go'

Repository: netbirdio/netbird

Length of output: 568

🏁 Script executed:

# Read the caller context in ephemeral manager
sed -n '180,210p' management/internals/modules/peers/ephemeral/manager/ephemeral.go

Repository: netbirdio/netbird

Length of output: 891

🏁 Script executed:

# Search for tests of DeletePeers to understand intended behavior
rg -n 'DeletePeers' --type=go -A 10 -B 2 | grep -E '(test|Test|_test\.go)' -A 10 -B 2

Repository: netbirdio/netbird

Length of output: 6933

🏁 Script executed:

# Look for all test files that test DeletePeers
find . -name '*_test.go' -type f -exec grep -l 'DeletePeers' {} \;

Repository: netbirdio/netbird

Length of output: 133

🏁 Script executed:

# Read the DeletePeers function signature and any comments
sed -n '99,110p' management/internals/modules/peers/manager.go

Repository: netbirdio/netbird

Length of output: 630

🏁 Script executed:

# Also check if there's an interface definition that documents expected behavior
rg -n 'interface Manager' -A 20 management/internals/modules/peers/manager.go

Repository: netbirdio/netbird

Length of output: 43

🏁 Script executed:

# Check the interface definition for DeletePeers
rg -n 'DeletePeers' management/internals/modules/peers/manager.go | head -5

Repository: netbirdio/netbird

Length of output: 619

🏁 Script executed:

# Look for the interface definition in the same file
sed -n '1,50p' management/internals/modules/peers/manager.go

Repository: netbirdio/netbird

Length of output: 2218

The error-on-continue behavior is intentional for ephemeral cleanup, but clarify the misleading error contract.

The function's design—logging errors and continuing to the next peer without failing—is appropriate for ephemeral peer cleanup (temporary peers will expire anyway). However, the function signature declares an error return type but always returns nil, which is misleading. The single caller (ephemeral peer expiration cleanup) correctly handles this, but the design could confuse future developers or cause bugs if the function is extended to other contexts.

Consider documenting this as best-effort cleanup, or distinguish between ephemeral cleanup (which can be best-effort) and other deletion scenarios that may require stricter error handling. Alternatively, return an error summary or retry count to make the contract explicit.

🤖 Prompt for AI Agents 
In `@management/internals/modules/peers/manager.go` around lines 161 - 164, The
DeletePeers function currently declares an error return but always logs errors
and continues (best-effort), which is misleading; change the function signature
(DeletePeers) to return no error (remove the error return) and treat it
explicitly as a best-effort/ephemeral cleanup routine, update its single caller
(the ephemeral peer expiration cleanup) to stop expecting an error, and keep the
existing per-peer log-on-error-and-continue behavior
(log.WithContext(ctx).Errorf(...) and continue) so the contract matches the
implementation; alternatively, if you prefer to preserve an error return,
replace it with an aggregated error (e.g., collect errors into a slice or use a
multierror) and return that summary instead—pick one approach and apply it
consistently to DeletePeers and its caller.


if m.integratedPeerValidator != nil {
Expand Down
Loading