Skip to content

fix(core): prevent dangling prevConsumer reference from leaking v2#68681

Closed
milomg wants to merge 3 commits into
angular:mainfrom
milomg:fix/signal-prevConsumer-leak
Closed

fix(core): prevent dangling prevConsumer reference from leaking v2#68681
milomg wants to merge 3 commits into
angular:mainfrom
milomg:fix/signal-prevConsumer-leak

Conversation

@milomg

@milomg milomg commented May 12, 2026

Copy link
Copy Markdown
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The current signals implementation leaks

What is the new behavior?

See the analysis in #68137, this is just a new PR so that I can add additional edits to the test implementation

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

…oyed views

When `producerAccessed` creates a new link for a non-live consumer (e.g.
a computed signal with no readers), it eagerly sets `prevConsumer` to the
producer's current `consumersTail`. However, because the consumer is not
live, `producerAddLiveConsumer` is skipped and the link is never inserted
into the producer's consumer doubly-linked list.

This means the link holds a reference *into* the producer's consumer list
without being *part* of it. When the node that `prevConsumer` points to is
later removed via `producerRemoveLiveConsumerLink`, the dangling link is
not patched because it isn't traversable from the list.

The result is that the removed consumer link — and everything it
references — is kept alive by the dangling `prevConsumer` pointer on the
non-live link, which itself is kept alive through the computed signal's
`producers` linked list.

In practice this causes multi-MB memory leaks in Angular apps: a
root-provided service with a computed signal (e.g. `AttachmentApiService.urls`)
holds a producer link to `ApplicationEnvironmentService.environmentSignal`.
That link's `prevConsumer` captures a stale reference to a destroyed view's
`ReactiveLViewConsumer` link, retaining the entire LView hierarchy —
components, QueryLists, ElementRefs, and detached DOM — after the view is
destroyed.

The fix initializes `prevConsumer` to `undefined` at link creation time.
This is safe because `producerAddLiveConsumer` unconditionally sets
`link.prevConsumer = consumersTail` (line 513) when the link is actually
inserted into the consumer list. The value set in `producerAccessed` was
always overwritten for live consumers, and was never correct for non-live
consumers.

Made-with: Cursor
Uses WeakRef + global.gc() to verify that destroyed effect consumers
become garbage-collectable when a non-live computed reads the same
producer.

The jasmine_test target is configured with node_options: --expose-gc.
GC tests are skipped in browser targets via isBrowser from
@angular/private/testing.

Made-with: Cursor
@pullapprove pullapprove Bot requested review from atscott, crisbeto and csmick May 12, 2026 00:13
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime requires: TGP This PR requires a passing TGP before merging is allowed labels May 12, 2026
@ngbot ngbot Bot added this to the Backlog milestone May 12, 2026
@milomg milomg changed the title Fix/signal prev consumer leak v2 Fix signal prev consumer leak v2 May 12, 2026
@milomg milomg changed the title Fix signal prev consumer leak v2 fix(core): prevent dangling prevConsumer reference from leaking v2 May 12, 2026
@milomg milomg force-pushed the fix/signal-prevConsumer-leak branch from 2257e64 to c700fd3 Compare May 14, 2026 06:45
@milomg

milomg commented May 25, 2026

Copy link
Copy Markdown
Member Author

the failing google-internal-tests are flakes

@JeanMeche JeanMeche removed the request for review from crisbeto May 29, 2026 12:19
@JeanMeche JeanMeche added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels May 29, 2026
@ngbot

ngbot Bot commented May 29, 2026

Copy link
Copy Markdown

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending status "mergeability" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@milomg

milomg commented May 31, 2026

Copy link
Copy Markdown
Member Author

passing TGP: http://fusion2/OCL:915259433:BASE:924009792:1780173393700:cd028c0f

@atscott

atscott commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

This PR was merged into the repository. The changes were merged into the following branches:

atscott pushed a commit that referenced this pull request Jun 4, 2026
…oyed views (#68681)

When `producerAccessed` creates a new link for a non-live consumer (e.g.
a computed signal with no readers), it eagerly sets `prevConsumer` to the
producer's current `consumersTail`. However, because the consumer is not
live, `producerAddLiveConsumer` is skipped and the link is never inserted
into the producer's consumer doubly-linked list.

This means the link holds a reference *into* the producer's consumer list
without being *part* of it. When the node that `prevConsumer` points to is
later removed via `producerRemoveLiveConsumerLink`, the dangling link is
not patched because it isn't traversable from the list.

The result is that the removed consumer link — and everything it
references — is kept alive by the dangling `prevConsumer` pointer on the
non-live link, which itself is kept alive through the computed signal's
`producers` linked list.

In practice this causes multi-MB memory leaks in Angular apps: a
root-provided service with a computed signal (e.g. `AttachmentApiService.urls`)
holds a producer link to `ApplicationEnvironmentService.environmentSignal`.
That link's `prevConsumer` captures a stale reference to a destroyed view's
`ReactiveLViewConsumer` link, retaining the entire LView hierarchy —
components, QueryLists, ElementRefs, and detached DOM — after the view is
destroyed.

The fix initializes `prevConsumer` to `undefined` at link creation time.
This is safe because `producerAddLiveConsumer` unconditionally sets
`link.prevConsumer = consumersTail` (line 513) when the link is actually
inserted into the consumer list. The value set in `producerAccessed` was
always overwritten for live consumers, and was never correct for non-live
consumers.

Made-with: Cursor

PR Close #68681
atscott pushed a commit that referenced this pull request Jun 4, 2026
…#68681)

Uses WeakRef + global.gc() to verify that destroyed effect consumers
become garbage-collectable when a non-live computed reads the same
producer.

The jasmine_test target is configured with node_options: --expose-gc.
GC tests are skipped in browser targets via isBrowser from
@angular/private/testing.

Made-with: Cursor

PR Close #68681
@atscott atscott closed this in 55639ac Jun 4, 2026
atscott pushed a commit that referenced this pull request Jun 4, 2026
…#68681)

Uses WeakRef + global.gc() to verify that destroyed effect consumers
become garbage-collectable when a non-live computed reads the same
producer.

The jasmine_test target is configured with node_options: --expose-gc.
GC tests are skipped in browser targets via isBrowser from
@angular/private/testing.

Made-with: Cursor

PR Close #68681
@milomg milomg deleted the fix/signal-prevConsumer-leak branch June 5, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime requires: TGP This PR requires a passing TGP before merging is allowed target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants