Skip to content

fix(signals): attribution tables and history release disposed nodes - #4837

Draft
brenelz wants to merge 1 commit into
solidjs:nextfrom
brenelz:fix/attribution-tables-retain-nodes
Draft

brenelz wants to merge 1 commit into
solidjs:nextfrom
brenelz:fix/attribution-tables-retain-nodes

Conversation

@brenelz

@brenelz brenelz commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

Summary

With the attribution engine enabled, a disposed component stays reachable, with its owner and its DOM, until the engine is reset. @solidjs/vite-plugin 3.0.0-next.57 turns performance tracks on under vite dev, and the tracks enable the engine, so this reaches any app on the dev server. I found it in vercel-labs/ui, whose leak spec mounts and unmounts each component 50 to 200 times: on rc.21 all 47 of its Solid tests fail on the dev server. #4836 fixes the other leak that spec finds.

Three things held the nodes:

  • costs() and feedback() kept their rows in Maps keyed by the node (scopeCosts, flightStats, fallbackStats). The rows are now found through a WeakMap and listed from a separate collection of plain rows.
  • The rerun history (200 records by default) reached the written signal through recordNodes and the writing effect through effectFrames. Both now hold the node in a WeakRef. A diagnostic that needs the node reads it through effectFrame(origin) or deref(), and skips a node that was collected.
  • costs() kept one row for each scope ever created, so the table grew with every mount. A FinalizationRegistry now folds the row of a collected scope into one row per kind and name. reset() replaces the registry, so a new window releases the rows of the window before it.

Size: "app: CSR, observe tier + attribution engine enabled" grows by 113 B minified and 31 B brotli and stays under its cap. Every other scenario is byte-identical.

Open questions:

  • The fold changes what costs().scopes returns. Live scopes keep one row each, and collected scopes that share a kind and a name share one row, so the same scenario can list different rows depending on when the collector ran. @solidjs/diagnostics writes this table into its artifacts. The alternative is to keep one row per scope and let the table grow until reset. I can drop the fold if you prefer that.
  • feedback() rows are not folded and still grow by one per source or boundary. They hold no node.
  • Where FinalizationRegistry is missing, the rows are not folded. WeakRef is used without a guard on the engine's write path, as dev.ts already does for tailFlights.
  • I left other holders alone because they did not show up in the leak spec: asyncStartValue keeps one pre-landing value across disable(), and an interaction that never settles keeps its transitions until reset.

How did you test this change?

packages/signals/tests/attribution-retention.test.ts has five tests. Each one disposes a root, collects garbage through the inspector, and checks that WeakRefs to the payload and the owner are empty while the row or history record is still there. They cover a costs() row, a feedback() flight and fallback, a rerun record written by a relay effect, the fold of three remounts into one row, and the release of a window's rows on the next enable().

  • pnpm test in packages/signals: 513 files, 7,108 tests pass.
  • pnpm test in packages/solid: 55 files, 929 tests pass.
  • In packages/web, npx vitest run: 266 files, 1,899 tests pass. With --config vite.config.hydrate.mjs: 147 files, 779 tests pass. With --config vite.config.server.mjs: 228 files pass, and test/server/foreign-raw-text-4704.spec.tsx does not compile with my local compiler binary, which is older than the fix for <style> and <script> children inside MathML are not escaped in SSR (and static DOM templates), allowing markup injection #4704. The hydrate and server runs predate the last revision of attribution-costs.ts; the client run does not.
  • Size from node size.mjs --no-gate --json on this branch and on its base, then node gate.mjs head.json base.json. The gate's one failure, "page: compiled live server components", is over its cap on the base by the same amount.

In vercel-labs/ui on rc.21 I applied this change and #4836 to the installed @solidjs/signals and @solidjs/web builds and ran npx playwright test specs/leaks.spec.ts --project solid --workers 1 --retries 0 against its dev server with performance tracks on: 47 of 47 pass. That run used the first version of this change, before reset() replaced the registry and before the FinalizationRegistry guard.

🤖 Generated with Claude Code

@changeset-bot

changeset-bot Bot commented Nov 13, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5e52e28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@solidjs/signals Patch
test-integration Patch
@solidjs/web Patch
@solidjs/babel-plugin Patch
@solidjs/compiler Patch
@solidjs/diagnostics Patch
@solidjs/element Patch
@solidjs/h Patch
@solidjs/html Patch
solid-js Patch
@solidjs/universal Patch
board-example Patch
canvas-example Patch
server-dbmon-example Patch
todos-server-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codspeed

codspeed Bot commented Nov 13, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

2 warnings

⚠️ Upgrade your CodSpeed plugin for Node.js 24

@codspeed/* 5.4.0 is only tested on Node.js 18, 20, and 22, so your Node.js 24 measurements may be inaccurate. Upgrade your @codspeed/* packages to 6.0.0 or later.

See affected benchmarks

⚠️ 3 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 201 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing brenelz:fix/attribution-tables-retain-nodes (5e52e28) with next (f00ce6f)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

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.

1 participant