Don't you need to receive the results from all three tablets here?

I've not dug into why, but adding a third channel read causes the test to be stuck here. 🤷

because Subscribe returns a channel with a buffer of size 2 😅. Clearly, we were expecting that subscribers will process the results in a timely manner.
We could increase the channel buffer to 10, or the test can interleave receiving from the channel with the calls to AddTablet like so

        hc.AddTablet(firstTablet)
	<-resultChan
	hc.AddTablet(secondTablet)
	<-resultChan
	hc.AddTablet(thirdTablet)
	<-resultChan

Why does removing an unrelated tablet trigger the bug?

Oh I see this

When RemoveTablet was called, the list of healthy tablets was recomputed, but the logic did not account for the fact that there can only be a single entry in the list of healthy primary tablets.

This comment is wrong, it should be

This is an interesting bug. We have safeguards in updateHealth which make sure that there is exactly one element in healthy for the PRIMARY tablet type, but the recomputation here had no such checks.
While the fix seems correct, what do you think about pushing it into recomputeHealthy instead of creating a new function call here?
Right now this is the only call site where recomputeHealthy is called for the PRIMARY tablet type, but it seems more compact and future proof to change recomputeHealthy to maintain the invariant.

While the fix seems correct, what do you think about pushing it into recomputeHealthy instead of creating a new function call here?

I feel like having two separate methods helps highlighting the difference between the recomputation logic for primaries vs any other replica type more clearly.

I was thinking that maybe having a recomputeHealthy method that then calls out to either recomputeHealthyReplicas and recomputeHealthyPrimaries would probably be the best for clarity (and the go compiler should just inline all of these calls anyway, so not a performance issue either).

But then I noticed that pushing down the tablet type check into recomputeHealth would require passing down the key and tabletType as arguments, which seems redundant, because key contains the tablet type already, just as a string. Not passing down key would require passing down other arguments to be able to build the key, and then things just start to become more messy.

As I mentioned in Slack, I'm very interested giving the healthcheck code a polishing pass separately from this fix, so I'd suggest we punt on this for now. 🙇‍♂️ What do you think?

I can see that instead of passing the "key", you would now have to pass the target. When this was written, it just seemed convenient to use the key because it has already been computed. So we have

			hc.recomputeHealthy(targetKey)

However, later on, we are doing a key computation for the second call

			oldTargetKey := KeyFromTarget(prevTarget)
			hc.recomputeHealthy(oldTargetKey)

It does not seem too invasive to change recomputeHealthy to be

func (hc *HealthCheckImpl) recomputeHealthy(target *query.Target) {
			key := KeyFromTarget(target)
			if target.TabletType == ..._PRIMARY {
			...
}

I do also see issues with the function name recomputeHealthyReplicas. replica is an overloaded term, and it could mean replicas in the sense of replicating from the primary, or it could mean the specific tablet type (as opposed to RDONLY and other tablet types). Refactoring it this way avoids that naming issue as well.

Thinking about this some more..

• we are deleting a tablet. if that tablet's type is not PRIMARY, we should not even "recompute the healthy primary"
• even if that tablet's type is PRIMARY, why should we iterate over the healthData for PRIMARY? we know that at any point there should only be one "healthy" primary. i.e healthy[PRIMARY] should have a size of either 0 or 1. If the tablet that we are deleting is THE current healthy primary, we should just set healthy[PRIMARY] to an empty slice. If it is not, we should do nothing.

It seems to me that the problem with the original implementation of this defer func was that it was calling recomputeHealthy for PRIMARY type (which was never done before this defer func was added) instead of handling PRIMARY separately. This caused it to violate the assumption that there is at most one healthy PRIMARY at any point in time.

Doing this may have the effect of "promoting" an old primary?