client/v3: closeRequireLeader leaves an orphaned keepAlives entry that keeps sending keepalive requests forever
type/bug
### Bug report criteria
- [x] This bug report is not security related, security issues should be disclosed privately via security@etcd.io.
- [x] This is not a support request or question, support requests or questions should be raised in the etcd [discussion forums](https://github.com/etcd-io/etcd/discussions).
- [x] You have read the etcd [bug reporting guidelines](https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/reporting_bugs.md).
- [x] Existing open issues along with etcd [frequently asked questions](https://etcd.io/docs/latest/faq) 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:
```go
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:
```go
// 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):
```go
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:
```go
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 `LeaseKeepAliveRequest`s 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.
0 条评论