storage: improve safety of merge lock

Fix a known correctness bug in the merge locking scheme. See the comment
updates within for details.

As an added benefit, this fix frees store.RemoveReplica of conditionals
to deal with uninitialized replicas. Acquiring the merge lock now always
results in an initialized right-hand replica, so store.MergeRange is
always removing an initialized right-hand replica.

There are no tests for this because testing this particular edge case is
quite difficult and I'm primarily interested in avoiding the obviously
wrong call to batch.ClearRange(nil, nil) that occurs when calling
store.RemoveReplica on an unintialized replica. This nonsensical
ClearRange is, somehow, not currently an error, but it is preventing
landing.

Release note: None

Author: benesch

pkg/storage/replica.go

@@ -5181,33 +5181,38 @@ func (r *Replica) acquireMergeLock(
 	ctx context.Context, merge *roachpb.MergeTrigger,
 ) (func(storagebase.ReplicatedEvalResult), error) {
 	// The merge lock is the right-hand replica's raftMu. If the right-hand
-	// replica does not exist, we create an uninitialized replica. This is
-	// required for correctness: without the uninitialized replica, an incoming
-	// snapshot could create the right-hand replica before the merge trigger has a
-	// chance to widen the left-hand replica's end key. The merge trigger would
-	// then fatal the node upon realizing the right-hand replica already exists.
-	// With an uninitialized and locked right-hand replica in place, any snapshots
-	// for the right-hand range will block on raftMu, waiting for the merge to
-	// complete, after which the replica will realize it has been destroyed and
-	// reject the snapshot.
-	//
-	// TODO(benesch): if the right-hand replica splits, we could probably see a
-	// snapshot for the right-hand side of the right-hand side. The uninitialized
-	// replica this method installs will not block this snapshot from applying as
-	// the snapshot's range ID will be different. Add a placeholder to the
-	// replicasByKey map as well to be safe.
-	//
-	// TODO(benesch): make terminology for uninitialized replicas, which protect a
-	// range ID, and placeholder replicas, which protect a key range, consistent.
-	// Several code comments refer to "uninitialized placeholders," which is
-	// downright confusing.
-	rightRng, _, err := r.store.getOrCreateReplica(ctx, merge.RightDesc.RangeID, 0, nil)
+	// replica does not exist, we create one. This is required for correctness.
+	// Without a right-hand replica on this store, an incoming snapshot could
+	// create the right-hand replica before the merge trigger has a chance to
+	// widen the left-hand replica's end key. The merge trigger would then fatal
+	// the node upon realizing the right-hand replica already exists. With a
+	// right-hand replica in place, any snapshots for the right-hand range will
+	// block on raftMu, waiting for the merge to complete, after which the replica
+	// will realize it has been destroyed and reject the snapshot.
+	rightRepl, created, err := r.store.getOrCreateReplica(ctx, merge.RightDesc.RangeID, 0, nil)
 	if err != nil {
 		return nil, err
 	}
+	if created {
+		// rightRepl does not know its start and end keys. It can only block
+		// incoming snapshots with the same range ID. This is an insufficient lock:
+		// it's possible for this store to receive a snapshot with a different range
+		// ID that overlaps the right-hand keyspace while the merge is in progress.
+		// Consider the case where this replica is behind and the merged range
+		// resplits before this replica processes the merge. An up-to-date member of
+		// the new right-hand range could send this store a snapshot that would
+		// incorrectly sneak past rightRepl.
+		//
+		// So install a placeholder for the right range's keyspace in the
+		// replicasByKey map to block any intersecting snapshots.
+		placeholder := &ReplicaPlaceholder{rangeDesc: merge.RightDesc}
+		if err := r.store.addPlaceholder(placeholder); err != nil {
+			return nil, err
+		}
+	}
 
 	return func(storagebase.ReplicatedEvalResult) {
-		rightRng.raftMu.Unlock()
+		rightRepl.raftMu.Unlock()
 	}, nil
 }
 

pkg/storage/store.go

@@ -2370,8 +2370,14 @@ func (s *Store) addReplicaInternalLocked(repl *Replica) error {
 	return nil
 }
 
+func (s *Store) addPlaceholder(placeholder *ReplicaPlaceholder) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.addPlaceholderLocked(placeholder)
+}
+
 // addPlaceholderLocked adds the specified placeholder. Requires that Store.mu
-// and Replica.raftMu are held.
+// is held.
 func (s *Store) addPlaceholderLocked(placeholder *ReplicaPlaceholder) error {
 	rangeID := placeholder.Desc().RangeID
 	if exRng := s.mu.replicasByKey.ReplaceOrInsert(placeholder); exRng != nil {
@@ -2386,15 +2392,15 @@ func (s *Store) addPlaceholderLocked(placeholder *ReplicaPlaceholder) error {
 
 // removePlaceholder removes a placeholder for the specified range if it
 // exists, returning true if a placeholder was present and removed and false
-// otherwise. Requires that Replica.raftMu is held.
+// otherwise.
 func (s *Store) removePlaceholder(ctx context.Context, rngID roachpb.RangeID) bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	return s.removePlaceholderLocked(ctx, rngID)
 }
 
 // removePlaceholderLocked removes the specified placeholder. Requires that
-// Store.mu and Replica.raftMu are held.
+// Store.mu is held.
 func (s *Store) removePlaceholderLocked(ctx context.Context, rngID roachpb.RangeID) bool {
 	placeholder, ok := s.mu.replicaPlaceholders[rngID]
 	if !ok {
@@ -2527,8 +2533,12 @@ func (s *Store) removeReplicaImpl(
 	rep.mu.Unlock()
 	rep.readOnlyCmdMu.Unlock()
 
-	if err := rep.destroyRaftMuLocked(ctx, consistentDesc, opts.DestroyData); err != nil {
-		return err
+	// Uninitialized replicas don't know their start and keys. It is both
+	// incorrect and unnecessary to destroy their data.
+	if rep.IsInitialized() {
+		if err := rep.destroyRaftMuLocked(ctx, consistentDesc, opts.DestroyData); err != nil {
+			return err
+		}
 	}
 
 	s.mu.Lock()