[YUNIKORN-3357] Add goroutine leak detection to tests via goleak by tigerquoll · Pull Request #1124 · apache/yunikorn-core

tigerquoll · 2026-08-11T07:58:00Z

What is this PR for?

Adds goroutine leak detection to the test suite using uber-go/goleak, wired into every test package (20 of them) via a shared pkg/common/leakcheck helper:

One TestMain per package; all shared exemptions live in a single documented list in leakcheck.options(); VerifyTestMain accepts per-package extra options so the shared list never needs widening
pkg/rmproxy is the one deliberate exception: it has no test functions, so a hook there would guard nothing (documented in the package doc)

No existing test or production code is modified. goleak was already a transitive dependency (go.sum unchanged); it is promoted to a direct test-only dependency.

The baseline exemption list is a burn-down list

Switching detection on surfaced seven pre-existing leaked-goroutine occurrences. Each has a documented exemption so this change lands green and blocks new kinds of leaks immediately; each exemption is to be removed by its own follow-up fix:

#	Goroutine	Cause
1	`EventSystemImpl.StartServiceWithPublisher.func1`	tests start the event system without `Stop()`
2	`eventPublisher.start.func1`	`Init()` registers a configmap callback that `Stop()` never removes — a config update after shutdown restarts the event system
3	`EventStreaming.CreateEventStream.func1`	(a) test never calls `RemoveStream`; (b) forwarder blocks on a bare `consumer <- event` send, so slow-consumer eviction cannot release it
4	`partitionManager.cleanRoot`	tests build a `ClusterContext` without `Stop()`
5	`partitionManager.cleanExpiredApps`	same as 4
6	`security.UserGroupCache.run`	same as 4 — `ClusterContext.Stop()` already stops it
7	`ClusterContext.notifyRMNewAllocation`	shutdown race: `StopAll` stops the scheduler before the RM proxy; a queued allocation event can win the select over the closed stop channel, then the notify blocks forever on its unbuffered reply because `handleRMEvents` exits without draining (reproduced ~43% of full `pkg/scheduler/tests` runs before the exemption)

Items 2, 3(b) and 7 are production shutdown gaps rather than test hygiene; they will be filed as separate JIRAs.

Known limitation, stated in the code: the exemptions match on top stack frame, so the baseline is a ratchet against new leak kinds, not an instance count; three entries key on positional .funcN closure names that the follow-up fixes should replace by hoisting the goroutine bodies to named methods.

What type of PR is it?

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-3357

How should this be tested?

go test -count=1 ./pkg/scheduler/tests/ ×5: all green (this package's leak is the intermittent Changes to README to clarify build process #7 above — before its exemption it failed ~43% of full runs)
go test -count=3 on the other instrumented packages: no goleak failures
go test -race ./pkg/... and the CI-style -race -tags deadlock run with deadlock detection enabled: all 20 packages ok
make lint, make license-check, go vet ./pkg/...: clean

Note: four packages (common/security, entrypoint, metrics, scheduler/objects) fail under go test -count=3 on unmodified master as well — pre-existing non-idempotent tests over process-global state, unrelated to this change and not goleak failures.

Generated by the Author with assistance from Claude Code

Wire uber-go/goleak into the test suite of every test package (20 of them; pkg/rmproxy has no test functions) through a shared pkg/common/leakcheck helper. All shared exemptions live in a single documented baseline list: it blocks new kinds of goroutine leaks from being introduced while the seven pre-existing leaked shapes are fixed and removed from the list in follow-up changes. VerifyTestMain accepts per-package extra options so the shared list never needs widening. No existing test or production code is modified. goleak was already a transitive dependency and becomes a direct test-only dependency.

codecov · 2026-08-11T18:19:38Z

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.55%. Comparing base (7dc1287) to head (34c61e7).

Files with missing lines	Patch %	Lines
pkg/common/leakcheck/leakcheck.go	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1124      +/-   ##
==========================================
- Coverage   81.62%   81.55%   -0.07%     
==========================================
  Files         104      105       +1     
  Lines       14251    14262      +11     
==========================================
  Hits        11632    11632              
- Misses       2330     2341      +11     
  Partials      289      289

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

chenyulin0719

+1 LGTM

It's a good start to cleaning up goroutine leaks. Exemptions should be tracked later.
funcN closure names should be tracked later too — positional naming is fragile.
Comments in leakcheck.go are verbose but clear.

wilfred-s

Instead of the large comments for the exclusions, point to the jira logged for the fix and use the comment as the jira description.
if we're not fixing it we should have a comment.

Missing: the go.sum update

wilfred-s · 2026-08-12T08:24:30Z

+		// defect; delete this entry once Stop() deregisters the callback.
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/events.(*eventPublisher).start.func1"),


This is done on purpose we need that routine to keep running after a stop to make sure the system can be turned on and off via the config update.
It is also a false positive as the function exits directly after calling. The function is registered but the call only happens on change, no leaks here.

wilfred-s · 2026-08-12T08:44:02Z

+		// replies. Note notifyRMAllocationReleased has the same unbuffered-reply
+		// shape and would leak under a different top frame; it has not been seen
+		// yet, so it is deliberately not exempted here.


Can be added as there is no reason to fix either of these: the events are dropped either way and the app is exiting so nothing to be handled.

wilfred-s · 2026-08-12T08:44:06Z

+		// Two distinct causes share this top frame. One is a test that creates a
+		// stream and never calls RemoveStream. The other is a consumer that stops
+		// reading, which wedges the forwarder on the "consumer <- event" send
+		// outside its select, where neither stop channel can reach it; that one is


The push outside of the select is the first batch of records during the opening of the stream. This would mean that the requestor closed the connection even before it was really started and they did request a non 0 history (not the default).
This is a defect and needs a jira to fix the push of the history.

wilfred-s · 2026-08-13T01:53:14Z

+		// Tests across events, scheduler and objects call events.Init() followed by
+		// StartService()/StartServiceWithPublisher() without a matching Stop().
+		// Stoppable: delete this entry once those tests defer Stop().
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/events.(*EventSystemImpl).StartServiceWithPublisher.func1"),


This should be fixed on the core side before we turn this check on

wilfred-s · 2026-08-13T01:55:01Z

+		// configmap callback that Stop() never removes, so a reload after Stop()
+		// resurrects a system nobody holds a reference to. Suspected production
+		// defect; delete this entry once Stop() deregisters the callback.
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/events.(*eventPublisher).start.func1"),


false positive, the function to update from config cannot be called after shutdown as the system is exiting.

wilfred-s · 2026-08-13T01:55:44Z

+
+		// Partition expired application cleaner, the second goroutine started by
+		// partitionManager.Run. Same cause and same fix as cleanRoot above.
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/scheduler.(*partitionManager).cleanExpiredApps"),


wilfred-s · 2026-08-13T01:56:19Z

+		// a partition resolves users. Stoppable: UserGroupCache.Stop() closes the
+		// cleaner and resets the singleton, and ClusterContext.Stop() calls it.
+		// Delete this entry once those tests defer Stop().
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/common/security.(*UserGroupCache).run"),


wilfred-s · 2026-08-13T08:24:11Z

+		// Tests across events, scheduler and objects call events.Init() followed by
+		// StartService()/StartServiceWithPublisher() without a matching Stop().
+		// Stoppable: delete this entry once those tests defer Stop().
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/events.(*EventSystemImpl).StartServiceWithPublisher.func1"),


this should get a follow up jira

wilfred-s · 2026-08-13T08:27:30Z

+
+		// Event stream forwarder, started by EventStreaming.CreateEventStream.
+		// Two distinct causes share this top frame. One is a test that creates a
+		// stream and never calls RemoveStream. The other is a consumer that stops


that is a test breakage and needs a follow up jira

wilfred-s · 2026-08-13T08:29:35Z

+		// a partition resolves users. Stoppable: UserGroupCache.Stop() closes the
+		// cleaner and resets the singleton, and ClusterContext.Stop() calls it.
+		// Delete this entry once those tests defer Stop().
+		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/common/security.(*UserGroupCache).run"),


one follow up jira for all 3 cases in one jira: a call to create a new cluster context should have a defer to Stop()

Replace the per-exemption prose with a one-line identifier and the JIRA that tracks removing it, and condense the header to the mechanism note. The exemption list itself is unchanged: comments only.

The two IgnoreTopFunction matching gotchas are properties of the matching mechanism rather than of any one exemption, so they are not covered by the follow-up JIRAs and belong in the file. Comments only.

tigerquoll · 2026-08-14T16:07:42Z

Instead of the large comments for the exclusions, point to the jira logged for the fix and use the comment as the jira description. if we're not fixing it we should have a comment.

Missing: the go.sum update

@wilfred-s go.sum update not required - this module was previously brought in as a secondary dependency

tigerquoll · 2026-08-14T16:08:39Z

JIRA Issues added to https://issues.apache.org/jira/browse/YUNIKORN-3357 , with comments updated to point to the JIRA

This was referenced Aug 11, 2026

[YUNIKORN-3357] Add goroutine leak detection to tests via goleak tigerquoll/yunikorn-k8shim#3

Open

[YUNIKORN-3357] Add goroutine leak detection to tests via goleak apache/yunikorn-k8shim#1061

Open

chenyulin0719 assigned tigerquoll Aug 11, 2026

chenyulin0719 approved these changes Aug 11, 2026

View reviewed changes

wilfred-s requested changes Aug 13, 2026

View reviewed changes

tigerquoll added 2 commits August 15, 2026 00:44

[YUNIKORN-3357] Reference follow-up JIRAs from leakcheck exemptions

11b9144

Replace the per-exemption prose with a one-line identifier and the JIRA that tracks removing it, and condense the header to the mechanism note. The exemption list itself is unchanged: comments only.

[YUNIKORN-3357] Keep the goleak top-frame matching caveats in leakcheck

b21615d

The two IgnoreTopFunction matching gotchas are properties of the matching mechanism rather than of any one exemption, so they are not covered by the follow-up JIRAs and belong in the file. Comments only.

tigerquoll requested a review from wilfred-s August 14, 2026 16:08

		// defect; delete this entry once Stop() deregisters the callback.
		goleak.IgnoreTopFunction("github.com/apache/yunikorn-core/pkg/events.(*eventPublisher).start.func1"),

Uh oh!

Conversation

tigerquoll commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

The baseline exemption list is a burn-down list

What type of PR is it?

What is the Jira issue?

How should this be tested?

Uh oh!

codecov Bot commented Aug 11, 2026

Codecov Report

Uh oh!

chenyulin0719 left a comment

Choose a reason for hiding this comment

Uh oh!

wilfred-s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tigerquoll commented Aug 14, 2026

Uh oh!

tigerquoll commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tigerquoll commented Aug 11, 2026 •

edited

Loading