Skip to content

Release dynamic config subscriptions when losing the partition load race#11053

Open
harrisleesh wants to merge 1 commit into
temporalio:mainfrom
harrisleesh:fix-partition-manager-race-leak
Open

Release dynamic config subscriptions when losing the partition load race#11053
harrisleesh wants to merge 1 commit into
temporalio:mainfrom
harrisleesh:fix-partition-manager-race-leak

Conversation

@harrisleesh

@harrisleesh harrisleesh commented Jul 14, 2026

Copy link
Copy Markdown

What changed?

getTaskQueuePartitionManager now 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 Stop cannot be used on a never-started manager (it blocks on defaultQueueFuture, which is only set once a started manager finishes initializing), this adds closeUnstarted, 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 newRateLimitManager registers 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 GetTaskQueueUserData long-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?

  • built
  • added new unit test(s)

TestCloseUnstartedReleasesDynamicConfigSubscriptions verifies that an unstarted manager's dynamic config subscriptions are released and that closeUnstarted does not block (which Stop would). Passes in all three suite variants (Classic/Pri/Fair). The leak itself was diagnosed from production heap profile diffs (growth signature rooted at newTaskQueuePartitionManager under the GetTaskQueueUserData handler, pinned via dynamicconfig.subscribe[...]), matching exactly the loser path this PR cleans up.

Potential risks

closeUnstarted must only be called on a manager that was never started; the only call site is the loser path in getTaskQueuePartitionManager, before the manager is ever returned or started. userDataManager.Stop and rateLimitManager.Stop are safe to call on unstarted instances.

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>
@harrisleesh
harrisleesh requested a review from a team as a code owner July 14, 2026 07:51
@CLAassistant

CLAassistant commented Jul 14, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@dnr

dnr commented Jul 17, 2026

Copy link
Copy Markdown
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()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants