Skip to content

[client] change notifyDisconnected call on receiveJobRequest error#5138

Merged
doromaraujo merged 1 commit intonetbirdio:mainfrom
doromaraujo:bug/job-stream-notify-disconnected-mobile-issue
Jan 20, 2026
Merged

[client] change notifyDisconnected call on receiveJobRequest error#5138
doromaraujo merged 1 commit intonetbirdio:mainfrom
doromaraujo:bug/job-stream-notify-disconnected-mobile-issue

Conversation

@doromaraujo
Copy link
Copy Markdown
Contributor

@doromaraujo doromaraujo commented Jan 20, 2026

On handleJobStream, when handling error codes from receiveJobRequest in the switch-case, calling notifyDisconnected() in cases where it isn't a disconnection breaks connection status reporting on mobile peers.

This PR changes it so it isn't called on Canceled or Unimplemented status codes.

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
    • Improved error handling in job streaming to signal disconnection only in specific error scenarios, preventing unnecessary disconnection notifications and enhancing reliability.

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

On handleJobStream, when handling error codes in
the switch-case, notifying disconnected in cases
where it isn't a disconnection breaks
connection status reporting on mobile peers.

This commit changes it so it isn't called on
Canceled or Unimplemented status codes.
Copilot AI review requested due to automatic review settings January 20, 2026 09:29
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 20, 2026

📝 Walkthrough

Walkthrough

The GrpcClient's Job streaming error handler has been refactored to selectively invoke disconnection notifications. Rather than unconditionally notifying disconnection for all errors, the change restricts the notification to specific error scenarios: PermissionDenied, other gRPC errors, and non-gRPC errors, narrowing the conditions triggering disconnection signals.

Changes

Cohort / File(s) Change Summary
Job stream error handling
shared/management/client/grpc.go
Modified error handling in Job streaming path to conditionally invoke notifyDisconnected only for PermissionDenied, other gRPC errors, and non-gRPC errors, removing the previous unconditional notification for all error cases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops through error streams,
With selective care, not careless dreams,
Disconnection whispers now ring true,
Only when the errors call on cue. 🐰✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adjusting when notifyDisconnected is called in receiveJobRequest error handling.
Description check ✅ Passed The description follows the template structure, explains the bug fix with context about mobile peers, and completes all required sections including checklist and documentation selection.
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

return backoff.Permanent(err) // unrecoverable error, propagate to the upper layer
case codes.Canceled:
log.Debugf("management connection context has been canceled, this usually indicates shutdown")
return err
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn’t we add c.notifyDisconnected(err) here too?

@doromaraujo doromaraujo merged commit 50da507 into netbirdio:main Jan 20, 2026
44 checks passed
@doromaraujo doromaraujo deleted the bug/job-stream-notify-disconnected-mobile-issue branch January 20, 2026 10:14
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