cockroachdb / cockroachdb/cockroach
kvclient/rangefeed: assorted minor API hazards (nil-cancel Close panic, in-place span sort, shared retry state, stale concurrency doc)
- 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
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