EmergencyReparentShard: skip zero-position candidates in errant GTID detection

go/vt/vtctl/reparentutil/emergency_reparenter.go

@@ -1347,10 +1347,23 @@ func (erp *EmergencyReparenter) findErrantGTIDs(
 		return maps.Clone(validCandidates), nil, nil
 	}
 
+	// A tablet with nil or zero positions has no GTIDs to corroborate anyone and can't be
+	// promoted over tablets with real history, so it is dropped from candidacy up front:
+	// letting a wiped tablet that kept its reparent journal rows define the evidence tier
+	// would leave the real candidates compared against nothing at all.
+	nonZeroCandidates := make(map[string]*RelayLogPositions, len(validCandidates))
+	for alias, positions := range validCandidates {
+		if positions == nil || positions.IsZero() {
+			erp.logger.Warningf("skipping candidate %s during errant GTID detection: nil or zero positions", alias)
+			continue
+		}
+		nonZeroCandidates[alias] = positions
+	}
+
 	// First we need to collect the reparent journal length for all the candidates.
 	// This will tell us, which of the tablets are severly lagged, and haven't even seen all the primary promotions.
 	// Such severely lagging tablets cannot be used to find errant GTIDs in other tablets, seeing that they themselves don't have enough information.
-	reparentJournalLen, err := erp.gatherReparenJournalInfo(ctx, validCandidates, tabletMap, waitReplicasTimeout)
+	reparentJournalLen, err := erp.gatherReparenJournalInfo(ctx, nonZeroCandidates, tabletMap, waitReplicasTimeout)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -1374,12 +1387,7 @@ func (erp *EmergencyReparenter) findErrantGTIDs(
 	var starvedCandidates []string
 	updatedValidCandidates := make(map[string]*RelayLogPositions)
 	for _, candidate := range maxLenCandidates {
-		candidatePositions := validCandidates[candidate]
-		if candidatePositions == nil {
-			erp.logger.Warningf("skipping candidate %s during errant GTID detection: nil or zero positions", candidate)
-			continue
-		}
-
+		candidatePositions := nonZeroCandidates[candidate]
 		status, ok := statusMap[candidate]
 		if !ok {
 			// If the tablet is not in the status map, and has the maximum length of the reparent journal,
@@ -1393,11 +1401,7 @@ func (erp *EmergencyReparenter) findErrantGTIDs(
 			// Even in this case, the best we can do is not run errant GTID detection on either, and let the split brain detection code
 			// deal with it, if A in fact has errant GTIDs.
 			maxLenPositions = append(maxLenPositions, candidatePositions.Combined)
-			updatedValidCandidates[candidate] = validCandidates[candidate]
-			continue
-		}
-		if candidatePositions.IsZero() {
-			erp.logger.Warningf("skipping candidate %s during errant GTID detection: nil or zero positions", candidate)
+			updatedValidCandidates[candidate] = candidatePositions
 			continue
 		}
 		// Store all the other candidate's positions so that we can run errant GTID detection using them.
@@ -1406,10 +1410,7 @@ func (erp *EmergencyReparenter) findErrantGTIDs(
 			if otherCandidate == candidate {
 				continue
 			}
-			otherPosition := validCandidates[otherCandidate]
-			if otherPosition != nil && !otherPosition.IsZero() {
-				otherPositions = append(otherPositions, otherPosition.Combined)
-			}
+			otherPositions = append(otherPositions, nonZeroCandidates[otherCandidate].Combined)
 		}
 		otherPositions = append(otherPositions, extraEvidence...)
 		// FindErrantGTIDs accepts a candidate's GTID set as-is when there is nothing to
@@ -1429,7 +1430,7 @@ func (erp *EmergencyReparenter) findErrantGTIDs(
 			continue
 		}
 		maxLenPositions = append(maxLenPositions, candidatePositions.Combined)
-		updatedValidCandidates[candidate] = validCandidates[candidate]
+		updatedValidCandidates[candidate] = candidatePositions
 	}
 
 	// The extra evidence positions also corroborate the lagged tablets below.
@@ -1457,10 +1458,7 @@ func (erp *EmergencyReparenter) findErrantGTIDs(
 		// This exact scenario outlined above, can be found in the test for this function, subtest `Case 5a`.
 		// The idea is that if the tablet is lagged, then even the server UUID that it is replicating from
 		// should not be considered a valid source of writes that no other tablet has.
-		candidatePositions := validCandidates[alias]
-		if candidatePositions == nil || candidatePositions.IsZero() {
-			continue
-		}
+		candidatePositions := nonZeroCandidates[alias]
 		errantGTIDs, err := replication.FindErrantGTIDs(candidatePositions.Combined, replication.SID{}, maxLenPositions)
 		if err != nil {
 			return nil, nil, err

go/vt/vtctl/reparentutil/emergency_reparenter_test.go

@@ -9989,8 +9989,8 @@ func TestEmergencyReparenterFindErrantGTIDs(t *testing.T) {
 // for a bug where a nil *RelayLogPositions entry in validCandidates would
 // cause a nil pointer panic. The test includes:
 //   - zone1-0000000102: valid candidate with max reparent journal length
-//   - zone1-0000000103: nil position with max reparent journal length (exercises the maxLenCandidates loop)
-//   - zone1-0000000104: nil position with a lower reparent journal length (exercises the lagged-candidate loop)
+//   - zone1-0000000103: nil position with max reparent journal length
+//   - zone1-0000000104: nil position with a lower reparent journal length
 func TestEmergencyReparenterFindErrantGTIDs_NilPosition(t *testing.T) {
 	u1 := "00000000-0000-0000-0000-000000000001"
 	erp := NewEmergencyReparenter(nil, &testutil.TabletManagerClient{
@@ -10053,10 +10053,8 @@ func TestEmergencyReparenterFindErrantGTIDs_NilPosition(t *testing.T) {
 		},
 	}
 	// Construct validCandidates with nil entries for zone1-0000000103 and
-	// zone1-0000000104. zone1-0000000103 has the same reparent journal length
-	// as zone1-0000000102 (maxLen), so it exercises the nil guard in the
-	// maxLenCandidates loop. zone1-0000000104 has a lower reparent journal
-	// length, so it exercises the nil guard in the lagged-candidate loop.
+	// zone1-0000000104, at max and lower reparent journal lengths respectively;
+	// both must be dropped from candidacy before the evidence tier is computed.
 	validCandidates := map[string]*RelayLogPositions{
 		"zone1-0000000102": {
 			Combined: replication.MustParsePosition(replication.Mysql56FlavorID, u1+":1-100"),
@@ -10072,3 +10070,162 @@ func TestEmergencyReparenterFindErrantGTIDs_NilPosition(t *testing.T) {
 	// accepted without any comparison
 	assert.ElementsMatch(t, []string{"zone1-0000000102"}, starved)
 }
+
+// TestEmergencyReparenterFindErrantGTIDs_EmptyPrimaryPosition is a regression test
+// for a bug where a demoted primary with a zero GTID position but the maximum reparent
+// journal count (its GTID state was wiped while the journal table kept its rows) was
+// accepted as a candidate and its empty position added to the evidence set. Empty
+// evidence corroborates nothing, so every GTID on a lagged replica was flagged errant,
+// leaving the empty primary as the only candidate left to promote.
+func TestEmergencyReparenterFindErrantGTIDs_EmptyPrimaryPosition(t *testing.T) {
+	u1 := "00000000-0000-0000-0000-000000000001"
+	emptyPos, err := replication.DecodePosition("MySQL56/")
+	require.NoError(t, err)
+	require.True(t, emptyPos.IsZero())
+
+	erp := NewEmergencyReparenter(nil, &testutil.TabletManagerClient{
+		ReadReparentJournalInfoResults: map[string]int32{
+			"zone1-0000000100": 2,
+			"zone1-0000000101": 1,
+		},
+	}, nil)
+	tabletMap := map[string]*topo.TabletInfo{
+		"zone1-0000000100": {
+			Tablet: &topodatapb.Tablet{
+				Hostname: "zone1-0000000100",
+				Alias: &topodatapb.TabletAlias{
+					Cell: "zone1",
+					Uid:  100,
+				},
+				Type: topodatapb.TabletType_PRIMARY,
+			},
+		},
+		"zone1-0000000101": {
+			Tablet: &topodatapb.Tablet{
+				Hostname: "zone1-0000000101",
+				Alias: &topodatapb.TabletAlias{
+					Cell: "zone1",
+					Uid:  101,
+				},
+				Type: topodatapb.TabletType_REPLICA,
+			},
+		},
+	}
+	// The demoted primary is not in statusMap: it answered the stop-replication phase
+	// as a primary.
+	statusMap := map[string]*replicationdatapb.StopReplicationStatus{
+		"zone1-0000000101": {
+			After: &replicationdatapb.Status{
+				RelayLogPosition: getRelayLogPosition("1-100"),
+				SourceUuid:       u1,
+			},
+		},
+	}
+	validCandidates := map[string]*RelayLogPositions{
+		"zone1-0000000100": {Combined: emptyPos},
+		"zone1-0000000101": {
+			Combined: replication.MustParsePosition(replication.Mysql56FlavorID, u1+":1-100"),
+		},
+	}
+
+	candidates, starved, err := erp.findErrantGTIDs(t.Context(), validCandidates, statusMap, tabletMap, 10*time.Second, nil)
+	require.NoError(t, err)
+	// the empty primary contributed no evidence, so the lagged replica must be accepted
+	// as-is rather than have its entire GTID set flagged errant
+	require.Contains(t, candidates, "zone1-0000000101")
+	// a candidate with no GTIDs corroborates nothing and cannot be promoted over tablets
+	// with real history, so it is dropped from candidacy
+	assert.NotContains(t, candidates, "zone1-0000000100")
+	// with the empty primary dropped, the replica forms the evidence tier on its own and
+	// was accepted with nothing to compare against; report it starved so the caller can
+	// decide whether the blind spot is acceptable
+	assert.ElementsMatch(t, []string{"zone1-0000000101"}, starved)
+}
+
+// TestEmergencyReparenterFindErrantGTIDs_EmptyPrimaryErrantReplica covers the case where
+// the only max-reparent-journal candidate has a zero GTID position and the remaining
+// candidates disagree. Dropping the wiped tablet must not leave the real candidates
+// compared against nothing: with the wiped tablet out of the way the replicas form the
+// evidence tier and are compared against each other, catching the errant one.
+func TestEmergencyReparenterFindErrantGTIDs_EmptyPrimaryErrantReplica(t *testing.T) {
+	u1 := "00000000-0000-0000-0000-000000000001"
+	u3 := "00000000-0000-0000-0000-000000000003"
+	emptyPos, err := replication.DecodePosition("MySQL56/")
+	require.NoError(t, err)
+	require.True(t, emptyPos.IsZero())
+
+	erp := NewEmergencyReparenter(nil, &testutil.TabletManagerClient{
+		ReadReparentJournalInfoResults: map[string]int32{
+			"zone1-0000000100": 2,
+			"zone1-0000000101": 1,
+			"zone1-0000000102": 1,
+		},
+	}, nil)
+	tabletMap := map[string]*topo.TabletInfo{
+		"zone1-0000000100": {
+			Tablet: &topodatapb.Tablet{
+				Hostname: "zone1-0000000100",
+				Alias: &topodatapb.TabletAlias{
+					Cell: "zone1",
+					Uid:  100,
+				},
+				Type: topodatapb.TabletType_PRIMARY,
+			},
+		},
+		"zone1-0000000101": {
+			Tablet: &topodatapb.Tablet{
+				Hostname: "zone1-0000000101",
+				Alias: &topodatapb.TabletAlias{
+					Cell: "zone1",
+					Uid:  101,
+				},
+				Type: topodatapb.TabletType_REPLICA,
+			},
+		},
+		"zone1-0000000102": {
+			Tablet: &topodatapb.Tablet{
+				Hostname: "zone1-0000000102",
+				Alias: &topodatapb.TabletAlias{
+					Cell: "zone1",
+					Uid:  102,
+				},
+				Type: topodatapb.TabletType_REPLICA,
+			},
+		},
+	}
+	// The demoted primary is not in statusMap: it answered the stop-replication phase
+	// as a primary.
+	statusMap := map[string]*replicationdatapb.StopReplicationStatus{
+		"zone1-0000000101": {
+			After: &replicationdatapb.Status{
+				RelayLogPosition: getRelayLogPosition("1-100"),
+				SourceUuid:       u1,
+			},
+		},
+		"zone1-0000000102": {
+			After: &replicationdatapb.Status{
+				RelayLogPosition: getRelayLogPosition("1-100", "", "1"),
+				SourceUuid:       u1,
+			},
+		},
+	}
+	validCandidates := map[string]*RelayLogPositions{
+		"zone1-0000000100": {Combined: emptyPos},
+		"zone1-0000000101": {
+			Combined: replication.MustParsePosition(replication.Mysql56FlavorID, u1+":1-100"),
+		},
+		"zone1-0000000102": {
+			Combined: replication.MustParsePosition(replication.Mysql56FlavorID, u1+":1-100,"+u3+":1"),
+		},
+	}
+
+	candidates, starved, err := erp.findErrantGTIDs(t.Context(), validCandidates, statusMap, tabletMap, 10*time.Second, nil)
+	require.NoError(t, err)
+	require.Contains(t, candidates, "zone1-0000000101")
+	// no other tablet corroborates u3:1, so zone1-0000000102 has an errant GTID and
+	// must not survive detection
+	assert.NotContains(t, candidates, "zone1-0000000102")
+	assert.NotContains(t, candidates, "zone1-0000000100")
+	// both replicas had a peer to compare against, so nobody was accepted blindly
+	assert.Empty(t, starved)
+}