🐛 Fix goroutine leaks in cache Start() methods

[dongjiang1989]

What does this PR do?

This PR fixes goroutine leaks in multiNamespaceCache.Start() and delegatingByGVKCache.Start() methods in the pkg/cache package.

Why do we need it?

Problem

Both Start() methods used unbuffered error channels (errs := make(chan error)) to collect errors from goroutines. This caused goroutine leaks in two scenarios:

1. Multiple caches return errors simultaneously: The select statement only receives the first error and returns, leaving other goroutines blocked forever on errs <- err with no receiver.

2. Context cancelled while goroutines try to send errors: When ctx.Done() triggers first, the function returns immediately. Any subsequent error sends from goroutines will block forever.

Example

// Original problematic code
errs := make(chan error)  // unbuffered
for _, cache := range caches {
    go func() {
        if err := cache.Start(ctx); err != nil {
            errs <- err  // blocks forever if no receiver!
        }
    }()
}
select {
case err := <-errs:
    return err  // only receives ONE error, others leak
case <-ctx.Done():
    return nil  // returns immediately, senders leak
}

Fix

• Use buffered channels sized to the number of caches
• Use sync.WaitGroup to track all goroutines
• Wait for all goroutines to complete before returning

// Fixed code
errs := make(chan error, len(caches))  // buffered
var wg sync.WaitGroup
for _, cache := range caches {
    wg.Go(func() {
        if err := cache.Start(ctx); err != nil {
            errs <- err  // never blocks
        }
    })
}
done := make(chan struct{})
go func() { wg.Wait(); close(done) }()
// ... wait for all goroutines to complete

[thc1006]

TLDR: the leak is real and worth fixing, but waiting on done after the first error hangs Start whenever any sibling cache is healthy, which is the ordinary case. I ran it against this branch and against main, results below. The existing test only passes because its parent context expires on its own after 50ms.

A healthy Cache.Start runs until its context is cancelled. In the case err := <-errs branch this waits for every other Start goroutine to return, and nothing cancels their context, since the children are handed the same ctx the parent is still using. So done never closes and Start never returns. Both files have this shape.

One child returning a sentinel error, one child on <-ctx.Done(), parent context left alive:

	Start returns	healthy child sees cancellation
main	promptly, with the error	no, it keeps running, which is the leak this PR is about
this branch	never, 5s timeout in my test	n/a
errgroup.WithContext	promptly, with the error	yes

So the leak is traded for a hang, and only the third row has both properties.

On the tests. TestMultiNamespaceCacheStart_GoroutineLeak_ContextCancelWithError is nearly the right shape already, a slow erroring cache alongside two on <-ctx.Done(), but it builds the parent with context.WithTimeout(t.Context(), 50*time.Millisecond). The timeout fires, the healthy children return, and done closes. Swapping that one line for context.WithCancel and asserting Start returns is enough to turn it red:

NOTIMEOUT Start never returned once the parent context stopped expiring on its own
--- FAIL: TestMultiNamespaceCacheStart_GoroutineLeak_ContextCancelWithError (3.00s)

That matters because a manager's context lives for the process rather than 50ms, so production looks like the WithCancel version.

What worked for me in both files:

group, childCtx := errgroup.WithContext(ctx)
for idx := range allCaches {
	cache := allCaches[idx]
	group.Go(func() error {
		return cache.Start(childCtx)
	})
}
return group.Wait()

The first error is kept, childCtx is cancelled, the healthy children return from their own <-ctx.Done(), and Wait gives back the first error. The buffered channel and the done goroutine both go away with it, so the leak stays fixed. One detail worth deciding: on the plain ctx.Done() path the group returns context.Canceled, where the current code returns nil, so you may want to keep that mapping.

Worth adding the shape that is missing from the suite either way: one cache returning an error immediately, another blocking on <-ctx.Done(), a parent context nobody cancels, asserting the parent returns the sentinel promptly and the blocking child observes cancellation. That case fails on this branch and on main, for different reasons, and passes with the group.

This review was written in part with the assistance of generative AI. Bcuz my English might be sucks.

[dongjiang1989]

@thc1006 Thanks for the review!

Updated to use errgroup.WithContext() as suggested. Added tests with context.WithCancel to verify healthy children are cancelled and Start() returns promptly.

[dongjiang1989]

Thanks @alvaroaleman
Please re-check it. thanks

[sbueringer]

/assign
(also want to take a look before merge)

[alvaroaleman]

Looks good, just the point about ignoreContextCanceled, lets wait for Stefans take there

[alvaroaleman]

lgtm, leaving on hold so stefan can have a look
/hold

[kubernetes-prow]

LGTM label has been added.

Details

Git tree hash: 79d08313e2ea39643d436524fafa06da3a03580a

[sbueringer]

Thx!

/lgtm
/approve
/hold cancel

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, dongjiang1989, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment