cockroachdb / cockroachdb/cockroach
kv: dropping latches after failed raft reproposal is unsafe
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
The following logic rejects a raft proposal with an AmbiguousResultError if an attempt to repropose it fails:
https://github.com/cockroachdb/cockroach/blob/59fb4ec268b456b7c5ad694c43202731f3299efe/pkg/kv/kvserver/replica_raft.go#L1491-L1495
In doing so, it calls `finishApplication`, which releases latches and cleans up the request.
It's not clear how this is safe. I don't think it is. Unlike the other case where we reject requests during a raft reproposal attempt ([here](https://github.com/cockroachdb/cockroach/blob/59fb4ec268b456b7c5ad694c43202731f3299efe/pkg/kv/kvserver/replica_raft.go#L1421-L1428)), on this path we have no strong reason to believe that the original proposal won't eventually succeed. If it could eventually succeed then dropping latches is unsafe, as it could allow conflicting requests to proceed and evaluate before the original request applies, only for the original request to later apply. This kind of race could lead to any number of issues, including stats inconsistencies and lost updates due to clobbered writes.
I think we want one of two things here.
One option is to ignore the error from `ReinsertLocked` and don't reject the proposal, allowing it to be reproposed again later. This may lead to requests like lease acquisitions getting stuck indefinitely in the proposals map, so we'd need to careful.
The other option is to signal a result to the proposal without dropping latches. This is what we (correctly) do when poisoning requests:
https://github.com/cockroachdb/cockroach/blob/59fb4ec268b456b7c5ad694c43202731f3299efe/pkg/kv/kvserver/replica_raft.go#L1511-L1514
It's also possible that we never actually hit this error in practice and that the code is effectively dead. There are very few cases where `ReinsertLocked` returns an error. It only does when the replica is destroyed (at which point, all proposals are [already rejected](disconnectReplicationRaftMuLocked)) and it does in rare cases when the `propBuf` is full and flushing it returns an error. So I might be making a big deal about a non-issue. Either way, we should fix the code to not look so error-prone.
----
Original Slack discussion: https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1700688914732519?thread_ts=1700675982.566959&cid=C0KB9Q03D
> I’ve been looking into whether raft reproposals might be involved here. I don’t have any real evidence that they are beyond that the leaseholder of the range was not the raft leader and was receiving raft snapshots, which can create more reproposal traffic.
I’ll spell out the theory:
>- n39 is the leaseholder but not the leader
>- n39 evaluates, proposes, and applies the initial version of the intent with `txnDidNotUpdateMeta=true`
>- n39 evaluates and proposes a (hypothetical) update (e.g. ResolveIntent(PENDING), maybe it got pushed[1]) to the intent which sets `txnDidNotUpdateMeta=false`. This raft proposal is committed to the log, but n39 is not the leader and is having trouble communicating with its peers, so it does not apply it immediately
>- n39 attempts to repropose this proposal, maybe because it applies a snapshot, maybe not. The reproposal [fails here](https://github.com/cockroachdb/cockroach/blob/59fb4ec268b456b7c5ad694c43202731f3299efe/pkg/kv/kvserver/replica_raft.go#L1491-L1495). An ambiguous result error is returned to the update of the intent and that request releases its latches[2]. Again, this request committed in the log, but n39 hasn’t heard that yet and hasn’t applied it locally
>- n39 evaluates the EndTxn request and sync resolves the intent. This is allowed because the previous proposal dropped its latches. The EndTxn reads txnDidNotUpdateMeta=true and proposes a write batch to raft with a SingleDel on the key.
We’re non-deterministic from this point on. The SINGLEDEL may or may not delete both SETs on the intent key, depending on whether they clobber each other first. Replicas diverge.
>
>[1]: The missing link here is that the committed version was at the txn’s min timestamp, which is the timestamp that it was originally written at. So there’s no indication that the intent was ever updated.
>[2]: I have been able to create a stats inconsistency by randomly enabling this error path in a kvnemesis run, which gives weight to the idea that there’s something broken on this raft reproposal error-handling path.
Jira issue: CRDB-33844
Epic CRDB-37617
Contributor guide
Assessment
This issue has not been assessed yet.