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

.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.

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<source, Set<reporter>>` — 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).

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

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<any>, 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<any>, 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

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<any>): 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

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<T>(ms: number, value?: T): Promise<T> {
+  return new Promise<T>(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"]);
+  });
+});

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 () => {