cockroachdb / cockroachdb/cockroach

tenantcapabilitieswatcher: removeEntryForTenantIDLocked does not close changeCh, risking goroutine leak

Open
#170,633 1 comment 0 reactions 0 assignees View on GitHub
branch-master C-bug T-db-server
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.