Don't subscribe to dynamic config until partition manager Start, fixing loser-of-load-race leak#11053
Don't subscribe to dynamic config until partition manager Start, fixing loser-of-load-race leak#11053harrisleesh wants to merge 2 commits 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>
|
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()? |
Per review feedback: instead of adding a special cleanup path (closeUnstarted) for managers that lose the concurrent partition load race, make newTaskQueuePartitionManager pure allocation. The rate limit manager now registers its dynamic config subscriptions in Start, so an unstarted manager holds no external references and the loser of the load race can simply be dropped and garbage collected.
|
@dnr Ordering note: the manager is only published to the partitions map right before Tests that construct a |
What changed?
newTaskQueuePartitionManager(andnewRateLimitManager) now only allocate: the rate limit manager's dynamic config subscriptions are registered inStartinstead of at construction time.With that, the loser of the double-checked race in
getTaskQueuePartitionManager(constructed, but never started or published) holds no external references and can simply be dropped and garbage collected — no special cleanup path needed.Why?
Fixes #11051.
The full partition manager graph is constructed before taking the write lock. When the constructor lost the race, the abandoned manager stayed reachable from the dynamic config collection forever via the subscriptions
newRateLimitManagerregistered 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 could 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 root cause analysis.How did you test it?
TestConstructionDoesNotRegisterDynamicConfigSubscriptionsverifies that construction registers no dynamic config subscriptions, and thatStart/Stopregister and release them. Existing tests that construct arateLimitManagerdirectly now callStarton it, matching the production lifecycle. 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.Potential risks
Rate limits are now initialized in
Startrather than at construction. The manager is only published to the partitions map immediately beforeStart, and all dispatch/poll paths gate ondefaultQueueFuture, which is only set byinitialize(spawned at the end ofStart), so no path can observe the rate limit manager before its subscriptions are registered.