vtctld: preserve response keyspace/shard when reparent fails before ShardInfo is populated

go/vt/vtctl/grpcvtctldserver/server.go

@@ -1280,6 +1280,27 @@ func (s *VtctldServer) DeleteTablets(ctx context.Context, req *vtctldatapb.Delet
 	return &vtctldatapb.DeleteTabletsResponse{}, nil
 }
 
+// fillReparentResponseFromEvent copies keyspace, shard, and promoted primary from a reparent
+// event into the caller-supplied fields. When ev.ShardInfo is zero-valued (i.e.
+// reparentShardLocked returned before populating it), the caller's default values
+// are preserved unchanged.
+func fillReparentResponseFromEvent(keyspace, shard *string, promotedPrimary **topodatapb.TabletAlias, ev *events.Reparent) {
+	if ev == nil {
+		return
+	}
+	if k, s := ev.ShardInfo.Keyspace(), ev.ShardInfo.ShardName(); k != "" && s != "" {
+		if keyspace != nil {
+			*keyspace = k
+		}
+		if shard != nil {
+			*shard = s
+		}
+	}
+	if promotedPrimary != nil && ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) {
+		*promotedPrimary = ev.NewPrimary.Alias
+	}
+}
+
 // EmergencyReparentShard is part of the vtctldservicepb.VtctldServer interface.
 func (s *VtctldServer) EmergencyReparentShard(ctx context.Context, req *vtctldatapb.EmergencyReparentShardRequest) (resp *vtctldatapb.EmergencyReparentShardResponse, err error) {
 	span, ctx := trace.NewSpan(ctx, "VtctldServer.EmergencyReparentShard")
@@ -1331,15 +1352,7 @@ func (s *VtctldServer) EmergencyReparentShard(ctx context.Context, req *vtctldat
 		Keyspace: req.Keyspace,
 		Shard:    req.Shard,
 	}
-
-	if ev != nil {
-		resp.Keyspace = ev.ShardInfo.Keyspace()
-		resp.Shard = ev.ShardInfo.ShardName()
-
-		if ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) {
-			resp.PromotedPrimary = ev.NewPrimary.Alias
-		}
-	}
+	fillReparentResponseFromEvent(&resp.Keyspace, &resp.Shard, &resp.PromotedPrimary, ev)
 
 	m.RLock()
 	defer m.RUnlock()
@@ -3332,15 +3345,7 @@ func (s *VtctldServer) PlannedReparentShard(ctx context.Context, req *vtctldatap
 		Keyspace: req.Keyspace,
 		Shard:    req.Shard,
 	}
-
-	if ev != nil {
-		resp.Keyspace = ev.ShardInfo.Keyspace()
-		resp.Shard = ev.ShardInfo.ShardName()
-
-		if ev.NewPrimary != nil && !topoproto.TabletAliasIsZero(ev.NewPrimary.Alias) {
-			resp.PromotedPrimary = ev.NewPrimary.Alias
-		}
-	}
+	fillReparentResponseFromEvent(&resp.Keyspace, &resp.Shard, &resp.PromotedPrimary, ev)
 
 	m.RLock()
 	defer m.RUnlock()

go/vt/vtctl/grpcvtctldserver/server_test.go

@@ -52,6 +52,7 @@ import (
 	"vitess.io/vitess/go/vt/topo"
 	"vitess.io/vitess/go/vt/topo/memorytopo"
 	"vitess.io/vitess/go/vt/topo/topoproto"
+	"vitess.io/vitess/go/vt/topotools/events"
 	"vitess.io/vitess/go/vt/vtctl/grpcvtctldserver/testutil"
 	"vitess.io/vitess/go/vt/vtctl/localvtctldclient"
 	"vitess.io/vitess/go/vt/vtctl/schematools"
@@ -4670,6 +4671,81 @@ func TestDeleteTablets(t *testing.T) {
 	}
 }
 
+// TestFillReparentResponseFromEvent exercises fillReparentResponseFromEvent,
+// including the nil-pointer guards on each output parameter.
+func TestFillReparentResponseFromEvent(t *testing.T) {
+	t.Parallel()
+
+	makeEv := func(keyspace, shard string, newPrimary *topodatapb.Tablet) *events.Reparent {
+		ev := &events.Reparent{}
+		ev.ShardInfo = *topo.NewShardInfo(keyspace, shard, &topodatapb.Shard{}, nil)
+		ev.NewPrimary = newPrimary
+		return ev
+	}
+
+	t.Run("nil ev is a no-op", func(t *testing.T) {
+		t.Parallel()
+		k, s := "ks", "-"
+		var pp *topodatapb.TabletAlias
+		fillReparentResponseFromEvent(&k, &s, &pp, nil)
+		assert.Equal(t, "ks", k)
+		assert.Equal(t, "-", s)
+		assert.Nil(t, pp)
+	})
+
+	t.Run("nil keyspace pointer does not panic", func(t *testing.T) {
+		t.Parallel()
+		s := "-"
+		var pp *topodatapb.TabletAlias
+		assert.NotPanics(t, func() {
+			fillReparentResponseFromEvent(nil, &s, &pp, makeEv("ks", "-", nil))
+		})
+		assert.Equal(t, "-", s) // shard still written
+	})
+
+	t.Run("nil shard pointer does not panic", func(t *testing.T) {
+		t.Parallel()
+		k := "ks"
+		var pp *topodatapb.TabletAlias
+		assert.NotPanics(t, func() {
+			fillReparentResponseFromEvent(&k, nil, &pp, makeEv("ks", "-", nil))
+		})
+		assert.Equal(t, "ks", k) // keyspace still written
+	})
+
+	t.Run("nil promotedPrimary pointer does not panic", func(t *testing.T) {
+		t.Parallel()
+		k, s := "ks", "-"
+		tablet := &topodatapb.Tablet{Alias: &topodatapb.TabletAlias{Cell: "zone1", Uid: 100}}
+		assert.NotPanics(t, func() {
+			fillReparentResponseFromEvent(&k, &s, nil, makeEv("ks", "-", tablet))
+		})
+		assert.Equal(t, "ks", k)
+		assert.Equal(t, "-", s)
+	})
+
+	t.Run("populates all fields when ev is fully populated", func(t *testing.T) {
+		t.Parallel()
+		k, s := "default", "0"
+		var pp *topodatapb.TabletAlias
+		tablet := &topodatapb.Tablet{Alias: &topodatapb.TabletAlias{Cell: "zone1", Uid: 100}}
+		fillReparentResponseFromEvent(&k, &s, &pp, makeEv("ks", "-", tablet))
+		assert.Equal(t, "ks", k)
+		assert.Equal(t, "-", s)
+		assert.Equal(t, tablet.Alias, pp)
+	})
+
+	t.Run("preserves defaults when ShardInfo is zero-valued", func(t *testing.T) {
+		t.Parallel()
+		k, s := "ks", "-"
+		var pp *topodatapb.TabletAlias
+		fillReparentResponseFromEvent(&k, &s, &pp, &events.Reparent{}) // zero ShardInfo
+		assert.Equal(t, "ks", k)
+		assert.Equal(t, "-", s)
+		assert.Nil(t, pp)
+	})
+}
+
 func TestEmergencyReparentShard(t *testing.T) {
 	t.Parallel()
 
@@ -4868,6 +4944,57 @@ func TestEmergencyReparentShard(t *testing.T) {
 	}
 }
 
+// TestEmergencyReparentShardResponsePreservesReqOnEarlyFailure is a regression
+// test for the case where reparentShardLocked returns an error before populating
+// ev.ShardInfo. In that situation the response must still carry the keyspace/shard
+// from the request rather than empty strings.
+//
+// The test injects a topo Get error on the shard data path (after the shard lock
+// is acquired) so that GetShard fails before ev.ShardInfo is assigned. With the
+// unfixed code this produces resp.Keyspace == "" and resp.Shard == "".
+func TestEmergencyReparentShardResponsePreservesReqOnEarlyFailure(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+	ts, factory := memorytopo.NewServerAndFactory(ctx, "zone1")
+
+	testutil.AddTablets(ctx, t, ts, &testutil.AddTabletOptions{
+		AlsoSetShardPrimary:  true,
+		ForceSetShardPrimary: true,
+	}, &topodatapb.Tablet{
+		Alias: &topodatapb.TabletAlias{
+			Cell: "zone1",
+			Uid:  100,
+		},
+		Type: topodatapb.TabletType_PRIMARY,
+		PrimaryTermStartTime: &vttime.Time{
+			Seconds: 100,
+		},
+		Keyspace: "testkeyspace",
+		Shard:    "-",
+	})
+
+	// Inject a failure on shard data reads after the shard is created. This causes
+	// reparentShardLocked to fail at GetShard before ev.ShardInfo = *shardInfo is
+	// reached, leaving ev.ShardInfo zero-valued while ev itself is non-nil.
+	factory.AddOperationError(memorytopo.Get, `.*shards/-/Shard$`, topo.NewError(topo.NoNode, "injected"))
+
+	vtctld := testutil.NewVtctldServerWithTabletManagerClient(t, ts, &testutil.TabletManagerClient{}, func(ts *topo.Server) vtctlservicepb.VtctldServer {
+		return NewVtctldServer(vtenv.NewTestEnv(), ts)
+	})
+
+	resp, err := vtctld.EmergencyReparentShard(ctx, &vtctldatapb.EmergencyReparentShardRequest{
+		Keyspace: "testkeyspace",
+		Shard:    "-",
+	})
+	require.Error(t, err)
+	require.NotNil(t, resp)
+	testutil.AssertEmergencyReparentShardResponsesEqual(t, &vtctldatapb.EmergencyReparentShardResponse{
+		Keyspace: "testkeyspace",
+		Shard:    "-",
+	}, resp)
+}
+
 func TestExecuteFetchAsApp(t *testing.T) {
 	t.Parallel()
 
@@ -9011,6 +9138,46 @@ func TestPlannedReparentShard(t *testing.T) {
 			expectEventsToOccur: true,
 			expectedErr:         "global status vars failed",
 		},
+		{
+			// Regression test: when reparentShardLocked returns an error before populating
+			// ev.ShardInfo (e.g. the ExpectedPrimary check fires before ev.ShardInfo is
+			// assigned), the response must still carry the keyspace/shard from the request
+			// rather than empty strings.
+			name: "response preserves keyspace/shard when reparent fails before ShardInfo is populated",
+			ts:   memorytopo.NewServer(ctx, "zone1"),
+			tablets: []*topodatapb.Tablet{
+				{
+					Alias: &topodatapb.TabletAlias{
+						Cell: "zone1",
+						Uid:  100,
+					},
+					Type: topodatapb.TabletType_PRIMARY,
+					PrimaryTermStartTime: &vttime.Time{
+						Seconds: 100,
+					},
+					Keyspace: "testkeyspace",
+					Shard:    "-",
+				},
+			},
+			tmc: &testutil.TabletManagerClient{},
+			req: &vtctldatapb.PlannedReparentShardRequest{
+				Keyspace: "testkeyspace",
+				Shard:    "-",
+				// ExpectedPrimary deliberately points to a non-existent tablet so that
+				// reparentShardLocked fails the expected-primary check before assigning
+				// ev.ShardInfo, leaving ev.ShardInfo zero-valued.
+				ExpectedPrimary: &topodatapb.TabletAlias{
+					Cell: "zone1",
+					Uid:  999,
+				},
+			},
+			expected: &vtctldatapb.PlannedReparentShardResponse{
+				Keyspace: "testkeyspace",
+				Shard:    "-",
+			},
+			expectEventsToOccur: false,
+			expectedErr:         "primary zone1-0000000100 is not equal to expected alias zone1-0000000999",
+		},
 	}
 
 	for _, tt := range tests {
@@ -9041,6 +9208,10 @@ func TestPlannedReparentShard(t *testing.T) {
 
 			if tt.expectedErr != "" {
 				assert.EqualError(t, err, tt.expectedErr)
+				if tt.expected != nil {
+					require.NotNil(t, resp)
+					testutil.AssertPlannedReparentShardResponsesEqual(t, tt.expected, resp)
+				}
 
 				return
 			}