[client] Use ctx.Err() instead of gRPC codes.Canceled to detect shutdown#6019
[client] Use ctx.Err() instead of gRPC codes.Canceled to detect shutdown#6019
Conversation
Detecting shutdown by inspecting the gRPC status code conflates a local context cancellation with a server- or proxy-sent codes.Canceled. When the latter occurs (e.g. an intermediary proxy resets the stream), the retry loop silently terminates and the client never reconnects. Switch to ctx.Err() in the signal Receive loop and management Sync/Job handlers, and stop matching gRPC Canceled/DeadlineExceeded in the flow client's isContextDone helper. With this change, a server-sent Canceled is treated as a transient error and the backoff retry loop continues.
|
📝 WalkthroughWalkthroughThe PR refactors error-handling and retry logic in three gRPC client files to prioritize context cancellation ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flow/client/client.go`:
- Around line 302-309: The current isContextDone(err error) helper incorrectly
infers local shutdown from the returned error; change its signature to
isContextDone(ctx context.Context, err error) and make it return true if
ctx.Err() != nil OR errors.Is(err, context.Canceled) || errors.Is(err,
context.DeadlineExceeded) so local cancellations represented via ctx are
detected reliably; update all callers (e.g., the retry loop that currently calls
isContextDone(err)) to pass the current context variable into isContextDone(ctx,
err).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 763ff032-5c73-49e2-9538-60e29e648c04
📒 Files selected for processing (3)
flow/client/client.goshared/management/client/grpc.goshared/signal/client/grpc.go
1 participant
Detecting shutdown by inspecting the gRPC status code conflates a local context cancellation with a server- or proxy-sent codes.Canceled. When the latter occurs (e.g. an intermediary proxy resets the stream), the retry loop silently terminates and the client never reconnects.
Switch to ctx.Err() in the signal Receive loop and management Sync/Job handlers, and stop matching gRPC Canceled/DeadlineExceeded in the flow client's isContextDone helper. With this change, a server-sent Canceled is treated as a transient error and the backoff retry loop continues.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
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
Release Notes
Bug Fixes
Refactor