Release dynamic config subscriptions when losing the partition load race#11053
Open
harrisleesh wants to merge 1 commit into
Open
Release dynamic config subscriptions when losing the partition load race#11053harrisleesh wants to merge 1 commit into
harrisleesh wants to merge 1 commit into
Conversation
getTaskQueuePartitionManager constructs the full partition manager graph before taking the write lock. When it then loses the double-checked race against a concurrent load of the same partition, the freshly constructed manager was abandoned without any cleanup. Since newRateLimitManager registers dynamic config subscriptions at construction time, the abandoned manager stayed reachable from the dynamic config collection forever and was never garbage collected. On an idle cluster this leaks continuously: the root partition is unloaded every matching.maxTaskQueueIdleTime (5m by default, barely above the 4m50s user data long poll timeout), and the returning GetTaskQueueUserData polls from child partitions and sticky queues race to re-create it. Observed in production as unbounded heap growth (~50MB/h on an idle cluster, ~55KB per abandoned manager) leading to OOM. Stop cannot be used for cleanup here because it blocks on defaultQueueFuture, which is only set once a started manager finishes initializing. Add closeUnstarted, which releases only the resources acquired at construction time, and call it on the loser path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Instead of making the lifecycle more complicated by making the "initialized but not started" state require special transitions, how about we just avoid doing anything besides allocation in newTaskQueuePartitionManager, and not subscribing to dynamic config until Start()? |
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
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.
What changed?
getTaskQueuePartitionManagernow cleans up the freshly constructed partition manager when it loses the double-checked race against a concurrent load of the same partition, instead of abandoning it.Since
Stopcannot be used on a never-started manager (it blocks ondefaultQueueFuture, which is only set once a started manager finishes initializing), this addscloseUnstarted, which releases only the resources acquired at construction time — the rate limit manager's dynamic config subscriptions,initCancel, and the user data manager — and calls it on the loser path.Why?
Fixes #11051.
The full partition manager graph is constructed before taking the write lock. When the constructor loses the race, the abandoned manager stayed reachable from the dynamic config collection forever via the subscriptions
newRateLimitManagerregisters at construction time, so it was never garbage collected.Any repeated partition unload with user-data pollers attached triggers this: when a partition unloads, the returning
GetTaskQueueUserDatalong-polls from child partitions and sticky queues race to re-create it, and every reload can strand one or more losers (~55 KB each). In the deployment where this was found, lease contention unloaded every system task queue roughly every 30s, producing unbounded matching heap growth (~50 MB/h with no workflow traffic) ending in OOM — but healthy clusters hit the same race whenever partition ownership moves (rolling deploys, scaling matching nodes). See the issue for heap profile diffs and the full (updated) root cause analysis.How did you test it?
TestCloseUnstartedReleasesDynamicConfigSubscriptionsverifies that an unstarted manager's dynamic config subscriptions are released and thatcloseUnstarteddoes not block (whichStopwould). Passes in all three suite variants (Classic/Pri/Fair). The leak itself was diagnosed from production heap profile diffs (growth signature rooted atnewTaskQueuePartitionManagerunder theGetTaskQueueUserDatahandler, pinned viadynamicconfig.subscribe[...]), matching exactly the loser path this PR cleans up.Potential risks
closeUnstartedmust only be called on a manager that was never started; the only call site is the loser path ingetTaskQueuePartitionManager, before the manager is ever returned or started.userDataManager.StopandrateLimitManager.Stopare safe to call on unstarted instances.