fix: a throwing cleanup no longer aborts the rest of the teardown

.changeset/cleanup-throw-continues.md

@@ -0,0 +1,6 @@
+---
+"@solidjs/signals": patch
+"solid-js": patch
+---
+
+A cleanup (`onCleanup` or an effect's returned cleanup) that throws no longer aborts the rest of the teardown: every other cleanup in the tree still runs, and the first error is rethrown once the teardown is done. Outside a flush (a root's `dispose()`), the dispose call rethrows it. Inside a flush (a branch switch retiring a frame, a pass replacing an uncommitted one, an effect disposing a root), `flush()` rethrows it after the flush has finished, so commits and effects are never left half-applied, and lazy nodes released before the throw are still released. An `unobserved` hook that throws is handled the same way. If an uncaught error halts the reactive system, a collected cleanup error is dropped rather than surfacing later from an unrelated `dispose()`. On the server, `disposeOwner` runs every cleanup the same way, so `renderToStream`'s final dispose still reports the error (#4193).

packages/signals/src/core/core.ts

@@ -406,14 +406,13 @@ export function recompute(el: Computed<any>, create: boolean = false): void {
     // and own nothing to park.
     if (isEffect === EFFECT_TRACKED || el._config & CONFIG_STAGED) {
       // A lazy node only those children read is released when this pass
-      // ends, if the pass doesn't read it again (#4142).
+      // ends, if the pass doesn't read it again (#4142). A cleanup or
+      // `unobserved` hook that throws here neither stops the pass nor skips
+      // that drain: its error is rethrown when the flush ends (owner.ts).
       const prev = deferredReleases;
       deferReleases((released = []));
-      try {
-        disposeChildren(el);
-      } finally {
-        deferReleases(prev);
-      }
+      disposeChildren(el);
+      deferReleases(prev);
     } 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).
@@ -1510,8 +1509,9 @@ export function exitCallback(): void {
  * its owner (a list pass removing a row, a branch switching away), so its
  * writes are imperative like an effect callback's (#4165). Kept apart from
  * `callbackDepth` because its sites cannot use try/finally (the frame would
- * survive into prod): a throwing cleanup leaves it raised, and dev.ts resets
- * it on the next microtask, which teardown — synchronous — never spans.
+ * survive into prod). A throwing cleanup is caught (owner.ts `runCleanup`);
+ * anything else escaping a teardown leaves it raised, and dev.ts resets it on
+ * the next microtask, which teardown — synchronous — never spans.
  */
 export let disposalDepth = 0;
 export function enterDisposal(): void {

packages/signals/src/core/dev.ts

@@ -1267,9 +1267,10 @@ export function checkPostAwaitRead(
   if (callbackDepth !== 0) return;
   if (disposalDepth !== 0) {
     // Teardown is synchronous, so a depth still raised on the next microtask
-    // was left behind by a cleanup that threw (owner.ts brackets it without
-    // try/finally, which would survive into prod). Clear it then, or every
-    // later check would stay silent.
+    // was left behind by a throw that escaped it (owner.ts brackets it
+    // without try/finally, which would survive into prod; a cleanup's own
+    // throw is caught there). Clear it then, or every later check would stay
+    // silent.
     if (!disposalHealQueued) {
       disposalHealQueued = true;
       queueMicrotask(healDisposalDepth);

packages/signals/src/core/graph.ts

@@ -7,7 +7,7 @@ import {
   STATUS_PENDING
 } from "./constants.js";
 import { deleteFromHeap } from "./heap.js";
-import { disposeChildren } from "./owner.js";
+import { disposeChildren, runCleanup } from "./owner.js";
 import { bumpNotifyEpoch, dirtyQueue } from "./scheduler.js";
 import type { Computed, Link, Signal } from "./types.js";
 
@@ -35,8 +35,10 @@ export function unlinkSubs(link: Link): Link | null {
     if (nextSub === null) {
       // Store slot nodes dispatch to the ONE shared hook — no per-node
       // unobserved closure, no NodeExtension to hold it.
+      // A user hook that throws is collected like a cleanup (owner.ts): the
+      // unlink and the teardown around it carry on.
       if (dep._config & CONFIG_SLOT_NODE) slotUnobservedHook(dep as Signal<any>);
-      else dep._x?._unobserved?.();
+      else if (dep._x?._unobserved) runCleanup(dep._x._unobserved);
       // No more subscribers; only tear down if CONFIG_AUTO_DISPOSE is set.
       // A pending node is exempt: its in-flight async work is an observer —
       // tearing down would orphan the work and re-execute it on the next

packages/signals/src/core/owner.ts

@@ -29,7 +29,7 @@ import {
 } from "./dev.js";
 import { clearDeps, unobserved } from "./graph.js";
 import { deleteFromHeap } from "./heap.js";
-import { dirtyQueue, schedule } from "./scheduler.js";
+import { dirtyQueue, globalQueue, schedule } from "./scheduler.js";
 import type { Computed, Disposable, Owner, Root } from "./types.js";
 
 // L2 (step 2): the parked frame. A pass parks the previous frame's children
@@ -60,6 +60,32 @@ export function dispose(node: Computed<unknown>): void {
   unobserved(node);
 }
 
+// The first error a cleanup threw, boxed so a thrown `undefined` still
+// counts. A throwing cleanup stops neither its siblings nor the rest of the
+// tree: every cleanup runs. The error is rethrown once the teardown is over:
+// by a root's dispose call outside a flush (`disposeRootSelf`), otherwise when
+// the flush ends (`flush`; `runCleanup` schedules one), so a flush is never
+// left half-applied. A halt drops it: the error that halted is the one
+// reported, and a stale one must not surface from a later, unrelated dispose.
+let cleanupError: [unknown] | undefined;
+
+/** Runs a cleanup or `unobserved` hook; a throw is collected, not raised. @internal */
+export function runCleanup(fn: () => void): void {
+  try {
+    fn();
+  } catch (e) {
+    cleanupError ||= [e];
+    schedule();
+  }
+}
+
+/** Rethrows the error `runCleanup` collected, once; `drop` discards it. @internal */
+export function rethrowCleanupError(drop?: boolean): void {
+  const error = cleanupError;
+  cleanupError = undefined;
+  if (error && !drop) throw error[0];
+}
+
 export function disposeChildren(node: Owner, self: boolean = false, zombie?: boolean): void {
   const flags = (node as any)._flags;
   if (flags & REACTIVE_DISPOSED) return;
@@ -163,7 +189,7 @@ export function disposeChildren(node: Owner, self: boolean = false, zombie?: boo
     const effectCleanup = node._cleanup;
     node._cleanup = undefined;
     if (__DEV__) enterDisposal();
-    effectCleanup();
+    runCleanup(effectCleanup);
     if (__DEV__) exitDisposal();
   }
 }
@@ -188,21 +214,18 @@ function runDisposal(node: Owner, zombie?: boolean): void {
   if (zombie) node._x!._pendingDisposal = null;
   else node._disposal = null;
 
-  // No try/finally: it would survive into prod (rollup keeps the frame). A
-  // throw leaves the depth raised; dev.ts clears the stale count on the next
-  // microtask, since teardown never spans one (core.ts).
+  // Each cleanup is guarded (`runCleanup`), so the depth is always lowered.
   if (__DEV__) enterDisposal();
   if (Array.isArray(disposal)) {
     // Unwind order (#3572, restores 1.x #1562): later registrations run
     // before earlier ones. Children have already been disposed by the caller,
     // so with LIFO a body that registers cleanup before creating its children
     // tears down after them — the same order a per-component owner gives.
     for (let i = disposal.length - 1; i >= 0; i--) {
-      const callable = disposal[i];
-      callable.call(callable);
+      runCleanup(disposal[i]);
     }
   } else {
-    disposal.call(disposal);
+    runCleanup(disposal);
   }
   if (__DEV__) exitDisposal();
 }
@@ -335,6 +358,9 @@ export function isDisposed(node: Owner): boolean {
 
 function disposeRootSelf(this: Root, self: boolean = true): void {
   disposeChildren(this, self);
+  // Outside a flush the dispose call rethrows; inside one, the flush does.
+  // Kept out of `disposeChildren`, whose hot path it slows.
+  if (cleanupError && !globalQueue._running) rethrowCleanupError();
 }
 
 /**

packages/signals/src/core/scheduler.ts

@@ -43,6 +43,7 @@ import {
 import { sweepDormant, trimStaleDeps } from "./graph.js";
 import { deleteFromHeap, enqueueSub, runHeap, type Heap } from "./heap.js";
 import { devCheckQuiescent } from "./invariants.js";
+import { rethrowCleanupError } from "./owner.js";
 import type { Computed, Owner, Signal } from "./types.js";
 
 // Optional-module hooks: module bindings, not `GlobalQueue` statics, so a
@@ -702,6 +703,8 @@ export const ROOT_ERROR_HOOK: unique symbol = Symbol.for("solid-js/root-error-ho
 export function haltReactivity(cause?: unknown): void {
   if (halted) return;
   halted = true;
+  // A cleanup error collected before the halt is dropped (owner.ts).
+  rethrowCleanupError(true);
   let message = "[REACTIVITY_HALTED]";
   if (__DEV__) {
     message +=
@@ -1459,6 +1462,7 @@ export function flush<T>(fn?: () => T): T | void {
   if (__OBSERVE__ && attrHooks !== null && scheduled) attrHooks.flushStart();
   while (scheduled) {
     if (__DEV__ && ++count === 1e5) {
+      rethrowCleanupError(true);
       // Attribution beats a bare guard (#3140): say what kept the loop alive.
       throw new Error(
         `Potential Infinite Loop Detected. Kept alive by scheduled work${
@@ -1474,6 +1478,9 @@ export function flush<T>(fn?: () => T): T | void {
   // so this is the "committed and effects ran" instant for everything the
   // loop processed.
   if (__OBSERVE__ && drained && attrHooks !== null) attrHooks.flushEnd();
+  // A cleanup that threw during the drain, now that the drain is complete
+  // (owner.ts).
+  rethrowCleanupError();
 }
 
 function runQueue(queue: QueueCallback[], type: number): void {