cockroachdb / cockroachdb/cockroach

kvserver: clobbered RangePriorReadSummary can cause a write beneath a served read across a merge

Open
#173,373 3 comments 0 reactions 0 assignees View on GitHub
A-kv-transactions C-bug P-3 T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Summary**

The prior read summary persisted at `RangePriorReadSummaryKey` can be clobbered by a sequence-changing `RequestLease` that evaluates without latches and applies after a merge trigger. This is a correctness violation: on the path where a range's leaseholder learns about a merge through a Raft snapshot, the persisted read summary is the *only* record of the reads served on the RHS before the merge, so clobbering it lets that leaseholder serve writes beneath timestamps at which reads were already served.

**Mechanism**

1. A merge folds the RHS's read summary (including reads above the LHS leaseholder's clock, e.g. future-time reads) into the LHS's persisted read summary (`mergeTrigger`). The read summary is also updated in-memory and used to update the new leaseholder's timestamp cache after the merge trigger applies. However, if the new leaseholder only learns of the merge via a snapshot, the persisted read summary is the only way for it to learn about any future-time reads.
2. `evalNewLease` rewrites the persisted summary blindly on any sequence-changing lease request, using the worst-case `FromTimestamp(lease.Start)`, a value at or below the new lease's start, and thus potentially below the RHS's future-time reads. Because lease requests are unlatched, such a request can apply after the merge trigger and overwrite the folded summary with that lower value, discarding the RHS's read summary.

**Affected lease types**

For this bug, we need two components: (1) a valid leaseholder that's behind on its log (in order to need and receive a snapshot).
A latchless, same-holder lease change that is not considered equivalent (to write a new read summary and clobber the previous one written in the merge).

For epoch and expiration leases, the two conditions above are possible to occur. For leader leases, generally, the first condition doesn't apply because the leaseholder is always the leader, but during lease transfers we use expiration leases. In fact, during a leader lease transfer, it is quite possible to be stuck in a split state if the new leaseholder is far away from the leader and is always behind on its log; the leadership and lease upgrade will not finalize.

**Repro**

See `TestStoreRangeMergeClobberedReadSummary` in https://github.com/cockroachlabs/cockroach/pull/3557.

**Mechanism for MVCCStats mismatch**

This bug can also result in a stats mismatch due to overwriting the read summary.

`RequestLease` is unlatched (a lease must be acquirable when the prior leaseholder is unavailable) yet it writes `RangePriorReadSummaryKey` with a stats delta computed at *eval* time. A concurrent *latched* writer of the same key (e.g. `AdminMerge`) can change the value between the lease request's eval and apply. The lease request then overwrites a different-sized value than its delta assumed, leaving `SysBytes` off by the size difference. The persisted summary is compressed using `Compress(0)`, so an empty one is 13 bytes, while an entry with two real `low_water` marks is 33 bytes, resulting is a delta of +/- 20 `SysBytes`.

Note that a lease request may write an _empty_ read summary and result in a stats delta wrt to a concurrent non-empty read summary writer (as described above), but it's also possible that a lease request writes a _non-empty_ summary, which overwrites a potential concurrent writer's read summary but doesn't result in a stats inconsistency (both read summaries are of the same size).

**KVNemesis failures**

The confirmed signature of this failure is a `SysBytes` delta of +/- 20 bytes. Kvnemesis failures of that shape can be marked as duplicates. If other shapes are discovered to be due to the same read summary race, please, update this issue.

Jira issue: CRDB-66688

Contributor guide

Open the contributing guide

Research direction

Start with TestStoreRangeMergeClobberedReadSummary in the linked pull request, then trace evalNewLease, RequestLease, mergeTrigger, and RangePriorReadSummaryKey. Reproduce the merge and lease-request race, including the KVNemesis SysBytes mismatch, and verify that the persisted read summary and stats remain correct across the concurrent operations.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, databases, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.