cockroachdb / cockroachdb/cockroach

kvclient/rangefeed: assorted minor API hazards (nil-cancel Close panic, in-place span sort, shared retry state, stale concurrency doc)

Open
#172,425 0 comments 0 reactions 0 assignees View on GitHub
A-kv-rangefeed branch-master C-bug O-agent P-3 T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

Four small, independent hazards in the client-side rangefeed API, grouped because each is a few-line fix:

1. **`Close()` on a never-started `RangeFeed` panics.** `f.cancel` is only assigned in `Start`, so `Close` dereferences a nil func for a feed built via `Factory.New` whose `Start` failed or was never called ([rangefeed.go#L290-L293](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvclient/rangefeed/rangefeed.go#L290-L293)). Callers doing `defer r.Close()` around a failable `Start` are one refactor away from a nil deref.
2. **`DistSender.RangeFeed` sorts the caller's spans slice in place.** `divideAllSpansOnRangeBoundaries` reorders the argument slice ([dist_sender_rangefeed.go#L228-L230](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go#L228-L230)). The `dbAdapter` copies first, but direct callers (e.g. the changefeed kvfeed) pass their own slices and may not expect mutation.
3. **Retry state is shared between the initial scan and the rangefeed loop.** Failures during the initial scan inflate the backoff the rangefeed loop starts with; the retry object is only reset after 30s of successful running ([rangefeed.go#L309](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvclient/rangefeed/rangefeed.go#L309), [scanner.go#L91](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvclient/rangefeed/scanner.go#L91)).
4. **`Start`'s "single thread" callback doc is wrong with parallel initial scans.** With `WithInitialScanParallelismFn`, `onValue`/`onValues`/`OnSpanDone` are invoked concurrently from scan workers ([rangefeed.go#L196-L197](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvclient/rangefeed/rangefeed.go#L196-L197) vs [db_adapter.go#L293-L297](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvclient/rangefeed/db_adapter.go#L293-L297)); the code itself knows this (`span.MakeConcurrentFrontier` exists precisely for the concurrent `OnSpanDone`). A caller adding parallelism to an existing feed gets a data race the documentation says cannot happen.

**Additional context**

Found during an agent-assisted correctness audit of the rangefeed subsystem. Code links are pinned to master @ a7e1178.

Jira issue: CRDB-65661

Contributor guide

Open the contributing guide

Research direction

Read rangefeed.go, scanner.go, dist_sender_rangefeed.go, and db_adapter.go, then run the rangefeed package tests. Trace Close, span handling, retry setup, and parallel initial-scan callbacks against the issue's four hazards; done means each behavior is safe and the Start callback documentation matches actual concurrency.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, 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.