[BUGFIX] clear stale metadata references after destruction to allow GC#21189
Draft
[BUGFIX] clear stale metadata references after destruction to allow GC#21189
Conversation
…ruction to allow GC Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix stale references in metadata after destroy()
fix(@glimmer/destroyable): clear stale metadata references after destruction to allow GC
Mar 5, 2026
Contributor
📊 Package size report -0%↓
🤖 This report was automatically generated by pkg-size-action |
Contributor
|
This isn't correct. The I see that there is a debugging feature that replaces the So either something is using the debug feature and not pairing those calls correctly, or (more likely) there's something elsewhere retaining the objects that |
Contributor
|
@copilot see ef4's comment <3 |
…s elsewhere Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
Copilot stopped work on behalf of
NullVoxPopuli due to an error
March 5, 2026 22:45
…NG_STATE to prevent GC retention When destroy(parent) triggers cascaded destruction of children, the child's scheduleDestroyed callback calls removeChildFromParent(child, parent). The previous guard `parentMeta.state === LIVE_STATE` prevented this from working when the parent was in DESTROYING_STATE, leaving stale strong references in parent_meta.children that prevent efficient GC of destroyed trees. Change the guard to `parentMeta.state !== DESTROYED_STATE` so that children properly remove themselves from a parent's children list even when the parent is in the process of being destroyed. The synchronous iterate(children, destroy) call has already completed by the time scheduleDestroyed callbacks run, so this modification is safe. Also add a test that verifies parent_meta.children is cleared after cascaded destruction. Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
Contributor
|
This new change is way more targeted. 🤔 Anyone know how to test for memory leaks in qunit? 🤔 |
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.
After
destroy()completes,DESTROYABLE_METAentries retained strong references toparents,children,eagerDestructors, anddestructors, forming reference cycles through the WeakMap values that prevented GC from reclaiming destroyed object trees. In large test suites (e.g. Discourse's ~5,000-test suite) this caused OOM on CI.Two issues combine to cause this:
scheduleDestroyedonly setsmeta.state = DESTROYED_STATE— never nulls the other fieldsremoveChildFromParentguards onparentMeta.state === LIVE_STATE, so during cascaded destruction the child's callback skips removing itself from the parent (parent is alreadyDESTROYING_STATE), leavingparent.meta.childrenholding a stale referenceChanges
packages/@glimmer/destroyable/index.ts— null out all metadata references inscheduleDestroyedafterDESTROYED_STATEis set:Safe because once
DESTROYED_STATEis set,destroy()returns early and no other API reads these fields.packages/@glimmer/destroyable/test/destroyables-test.ts— adds a regression test asserting_hasDestroyableChildren(parent)returnsfalseafterdestroy(parent)+flush(), which fails without the fix since the stalechildreference lingers inmeta.children.Original prompt
This section details on the original issue you should resolve
<issue_title>
@glimmer/destroyable:destroy()retains stale references in metadata after destruction, delaying GC of parent-child trees</issue_title><issue_description>### 🐞 Describe the Bug
While upgrading Discourse from Ember 6.6 to 6.10, our QUnit test suite (~5,000 tests) started running out of memory in Chrome on CI. Through bisecting
ember-sourceversions we traced the regression to Ember 6.8, where the renderer refactoring added multiple newassociateDestroyableChild()calls, creating deeper destroyable ownership trees.The underlying issue is in
@glimmer/destroyable'sdestroy()function: after destruction completes, the metadata entries inDESTROYABLE_METAretain stale object references, preventing the garbage collector from reclaiming the destroyed object tree.Two issues combine to cause this:
The
scheduleDestroyedcallback setsmeta.state = DESTROYED_STATEbut never clears theparents,children,eagerDestructors, ordestructorsproperties.removeChildFromParenthas aparentMeta.state === LIVE_STATEguard that checks the parent's state. During cascaded destruction (destroy(parent)→destroy(child)), the child'sscheduleDestroyedcallback is queued before the parent's. When it runs and callsremoveChildFromParent(child, parent), the parent's state is alreadyDESTROYING_STATE(set synchronously at the start ofdestroy(parent)), so the guard skips the removal.After
destroy(parent)completes:meta.childrenmeta.parentsWeakMapvalues: each entry's value (the metadata) holds a strong reference to another entry's key (the related destroyable), keeping the entire graph aliveIn workloads that rapidly create and destroy large destroyable trees (such as test suites that spin up an Application per test), these stale references put significant pressure on the garbage collector, causing memory to accumulate and eventually leading to OOM.
🔬 Minimal Reproduction
The core issue can be illustrated with this pattern:
In Discourse's QUnit test suite (~5,000 tests), each test creates and destroys an Application instance. Even with explicit
app.destroy()and releasing the app reference, Chrome runs out of memory on CI. Adding the metadata cleanup in thescheduleDestroyedcallback (see proposed fix below) resolves the OOM completely.😕 Actual Behavior
After
destroy()completes andmeta.statereachesDESTROYED_STATE, the metadata retains allparents,children,eagerDestructors, anddestructorsreferences. Combined with theLIVE_STATEguard inremoveChildFromParent, this means:meta.childrenstill references the child —removeChildFromParentin the child'sscheduleDestroyedcallback checksparentMeta.state === LIVE_STATE, but the parent is already inDESTROYING_STATEby that point, so the removal is skippedmeta.parentsstill references the parent — no code path clears this after destructionDESTROYABLE_METAWeakMap entries form reference cycles through their values, keeping the entire destroyed tree alive🤔 Expected Behavior
After destruction completes (
meta.state = DESTROYED_STATE), the metadata should eagerly release all object references so the garbage collector can reclaim the destroyed tree sooner.Proposed fix in
@glimmer/destroyable: