Skip to content

fix: dial peers outside the router lock and honour caller context - #277

Merged
ovaistariq merged 2 commits into
mainfrom
fix/router-dial-outside-lock
Sep 27, 2026
Merged

ovaistariq merged 2 commits into
mainfrom
fix/router-dial-outside-lock

Conversation

@ovaistariq

@ovaistariq ovaistariq commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Router.getClient dialed a peer while holding the router-wide write lock, and the dial is blocking with a 5 s ConnectionTimeout. One unreachable peer, which is the normal state of a node mid-restart, stalled every remote Put, Get and Delete to every other node behind that dial. With the default retry budget a single request could hold the lock for four dials before the circuit breaker opened. Rolling deploys hit this on every node restart.

Route also took no context. Retries slept with time.Sleep and the dial used context.Background, so a request the client had already abandoned kept dialing and backing off against the owner.

Change

  • Router.mu now guards only the clients map and is never held across a dial. Each node's clientState carries its own dialMu; callers for the same node share one attempt, callers for other nodes are unaffected. conn/client are read and swapped under the state's own mutex.
  • A dial that completes after RemoveClient or RefreshConnections dropped the node closes its connection instead of leaking it.
  • RouteContext(ctx, key) on Router, Coordinator and Operations binds the retry backoff and the dial to the caller's context. The remote paths in server/operations and the streaming forwarders in server/service pass theirs. Route and RouteWithRetry keep their signatures.
  • An abandoned dial returns the caller's context error and is not counted toward the node's circuit breaker.

Tests

Three new tests in coordinator/router_dial_test.go, using a context dialer that blackholes one address:

  • routing to a healthy node completes while another node's dial is stuck;
  • a caller whose context expires mid-dial gets DeadlineExceeded promptly and the node's failure count stays 0;
  • a cancelled context ends the retry backoff.

make lint, make test-coordinator, make test-server and make test-race TESTRUN=Router pass.

Closes perfloop cases case_vzhjv4f9s4 and case_yf8b86gxc0.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YCoN4Him5ZY4hZovVp37ra


Note

Medium Risk
Changes core cluster routing concurrency and connection lifecycle; mistakes could cause connection leaks, incorrect circuit-breaker behavior, or subtle routing races, though behavior is covered by targeted tests.

Overview
Fixes cluster-wide forwarding stalls during rolling restarts by not holding Router.mu across peer dials. Each node’s clientState uses a per-node dial semaphore so one slow/unreachable peer no longer blocks routing to every other node; concurrent callers to the same node share one dial attempt.

Adds RouteContext on Router, Coordinator, and Operations so retry backoff and gRPC dials respect the caller’s deadline/cancellation. Remote operations and streaming forwarders now pass request context instead of Route with context.Background. Abandoned dials return the caller’s context error and do not increment the peer’s circuit breaker; dials that finish after a node was removed from the ring are closed instead of leaked.

New tests in router_dial_test.go cover non-blocking routing to healthy nodes, context-cancelled dials, dial-slot waiters, and cancelled retry backoff.

Reviewed by Cursor Bugbot for commit b036fb7. Bugbot is set up for automated code reviews on this repo. Configure here.

The router dialed a peer while holding its router-wide write lock, with a
blocking dial of up to ConnectionTimeout (5s). One unreachable peer, the
normal state of a node mid-restart, therefore stalled every remote Put,
Get and Delete to every other node behind that dial, for up to four
attempts per request until the circuit breaker opened. Dials now run
under a per-node lock only: Router.mu guards just the clients map, callers
for the same node share one attempt, and callers for other nodes are
unaffected. A dial that lands after RemoveClient or RefreshConnections
dropped the node is closed rather than leaked.

Route also had no context: retries slept with time.Sleep and the dial was
bound to context.Background, so a request the client had already abandoned
kept dialing and backing off against the owner. RouteContext binds both to
the caller's context and the remote paths in server/operations and the
streaming forwarders pass theirs. An abandoned dial is not counted against
the node's circuit breaker. Route and RouteWithRetry keep their signatures.

Perfloop cases case_vzhjv4f9s4 and case_yf8b86gxc0.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCoN4Him5ZY4hZovVp37ra
Signed-off-by: Ovais Tariq <ot@tigrisdata.com>
efirs
efirs previously approved these changes Sep 26, 2026
@greptile-apps

greptile-apps Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Refactors peer routing to respect caller context and avoid lock contention.

The PR appears safe to merge; both previously reported cancellation gaps are fixed.

Summary

The PR moves peer dials out of the router-wide lock and passes caller contexts through routing, retry backoff, and List peer acquisition. The changes since the previous review make waiting for a same-node dial cancelable and pass List’s context to its dial. Both previous findings are fixed.

Reviews (2) · Last reviewed commit: "fix(router): let waiters give up on a st..."

Comment thread coordinator/router.go Outdated
Comment thread coordinator/router.go Outdated
…nder ctx

Review follow-ups on the dial-outside-the-lock change. A second caller for
a node whose dial was already in flight blocked on the node's dial mutex
without regard to its own deadline, so it could wait out the full
connection timeout behind the first caller. The per-node lock is now a
one-slot semaphore taken with a select on ctx.Done, so a waiter returns
its own context error as soon as its deadline passes. GetClientForNode,
which the cluster-wide List fan-out uses, now takes the request context
so a cancelled List does not wait for a blackholed peer's dial either.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCoN4Him5ZY4hZovVp37ra
Signed-off-by: Ovais Tariq <ot@tigrisdata.com>
@ovaistariq
ovaistariq merged commit 153a6ba into main Sep 27, 2026
8 checks passed
@ovaistariq
ovaistariq deleted the fix/router-dial-outside-lock branch September 27, 2026 07:27
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