Merge pull request #2760 from spokodev/fix/scheduler-zombie-heap-migration

fix(signals): migrate height-adjust heap entries in markDisposal to keep zombie flag and queue in sync

Author: ryansolid

.changeset/brave-heaps-refuse.md

@@ -0,0 +1,5 @@
+---
+"@solidjs/signals": patch
+---
+
+fix scheduler livelock after disposing a subtree with a stale height-adjust heap entry

packages/solid-signals/src/core/owner.ts

@@ -4,6 +4,7 @@ import {
   defaultContext,
   REACTIVE_DISPOSED,
   REACTIVE_IN_HEAP,
+  REACTIVE_IN_HEAP_HEIGHT,
   REACTIVE_ZOMBIE
 } from "./constants.js";
 import {
@@ -16,7 +17,7 @@ import {
 } from "./core.js";
 import { clearSignals, DEV, emitDiagnostic } from "./dev.js";
 import { unlinkSubs } from "./graph.js";
-import { deleteFromHeap, insertIntoHeap } from "./heap.js";
+import { deleteFromHeap, insertIntoHeap, insertIntoHeapHeight } from "./heap.js";
 import { dirtyQueue, globalQueue, zombieQueue } from "./scheduler.js";
 import type { Computed, Disposable, Owner, Root } from "./types.js";
 
@@ -25,10 +26,20 @@ const PENDING_OWNER = {} as Owner; // Dummy owner to trigger store's read() path
 export function markDisposal(el: Owner): void {
   let child = el._firstChild;
   while (child) {
-    (child as Computed<unknown>)._flags |= REACTIVE_ZOMBIE;
-    if ((child as Computed<unknown>)._flags & REACTIVE_IN_HEAP) {
-      deleteFromHeap(child as Computed<unknown>, dirtyQueue);
-      insertIntoHeap(child as Computed<unknown>, zombieQueue);
+    const flags = (child as Computed<unknown>)._flags;
+    (child as Computed<unknown>)._flags = flags | REACTIVE_ZOMBIE;
+    // migrate height-adjust entries too, not just recompute entries: every
+    // `deleteFromHeap` call site picks the queue from the zombie flag, so a
+    // node left physically linked in `dirtyQueue` after being zombified gets
+    // unlinked from the wrong queue on dispose, corrupting the bucket and
+    // livelocking the next `runHeap` that reaches it (#2759)
+    if (flags & (REACTIVE_IN_HEAP | REACTIVE_IN_HEAP_HEIGHT)) {
+      deleteFromHeap(
+        child as Computed<unknown>,
+        flags & REACTIVE_ZOMBIE ? zombieQueue : dirtyQueue
+      );
+      if (flags & REACTIVE_IN_HEAP) insertIntoHeap(child as Computed<unknown>, zombieQueue);
+      else insertIntoHeapHeight(child as Computed<unknown>, zombieQueue);
     }
     markDisposal(child);
     child = child._nextSibling;

packages/solid-signals/tests/scheduler-livelock.test.ts

@@ -0,0 +1,123 @@
+import { expect, it } from "vitest";
+import { createMemo, createRenderEffect, createRoot, createSignal, flush } from "../src/index.js";
+import { REACTIVE_IN_HEAP, REACTIVE_IN_HEAP_HEIGHT } from "../src/core/constants.js";
+import { dirtyQueue, zombieQueue } from "../src/core/scheduler.js";
+import type { Computed } from "../src/core/types.js";
+
+/** A chain of pass-through memos, standing in for a deep derived-data pipeline. */
+function chain<T>(src: () => T, n: number): () => T {
+  let cur = src;
+  for (let i = 0; i < n; i++) {
+    const prev = cur;
+    cur = createMemo(() => prev());
+  }
+  return cur;
+}
+
+/**
+ * Every node physically linked in a heap must carry an in-heap flag, because
+ * `runHeap` makes no progress on a bucket head that `deleteFromHeap` refuses
+ * to unlink (it early-returns on the flag check), and every `deleteFromHeap`
+ * call site picks the queue from the node's flags.
+ */
+function entriesWithoutInHeapFlags(heap: typeof dirtyQueue): number[] {
+  const corrupted: number[] = [];
+  for (let height = 0; height < heap._heap.length; height++) {
+    for (
+      let el: Computed<unknown> | undefined = heap._heap[height];
+      el !== undefined;
+      el = el._nextHeap
+    ) {
+      if (!(el._flags & (REACTIVE_IN_HEAP | REACTIVE_IN_HEAP_HEIGHT))) {
+        corrupted.push(el._flags);
+      }
+    }
+  }
+  return corrupted;
+}
+
+/**
+ * @see https://github.com/solidjs/solid/issues/2759
+ */
+it("disposing a subtree with a stale height-adjust entry does not corrupt the dirty queue", () => {
+  const [open, setOpen] = createSignal(true);
+  let setRows!: (v: number[] | null) => void;
+  let view: () => unknown = () => null;
+  let dispose!: () => void;
+
+  createRoot(d => {
+    dispose = d;
+    // keeps the unmount recompute at a low height, so the flush that disposes
+    // the subtree never advances the cursor up to the stale entry's bucket
+    const gate = chain(() => open(), 4);
+    const appView = createMemo(() => {
+      if (!gate()) return null;
+
+      // ~<Inner>: an async data source that lands after mount
+      const [rows, set] = createSignal<number[] | null>(null);
+      setRows = set;
+
+      // parks the "condition value" memo at a height above everything the
+      // unmount flush will visit
+      const tall = chain(
+        createMemo(() => 1),
+        12
+      );
+      const deepA = chain(
+        createMemo(() => 1),
+        20
+      );
+      const deepB = chain(() => rows(), 45);
+
+      // mirrors <Show when={expr}>: the compiler wraps the `when` expression
+      // in a memo that is created lazily inside (and owned by) Show's
+      // "condition value" memo, so the parent's height never tracks the
+      // child's height
+      let expr: (() => unknown) | undefined;
+      const conditionValue = createMemo(() => {
+        tall();
+        expr ??= createMemo(() => (rows() === null ? deepA() : deepB()) && true);
+        return expr();
+      });
+      const condition = createMemo(() => conditionValue(), {
+        equals: (a, b) => !a === !b
+      });
+      const value = createMemo(() => (condition() ? "inner ready" : null));
+      return value();
+    });
+    view = appView;
+    createRenderEffect(appView, () => undefined);
+  });
+
+  flush();
+  expect(view()).toBe("inner ready");
+
+  // The async data lands: the condition expression memo recomputes, its
+  // height grows past the already-advanced cursor while its value stays the
+  // same, so its subscriber is re-inserted for height adjustment at a stale
+  // height the cursor has already passed and survives the flush linked in
+  // `dirtyQueue` with only `REACTIVE_IN_HEAP_HEIGHT` set.
+  setRows([1, 2, 3]);
+  flush();
+
+  // Unmount: `markDisposal` zombifies the subtree and the pending disposal
+  // commits. The height-only entry must migrate queues together with its
+  // zombie flag, otherwise the commit deletes it from the queue its flag
+  // names (`zombieQueue`) while it is physically linked in `dirtyQueue`.
+  setOpen(false);
+  flush();
+
+  expect(entriesWithoutInHeapFlags(dirtyQueue)).toEqual([]);
+  expect(entriesWithoutInHeapFlags(zombieQueue)).toEqual([]);
+
+  // Remount: on corrupted state this flush livelocks in `runHeap`, pinning
+  // the thread forever.
+  setOpen(true);
+  flush();
+  setRows([1, 2, 3]);
+  flush();
+  expect(view()).toBe("inner ready");
+
+  dispose();
+  flush();
+});