kvserver: update splitPreApply comments

Epic: none
Release note: none

Author: pav-kv

pkg/kv/kvserver/store_split.go

@@ -46,12 +46,14 @@ func splitPreApply(
 			r.StoreID(), split)
 	}
 
-	// Check on the RHS, we need to ensure that it exists and has a minReplicaID
-	// less than or equal to the replica we're about to initialize.
+	// Obtain the RHS replica. In the common case, it exists and its ReplicaID
+	// matches the one in the split trigger. It is the uninitialized replica that
+	// has just been created or obtained in Replica.acquireSplitLock, and its
+	// raftMu is locked.
 	//
-	// The right hand side of the split was already created (and its raftMu
-	// acquired) in Replica.acquireSplitLock. It must be present here if it hasn't
-	// been removed in the meantime (handled below).
+	// In the less common case, the ReplicaID is already removed from this Store,
+	// and rightRepl is either nil or an uninitialized replica with a higher
+	// ReplicaID. Its raftMu is not locked.
 	rightRepl := r.store.GetReplicaIfExists(split.RightDesc.RangeID)
 	// Check to see if we know that the RHS has already been removed from this
 	// store at the replica ID implied by the split.
@@ -76,6 +78,8 @@ func splitPreApply(
 		// it's always present).
 		var hs raftpb.HardState
 		if rightRepl != nil {
+			// TODO(pav-kv): rightRepl could have been destroyed by the time we get to
+			// lock it here. The HardState read-then-write appears risky in this case.
 			rightRepl.raftMu.Lock()
 			defer rightRepl.raftMu.Unlock()
 			// Assert that the rightRepl is not initialized. We're about to clear out
@@ -90,6 +94,10 @@ func splitPreApply(
 				log.Fatalf(ctx, "failed to load hard state for removed rhs: %v", err)
 			}
 		}
+		// TODO(#152199): the rightRepl == nil condition is flaky. There can be a
+		// racing replica creation for a higher ReplicaID, and it can subsequently
+		// update its HardState. Here, we can accidentally clear the HardState of
+		// that new replica.
 		if err := kvstorage.ClearRangeData(ctx, split.RightDesc.RangeID, readWriter, readWriter, kvstorage.ClearRangeDataOptions{
 			// We know there isn't anything in these two replicated spans below in the
 			// right-hand side (before the current batch), so setting these options
@@ -99,19 +107,17 @@ func splitPreApply(
 			ClearReplicatedByRangeID: true,
 			// See the HardState write-back dance above and below.
 			//
-			// TODO(tbg): we don't actually want to touch the raft state of the right
-			// hand side replica since it's absent or a more recent replica than the
-			// split. Now that we have a boolean targeting the unreplicated
-			// RangeID-based keyspace, we can set this to false and remove the
-			// HardState+ReplicaID write-back. (The WriteBatch does not contain
-			// any writes to the unreplicated RangeID keyspace for the RHS, see
-			// splitTriggerHelper[^1]).
+			// TODO(tbg): we don't actually want to touch the raft state of the RHS
+			// replica since it's absent or a more recent one than in the split. Now
+			// that we have a bool targeting unreplicated RangeID-local keys, we can
+			// set it to false and remove the HardState+ReplicaID write-back. However,
+			// there can be historical split proposals with the RaftTruncatedState key
+			// set in splitTriggerHelper[^1]. We must first make sure that such
+			// proposals no longer exist, e.g. with a below-raft migration.
 			//
 			// [^1]: https://github.com/cockroachdb/cockroach/blob/f263a765d750e41f2701da0a923a6e92d09159fa/pkg/kv/kvserver/batcheval/cmd_end_transaction.go#L1109-L1149
 			//
-			// See also:
-			//
-			// https://github.com/cockroachdb/cockroach/issues/94933
+			// See also: https://github.com/cockroachdb/cockroach/issues/94933
 			ClearUnreplicatedByRangeID: true,
 		}); err != nil {
 			log.Fatalf(ctx, "failed to clear range data for removed rhs: %v", err)