diff --git a/.changeset/fix-held-transition-child-disposal.md b/.changeset/fix-held-transition-child-disposal.md new file mode 100644 index 000000000..fe9a1d488 --- /dev/null +++ b/.changeset/fix-held-transition-child-disposal.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +A node recomputed under a held transaction no longer tears down the committed frame's children immediately. Status propagation stamps a parked dependent with the transaction without recomputing it, so its owned children (nested render effects, memos, `onCleanup` registrations) still belong to what is on screen; when the pending source landed, the recompute disposed them on the spot and their cleanups ran mid-hold, before the transaction's atomic reveal (#3404). Those children are now deferred as zombies until the node commits, matching the plain-flush path. Children built by a recompute that never committed (a staged value, a pending window, a run under the transaction) are still disposed immediately on the next re-run — no frame ever showed them. diff --git a/packages/signals/docs/INTERNALS-ASYNC-STATE.md b/packages/signals/docs/INTERNALS-ASYNC-STATE.md index 8592ba14b..5e181a6a3 100644 --- a/packages/signals/docs/INTERNALS-ASYNC-STATE.md +++ b/packages/signals/docs/INTERNALS-ASYNC-STATE.md @@ -95,6 +95,7 @@ Semantics of the `(_pendingValue, _overrideValue)` pair for an optimistic node - `initTransition` ends by scheduling a flush: the ambient window is one flush by definition, but parking is flush-driven, so a transaction opened with no writes (an action that only awaits) would otherwise leave `activeTransition` and the adopted batch armed across the async gap, capturing the next unrelated work to arrive — the A26-rejected behavior (#3141). - `_asyncReporters: Map>` — which computeds are blocked on which async sources. **Entries open only from `GlobalQueue.notify` during render-effect status notification** `[ruled — async-registration-invariants rule]`. One reporter may join an entry that already exists from elsewhere: a stale reader served a pending node's committed value by the reveal carve-out (`heldFromStale`, §3 below) joins that node's entry (#3374) — it observes the flight, so it holds the transaction on it the way the reader that opened the entry did, and dies with disposal the same way (`reporterBlocksSource`: the read linked the node as a dep). - `_pendingNodes` — nodes whose `_pendingValue` commits when the transition completes (`commitPendingNodes` → `commitPendingNode`). +- Held children (#3404): a node's owned children (nested effects, memos, `onCleanup` registrations) belong to the frame that committed them. `recompute` defers the previous pass's children as zombies (`_pendingFirstChild` / `_pendingDisposal`, rendering mainline until `commitPendingNode` disposes them) unless `CONFIG_HELD_CHILDREN` is set — the pass that built them never committed (a staged value, a pending window, a run under a held transaction), so no frame ever showed them and they die on the spot. Set at recompute's tail whenever the pass's result waits on a commit, cleared by `commitPendingNode`. A `_transition` stamp alone says nothing about the children: status propagation stamps a parked dependent without recomputing it, so its children are still the committed frame's, and disposing them when the source lands ran their cleanups mid-hold. Exception: a transaction-owned effect recomputed mainline (contested, #3322) publishes directly, so that pass's children are the frame's and it releases its zombies itself — its commit rides the transaction, not the flush. - `_optimisticNodes` — nodes whose override reverts at completion (`resolveOptimisticNodes`). - Incomplete-transition flush stashes queues (`stashQueues`) and continues with a fresh view; completion restores them, commits pending, reverts optimistic, replays `_gatedSubs`, cleans lanes. - `_contested` — effects whose single value slot was written under this transaction and then overwritten by another live transaction or by mainline (#3322). Effects are not shared state, so a shared effect never merges transactions (memos do, via their `_transition` stamp); instead `Effect._valueTransition` records which view produced `_value`, `recompute`, when that owner changes, registers the effect on every owed live transaction, and `finalizePureQueue` re-dirties them **before** its heap run so the re-derive and the effect phase land in the same pass — the other view's value is never published. Exception: a settle that reverts optimism (a non-empty `_optimisticNodes`) re-dirties them **after** `_resolveOptimistic`, with the gated replay — between `commitPendingNodes` and the revert the truth is committed but the overrides still display, and a re-derive there composes the two (the #3164 tear; the reveal wake sits post-revert for the same reason). The slot meanwhile holds the frame already on screen, so nothing new is published early. Rules that fall out: a stale (render) reader with no transaction active is mainline and sees a foreign transaction's staged signal as committed (`read`'s fast path and `readNodeFast` apply `stale && el._transition !== null`, matching the slow path); a value computed mainline needs no protection (mainline publishes what it computes, and a transaction whose writes never touched the effect finds it still correct at commit). diff --git a/packages/signals/src/core/constants.ts b/packages/signals/src/core/constants.ts index 09fef26a6..0747839ab 100644 --- a/packages/signals/src/core/constants.ts +++ b/packages/signals/src/core/constants.ts @@ -134,6 +134,19 @@ export const CONFIG_SLOT_NODE = 1 << 18; * write (a new override re-masks) and by the revert. */ export const CONFIG_OVERRIDE_SUPERSEDED = 1 << 19; +/** HELD children (#3404): this node's `_firstChild` chain (and `_disposal` + * list) was built by a recompute whose result has not committed — a staged + * value, a pending window, or a run under a held transaction. A later + * recompute may tear those children down immediately: nothing observable + * was ever built on them. Unset, the children belong to the committed frame + * and a recompute defers them as zombies (`_pendingFirstChild`) until this + * node commits — regardless of whether the recompute runs under a + * transaction. A parked node (status propagation stamps `_transition` + * without recomputing) recomputed when its source lands otherwise disposed + * its committed children mid-hold, running their cleanups before the + * transaction's atomic reveal. Cleared by `commitPendingNode`. */ +export const CONFIG_HELD_CHILDREN = 1 << 20; + /** In-flight async node whose inputs were PUBLISHED while it was pending: a * batch or transaction committed with the node still `STATUS_PENDING` (an * unobserved flight, #3305), so the inputs are on screen and the node's diff --git a/packages/signals/src/core/core.ts b/packages/signals/src/core/core.ts index e25228c8c..e22aa75ac 100644 --- a/packages/signals/src/core/core.ts +++ b/packages/signals/src/core/core.ts @@ -19,6 +19,7 @@ import { CONFIG_FRESH_READ, CONFIG_IN_SNAPSHOT_SCOPE, CONFIG_HAS_COMPANIONS, + CONFIG_HELD_CHILDREN, CONFIG_HAS_LANE, CONFIG_HAS_SNAPSHOT, CONFIG_INPUTS_PUBLISHED, @@ -240,8 +241,14 @@ export function recompute(el: Computed, create: boolean = false): void { // work that replaced it. Idempotent with the cleanup-channel close. releaseFlightTeardown(el); } - // Tracked effects run after finalizePureQueue, so dispose immediately instead of deferring - if (el._transition || isEffect === EFFECT_TRACKED) disposeChildren(el); + // Tracked effects run after finalizePureQueue, so dispose immediately + // instead of deferring. Children built by an uncommitted recompute + // (CONFIG_HELD_CHILDREN) die immediately too: no frame ever showed them. + // Everything else is the committed frame's and is deferred as zombies + // until this node's commit — a transaction-owned node included (#3404): + // a parked node's children predate the hold, and tearing them down when + // the source lands ran cleanups before the transaction's atomic reveal. + if (isEffect === EFFECT_TRACKED || el._config & CONFIG_HELD_CHILDREN) disposeChildren(el); else if (el._firstChild !== null || el._disposal !== null) { markDisposal(el); const x = ext(el); @@ -653,10 +660,28 @@ export function recompute(el: Computed, create: boolean = false): void { // under the override (A17). Revert no longer commits anything, so an // unqueued covered hold would leak (INV-7) once the revert clears // _transition. - needsPendingCommit && - (!create || el._statusFlags & STATUS_PENDING) && - (!el._transition || hasOverride) && - queuePendingNode(el); + // + // While a pass's result waits on a commit, its children (and `_disposal`) + // wait with it, and a re-run may tear them down immediately + // (CONFIG_HELD_CHILDREN, #3404). One exception: a transaction-owned effect + // recomputed mainline (contested, #3322) published its value directly — a + // `_pendingValue` left from an earlier held pass is that transaction's, + // not this one's — so this pass's children are the frame's, and any + // zombies deferred at the top are superseded on that same frame. Its + // commit rides the transaction, not this flush: release them here rather + // than let two generations render at once. + let held = needsPendingCommit && (!create || (el._statusFlags & STATUS_PENDING) !== 0); + if (held && (!el._transition || hasOverride)) queuePendingNode(el); + else if ( + held && + activeTransition === null && + !(el._statusFlags & (STATUS_PENDING | STATUS_UNINITIALIZED)) + ) { + held = false; + disposeChildren(el, false, true); + } + if (held) el._config |= CONFIG_HELD_CHILDREN; + else el._config &= ~CONFIG_HELD_CHILDREN; if (el._transition && isEffect && activeTransition !== el._transition) { // The re-run refreshes the transaction's STAGED view (_pendingValue); the // value this pass published in _value belongs to the run that just diff --git a/packages/signals/src/core/scheduler.ts b/packages/signals/src/core/scheduler.ts index 5326ce662..d47d0d3ae 100644 --- a/packages/signals/src/core/scheduler.ts +++ b/packages/signals/src/core/scheduler.ts @@ -11,6 +11,7 @@ import { REACTIVE_DISPOSED, REACTIVE_IN_HEAP, CONFIG_HAS_COMPANIONS, + CONFIG_HELD_CHILDREN, CONFIG_HAS_LANE, CONFIG_HAS_SNAPSHOT, CONFIG_INPUTS_PUBLISHED, @@ -1084,6 +1085,8 @@ function commitPendingNode(n: Signal): void { // store to an always-present computed slot. c._loading = false; c._flags! &= ~REACTIVE_MANUAL_WRITE; + // The children this commit publishes are the frame's now (#3404). + c._config! &= ~CONFIG_HELD_CHILDREN; if (!(c._statusFlags! & STATUS_PENDING)) c._statusFlags! &= ~STATUS_UNINITIALIZED; // A flight this commit leaves in the air (unobserved, or observed only by // a boundary) now has PUBLISHED inputs: its committed value is stale diff --git a/packages/signals/tests/nested-render-effect-async-cleanup.test.ts b/packages/signals/tests/nested-render-effect-async-cleanup.test.ts new file mode 100644 index 000000000..c35e3e5a4 --- /dev/null +++ b/packages/signals/tests/nested-render-effect-async-cleanup.test.ts @@ -0,0 +1,202 @@ +import { describe, expect, it } from "vitest"; +import { + createLoadingBoundary, + createMemo, + createRenderEffect, + createRoot, + createSignal, + flush, + onCleanup +} from "../src/index.js"; + +// Manual clock (see async-chain-supersession.test.ts). +let now = 0; +let timers: { at: number; run: () => void }[] = []; +function delay(ms: number, value?: T): Promise { + return new Promise(r => timers.push({ at: now + ms, run: () => r(value as T) })); +} +async function settle() { + for (let r = 0; r < 3; r++) { + for (let i = 0; i < 10; i++) await Promise.resolve(); + flush(); + } +} +async function advanceTo(t: number) { + while (true) { + timers.sort((a, b) => a.at - b.at); + const next = timers[0]; + if (!next || next.at > t) break; + timers.shift(); + now = next.at; + next.run(); + await settle(); + } + now = t; + await settle(); +} +function reset() { + now = 0; + timers = []; +} + +describe("#3404 nested render effect reading a downstream async value", () => { + it("does not clean up the inner effect until the whole chain has settled", async () => { + reset(); + const log: string[] = []; + let setA!: (fn: (v: number) => number) => void; + createRoot(() => { + const [a, sa] = createSignal(1); + setA = sa; + const b = createMemo(() => delay(500, a())); + const c = createMemo(() => delay(1000, b())); + createLoadingBoundary( + () => { + createRenderEffect( + () => { + b(); + createRenderEffect(c, v => { + log.push(`run ${v}@${now}`); + return () => log.push(`cleanup ${v}@${now}`); + }); + }, + () => {} + ); + }, + () => {} + ); + }); + flush(); + await advanceTo(2000); + expect(log).toEqual(["run 1@1500"]); + + setA(x => x + 1); + flush(); + await advanceTo(2400); + expect(log).toEqual(["run 1@1500"]); + // b lands at 2500 but c is still in flight until 3500: nothing observable. + await advanceTo(3000); + expect(log).toEqual(["run 1@1500"]); + await advanceTo(4000); + expect(log).toEqual(["run 1@1500", "cleanup 1@3500", "run 2@3500"]); + }); + + it("a memo's onCleanup registrations wait for the commit the same way", async () => { + reset(); + const log: string[] = []; + let setA!: (fn: (v: number) => number) => void; + createRoot(() => { + const [a, sa] = createSignal(1); + setA = sa; + const b = createMemo(() => delay(500, a())); + const c = createMemo(() => delay(1000, b())); + createLoadingBoundary( + () => { + const outer = createMemo(() => { + const v = b(); + onCleanup(() => log.push(`cleanup ${v}@${now}`)); + return v; + }); + createRenderEffect( + () => [outer(), c()], + ([o, cc]) => { + log.push(`run ${o}/${cc}@${now}`); + } + ); + }, + () => {} + ); + }); + flush(); + await advanceTo(2000); + expect(log).toEqual(["run 1/1@1500"]); + + setA(x => x + 1); + flush(); + await advanceTo(3000); + expect(log).toEqual(["run 1/1@1500"]); + await advanceTo(4000); + expect(log).toEqual(["run 1/1@1500", "cleanup 1@3500", "run 2/2@3500"]); + }); + + it("children built under the hold are torn down on the next held re-run, the frame's at commit", async () => { + reset(); + const log: string[] = []; + let setA!: (fn: (v: number) => number) => void; + let setTick!: (fn: (v: number) => number) => void; + createRoot(() => { + const [a, sa] = createSignal(1); + const [tick, st] = createSignal(0); + setA = sa; + setTick = st; + const b = createMemo(() => delay(500, a())); + const c = createMemo(() => delay(1000, b())); + createLoadingBoundary( + () => { + createRenderEffect( + () => { + const v = b(); + const t = tick(); + createRenderEffect(c, cv => { + log.push(`run ${v}.${t}/${cv}@${now}`); + return () => log.push(`cleanup ${v}.${t}/${cv}@${now}`); + }); + }, + () => {} + ); + }, + () => {} + ); + }); + flush(); + await advanceTo(2000); + expect(log).toEqual(["run 1.0/1@1500"]); + + setA(x => x + 1); + flush(); + // b lands at 2500: the outer effect re-runs under the hold, building an + // inner effect that never runs (c is in flight). A sync write while the + // hold is open re-runs it mainline (contested, #3322): the held inner + // effect dies silently, the frame's is replaced on the spot, and the + // transaction's re-derive builds a fresh held one for the reveal. + await advanceTo(2600); + setTick(t => t + 1); + flush(); + await advanceTo(3000); + expect(log).toEqual(["run 1.0/1@1500", "run 1.1/1@2600", "cleanup 1.0/1@2600"]); + await advanceTo(4000); + expect(log).toEqual([ + "run 1.0/1@1500", + "run 1.1/1@2600", + "cleanup 1.0/1@2600", + "cleanup 1.1/1@3500", + "run 2.1/2@3500" + ]); + }); + + it("a plain sync re-run still releases the previous children at its own commit", () => { + const log: string[] = []; + let setA!: (fn: (v: number) => number) => void; + createRoot(() => { + const [a, sa] = createSignal(1); + setA = sa; + createRenderEffect( + () => { + const v = a(); + createRenderEffect( + () => v, + cv => { + log.push(`run ${cv}`); + return () => log.push(`cleanup ${cv}`); + } + ); + }, + () => {} + ); + }); + flush(); + expect(log).toEqual(["run 1"]); + setA(x => x + 1); + flush(); + expect(log).toEqual(["run 1", "run 2", "cleanup 1"]); + }); +}); diff --git a/packages/signals/tests/treeshake.test.ts b/packages/signals/tests/treeshake.test.ts index 4b0f23619..5a2b1c791 100644 --- a/packages/signals/tests/treeshake.test.ts +++ b/packages/signals/tests/treeshake.test.ts @@ -252,7 +252,13 @@ describe("pay-for-use tree-shaking (#2883)", () => { // (22,915 → 22,973). // Companion lane parented (#3379): `notifyStatus` assigns the node's lane // before poking its companions — a reorder, -4 B (22,973 → 22,969). - expect(minifiedBytes).toBeLessThan(23_050); + // Held children (#3404): recompute defers a node's children as zombies + // unless the pass that built them never committed (CONFIG_HELD_CHILDREN, + // set at recompute's tail, cleared by commitPendingNode) — a + // transaction-owned node's committed children previously died on the + // spot — and a contested effect's mainline pass releases its zombies + // itself. +102 B (22,969 → 23,071). + expect(minifiedBytes).toBeLessThan(23_150); }); it("plain stores shed the verdict layer, affects, boundaries, and map", async () => { diff --git a/packages/web/test/nested-render-effect-cleanup-issue-3404.spec.tsx b/packages/web/test/nested-render-effect-cleanup-issue-3404.spec.tsx new file mode 100644 index 000000000..ad5bd9e43 --- /dev/null +++ b/packages/web/test/nested-render-effect-cleanup-issue-3404.spec.tsx @@ -0,0 +1,97 @@ +/** + * @jsxImportSource @solidjs/web + * @vitest-environment jsdom + */ + +import { expect, test } from "vitest"; +import { createMemo, createRenderEffect, createSignal, flush, Loading, Show } from "solid-js"; +import { Portal, render } from "../src/index.js"; + +// Manual clock: async memos resolve when the clock passes their due time, in +// due-time order, with a full settle between landings. +let now = 0; +let timers: { at: number; run: () => void }[] = []; +function delay(ms: number, value: T): Promise { + return new Promise(r => timers.push({ at: now + ms, run: () => r(value) })); +} +async function settle() { + for (let r = 0; r < 3; r++) { + for (let i = 0; i < 10; i++) await Promise.resolve(); + flush(); + } +} +async function advanceTo(t: number) { + while (true) { + timers.sort((a, b) => a.at - b.at); + const next = timers[0]; + if (!next || next.at > t) break; + timers.shift(); + now = next.at; + next.run(); + await settle(); + } + now = t; + await settle(); +} + +test("a nested render effect reading a downstream async value is not cleaned up mid-hold (#3404)", async () => { + now = 0; + timers = []; + const root = document.createElement("div"); + const target = document.createElement("div"); + const log: string[] = []; + let bump!: () => void; + + const dispose = render( + () => ( + + {(() => { + const [a, setA] = createSignal(1); + bump = () => setA(x => x + 1); + const b = createMemo(() => delay(500, a())); + const c = createMemo(() => delay(1000, b())); + + createRenderEffect( + () => { + b(); + createRenderEffect(c, v => { + log.push(`run ${v}@${now}`); + return () => log.push(`cleanup ${v}@${now}`); + }); + }, + () => {} + ); + + return ( + + + +
{c()}
+ + + + ); + })()} + + ), + root + ); + + await advanceTo(2000); + expect(target.innerHTML).toBe("
1
"); + expect(log).toEqual(["run 1@1500"]); + + bump(); + flush(); + // b lands at 2500 while c is still in flight until 3500: the portal must + // keep showing the previous frame until the whole update can reveal. + await advanceTo(3000); + expect(target.innerHTML).toBe("
1
"); + expect(log).toEqual(["run 1@1500"]); + + await advanceTo(4000); + expect(target.innerHTML).toBe("
2
"); + expect(log).toEqual(["run 1@1500", "cleanup 1@3500", "run 2@3500"]); + + dispose(); +}); diff --git a/scripts/size/.size-limit.js b/scripts/size/.size-limit.js index 50afb9b49..4d86f3ae8 100644 --- a/scripts/size/.size-limit.js +++ b/scripts/size/.size-limit.js @@ -425,7 +425,10 @@ module.exports = [ // Companion lane parented (#3379, 2026-09-12): 15.50 -> 15.55 KB, measured // at 15522 B against 15485 (+37 brotli on a -4 B minified reorder — the // statement moved across a block boundary; noise, not weight). - limit: "15.55 KB", + // Held children (#3404, 2026-09-13): 15.55 -> 15.60 KB, measured at + // 15552 B on the merge with `next` — CONFIG_HELD_CHILDREN set/cleared + // around recompute and commitPendingNode. + limit: "15.60 KB", modifyEsbuildConfig }, { @@ -863,6 +866,8 @@ module.exports = [ // (#3412, 2026-09-13): 28.40 -> 28.45 KB, measured at 28408 B on the // merge with `next` — the save/restore of `_valueTransition` in // recompute, on top of #3413's companion-gate change. + // Held children (#3404, 2026-09-13): cap held at 28.45 KB; see the + // createStore note. limit: "28.45 KB", modifyEsbuildConfig }, @@ -1011,7 +1016,9 @@ module.exports = [ // provider). One literal property on the observe object, inert on the // client by design. Measured at 15501 B against 15485 without the slot // on the same `next` (+16), within the 15.55 KB cap. Observe-only. - limit: "15.55 KB", + // Held children (#3404, 2026-09-13): 15.55 -> 15.60 KB, measured at + // 15562 B on the merge with `next`; see the createStore note. + limit: "15.60 KB", modifyEsbuildConfig: observeEsbuildConfig }, {