Skip to content

Separate backoff strategies for peer registration vs login operations #4835

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

Open
hurricanehrndz wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Separate backoff strategies for peer registration vs login operations #4835

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a1905a
Separate backoff strategies for peer registration vs login operations
hurricanehrndz Nov 20, 2025
d226e21
[client] Prevent registration attempts without credentials
hurricanehrndz Jan 8, 2026
c1ac190
[client] Handle "peer already registered" during registration retries
hurricanehrndz Jan 8, 2026
f706892
[client] Prevent duplicate device registration on timeout
hurricanehrndz Jan 8, 2026
0c9f55d
[client] Gracefully recover from registration timeout with login retries
hurricanehrndz Jan 8, 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
5 changes: 5 additions & 0 deletions client/internal/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ func Login(ctx context.Context, config *profilemanager.Config, setupKey string,

serverKey, _, err := doMgmLogin(ctx, mgmClient, pubSSHKey, config)
if serverKey != nil && isRegistrationNeeded(err) {
// Only attempt registration if we have credentials (setup key or JWT token)
if setupKey == "" && jwtToken == "" {
log.Debugf("peer registration required but no credentials provided")
return err
}
Comment on lines +76 to +80
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Consider wrapping the error to indicate registration was skipped.

Returning the original PermissionDenied error from doMgmLogin doesn't convey that registration was skipped due to missing credentials. This may complicate troubleshooting when callers or operators try to understand why the operation failed.

Additionally, the debug log level might be insufficient for tracking this control flow decision in production environments.

🔧 Suggested improvements

Option 1: Wrap the error with context

 // Only attempt registration if we have credentials (setup key or JWT token)
 if setupKey == "" && jwtToken == "" {
-	log.Debugf("peer registration required but no credentials provided")
-	return err
+	log.Warnf("peer registration required but no credentials provided")
+	return status.Errorf(codes.FailedPrecondition, "peer registration required but no credentials (setup key or JWT token) provided: %v", err)
 }

Option 2: Keep original error but improve logging

 // Only attempt registration if we have credentials (setup key or JWT token)
 if setupKey == "" && jwtToken == "" {
-	log.Debugf("peer registration required but no credentials provided")
+	log.Warnf("peer registration required but no credentials provided, returning original error: %v", err)
 	return err
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Only attempt registration if we have credentials (setup key or JWT token)
if setupKey == "" && jwtToken == "" {
log.Debugf("peer registration required but no credentials provided")
return err
}
// Only attempt registration if we have credentials (setup key or JWT token)
if setupKey == "" && jwtToken == "" {
log.Warnf("peer registration required but no credentials provided")
return status.Errorf(codes.FailedPrecondition, "peer registration required but no credentials (setup key or JWT token) provided: %v", err)
}
Suggested change
// Only attempt registration if we have credentials (setup key or JWT token)
if setupKey == "" && jwtToken == "" {
log.Debugf("peer registration required but no credentials provided")
return err
}
// Only attempt registration if we have credentials (setup key or JWT token)
if setupKey == "" && jwtToken == "" {
log.Warnf("peer registration required but no credentials provided, returning original error: %v", err)
return err
}
🤖 Prompt for AI Agents 
In @client/internal/login.go around lines 74 - 78, The code currently logs at
debug and returns the raw PermissionDenied error when both setupKey and jwtToken
are empty; change this to log a higher-severity message (e.g., log.Warnf or
log.Infof) that clearly states peer registration was skipped due to missing
credentials and wrap the returned error with contextual text (e.g.,
"registration skipped: missing credentials") so callers can distinguish a
skipped-registration case from other PermissionDenied errors; update the branch
around setupKey, jwtToken, doMgmLogin to log the new message and return a
wrapped error (using fmt.Errorf or errors.Wrap) that includes the original err.

log.Debugf("peer registration required")
_, err = registerPeer(ctx, *serverKey, mgmClient, setupKey, jwtToken, pubSSHKey, config)
if err != nil {
Expand Down
66 changes: 61 additions & 5 deletions shared/management/client/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,17 @@ import (
"github.com/netbirdio/netbird/util/wsproxy"
)

const ConnectTimeout = 10 * time.Second
const (
ConnectTimeout = 10 * time.Second

RegistrationRetryTimeout = 180 * time.Second
RegistrationRetryInterval = 5 * time.Second
RegistrationRetryMaxDelay = 30 * time.Second

LoginRetryTimeout = 45 * time.Second
LoginRetryInterval = 3 * time.Second
LoginRetryMaxDelay = 15 * time.Second
)

const (
errMsgMgmtPublicKey = "failed getting Management Service public key: %s"
Expand Down Expand Up @@ -312,6 +322,38 @@ func (c *GrpcClient) IsHealthy() bool {
return true
}

// newRegistrationBackoff creates a backoff strategy for peer registration operations
func newRegistrationBackoff(ctx context.Context) backoff.BackOff {
return backoff.WithContext(&backoff.ExponentialBackOff{
InitialInterval: RegistrationRetryInterval,
RandomizationFactor: backoff.DefaultRandomizationFactor,
Multiplier: backoff.DefaultMultiplier,
MaxInterval: RegistrationRetryMaxDelay,
MaxElapsedTime: RegistrationRetryTimeout,
Stop: backoff.Stop,
Clock: backoff.SystemClock,
}, ctx)
}

// newLoginBackoff creates a backoff strategy for peer login operations
func newLoginBackoff(ctx context.Context) backoff.BackOff {
return backoff.WithContext(&backoff.ExponentialBackOff{
InitialInterval: LoginRetryInterval,
RandomizationFactor: backoff.DefaultRandomizationFactor,
Multiplier: backoff.DefaultMultiplier,
MaxInterval: LoginRetryMaxDelay,
MaxElapsedTime: LoginRetryTimeout,
Stop: backoff.Stop,
Clock: backoff.SystemClock,
}, ctx)
}

// isRegistrationRequest determines if a login request is actually a registration.
// Returns true if either the setup key or the JWT token is present in the request.
func isRegistrationRequest(req *proto.LoginRequest) bool {
return req.SetupKey != "" || req.JwtToken != ""
}

func (c *GrpcClient) login(serverKey wgtypes.Key, req *proto.LoginRequest) (*proto.LoginResponse, error) {
if !c.ready() {
return nil, errors.New(errMsgNoMgmtConnection)
Expand All @@ -334,17 +376,31 @@ func (c *GrpcClient) login(serverKey wgtypes.Key, req *proto.LoginRequest) (*pro
Body: loginReq,
})
if err != nil {
// retry only on context canceled
if s, ok := gstatus.FromError(err); ok && s.Code() == codes.Canceled {
return err
if s, ok := gstatus.FromError(err); ok {
if s.Code() == codes.Canceled {
log.Debugf("login canceled")
return backoff.Permanent(err)
}
if s.Code() == codes.DeadlineExceeded {
log.Debugf("login timeout, retrying...")
return err
}
Comment on lines 386 to 398
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The log message 'login timeout, retrying...' doesn't distinguish between registration and login operations. Since these now have different timeout configurations, the message should indicate which operation timed out to aid debugging. Consider using a message like 'operation timeout, retrying...' or including the operation type.

Copilot uses AI. Check for mistakes.
}
return backoff.Permanent(err)
}

return nil
}

err = backoff.Retry(operation, nbgrpc.Backoff(c.ctx))
isRegistration := isRegistrationRequest(req)
var backoffStrategy backoff.BackOff
if isRegistration {
backoffStrategy = newRegistrationBackoff(c.ctx)
} else {
backoffStrategy = newLoginBackoff(c.ctx)
}

err = backoff.Retry(operation, backoffStrategy)
if err != nil {
log.Errorf("failed to login to Management Service: %v", err)
return nil, err
Expand Down
Loading