Skip to content

fix(signals): defer a transaction-owned node's committed children to its commit (#3404) - #3405

Merged
ryansolid merged 4 commits into
solidjs:nextfrom
brenelz:fix/nested-render-effect-cleanup-3404
Sep 14, 2026
Merged

ryansolid merged 4 commits into
solidjs:nextfrom
brenelz:fix/nested-render-effect-cleanup-3404

Conversation

@brenelz

@brenelz brenelz commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #3404.

Bug

A render effect parked on a pending async source is stamped with the transaction by status propagation without being recomputed, so its owned children (nested render effects, memos, onCleanup registrations) still belong to the committed frame. When the source landed, recompute saw the _transition stamp and took the "dispose children immediately" branch, running their cleanups mid-hold. In the issue's repro a Portal under a keyed Show vanished when the first hop of an async chain landed and only reappeared once the whole chain settled, instead of swapping atomically.

Fix

  • New CONFIG_HELD_CHILDREN bit: this node's current children were built by a pass whose result has not committed (a staged value, a pending window, a run under a held transaction).
  • recompute disposes children immediately only for tracked effects or when the bit is set (no frame ever showed them). Everything else is deferred as zombies until the node's commit, whether or not a transaction owns the node.
  • The bit is set at the tail of recompute whenever the pass's result waits on a commit, and cleared by commitPendingNode.
  • Exception: a transaction-owned effect recomputed mainline (contested, 2.0.0-rc.7 render effect depending on non async values from two concurrent updates commit with the wrong data #3322) publishes its value directly, so that pass's children are the frame's and it releases its zombies itself rather than leaving two generations rendering until the transaction commits.

Tests

  • packages/signals/tests/nested-render-effect-async-cleanup.test.ts: the issue's shape, a memo with onCleanup, a sync write during the hold (held children die silently, the frame's swap and then wait for the reveal), and the plain sync ordering pinned as it already was.
  • packages/web/test/nested-render-effect-cleanup-issue-3404.spec.tsx: the Portal / Show / Loading scenario from the playground. Fails on next with an empty portal target mid-hold, passes with the fix.
  • Full signals and web suites pass. Tree-shake floor +102 B, recorded in the budget ledger with the threshold raised accordingly.

Changeset and a note in INTERNALS-ASYNC-STATE.md included.

🤖 Generated with Claude Code

…its commit (solidjs#3404)

A render effect parked on a pending source is stamped with the transaction
without recomputing, so its owned children (nested render effects, memos,
onCleanup registrations) still belong to the committed frame. When the source
landed, recompute saw the stamp and disposed them on the spot, running their
cleanups mid-hold — a Portal under a keyed Show disappeared when the first hop
of an async chain landed and reappeared only when the whole chain settled.

Children are now deferred as zombies until the node commits unless
CONFIG_HELD_CHILDREN says the pass that built them never committed (a staged
value, a pending window, a run under a held transaction), in which case no
frame ever showed them and they die immediately. The bit is set at recompute's
tail and cleared by commitPendingNode. A transaction-owned effect recomputed
mainline (contested, solidjs#3322) publishes directly and releases its zombies itself
rather than letting two generations render until the transaction commits.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0c48456

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

This PR includes changesets to release 11 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

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 Sep 13, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 36.55%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 2 regressed benchmarks
✅ 158 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
projection derive: write one NESTED field (reference) 222 µs 523.2 µs -57.56%
memo + sync render effect only (reference) 82.1 ms 86.6 ms -5.13%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing brenelz:fix/nested-render-effect-cleanup-3404 (0c48456) with next (50323b4)

Open in CodSpeed

…aps for solidjs#3404's held-children bit (+2/+14/+12 B)

Co-authored-by: Cursor <cursoragent@cursor.com>
ryansolid and others added 2 commits September 14, 2026 02:03
Resolve recompute's tail against solidjs#3420 (solidjs#3412): keep the held-children
bookkeeping and the _valueTransition save/restore around the forced
in-transaction re-run. Size ledger notes merged; caps unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@ryansolid
ryansolid merged commit e80f241 into solidjs:next Sep 14, 2026
6 checks passed
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.

2 participants