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

@@ -5134,33 +5134,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 the right-hand descriptor from the merge trigger in
+		// rightRepl. This adds rightRepl to the replicasByKey map so that snapshots
+		// for overlapping keyspaces will block on its raftMu.
+		if err := rightRepl.setDesc(&merge.RightDesc); err != nil {
+			return nil, err
+		}
+	}
 
 	return func(storagebase.ReplicatedEvalResult) {
-		rightRng.raftMu.Unlock()
+		rightRepl.raftMu.Unlock()
 	}, nil
 }
 

pkg/storage/store.go

@@ -2317,8 +2317,7 @@ func (s *Store) MergeRange(
 	// call removeReplicaImpl directly to avoid deadlocking on the right-hand
 	// replica's raftMu.
 	if err := s.removeReplicaImpl(ctx, rightRepl, rightDesc, RemoveOptions{
-		DestroyData:       false,
-		AllowPlaceholders: true,
+		DestroyData: false,
 	}); err != nil {
 		return errors.Errorf("cannot remove range: %s", err)
 	}
@@ -2426,8 +2425,7 @@ func (s *Store) addReplicaToRangeMapLocked(repl *Replica) error {
 
 // RemoveOptions bundles boolean parameters for Store.RemoveReplica.
 type RemoveOptions struct {
-	DestroyData       bool
-	AllowPlaceholders bool
+	DestroyData bool
 }
 
 // RemoveReplica removes the replica from the store's replica map and
@@ -2439,9 +2437,6 @@ type RemoveOptions struct {
 // If opts.DestroyData is true, data in all of the range's keyspaces
 // is deleted. Otherwise, only data in the range-ID local keyspace is
 // deleted. In either case a tombstone record is written.
-//
-// If opts.AllowPlaceholders is true, it is not an error if the replica
-// targeted for removal is an uninitialized placeholder.
 func (s *Store) RemoveReplica(
 	ctx context.Context, rep *Replica, consistentDesc roachpb.RangeDescriptor, opts RemoveOptions,
 ) error {
@@ -2498,14 +2493,12 @@ func (s *Store) removeReplicaImpl(
 		s.mu.Unlock()
 		return err
 	}
-	if !opts.AllowPlaceholders {
-		if placeholder := s.getOverlappingKeyRangeLocked(desc); placeholder != rep {
-			// This is a fatal error because uninitialized replicas shouldn't make it
-			// this far. This method will need some changes when we introduce GC of
-			// uninitialized replicas.
-			s.mu.Unlock()
-			log.Fatalf(ctx, "replica %+v unexpectedly overlapped by %+v", rep, placeholder)
-		}
+	if placeholder := s.getOverlappingKeyRangeLocked(desc); placeholder != rep {
+		// This is a fatal error because uninitialized replicas shouldn't make it
+		// this far. This method will need some changes when we introduce GC of
+		// uninitialized replicas.
+		s.mu.Unlock()
+		log.Fatalf(ctx, "replica %+v unexpectedly overlapped by %+v", rep, placeholder)
 	}
 	// Adjust stats before calling Destroy. This can be called before or after
 	// Destroy, but this configuration helps avoid races in stat verification
@@ -2539,12 +2532,10 @@ func (s *Store) removeReplicaImpl(
 	s.mu.replicas.Delete(int64(rep.RangeID))
 	delete(s.mu.uninitReplicas, rep.RangeID)
 	s.replicaQueues.Delete(int64(rep.RangeID))
-	if !opts.AllowPlaceholders {
-		if placeholder := s.mu.replicasByKey.Delete(rep); placeholder != rep {
-			// We already checked that our replica was present in replicasByKey
-			// above. Nothing should have been able to change that.
-			log.Fatalf(ctx, "replica %+v unexpectedly overlapped by %+v", rep, placeholder)
-		}
+	if placeholder := s.mu.replicasByKey.Delete(rep); placeholder != rep {
+		// We already checked that our replica was present in replicasByKey
+		// above. Nothing should have been able to change that.
+		log.Fatalf(ctx, "replica %+v unexpectedly overlapped by %+v", rep, placeholder)
 	}
 	delete(s.mu.replicaPlaceholders, rep.RangeID)
 	// TODO(peter): Could release s.mu.Lock() here.