feat: reconnect worker to coordinator after heartbeat timeout#2408
Open
pirm-in wants to merge 2 commits into
Open
feat: reconnect worker to coordinator after heartbeat timeout#2408pirm-in wants to merge 2 commits into
pirm-in wants to merge 2 commits into
Conversation
Cache the worker's coordinator gRPC clients until a call fails with a transient error (Unavailable or a deadline). On such failures the cached client is closed and dropped so the next call re-dials, which lets a worker recover when a coordinator moves to a new address under the same ID. Add a configurable HeartbeatTimeout (default 10s) bounding the worker heartbeat so a stalled coordinator cannot block the heartbeat loop.
📝 WalkthroughWalkthroughThe coordinator client now applies configurable heartbeat timeouts, performs health checks against cached client instances, and conditionally evicts stale clients during transient failures. Stream and workspace-bundle failover paths use the updated checks, with recovery and timeout behavior covered by tests. ChangesCoordinator client resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HeartbeatClient
participant CoordinatorA
participant CoordinatorB
HeartbeatClient->>CoordinatorA: Send heartbeat
CoordinatorA-->>HeartbeatClient: Deadline exceeded
HeartbeatClient->>CoordinatorB: Retry heartbeat
CoordinatorB-->>HeartbeatClient: Successful heartbeat
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/coordinator/client.go`:
- Around line 965-968: The health-check failure branches in attemptCall’s
failover paths must evict reset-eligible cached clients before continuing. In
internal/service/coordinator/client.go at lines 965-968 and 1127-1130, call
resetClientOnTransientError(member, memberClient, err) after recording the
failure and before continue; update both sites consistently.
🪄 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 Plus
Run ID: 66955373-d5da-4c67-bb6a-cff7fc04d4dd
📒 Files selected for processing (4)
internal/service/coordinator/client.gointernal/service/coordinator/client_test.gointernal/service/coordinator/config.gointernal/service/coordinator/config_test.go
Comment on lines
965
to
968
| if err := cli.isHealthy(ctx, memberClient); err != nil { | ||
| cli.recordFailure(err) | ||
| lastErr = err | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Evict reset-eligible clients after health-check failures.
attemptCall resets the exact cached instance, but these failover paths retain it after isHealthy fails. A stale connection therefore remains cached and is reused on later stream or bundle operations.
internal/service/coordinator/client.go#L965-L968: callcli.resetClientOnTransientError(member, memberClient, err)before continuing.internal/service/coordinator/client.go#L1127-L1130: callcli.resetClientOnTransientError(member, memberClient, err)before continuing.
📍 Affects 1 file
internal/service/coordinator/client.go#L965-L968(this comment)internal/service/coordinator/client.go#L1127-L1130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/coordinator/client.go` around lines 965 - 968, The
health-check failure branches in attemptCall’s failover paths must evict
reset-eligible cached clients before continuing. In
internal/service/coordinator/client.go at lines 965-968 and 1127-1130, call
resetClientOnTransientError(member, memberClient, err) after recording the
failure and before continue; update both sites consistently.
Extend the transient-error client reset to the stream-failover and workspace-bundle paths so a coordinator that moved address under the same ID is evicted there too, not only on the dispatch path. Fold the health-check and reset-on-failure into a single checkHealthy helper so every member-loop site shares one policy, and cover the stream and bundle failover paths with a regression test.
yohamta0
reviewed
Jul 23, 2026
| // checkHealthy verifies the coordinator is serving and evicts the cached | ||
| // client when the failure indicates a stale connection. | ||
| func (cli *clientImpl) checkHealthy(ctx context.Context, member exec.HostInfo, client *client) error { | ||
| if err := cli.isHealthy(ctx, client); err != nil { |
Collaborator
There was a problem hiding this comment.
Could we bail out before the health check when ctx is already done?
Heartbeat shares one timeout across all coordinator attempts. If one coordinator uses the full timeout, the next healthy coordinator gets DeadlineExceeded immediately and this code closes its cached ClientConn, canceling any active log or artifact stream. I reproduced this with two coordinators.
Suggested change
| if err := cli.isHealthy(ctx, client); err != nil { | |
| if err := ctx.Err(); err != nil { | |
| return err | |
| } | |
| if err := cli.isHealthy(ctx, client); err != nil { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workers can get stuck talking to a dead cached coordinator connection when a
coordinator restarts or moves to a new address under the same ID. This drops
and re-dials the cached gRPC client on transient failures, and bounds the
worker heartbeat with a configurable timeout so a stalled coordinator cannot
block the heartbeat loop.
Changes
HeartbeatTimeout(default 10s) bounding the worker heartbeatRelated Issues
none
Checklist
make test,make lint)Summary by cubic
Workers now recover from coordinator restarts/moves by dropping stale cached gRPC clients on transient errors and re-dialing on the next call. This now also covers stream failover and workspace-bundle paths, and the heartbeat is bounded by a configurable timeout (default 10s).
Bug Fixes
Unavailable, deadline) and only remove it if it matches the current cached instance.checkHealthyand retry with a fresh connection; health checks reuse the resolved client.New Features
HeartbeatTimeouttocoordinator.Config; applied per heartbeat (default 10s).Written for commit 80e93f8. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Bug Fixes