From 888ca4a7a1806fae82ebdee8acaddce4a3e6ce36 Mon Sep 17 00:00:00 2001 From: thomaflette Date: Thu, 2 Jul 2026 12:49:04 +0900 Subject: [PATCH 1/2] fix(signals): contain unhandled effect errors instead of unwinding the flush (closes #2761, #2762) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A throwing effect unwound the flush mid-queue: sibling effects in the already-detached queue array were lost forever (#2762), and a signal write earlier in the same flush left `scheduled` set with no microtask armed, so every later write short-circuited in schedule() and reactivity was permanently dead until a manual flush() (#2761). Unhandled effect errors now funnel through reportUncaughtError: during a drain they are collected and rethrown from flush() once it completes (first error thrown, the rest reported via console.error); outside a drain they throw in place, preserving creation-time semantics. runQueue additionally contains throws from non-effect queue callbacks so one bad callback cannot abandon its siblings. Scheduling is also made self-healing: a drainArmed flag kept separate from `scheduled` means an aborted drain can never strand pending work (the microtask clears the flag itself), and an abnormal unwind of the flush — now reachable only by internal invariant failures — re-arms a drain conservatively. Co-Authored-By: Claude Fable 5 --- .../fix-scheduler-throwing-effect-stall.md | 5 + packages/solid-signals/src/core/effect.ts | 7 +- packages/solid-signals/src/core/scheduler.ts | 57 +++++- packages/solid-signals/tests/flush.test.ts | 188 +++++++++++++++++- .../solid-signals/tests/onSettled.test.ts | 4 +- 5 files changed, 252 insertions(+), 9 deletions(-) create mode 100644 .changeset/fix-scheduler-throwing-effect-stall.md diff --git a/.changeset/fix-scheduler-throwing-effect-stall.md b/.changeset/fix-scheduler-throwing-effect-stall.md new file mode 100644 index 000000000..59d1e4b05 --- /dev/null +++ b/.changeset/fix-scheduler-throwing-effect-stall.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Fix one throwing effect killing all reactivity (#2761, #2762). An unhandled effect error no longer unwinds the flush: sibling effects queued behind it still run, and a write during the failed flush can no longer permanently stall scheduling. Errors are collected and rethrown from `flush()` once the drain completes; when several effects throw in one drain, the first is rethrown and the rest are reported via `console.error`. diff --git a/packages/solid-signals/src/core/effect.ts b/packages/solid-signals/src/core/effect.ts index b791f86d3..1d426a12f 100644 --- a/packages/solid-signals/src/core/effect.ts +++ b/packages/solid-signals/src/core/effect.ts @@ -22,6 +22,7 @@ import { cleanup } from "./owner.js"; import { _hitUnhandledAsync, GlobalQueue, + reportUncaughtError, resetUnhandledAsync, setTrackedQueueCallback } from "./scheduler.js"; @@ -97,7 +98,7 @@ function notifyEffectStatus(this: Effect, status?: number, error?: any): vo err = e; } } - if (!this._queue.notify(this, STATUS_ERROR, STATUS_ERROR)) throw err; + if (!this._queue.notify(this, STATUS_ERROR, STATUS_ERROR)) reportUncaughtError(err); } else if (this._type === EFFECT_RENDER) { this._queue.notify(this, STATUS_PENDING | STATUS_ERROR, actualStatus, actualError); if (__DEV__ && _hitUnhandledAsync) { @@ -142,7 +143,7 @@ function runEffect(node: Effect): void { } catch (error) { node._error = new StatusError(node, error); node._statusFlags |= STATUS_ERROR; - if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) throw error; + if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) reportUncaughtError(error); } finally { if (__DEV__) setStrictRead(prevStrictRead); node._prevValue = node._value; @@ -200,7 +201,7 @@ export function trackedEffect(fn: () => void | (() => void), options?: NodeOptio if (actualStatus & STATUS_ERROR) { node._queue.notify(node, STATUS_PENDING, 0); const err = error !== undefined ? error : node._error; - if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) throw err; + if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) reportUncaughtError(err); } }; node._run = run; diff --git a/packages/solid-signals/src/core/scheduler.ts b/packages/solid-signals/src/core/scheduler.ts index f3a0deb7a..ea0dcf020 100644 --- a/packages/solid-signals/src/core/scheduler.ts +++ b/packages/solid-signals/src/core/scheduler.ts @@ -46,10 +46,16 @@ export const zombieQueue: Heap = { export let clock = 0; export let activeTransition: Transition | null = null; let scheduled = false; +// `scheduled` means dirty work exists; `drainArmed` means a microtask exists +// to drain it. Keeping them separate lets a failed drain recover on next write. +let drainArmed = false; let syncDepth = 0; export let projectionWriteActive = false; let inTrackedQueueCallback = false; +// Unhandled errors collected during a drain and rethrown after queues finish. +let flushErrors: unknown[] | null = null; + let _enforceLoadingBoundary = false; export let _hitUnhandledAsync = false; // When a background transition is stashed, plain optimistic signals need one @@ -218,10 +224,31 @@ function cleanupCompletedLanes(completingTransition: Transition | null): void { } } +function drainMicrotask(): void { + drainArmed = false; + flush(); +} + export function schedule() { - if (scheduled) return; scheduled = true; - if (!syncDepth && !globalQueue._running && !projectionWriteActive) queueMicrotask(flush); + if (!drainArmed && !syncDepth && !globalQueue._running && !projectionWriteActive) { + drainArmed = true; + queueMicrotask(drainMicrotask); + } +} + +/** + * Surface an error that no queue (error boundary) handled. During a drain, + * collect it so queued siblings still run (#2762); outside a drain, throw in + * place to preserve creation-time semantics. + */ +export function reportUncaughtError(error: unknown): void { + if (globalQueue._running) { + if (flushErrors === null) flushErrors = []; + flushErrors.push(error); + } else { + throw error; + } } export interface IQueue { @@ -384,6 +411,15 @@ export class GlobalQueue extends Queue { activeLanes.size && runLaneEffects(EFFECT_USER); this.run(EFFECT_USER); if (__DEV__) DEV.hooks.onUpdate?.(); + } catch (error) { + // User errors are funneled through `reportUncaughtError`; if an internal + // failure still unwinds the drain, conservatively re-arm pending work. + scheduled = true; + if (!drainArmed) { + drainArmed = true; + queueMicrotask(drainMicrotask); + } + throw error; } finally { this._running = false; } @@ -682,10 +718,25 @@ export function flush(fn?: () => T): T | void { if (__DEV__ && ++count === 1e5) throw new Error("Potential Infinite Loop Detected."); globalQueue.flush(); } + if (flushErrors !== null) { + const errors = flushErrors; + flushErrors = null; + // Only one error can be thrown; the rest must still surface, not vanish. + for (let i = 1; i < errors.length; i++) console.error(errors[i]); + throw errors[0]; + } } function runQueue(queue: QueueCallback[], type: number): void { - for (let i = 0; i < queue.length; i++) queue[i](type); + for (let i = 0; i < queue.length; i++) { + try { + queue[i](type); + } catch (error) { + // The queue array is already detached, so skipped siblings would be lost. + if (flushErrors === null) flushErrors = []; + flushErrors.push(error); + } + } } function reporterBlocksSource(reporter: Computed, source: Computed): boolean { diff --git a/packages/solid-signals/tests/flush.test.ts b/packages/solid-signals/tests/flush.test.ts index 3e9925e3e..31aefbe02 100644 --- a/packages/solid-signals/tests/flush.test.ts +++ b/packages/solid-signals/tests/flush.test.ts @@ -1,4 +1,5 @@ -import { createEffect, createRoot, createSignal, flush } from "../src/index.js"; +import { createEffect, createRenderEffect, createRoot, createSignal, flush } from "../src/index.js"; +import { globalQueue } from "../src/core/scheduler.js"; afterEach(() => flush()); @@ -33,6 +34,191 @@ describe("scheduler hygiene around throwing effects", () => { await Promise.resolve(); expect(seenC).toEqual([3]); }); + + it("recovers scheduling when an effect writes a signal and then throws (#2761)", async () => { + const [a, setA] = createSignal(0); + const [b, setB] = createSignal(0); + const [, setC] = createSignal(0); + const seenB: number[] = []; + + createRoot(() => { + createEffect(a, v => { + if (v === 1) { + setC(100); + throw new Error("boom"); + } + }); + createEffect(b, v => { + seenB.push(v); + }); + }); + flush(); + await Promise.resolve(); + seenB.length = 0; + + expect(() => flush(() => setA(1))).toThrow("boom"); + + setB(1); + await Promise.resolve(); + await Promise.resolve(); + expect(seenB).toEqual([1]); + }); + + it("runs sibling effects queued after a throwing effect (#2762)", () => { + const [n, setN] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createEffect(n, v => { + if (v === 1) throw new Error("boom"); + }); + createEffect(n, v => { + seen.push(v); + }); + }); + flush(); + seen.length = 0; + + setN(1); + expect(() => flush()).toThrow("boom"); + expect(seen).toEqual([1]); + + setN(2); + flush(); + expect(seen).toEqual([1, 2]); + }); + + it("throws the first error and reports the rest when multiple effects throw", () => { + const [n, setN] = createSignal(0); + const seen: number[] = []; + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + try { + createRoot(() => { + createEffect(n, v => { + if (v === 1) throw new Error("boom1"); + }); + createEffect(n, v => { + if (v === 1) throw new Error("boom2"); + }); + createEffect(n, v => { + seen.push(v); + }); + }); + flush(); + seen.length = 0; + + setN(1); + expect(() => flush()).toThrow("boom1"); + expect(seen).toEqual([1]); + expect(consoleError).toHaveBeenCalledWith(expect.objectContaining({ message: "boom2" })); + } finally { + consoleError.mockRestore(); + } + }); + + it("still runs user effects when a render effect throws", () => { + const [n, setN] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createRenderEffect(n, v => { + if (v === 1) throw new Error("render boom"); + }); + createEffect(n, v => { + seen.push(v); + }); + }); + flush(); + seen.length = 0; + + setN(1); + expect(() => flush()).toThrow("render boom"); + expect(seen).toEqual([1]); + }); + + it("completes the drain when a render effect's compute throws during the heap phase", async () => { + const [n, setN] = createSignal(0); + const [b, setB] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createRenderEffect( + () => { + if (n() === 1) throw new Error("compute boom"); + }, + () => {} + ); + createEffect(b, v => { + seen.push(v); + }); + }); + flush(); + await Promise.resolve(); + seen.length = 0; + + expect(() => + flush(() => { + setN(1); + setB(1); + }) + ).toThrow("compute boom"); + expect(seen).toEqual([1]); + + setB(2); + await Promise.resolve(); + await Promise.resolve(); + expect(seen).toEqual([1, 2]); + }); + + it("self-heals when the drain itself aborts", async () => { + const [b, setB] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createEffect(b, v => { + seen.push(v); + }); + }); + flush(); + await Promise.resolve(); + seen.length = 0; + + let explode = true; + const badChild = { + _parent: null, + _children: [], + created: 0, + enqueue() {}, + run() { + if (explode) { + explode = false; + throw new Error("internal abort"); + } + }, + addChild() {}, + removeChild() {}, + notify: () => false, + stashQueues() {}, + restoreQueues() {} + }; + globalQueue.addChild(badChild as any); + + try { + expect(() => flush(() => setB(1))).toThrow("internal abort"); + + await Promise.resolve(); + await Promise.resolve(); + expect(seen).toEqual([1]); + + setB(2); + await Promise.resolve(); + await Promise.resolve(); + expect(seen).toEqual([1, 2]); + } finally { + globalQueue.removeChild(badChild as any); + } + }); }); it("should batch updates", () => { diff --git a/packages/solid-signals/tests/onSettled.test.ts b/packages/solid-signals/tests/onSettled.test.ts index ea67d5047..3658830d4 100644 --- a/packages/solid-signals/tests/onSettled.test.ts +++ b/packages/solid-signals/tests/onSettled.test.ts @@ -97,9 +97,9 @@ it("should throw when owner-backed onSettled calls flush reentrantly", () => { expect(() => flush()).toThrow( "Cannot call flush() from inside onSettled or createTrackedEffect. flush() is not reentrant there." ); - expect(log).toEqual(["outer"]); + expect(log).toEqual(["outer", "inner"]); expect(values[0]).toBe(0); - expect(values.at(-1)).toBe(0); + expect(values.at(-1)).toBe(5); }); it("should forbid onCleanup inside owner-backed onSettled", () => { From 8877bd792836a9402912c40ebc9b526cf7828b24 Mon Sep 17 00:00:00 2001 From: thomaflette Date: Thu, 2 Jul 2026 12:49:51 +0900 Subject: [PATCH 2/2] fix(signals): detach effect cleanups before invoking them (closes #2813) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cleanup that threw was never cleared from its node — the throw skipped the clearing line — so every later run re-invoked the dead cleanup and the effect body never ran again. The throw also happened before runEffect's try, bypassing its catch and finally: error boundaries never saw cleanup errors, and the dev strict-read guard leaked, misattributing every subsequent untracked read in the app to "an effect callback". The previous cleanup is now detached before being invoked, inside the try, so a throwing cleanup only costs that round: the error takes the standard effect-error path (boundary notify, then the uncaught funnel), the finally restores the guard, and the next update runs the body again. The same reorder is applied to trackedEffect's compute and the error-handler cleanup callback. Co-Authored-By: Claude Fable 5 --- .changeset/fix-effect-cleanup-detach.md | 5 +++ packages/solid-signals/src/core/effect.ts | 11 ++++-- .../tests/createErrorBoundary.test.ts | 39 +++++++++++++++++++ .../tests/createTrackedEffect.test.ts | 28 +++++++++++++ packages/solid-signals/tests/flush.test.ts | 27 +++++++++++++ .../solid-signals/tests/strictRead.test.ts | 25 ++++++++++++ 6 files changed, 132 insertions(+), 3 deletions(-) create mode 100644 .changeset/fix-effect-cleanup-detach.md diff --git a/.changeset/fix-effect-cleanup-detach.md b/.changeset/fix-effect-cleanup-detach.md new file mode 100644 index 000000000..e4733dc98 --- /dev/null +++ b/.changeset/fix-effect-cleanup-detach.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Fix a throwing effect cleanup permanently breaking its effect (#2813). The previous cleanup is detached before it runs, so a cleanup that throws is never re-invoked on later runs and the effect recovers on the next update. Cleanup errors now take the standard effect error path: error boundaries can catch them, and the dev-mode strict-read guard is restored instead of leaking `STRICT_READ_UNTRACKED` false positives app-wide. diff --git a/packages/solid-signals/src/core/effect.ts b/packages/solid-signals/src/core/effect.ts index 1d426a12f..07fe8034f 100644 --- a/packages/solid-signals/src/core/effect.ts +++ b/packages/solid-signals/src/core/effect.ts @@ -90,8 +90,9 @@ function notifyEffectStatus(this: Effect, status?: number, error?: any): vo try { return this._errorFn ? this._errorFn(err, () => { - this._cleanup?.(); + const prevCleanup = this._cleanup; this._cleanup = undefined; + prevCleanup?.(); }) : console.error(err); } catch (e) { @@ -126,9 +127,11 @@ function runEffect(node: Effect): void { if (__DEV__) { prevStrictRead = setStrictRead("an effect callback"); } - node._cleanup?.(); + // Detach first so a throwing cleanup cannot wedge future runs. + const prevCleanup = node._cleanup; node._cleanup = undefined; try { + prevCleanup?.(); const nextCleanup = node._effectFn(node._value, node._prevValue); if (__DEV__ && nextCleanup !== undefined && typeof nextCleanup !== "function") { throw new Error( @@ -179,8 +182,10 @@ export function trackedEffect(fn: () => void | (() => void), options?: NodeOptio const node = computed( () => { - node._cleanup?.(); + // Detach first so a throwing cleanup cannot wedge future runs. + const prevCleanup = node._cleanup; node._cleanup = undefined; + prevCleanup?.(); const cleanup = staleValues(fn); if (__DEV__ && cleanup !== undefined && typeof cleanup !== "function") { throw new Error( diff --git a/packages/solid-signals/tests/createErrorBoundary.test.ts b/packages/solid-signals/tests/createErrorBoundary.test.ts index 32deb4b48..586fe1071 100644 --- a/packages/solid-signals/tests/createErrorBoundary.test.ts +++ b/packages/solid-signals/tests/createErrorBoundary.test.ts @@ -360,6 +360,45 @@ it("should catch errors thrown in user effect callbacks (back half)", () => { expect(result).toBe("errored"); }); +it("should catch errors thrown in user effect cleanups", () => { + const error = new Error("user effect cleanup error"); + const handler = vi.fn(); + const [$x, setX] = createSignal(0); + let result: any; + + createRoot(() => { + const b = createErrorBoundary( + () => { + createEffect( + () => $x(), + v => { + return () => { + if (v === 0) throw error; + }; + } + ); + return "content"; + }, + err => { + handler(err()); + return "errored"; + } + ); + createRenderEffect( + () => (result = b()), + () => {} + ); + }); + flush(); + expect(result).toBe("content"); + + setX(1); + flush(); + expect(handler).toHaveBeenCalledTimes(1); + expect(handler).toHaveBeenCalledWith(error); + expect(result).toBe("errored"); +}); + it("should catch errors thrown in user effect callbacks with error handler (back half)", () => { const error = new Error("user effect bundle error"); const handler = vi.fn(); diff --git a/packages/solid-signals/tests/createTrackedEffect.test.ts b/packages/solid-signals/tests/createTrackedEffect.test.ts index a8e238373..ef0ea46d4 100644 --- a/packages/solid-signals/tests/createTrackedEffect.test.ts +++ b/packages/solid-signals/tests/createTrackedEffect.test.ts @@ -252,6 +252,34 @@ it("should throw uncaught tracked effect errors during flush", () => { expect(() => flush()).toThrow("tracked boom"); }); +it("should not wedge on a throwing cleanup", () => { + const [$x, setX] = createSignal(0); + const ran: number[] = []; + const cleaned: number[] = []; + + createRoot(() => { + createTrackedEffect(() => { + const v = $x(); + ran.push(v); + return () => { + cleaned.push(v); + if (v === 0) throw new Error("cleanup boom"); + }; + }); + }); + flush(); + + setX(1); + expect(() => flush()).toThrow("cleanup boom"); + expect(ran).toEqual([0]); + expect(cleaned).toEqual([0]); + + setX(2); + flush(); + expect(ran).toEqual([0, 2]); + expect(cleaned).toEqual([0]); +}); + it("should throw on invalid cleanup values", () => { createRoot(() => { createTrackedEffect(() => ({}) as any); diff --git a/packages/solid-signals/tests/flush.test.ts b/packages/solid-signals/tests/flush.test.ts index 31aefbe02..bc3ef94b9 100644 --- a/packages/solid-signals/tests/flush.test.ts +++ b/packages/solid-signals/tests/flush.test.ts @@ -171,6 +171,33 @@ describe("scheduler hygiene around throwing effects", () => { expect(seen).toEqual([1, 2]); }); + it("does not wedge an effect whose cleanup throws", () => { + const [n, setN] = createSignal(0); + const ran: number[] = []; + const cleaned: number[] = []; + + createRoot(() => { + createEffect(n, v => { + ran.push(v); + return () => { + cleaned.push(v); + if (v === 0) throw new Error("cleanup boom"); + }; + }); + }); + flush(); + + setN(1); + expect(() => flush()).toThrow("cleanup boom"); + expect(ran).toEqual([0]); + expect(cleaned).toEqual([0]); + + setN(2); + flush(); + expect(ran).toEqual([0, 2]); + expect(cleaned).toEqual([0]); + }); + it("self-heals when the drain itself aborts", async () => { const [b, setB] = createSignal(0); const seen: number[] = []; diff --git a/packages/solid-signals/tests/strictRead.test.ts b/packages/solid-signals/tests/strictRead.test.ts index a51d93e80..be90c562d 100644 --- a/packages/solid-signals/tests/strictRead.test.ts +++ b/packages/solid-signals/tests/strictRead.test.ts @@ -36,6 +36,31 @@ describe("strictRead", () => { warn.mockRestore(); }); + it("does not leak the strict-read flag when a cleanup throws", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const [$x, setX] = createSignal(0); + const [$y] = createSignal(2); + + createRoot(() => { + createEffect( + () => $x(), + v => { + return () => { + if (v === 0) throw new Error("cleanup boom"); + }; + } + ); + }); + flush(); + + setX(1); + expect(() => flush()).toThrow("cleanup boom"); + + $y(); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + it("warns on store property read in effect callback", () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const [$x] = createSignal(1);