Skip to content

[BUGFIX] clear stale metadata references after destruction to allow GC#21189

Draft
Copilot wants to merge 5 commits intomainfrom
copilot/fix-stale-references-metadata
Draft

[BUGFIX] clear stale metadata references after destruction to allow GC#21189
Copilot wants to merge 5 commits intomainfrom
copilot/fix-stale-references-metadata

Conversation

Copy link
Contributor

Copilot AI commented Mar 5, 2026

After destroy() completes, DESTROYABLE_META entries retained strong references to parents, children, eagerDestructors, and destructors, 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:

  1. scheduleDestroyed only sets meta.state = DESTROYED_STATE — never nulls the other fields
  2. removeChildFromParent guards on parentMeta.state === LIVE_STATE, so during cascaded destruction the child's callback skips removing itself from the parent (parent is already DESTROYING_STATE), leaving parent.meta.children holding a stale reference

Changes

  • packages/@glimmer/destroyable/index.ts — null out all metadata references in scheduleDestroyed after DESTROYED_STATE is set:

    scheduleDestroyed(() => {
      iterate(parents, (parent) => {
        removeChildFromParent(destroyable, parent);
      });
    
      meta.state = DESTROYED_STATE;
    
      // Release references so GC can reclaim the destroyed tree
      meta.parents = null;
      meta.children = null;
      meta.eagerDestructors = null;
      meta.destructors = null;
    });

    Safe because once DESTROYED_STATE is 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) returns false after destroy(parent) + flush(), which fails without the fix since the stale child reference lingers in meta.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-source versions we traced the regression to Ember 6.8, where the renderer refactoring added multiple new associateDestroyableChild() calls, creating deeper destroyable ownership trees.

The underlying issue is in @glimmer/destroyable's destroy() function: after destruction completes, the metadata entries in DESTROYABLE_META retain stale object references, preventing the garbage collector from reclaiming the destroyed object tree.

Two issues combine to cause this:

  1. The scheduleDestroyed callback sets meta.state = DESTROYED_STATE but never clears the parents, children, eagerDestructors, or destructors properties.

  2. removeChildFromParent has a parentMeta.state === LIVE_STATE guard that checks the parent's state. During cascaded destruction (destroy(parent)destroy(child)), the child's scheduleDestroyed callback is queued before the parent's. When it runs and calls removeChildFromParent(child, parent), the parent's state is already DESTROYING_STATE (set synchronously at the start of destroy(parent)), so the guard skips the removal.

After destroy(parent) completes:

  • The parent's metadata still holds strong references to all children via meta.children
  • Each child's metadata still holds a strong reference back to the parent via meta.parents
  • These cross-references create cycles through the WeakMap values: each entry's value (the metadata) holds a strong reference to another entry's key (the related destroyable), keeping the entire graph alive

In 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:

import { associateDestroyableChild, destroy } from '@ember/destroyable';

// Create a parent-child tree
const parent = {};
const child = {};
associateDestroyableChild(parent, child);

destroy(parent);

// After the scheduleDestroyed callbacks run:
// - DESTROYABLE_META.get(parent).children still references child
// - DESTROYABLE_META.get(child).parents still references parent
// - Both entries are in DESTROYED_STATE but hold strong cross-references
// - These cross-references through the WeakMap keep the entire tree alive

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 the scheduleDestroyed callback (see proposed fix below) resolves the OOM completely.

😕 Actual Behavior

After destroy() completes and meta.state reaches DESTROYED_STATE, the metadata retains all parents, children, eagerDestructors, and destructors references. Combined with the LIVE_STATE guard in removeChildFromParent, this means:

  1. Parent's meta.children still references the child — removeChildFromParent in the child's scheduleDestroyed callback checks parentMeta.state === LIVE_STATE, but the parent is already in DESTROYING_STATE by that point, so the removal is skipped
  2. Child's meta.parents still references the parent — no code path clears this after destruction
  3. The DESTROYABLE_META WeakMap 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:

scheduleDestroyed(() => {
  iterate(parents, (parent) => {
    removeChildFromParent(destroyable, parent);
  });

  meta.state = DESTROYED_STATE;

  // Release references so GC can reclaim the destroyed tree
  meta.parents = null;
  meta.childr...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes emberjs/ember.js#21145

<!-- START COPILOT CODING AGENT TIPS -->
---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security)

…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
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

📊 Package size report   -0%↓

File Before (Size / Brotli) After (Size / Brotli)
Total (Includes all files) 17.1 MB / 3.2 MB -0%↓17.1 MB / 0.01%↑3.2 MB
Tarball size 3.8 MB -0%↓3.8 MB

🤖 This report was automatically generated by pkg-size-action

@NullVoxPopuli NullVoxPopuli changed the title fix(@glimmer/destroyable): clear stale metadata references after destruction to allow GC [BUFIIX LTS] clear stale metadata references after destruction to allow GC Mar 5, 2026
@NullVoxPopuli NullVoxPopuli changed the title [BUFIIX LTS] clear stale metadata references after destruction to allow GC [BUGIX LTS] clear stale metadata references after destruction to allow GC Mar 5, 2026
@NullVoxPopuli NullVoxPopuli changed the title [BUGIX LTS] clear stale metadata references after destruction to allow GC [BUGFIX LTS] clear stale metadata references after destruction to allow GC Mar 5, 2026
@NullVoxPopuli NullVoxPopuli changed the title [BUGFIX LTS] clear stale metadata references after destruction to allow GC [BUGFIX lts] clear stale metadata references after destruction to allow GC Mar 5, 2026
@NullVoxPopuli NullVoxPopuli changed the title [BUGFIX lts] clear stale metadata references after destruction to allow GC [BUGFIX] clear stale metadata references after destruction to allow GC Mar 5, 2026
@ef4
Copy link
Contributor

ef4 commented Mar 5, 2026

This isn't correct. The meta itself should also be released. It's supposed to be released because it's stored in a WeakMap.

I see that there is a debugging feature that replaces the WeakMap with a Map, but in that case you're supposed to managed paired calls to enableDestroyableTracking and assertDestroyablesDestroyed, which will eventually replace the Map again with a new WeakMap.

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 meta is associated with.

@NullVoxPopuli
Copy link
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
Copilot AI and others added 2 commits March 5, 2026 23:35
…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>
@NullVoxPopuli
Copy link
Contributor

This new change is way more targeted. 🤔

Anyone know how to test for memory leaks in qunit? 🤔

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants