fix(signals): contain throwing effects and detach cleanups (closes #2761, #2762, #2813)

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

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

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";
@@ -89,15 +90,16 @@ function notifyEffectStatus(this: Effect<any>, 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) {
         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) {
@@ -125,9 +127,11 @@ function runEffect(node: Effect<any>): 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(
@@ -142,7 +146,7 @@ function runEffect(node: Effect<any>): 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;
@@ -178,8 +182,10 @@ export function trackedEffect(fn: () => void | (() => void), options?: NodeOptio
 
   const node = computed<void>(
     () => {
-      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(
@@ -200,7 +206,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;

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<T>(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<any>, source: Computed<any>): boolean {

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();

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);