cockroachdb / cockroachdb/cockroach
kvserver/concurrency: tryClearLocksGE reads lock holder state without kl.mu
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
This is a latent hazard: race-free today, but only by a global argument that nothing enforces.
`tryClearLocksGE` reads `l.holders` and `unreplicatedInfo` without acquiring `kl.mu` ([lock_table.go#L4927-L4948](https://github.com/cockroachdb/cockroach/blob/a7e117882b4356096b3a8ea26046baa7e5d53fce/pkg/kv/kvserver/concurrency/lock_table.go#L4927-L4948)). This is currently safe because it holds `treeMu.mu` exclusively and every existing `holders` mutator happens to hold at least a tree read lock. But the `keyLocks` struct comment says `kl.mu` protects this state, so a future mutation path that (correctly, per the comment) holds only `kl.mu` would create a data race with no locally visible cause.
`ExportUnreplicatedLocks` takes `l.mu` for the identical read.
**Suggested fix**
Take `l.mu` in `tryClearLocksGE` as `ExportUnreplicatedLocks` does. The lock is uncontended in this context, so this is purely defensive cost.
**Additional context**
Found during an agent-assisted correctness audit of `pkg/kv/kvserver/concurrency`. Code links are pinned to master @ a7e1178.
Jira issue: CRDB-65665
Contributor guide
Research direction
Start in pkg/kv/kvserver/concurrency/lock_table.go at tryClearLocksGE and compare its reads with ExportUnreplicatedLocks. Verify how kl.mu protects holders and unreplicatedInfo, then confirm the change preserves the existing tree-lock context and removes the latent unsynchronized read.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100