opened
ajm188
username_0
When filing a bug, please include the following headings if
possible. Any example text in this template can be deleted.

#### Overview of the Issue

During the `WaitForAllRelayLogsToApply` step of an ERS, we iterate over the valid candidates, but then [spawn goroutines that refer to the unbound `status` variable](https://github.com/vitessio/vitess/blob/8329bb3bae939ed2ca5476f019e46c469f106f02/go/vt/vtctl/reparentutil/emergency_reparenter.go#L376-L386). Depending on the iteration order, and the exact scheduling of those goroutines, this can cause a `status` to get paired with the wrong tablet, meaning that if there was a "bad" status, we could accidentally skip over it, and think that a tablet had all its relay logs apply, when in fact it did not.

(Inlined code section for readability. I've added some additional commentary):

```go
	for candidate := range validCandidates {
		// This status changes on each iteration of the for loop,
		// but we don't bind it to the closure below.
		status, ok := statusMap[candidate]
		if !ok {
			erp.logger.Infof("EmergencyReparent candidate %v not in replica status map; this means it was not running replication (because it was formerly MASTER), so skipping WaitForRelayLogsToApply step for this candidate", candidate)
			continue
		}

		go func(alias string) {
			var err error
			defer func() { errCh <- err }()
			// status here _could_ refer to the status on the iteration that spawned this goroutine
			// or **any iteration from this point onward**
			err = WaitForRelayLogsToApply(groupCtx, erp.tmc, tabletMap[alias], status)
		}(candidate)
```

#### Reproduction Steps

I haven't actually been able to reproduce the bug in this code block, but it's definitely wrong.

[Here](https://play.golang.org/p/oK3DWwRzymd) is a go playground with a toy example demonstrating the unbound variable + goroutine issue.
[reparentutil | ERS] Unbound variable referenced in closure in loop
issue
closed
deepthi
username_1
issue