registry: stop background goroutines leaked across registry rebuilds - #1
Open
ranjanprasad96 wants to merge 1 commit into
Open
registry: stop background goroutines leaked across registry rebuilds#1ranjanprasad96 wants to merge 1 commit into
ranjanprasad96 wants to merge 1 commit into
Conversation
NewRegistry/NewApp start three pieces of background work that nothing ever stops, so they outlive App.Shutdown() and pin the entire App (router, storage driver, config) in memory. Any process that rebuilds a registry in-process — as anyscaled does on every credential rotation — leaks one full registry per rebuild: - The upload purger goroutine loops forever with no stop mechanism. Make the app's context cancelable, cancel it in Shutdown(), and have the purger select on ctx.Done() instead of sleeping. - The docker/go-events Broadcaster goroutine is never closed. Close the event sink in Shutdown(). - tracing.InitOpenTelemetry builds a new BatchSpanProcessor and replaces the global TracerProvider on every NewRegistry call without shutting down the previous one. It configures process-global state, so guard it with sync.Once. Also stop the app's goroutines on the NewRegistry error path that previously discarded the app without shutting it down. Upstream issue: distribution#4942 Signed-off-by: ranjanprasad96 <ranjan.prasad@anyscale.com>
| context.Context | ||
|
|
||
| // cancel stops background goroutines started by NewApp (e.g. the upload purger). | ||
| cancel context.CancelFunc |
There was a problem hiding this comment.
instead of setting the cancel func on the struct, shouldn't the caller that creates the app via NewApp just pass in a ctx that has a cancel, and call cancel when they want to stop it?
There was a problem hiding this comment.
oh i see we don't have a run(ctx context.Context) function... ok nevermind this is fine
| // subsequent calls are no-ops (a failed attempt may be retried). This also keeps | ||
| // repeated registry construction (e.g. on config reload) from leaking a | ||
| // BatchSpanProcessor per call. | ||
| func InitOpenTelemetry(ctx context.Context) error { |
There was a problem hiding this comment.
would it ever make sense to disable the otel setup entirely (ex: via a NewRegistry arg) and rely on the telemetry setup init'd by Anyscaled?
chrisfellowes-anyscale
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
anyscaled embeds this library to run a local image registry on every dataplane node, and rebuilds the registry in-process on every hourly STS credential rotation (
AnyscaledRegistry.onConfigUpdate→registry.NewRegistry()). Three pieces of background work started byNewRegistry/NewAppare never stopped byApp.Shutdown(), so each rebuild leaked one complete dead registry (~50 MB: mux router, S3 clients, config) pinned by orphaned goroutines — ~1.26 GB of heap growth per node over weeks of uptime.The three leaks (all inherited from upstream; reported there as distribution/distribution repo):
for { purge; sleep }goroutine with no stop mechanism; holds the wholeAppas its context.events.NewBroadcastergoroutine, neverClose()d.tracing.InitOpenTelemetryruns perNewRegistrycall, replacing the globalTracerProviderand leaking aBatchSpanProcessoreach time.Fix
Shutdown()cancels it and the purger loop selects onctx.Done().Shutdown()closes the event sink (broadcaster goroutine exits).InitOpenTelemetryinitializes process-global state at most once (success latches; failures stay retryable).NewRegistry's error path now shuts the app down instead of discarding it.Verification
TestAppShutdownCancelsContext: red without fix (context never canceled, sink stays open), green with.TestRebuildDoesNotLeakGoroutines(new, mirrors anyscaled's rebuild pattern): without fix goroutines grow 5 → 35 over 10 rebuilds (+3/rebuild — exactly the three leaks); with fix, flat.registry,registry/handlers,tracingsuites + race detector pass. (TestGracefulShutdownfails identically on the untouched tree on macOS — pre-existing local flake, expect CI to be green.)Branch base note
This branch is based on the
v3.0.0-anyscale.3tag (notmain) sov3.0.0-anyscale.4can be cut from it carrying all existing Anyscale patches — the diff therefore also shows the tag's golang-lru commit. Consumed byanyscale/productvia itsgo.modreplace.