Skip to content

[client] Don't mark management disconnected on transient job stream errors#6005

Merged
pappz merged 1 commit intomainfrom
fix/job-stream-state-leak
Apr 28, 2026
Merged

[client] Don't mark management disconnected on transient job stream errors#6005
pappz merged 1 commit intomainfrom
fix/job-stream-state-leak

Conversation

@pappz
Copy link
Copy Markdown
Collaborator

@pappz pappz commented Apr 27, 2026

Describe your changes

The JOB stream is a separate channel from the SYNC stream. Server-side EOF or other transient errors on the JOB stream do not indicate that the management connection is unhealthy — the SYNC stream remains the authoritative state signal.

Previously, a JOB stream EOF would call notifyDisconnected and the client would emit OnConnecting to the UI. The backoff retry would reconnect the JOB stream, but handleJobStream never calls notifyConnected on success, so the UI was stuck on "Connecting" until the next SYNC event or health check (i.e. from netbird status).

Keep notifyDisconnected for codes.PermissionDenied since IsLoginRequired relies on managementError to detect expired auth.

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

  • Chores
    • Improved error logging messages for better diagnostic clarity during job stream operations.

The JOB stream is a separate channel from the SYNC stream. Server-side
EOF or other transient errors on the JOB stream do not indicate that
the management connection is unhealthy — the SYNC stream remains the
authoritative state signal.

Previously, a JOB stream EOF would call notifyDisconnected and the
client would emit OnConnecting to the UI. The backoff retry would
reconnect the JOB stream, but handleJobStream never calls notifyConnected
on success, so the UI was stuck on "Connecting" until the next SYNC
event or health check.

Keep notifyDisconnected for codes.PermissionDenied since IsLoginRequired
relies on managementError to detect expired auth.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 02492fe6-1951-481a-811f-33787e8d390a

📥 Commits

Reviewing files that changed from the base of the PR and between 154b816 and cf26fa0.

📒 Files selected for processing (1)
  • shared/management/client/grpc.go

📝 Walkthrough

Walkthrough

Updated error logging messages in the handleJobStream function to more accurately describe job stream-specific scenarios instead of generic management connection terminology. Log messages now reference "job stream context" and "job stream disconnected" for improved clarity.

Changes

Cohort / File(s) Summary
Job Stream Logging
shared/management/client/grpc.go
Updated error logging messages in handleJobStream to use job stream-specific terminology: replaced "management connection context" with "job stream context" for canceled cases, and updated disconnect logs to reference "job stream disconnected" instead of generic "Management service" disconnection.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

Poem

🐰 A stream of logs, now clear as day,
Job context flows in every way,
No "Management" confusion here—
Just job streams, crystal and sincere! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description is mostly complete with detailed explanation of the issue, rationale, and checklist completed. However, the 'Issue ticket number and link' section is empty, which is a missing required field. Add the issue ticket number and link in the 'Issue ticket number and link' section of the PR description.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: preventing management disconnection notifications for transient job stream errors, which is the core fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/job-stream-state-leak

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

@pappz pappz merged commit 9c50819 into main Apr 28, 2026
60 of 62 checks passed
@pappz pappz deleted the fix/job-stream-state-leak branch April 28, 2026 13:04
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