🐛 Fix(manager): Prevent goroutine leak on shutdown timeout by jingyih · Pull Request #3247 · kubernetes-sigs/controller-runtime

jingyih · 2025-06-26T23:05:22Z

A race condition during manager shutdown can cause a goroutine leak if the graceful shutdown period is exceeded.

When the gracefulShutdownTimeout is reached, the manager stops waiting for runnables and terminates the goroutine responsible for draining errors from runnables. If a slow runnable subsequently fails, its attempt to send an error on the shared channel (errChan) will block its goroutine forever, causing a leak.

This change prevents the leak by replacing the blocking error send in the runnableGroup with a non-blocking select statement. If the context is canceled, indicating a shutdown is in progress, the error is logged and dropped. This allows the runnable's goroutine to terminate cleanly instead of blocking forever.

Example test failure without this fix:

$ go test ./pkg/manager -v -ginkgo.focus="should not leak goroutines when a runnable returns error slowly after being
 signaled to stop"
=== RUN   TestSource
Running Suite: Manager Suite - /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager
==============================================================================================================================
Random Seed: 1750977926

Will run 1 of 98 specs
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
------------------------------
• [FAILED] [1.712 seconds]
manger.Manager [It] should not leak goroutines when a runnable returns error slowly after being signaled to stop
/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/manager_test.go:1889

  Timeline >>
  2025-06-26T22:45:31Z  INFO    Stopping and waiting for non leader election runnables
  2025-06-26T22:45:31Z  INFO    Stopping and waiting for leader election runnables
  2025-06-26T22:45:31Z  INFO    Stopping and waiting for caches
  2025-06-26T22:45:31Z  INFO    Stopping and waiting for webhooks
  2025-06-26T22:45:31Z  INFO    Stopping and waiting for HTTP servers
  2025-06-26T22:45:31Z  INFO    Wait completed, proceeding to shutdown the manager
  [FAILED] in [It] - /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/manager_test.go:1934 @ 06/26/25 22:45:32.755
  << Timeline

  [FAILED] Timed out after 1.360s.
  Expected success, but got an error:
      <*errors.errorString | 0xc000194450>: 
      found unexpected goroutines:
      [Goroutine 65 in state chan send, with sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1 on top of the stack:
      sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1(0xc0002f4840)
        /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:227 +0xe5
      created by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile in goroutine 262
        /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:210 +0x191
       Goroutine 274 in state chan send, with sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1 on top of the stack:
      sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1(0xc0002f4860)
        /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:227 +0xe5
      created by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile in goroutine 262
        /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:210 +0x191
       Goroutine 269 in state sync.WaitGroup.Wait, with sync.runtime_SemacquireWaitGroup on top of the stack:
      sync.runtime_SemacquireWaitGroup(0xc0002e9f98?)
        /usr/local/google/home/jingyih/go/pkg/mod/golang.org/[email protected]/src/runtime/sema.go:110 +0x25
      sync.(*WaitGroup).Wait(0xc0002e9fd0?)
        /usr/local/google/home/jingyih/go/pkg/mod/golang.org/[email protected]/src/sync/waitgroup.go:118 +0x48
      sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).StopAndWait.func1.2()
        /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:307 +0x4e
      created by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).StopAndWait.func1 in goroutine 267
        /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:304 +0x118
      ]
      {
          s: "found unexpected goroutines:\n[Goroutine 65 in state chan send, with sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1 on top of the stack:\nsigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1(0xc0002f4840)\n\t/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:227 +0xe5\ncreated by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile in goroutine 262\n\t/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:210 +0x191\n Goroutine 274 in state chan send, with sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1 on top of the stack:\nsigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1(0xc0002f4860)\n\t/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:227 +0xe5\ncreated by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile in goroutine 262\n\t/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:210 +0x191\n Goroutine 269 in state sync.WaitGroup.Wait, with sync.runtime_SemacquireWaitGroup on top of the stack:\nsync.runtime_SemacquireWaitGroup(0xc0002e9f98?)\n\t/usr/local/google/home/jingyih/go/pkg/mod/golang.org/[email protected]/src/runtime/sema.go:110 +0x25\nsync.(*WaitGroup).Wait(0xc0002e9fd0?)\n\t/usr/local/google/home/jingyih/go/pkg/mod/golang.org/[email protected]/src/sync/waitgroup.go:118 +0x48\nsigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).StopAndWait.func1.2()\n\t/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:307 +0x4e\ncreated by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).StopAndWait.func1 in goroutine 267\n\t/usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/runnable_group.go:304 +0x118\n]",
      }
  In [It] at: /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/manager_test.go:1934 @ 06/26/25 22:45:32.755
------------------------------
SSSSSSSSSSSSS

Summarizing 1 Failure:
  [FAIL] manger.Manager [It] should not leak goroutines when a runnable returns error slowly after being signaled to stop
  /usr/local/google/home/jingyih/go/src/github.com/kubernetes-sigs/controller-runtime/pkg/manager/manager_test.go:1934

Ran 1 of 98 Specs in 7.215 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 97 Skipped
--- FAIL: TestSource (7.22s)
FAIL
FAIL    sigs.k8s.io/controller-runtime/pkg/manager      7.279s
FAIL

k8s-ci-robot · 2025-06-26T23:05:32Z

Hi @jingyih. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alvaroaleman

/ok-to-test

alvaroaleman · 2025-06-28T16:40:09Z

pkg/manager/runnable_group.go

+				select {
+				case r.errChan <- err:
+					// Error sent successfully
+				case <-r.ctx.Done():


Shutdown is triggered by cancelling the context and select is not ordered, i.E. if both clauses are valid, its not deterministic which it will use. Shouldn't this be a default instead to ensure we try sending on the errChan first?

Good catch! We should try sending on the errChan first. I added some complexity here because using a simple "default" could cause errors to be dropped during normal operation if errChan is momentarily blocked.

alvaroaleman

thanks!

k8s-ci-robot · 2025-07-25T16:48:33Z

LGTM label has been added.

Details

Git tree hash: f6b67196edd83d5e40548d8041fc169477b5a8d5

k8s-ci-robot · 2025-07-25T16:48:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, jingyih

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]

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

pmalek · 2025-07-25T17:09:18Z

pkg/manager/runnable_group.go

+					// During normal operation, always try to send errors (may block briefly)
+					r.errChan <- err


Wouldn't it be safer to use a select here with r.ctx.Done() to prevent blocking mentioned in the comment?

#3247 (comment)

sbueringer · 2025-08-12T09:46:42Z

Thx!
lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2025

k8s-ci-robot requested review from troy0820 and varshaprasad96 June 26, 2025 23:05

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 26, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2025

jingyih force-pushed the goroutine_leak branch from 875770c to ff4325f Compare June 27, 2025 00:46

alvaroaleman reviewed Jun 28, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2025

jingyih force-pushed the goroutine_leak branch from dd3a19b to 2b9342e Compare July 8, 2025 21:16

jingyih added 3 commits July 8, 2025 21:58

test: add goroutine leak test for slow runnables during shutdown

b34ac66

fix: prevent goroutine leak on manager shutdown timeout

88d9541

address comments

0279a29

jingyih force-pushed the goroutine_leak branch from 2b9342e to 0279a29 Compare July 8, 2025 22:03

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 25, 2025

alvaroaleman approved these changes Jul 25, 2025

View reviewed changes

k8s-ci-robot assigned alvaroaleman Jul 25, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2025

k8s-ci-robot merged commit 98b5b22 into kubernetes-sigs:main Jul 25, 2025
11 checks passed

pmalek reviewed Jul 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix(manager): Prevent goroutine leak on shutdown timeout#3247

🐛 Fix(manager): Prevent goroutine leak on shutdown timeout#3247
k8s-ci-robot merged 3 commits intokubernetes-sigs:mainfrom
jingyih:goroutine_leak

jingyih commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

alvaroaleman left a comment

Uh oh!

alvaroaleman Jun 28, 2025

Uh oh!

jingyih Jul 8, 2025

Uh oh!

alvaroaleman left a comment

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

Uh oh!

pmalek Jul 25, 2025

Uh oh!

alvaroaleman Jul 25, 2025

Uh oh!

sbueringer commented Aug 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		// During normal operation, always try to send errors (may block briefly)
		r.errChan <- err

Conversation

jingyih commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

jingyih Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

Uh oh!

pmalek Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Aug 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants