Skip to content

[management] Fix peer deletion error handling#5188

Merged
mlsmaycon merged 1 commit intomainfrom
fix/peer-deletion-error-handling
Jan 26, 2026
Merged

[management] Fix peer deletion error handling#5188
mlsmaycon merged 1 commit intomainfrom
fix/peer-deletion-error-handling

Conversation

@pappz
Copy link
Copy Markdown
Collaborator

@pappz pappz commented Jan 26, 2026

When a deleted peer tries to reconnect, GetUserIDByPeerKey was returning Internal error instead of NotFound, causing clients to retry indefinitely instead of recognizing the unrecoverable PermissionDenied error.

This fix:

  1. Updates GetUserIDByPeerKey to properly return NotFound when peer doesn't exist
  2. Updates Sync handler to convert NotFound to PermissionDenied with message 'peer is not registered', matching the behavior of GetAccountIDForPeerKey

Fixes the regression introduced in v0.61.1 where deleted peers would see:

  • Before: 'rpc error: code = Internal desc = failed handling request' (retry loop)
  • After: 'rpc error: code = PermissionDenied desc = peer is not registered' (exits)

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

  • Bug Fixes
    • Enhanced error handling for peer key lookups with more specific error messages when peer credentials are not found in the system.

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

When a deleted peer tries to reconnect, GetUserIDByPeerKey was returning
Internal error instead of NotFound, causing clients to retry indefinitely
instead of recognizing the unrecoverable PermissionDenied error.

This fix:
1. Updates GetUserIDByPeerKey to properly return NotFound when peer doesn't exist
2. Updates Sync handler to convert NotFound to PermissionDenied with message
   'peer is not registered', matching the behavior of GetAccountIDForPeerKey

Fixes the regression introduced in v0.61.1 where deleted peers would see:
- Before: 'rpc error: code = Internal desc = failed handling request' (retry loop)
- After: 'rpc error: code = PermissionDenied desc = peer is not registered' (exits)
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 26, 2026

📝 Walkthrough

Walkthrough

The changes enhance error handling for the GetUserIDByPeerKey operation by adding explicit peer-not-found error responses at both the database layer and the gRPC server layer, replacing delegation to generic error mapping with specific, immediate error returns.

Changes

Cohort / File(s) Summary
Peer key lookup error handling
management/server/store/sql_store.go, management/internals/shared/grpc/server.go
sql_store.go: Added detection of gorm.ErrRecordNotFound to return NotFound status with "peer not found: index lookup failed" message. server.go: Modified Sync path to immediately return gRPC PermissionDenied error with "peer is not registered" message when GetUserIDByPeerKey returns NotFound, bypassing mapError delegation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • pascal-fischer
  • crn4

Poem

🐰 A peer comes seeking passage through the net,
But alas, not registered in our database yet,
With a clear "not found" we kindly say,
"Permission denied, try again another day!" 🔐

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: fixing peer deletion error handling in the management package.
Description check ✅ Passed The description provides comprehensive details about the bug, the fix, expected behavior changes, and properly completes most required sections including the bug fix checkbox and documentation justification.
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

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

@sonarqubecloud
Copy link
Copy Markdown

@mlsmaycon mlsmaycon merged commit 44ab454 into main Jan 26, 2026
44 of 47 checks passed
@mlsmaycon mlsmaycon deleted the fix/peer-deletion-error-handling branch January 26, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants