fix(signals): stage a pass's error before the flush joins its hold (#4109)

.changeset/held-sync-throw-4109.md

@@ -0,0 +1,7 @@
+---
+"@solidjs/signals": patch
+---
+
+A synchronous memo that throws in a flush that later holds now waits for its frame like a value (D-41, #4109). The error was staged only when the flush had already joined a hold, so a pass that ran before its async sibling went pending published the error at once: an untracked read (a click handler) threw while the screen still showed the committed value.
+
+A later pass of the same node replaces a staged error: a value, a pending flight or a new error supersedes it, so the value is no longer thrown as an error (#4145) and the superseded error is no longer published at the commit.

packages/signals/src/core/async.ts

@@ -30,7 +30,6 @@ import { cleanup } from "./owner.js";
 import {
   clock,
   flush,
-  flushTransaction,
   globalQueue,
   joinFuture,
   MAINLINE_QUESTION,
@@ -182,13 +181,15 @@ export function releaseSettledDependents(el: Computed<any>): void {
  * as before. Returns whether the error was staged. */
 export function stageError(el: Computed<any>, error: any, inPass?: boolean): boolean {
   // Held, or — for a pass's own throw — derived from a staging of a flush
-  // that has joined a hold (REACTIVE_STAGED_READ is this pass's; a landing
-  // after the pass cannot read it, the flush it read a staging of has
-  // committed or parked since). A plain flush publishes now (S-38).
+  // (REACTIVE_STAGED_READ is this pass's; a landing after the pass cannot
+  // read it, the flush it read a staging of has committed or parked since).
+  // Staged whether or not the flush has joined a hold yet: a pass that runs
+  // before its async sibling joins is the seam's to decide, like a value
+  // (#4109). A flush that parks nothing publishes it at its commit (S-38).
   if (
     !(
       el._config & CONFIG_HELD ||
-      (inPass && el._flags & REACTIVE_STAGED_READ && flushTransaction)
+      (inPass && el._flags & REACTIVE_STAGED_READ && globalQueue._running)
     ) ||
     passLane
   )

packages/signals/src/core/core.ts

@@ -387,6 +387,12 @@ export function recompute(el: Computed<any>, create: boolean = false): void {
     // and own nothing to park.
     if (isEffect === EFFECT_TRACKED || el._config & CONFIG_STAGED) disposeChildren(el);
     else if (el._firstChild !== null || el._disposal !== null) parkChildren(el);
+    // An error a pass staged (D-41) is replaced by this pass's answer: a
+    // value, a flight, or an error of its own (#4109, #4145).
+    if (el._config & CONFIG_STAGED_ERROR) {
+      el._pendingValue = NOT_PENDING;
+      el._config ^= CONFIG_STAGED_ERROR;
+    }
     if (__DEV__) clearSignals(el);
   }
 

packages/signals/tests/held-sync-throw-4109.test.ts

@@ -0,0 +1,82 @@
+/**
+ * #4109 (GabbeV): D-41 for a synchronous memo. A write makes `condition`
+ * throw while an async sibling holds the frame. The screen keeps the
+ * committed `true`, so an ordinary outside read (an event handler) must
+ * serve it too; the error shows, and throws, when the frame reveals.
+ */
+import { expect, it } from "vitest";
+import {
+  createErrorBoundary,
+  createMemo,
+  createRenderEffect,
+  createRoot,
+  createSignal,
+  flush
+} from "../src/index.js";
+
+function deferred<T>() {
+  let resolve!: (value: T) => void;
+  const promise = new Promise<T>(res => (resolve = res));
+  return { promise, resolve };
+}
+const tick = async () => {
+  await new Promise(r => setTimeout(r, 0));
+  flush();
+};
+const read = (fn: () => unknown) => {
+  try {
+    return fn();
+  } catch (e) {
+    return (e as Error).cause ?? e;
+  }
+};
+
+it("#4109: a held synchronous throw keeps the committed value for outside reads", async () => {
+  const boom = new Error("Condition failed");
+  let sibling = deferred<string>();
+  const [count, setCount] = createSignal(0);
+  const screen = { cond: undefined as unknown, slow: undefined as unknown };
+  let condition!: () => boolean;
+  const dispose = createRoot(d => {
+    condition = createMemo(() => {
+      if (count() > 0) throw boom;
+      return true;
+    });
+    const slow = createMemo(() => {
+      count();
+      return sibling.promise;
+    });
+    const errored = createErrorBoundary(condition, () => "error");
+    createRenderEffect(errored, v => void (screen.cond = v), { schedule: true });
+    createRenderEffect(slow, v => void (screen.slow = v), { schedule: true });
+    return d;
+  });
+  try {
+    flush();
+    sibling.resolve("initial");
+    await tick();
+    expect(screen).toEqual({ cond: true, slow: "initial" });
+    expect(condition()).toBe(true);
+
+    sibling = deferred<string>();
+    setCount(1);
+    flush();
+    // Held: the frame still shows the button, and the click handler's read
+    // serves what the frame shows (D-41, D-29).
+    expect(screen).toEqual({ cond: true, slow: "initial" });
+    expect(read(condition)).toBe(true);
+
+    await tick();
+    expect(screen).toEqual({ cond: true, slow: "initial" });
+    expect(read(condition)).toBe(true);
+
+    // One frame: the sibling and the error show together.
+    sibling.resolve("updated");
+    await tick();
+    expect(screen).toEqual({ cond: "error", slow: "updated" });
+    expect(read(condition)).toBe(boom);
+  } finally {
+    dispose();
+    flush();
+  }
+});

packages/signals/tests/staged-error-rerun-4109.test.ts

@@ -0,0 +1,88 @@
+/**
+ * #4109 follow-up: a pass's own throw is staged whenever a flush is running
+ * (D-41). A later pass of the same node in that flush replaces the staging:
+ * a value or a pending flight supersedes the staged error, so the commit
+ * neither throws the value as an error nor publishes the superseded error.
+ */
+import { afterEach, expect, it } from "vitest";
+import {
+  createEffect,
+  createMemo,
+  createRenderEffect,
+  createRoot,
+  createSignal,
+  flush,
+  resetErrorHalt
+} from "../src/index.js";
+
+afterEach(() => resetErrorHalt());
+
+const tick = async () => {
+  await new Promise(r => setTimeout(r, 0));
+  flush();
+};
+
+function setup(rerun: (v: number) => number | Promise<number>) {
+  const log: unknown[] = [];
+  const [s, setS] = createSignal(0);
+  const [fix, setFix] = createSignal(false, { ownedWrite: true });
+  let m!: () => number;
+  const dispose = createRoot(d => {
+    m = createMemo(() => {
+      const v = s();
+      if (v === 1 && !fix()) throw new Error("boom");
+      return v === 0 ? 0 : rerun(v);
+    });
+    // Taller than `m`: seeing s === 1 it writes `fix`, so `m` re-runs in the
+    // same flush, after its throw was staged.
+    const a = createMemo(() => s() + 1);
+    const b = createMemo(() => {
+      const x = a();
+      if (x === 2) setFix(true);
+      return x;
+    });
+    createEffect(m, {
+      effect: v => void log.push(["value", v]),
+      error: e => void log.push(["error", (e as any)?.cause?.message ?? (e as any)?.message ?? e])
+    });
+    createRenderEffect(b, () => {});
+    return d;
+  });
+  flush();
+  return { log, setS, read: () => m(), dispose };
+}
+
+it("a memo that throws then re-runs to a value in the same flush shows the value", () => {
+  const { log, setS, read, dispose } = setup(v => v);
+  try {
+    setS(1);
+    flush();
+    flush();
+    expect(log).toEqual([
+      ["value", 0],
+      ["value", 1]
+    ]);
+    expect(read()).toBe(1);
+  } finally {
+    dispose();
+    flush();
+  }
+});
+
+it("a memo that throws then re-runs to a pending value does not publish the superseded error", async () => {
+  const { log, setS, read, dispose } = setup(v => Promise.resolve(v * 10));
+  try {
+    setS(1);
+    flush();
+    await tick();
+    await tick();
+    expect(log).toEqual([
+      ["value", 0],
+      ["value", 10]
+    ]);
+    expect(read()).toBe(10);
+  } finally {
+    dispose();
+    flush();
+  }
+});

scripts/size/floor-caps.json

@@ -1,15 +1,15 @@
 {
   "signals: core floor (createSignal/Memo/Effect/Root/flush)": {
-    "cap": "7.25 KB",
-    "minified": 19773
+    "cap": "7.27 KB",
+    "minified": 19827
   },
   "app: render + one signal (the simple-app floor)": {
-    "cap": "9.56 KB",
-    "minified": 26664
+    "cap": "9.59 KB",
+    "minified": 26715
   },
   "app: hydrating (no stores) with Show/For/Loading/Errored/lazy": {
-    "cap": "18.19 KB",
-    "minified": 53236
+    "cap": "18.24 KB",
+    "minified": 53291
   },
   "page: base server components (hydrating + dynamic + frames + sf reference)": {
     "cap": "33.12 KB",