Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via security@etcd.io.
• This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• You have read the etcd bug reporting guidelines.
• Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

Following up on #22081, I found a second, separate bug in the same function while double checking it.

When closeRequireLeader() removes every subscriber attached to a lease (all of them used clientv3.WithRequireLeader), it compacts keepAlive.chs and keepAlive.ctxs down to length zero, but it never deletes the entry from l.keepAlives. Here's the function, client/v3/lease.go around line 383:

func (l *lessor) closeRequireLeader() {
	l.mu.Lock()
	defer l.mu.Unlock()
	for _, ka := range l.keepAlives {
		reqIdxs := 0
		for i, ctx := range ka.ctxs {
			...
			close(ka.chs[i])
			ka.chs[i] = nil
			reqIdxs++
		}
		if reqIdxs == 0 {
			continue
		}
		newChs := make([]chan<- *LeaseKeepAliveResponse, len(ka.chs)-reqIdxs)
		newCtxs := make([]context.Context, len(newChs))
		newIdx := 0
		for i := range ka.chs {
			if ka.chs[i] == nil {
				continue
			}
			newChs[newIdx], newCtxs[newIdx] = ka.chs[i], ka.ctxs[i]
			newIdx++
		}
		ka.chs, ka.ctxs = newChs, newCtxs
	}
}

If reqIdxs ends up equal to the original length of ka.chs, newChs/newCtxs are both zero length, but ka stays sitting in l.keepAlives[id]. Compare with keepAliveCtxCloser, a few lines above, which does this correctly:

// remove if no one more listeners
if len(ka.chs) == 0 {
	delete(l.keepAlives, id)
}

closeRequireLeader has no equivalent check.

The effect of leaving that entry behind is worse than it sounds. In recvKeepAlive (around line 526):

ka.deadline = time.Now().Add(time.Duration(karesp.TTL) * time.Second)   // unconditional
for _, ch := range ka.chs {
	...
	ka.nextKeepAlive = nextKeepAlive   // only runs if len(ka.chs) > 0
}

ka.deadline refreshes on every server response no matter what, but ka.nextKeepAlive only advances inside the loop over ka.chs. Once ka.chs is empty, that loop runs zero times, so nextKeepAlive is frozen wherever it was left. sendKeepAliveLoop (around line 593) keeps checking ka.nextKeepAlive.Before(now) on a timer and, since it's stuck in the past, keeps sending a real LeaseKeepAliveRequest for that lease roughly every 500ms. The server answers normally, which refreshes deadline again, so deadlineLoop's reaper never times it out either. The whole thing is self sustaining, it only stops if the client is closed entirely, or if the application happens to later cancel one of the original (already detached) subscriber contexts, which triggers a keepAliveCtxCloser call that finds no match in the already compacted ka.ctxs but still falls through to its own len(ka.chs) == 0 cleanup check as a side effect.

What did you expect to happen?

Once closeRequireLeader removes every subscriber for a given lease id, the corresponding entry in l.keepAlives should be deleted, the same way keepAliveCtxCloser already does when it empties out ka.chs. No keepalive requests should keep going out for a lease that has zero local subscribers left.

How can we reproduce it (as minimally and precisely as possible)?

This can be shown directly against the internal lessor/keepAlive types, no live cluster needed:

ka := &keepAlive{
	chs:   []chan<- *LeaseKeepAliveResponse{ch}, // single subscriber, requireLeader
	ctxs:  []context.Context{ctxWithRequireLeader},
	donec: make(chan struct{}),
}
l := &lessor{keepAlives: map[LeaseID]*keepAlive{1: ka}}

l.closeRequireLeader()

survivor, ok := l.keepAlives[1]
// ok is true, the entry is still there
// len(survivor.chs) is 0, there is nobody left to deliver to

l.recvKeepAlive(&pb.LeaseKeepAliveResponse{ID: 1, TTL: 60})
// survivor.nextKeepAlive is unchanged, because the loop that would
// have advanced it never runs on a zero length ka.chs

// sendKeepAliveLoop's polling loop will keep treating id 1 as due
// and resend a LeaseKeepAliveRequest for it forever

To hit this for real: call KeepAlive() on a lease from one or more goroutines where every one of them uses clientv3.WithRequireLeader(ctx), then cause that member to lose its leader so closeRequireLeader() runs and empties the whole entry. Watch the connection afterward and you'll keep seeing LeaseKeepAliveRequests go out for that lease id even though nothing local is listening anymore.

Anything else we need to know?

This is a follow up to #22081, found while re-reading the same function after fixing that one. It's a separate root cause though, missing cleanup rather than wrong indexing, so I'm filing it separately.

Etcd version

Not applicable in the usual sense, this reproduces with a standalone Go program against the client/v3 package internals, no running etcd server needed.