cockroachdb / cockroachdb/cockroach
tenantcapabilitieswatcher: removeEntryForTenantIDLocked does not close changeCh, risking goroutine leak
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
## Summary
In `pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go`, the function `removeEntryForTenantIDLocked` deletes a tenant entry from the store but does not close the entry's `changeCh` channel before deletion. This creates a potential goroutine leak if any caller is blocking on the per-entry `changeCh` returned by `GetInfo` or `GetCapabilities`.
## Details
`handleIncrementalUpdate` correctly closes `entry.changeCh` before replacing an entry (line ~358), but there are two paths where removal happens without closing:
1. **Rangefeed restart path**: `handleRangefeedCacheEvent` → `removeEntriesBeforeTimestamp` → `removeEntryForTenantIDLocked` — channel is never closed.
2. **Incremental delete path**: `handleIncrementalUpdate` closes `changeCh` before calling `removeEntryForTenantIDLocked` when `update.Deleted` is true, but `removeEntryForTenantIDLocked` itself doesn't close it — so any direct caller of that function would miss the close.
After removal, the entry is gone from `store`, so a new `getInternal` call creates a fresh placeholder with a different channel. The old channel is orphaned — any goroutine blocked on it will hang forever.
**Current impact**: Low. The `anyChangeCh` mechanism (used by the server controller's `GetAllTenants` loop) works correctly since `onAnyChangeLocked()` is called. The authorizer (main `GetInfo` consumer) does not currently block on per-entry `changeCh`. But any future code that does would hit this.
## Suggested Fix
Close `entry.changeCh` in `removeEntryForTenantIDLocked` before deleting:
```go
func (w *Watcher) removeEntryForTenantIDLocked(ctx context.Context, tid roachpb.TenantID) {
if entry, ok := w.mu.store[tid]; ok {
close(entry.changeCh)
if entry.Entry != nil {
delete(w.mu.byName, entry.Name)
}
}
delete(w.mu.lastUpdate, tid)
delete(w.mu.store, tid)
}
```
Epic: none
Jira issue: CRDB-64101
Contributor guide
Assessment
This issue has not been assessed yet.