cockroachdb / cockroachdb/cockroach

kvcoord: DistSender takes a full rpc-retry backoff before using freshly-learned routing when the new leaseholder is absent from the transport

Open
#173,342 2 comments 0 reactions 0 assignees View on GitHub
A-kv-client A-kv-distribution branch-master C-bug O-agent O-support T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

When `DistSender` sends to a range whose cached descriptor predates a rebalance that *added* a replica and moved the lease onto it, the request path pays an unnecessary `rpcRetryOptions` backoff (`InitialBackoff` 50ms) even though it has *already* cached the correct, fresher routing.

The sequence: a replica returns a `NotLeaseHolderError` naming the new leaseholder. `sendToReplicas` updates the range cache with the fresh descriptor/lease and takes the `updatedLeaseholder` path, but then `transport.MoveToFront(newLeaseholder)` fails because the new leaseholder is a replica that isn't in the transport (the transport was built from the stale descriptor). It bails with a `sendError` ("leaseholder not found in transport"). Back in `sendPartialBatch`, that `sendError` triggers an eviction of the stale token and `continue` — but the outer retry loop's `r.Next()` sleeps the full backoff *before* re-looking-up, and the re-lookup then finds the descriptor that was already cached moments earlier. The backoff is pure latency: the client is not waiting for anything to become ready; it already knows where to route.

**Evidence**

Sanitized production trace (customer identifiers redacted; the originating build's line numbers differ, code refs below are mapped to master):

```
3.260ms [NotLeaseHolderError] ... r2740410: replica (n366,s475):1 not lease holder;
current lease is repl=(n389,s521):13 seq=864
3.333ms range_cache: caching new entry: desc r2740410 [... (n389,s521):13, next=14, gen=12441],
lease repl=(n389,s521):13 seq=864 <-- correct routing now cached
3.359ms dist_sender: updated leaseholder; resetting leaseholderUnavailable and routing to leaseholder
3.381ms dist_sender: transport incompatible with updated routing; bailing early
3.672ms dist_sender: evicting range desc et:desc r2740410 [... (n210,s240):5, next=13, gen=12437],
lease repl=(n210,s240):5 seq=863 ... after failed to send RPC: leaseholder not found in transport
3.687ms dist_sender: will retry after 44.221032ms <-- backoff begins
48.045ms dist_sender: r2740410: sending batch 1 Get to (n389,s521):13 <-- success
```

The range moved from `gen=12437` (replicas incl. `(n210,s240):5`, lease on n210) to `gen=12441` (n210 removed, `(n389,s521):13` added, lease moved to n389). The client's transport was built from `gen=12437`, so it could not route to n389. The correct descriptor was cached at `3.333ms`, but the request did not reach n389 until `48.045ms` — ~44ms of it a backoff the client did not need.

**Analysis (code references, master @ d45aaf0b8a7)**

- Outer retry loop that takes the backoff: [dist_sender.go:2281](https://github.com/cockroachdb/cockroach/blob/d45aaf0b8a706542689f0510308878bc6c856d2c/pkg/kv/kvclient/kvcoord/dist_sender.go#L2281)
- `updatedLeaseholder` path (cache learns the new leaseholder): [dist_sender.go:3195](https://github.com/cockroachdb/cockroach/blob/d45aaf0b8a706542689f0510308878bc6c856d2c/pkg/kv/kvclient/kvcoord/dist_sender.go#L3195)
- `MoveToFront` failure -> bail with sendError: [dist_sender.go:3218-3241](https://github.com/cockroachdb/cockroach/blob/d45aaf0b8a706542689f0510308878bc6c856d2c/pkg/kv/kvclient/kvcoord/dist_sender.go#L3218-L3241)
- sendError -> evict stale token + `continue` (which loops back to the blocking `r.Next()`): [dist_sender.go:2364-2388](https://github.com/cockroachdb/cockroach/blob/d45aaf0b8a706542689f0510308878bc6c856d2c/pkg/kv/kvclient/kvcoord/dist_sender.go#L2364-L2388). The comment there already documents that the fresh descriptor will be "reload[ed] ... from the cache on the next iteration" — but the backoff fires first.

**Impact**

- Adds ~`InitialBackoff` (50ms, jittered) of latency per occurrence, on the request that discovers a rebalance which added a replica and transferred the lease to it.
- Not counted in `distsender.errors.inleasetransferbackoffs` — this is the outer `sendPartialBatch` retry, not the in-lease-transfer backoff — so it is largely invisible in metrics and easy to under-diagnose.
- Amplified under heavy rebalancing (e.g. zone-config-driven), which is exactly when replica-set changes (not just lease moves) are frequent.

**Expected behavior**

When the `sendError` was caused by the routing being updated to a strictly-newer descriptor generation during `sendToReplicas` (i.e. we bailed *because* we learned fresher info, not because replicas are unreachable), the outer loop should retry immediately rather than sleeping the full `rpcRetryOptions` backoff.

**Proposed direction / considerations**

- Skip or reset the outer-loop backoff on a `sendError` when the eviction token was refreshed to a strictly-newer generation before the retry.
- Must retain a bound to avoid a hot loop if re-lookup keeps returning a descriptor the transport cannot use (e.g. cap immediate retries, then fall back to the normal backoff).
- Related to the existing `TODO(baptist)` near [dist_sender.go:3207](https://github.com/cockroachdb/cockroach/blob/d45aaf0b8a706542689f0510308878bc6c856d2c/pkg/kv/kvclient/kvcoord/dist_sender.go#L3207) about not evicting valid range information.

Jira issue: CRDB-66658

Contributor guide

Open the contributing guide

Research direction

Start in pkg/kv/kvclient/kvcoord/dist_sender.go at the sendPartialBatch retry loop around line 2281, then trace the sendError handling around lines 2364-2388 and the updatedLeaseholder path around lines 3195-3241. Confirm how a refreshed descriptor generation reaches the retry loop and account for the hot-loop concern near the TODO at line 3207. Done means freshly learned routing retries without the unnecessary full backoff while retaining a bounded fallback.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, databases, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.