2.0.0-beta.15: A throwing effect cleanup permanently wedges its effect (stale cleanup re-invoked on every run)

[yumemi-thomas]

Describe the bug

If a cleanup function returned by a createEffect callback throws, the effect is permanently broken:

1. The effect body never runs again for any subsequent update.
2. Instead, the same dead cleanup is re-invoked (and re-throws) on every later run — the stale cleanup reference is never cleared.
3. An <ErrorBoundary> wrapping the effect cannot catch the cleanup error — it escapes as uncaught even though effect-body errors thrown one line later are caught fine.
4. Dev-only side effect: the strict-read guard armed for the effect run is never restored, so after the first throw every legal untracked signal read anywhere in the app (e.g. n() inside a click handler) falsely warns [STRICT_READ_UNTRACKED] Reactive value read directly in an effect callback will not update. for the rest of the session.

Related to but distinct from #2761 / #2762: a throwing cleanup can also trigger the scheduler stall those issues describe, but this wedge is a separate per-effect defect that survives fixing them. The repro is arranged so neither scheduler bug masks it (see Additional context).

Your Example Website or App

https://stackblitz.com/edit/solidjs-templates-uphtinhn?file=src%2FApp.tsx

import { createSignal, createEffect } from 'solid-js';

export default function App() {
  const [n, setN] = createSignal(0);
  const [b, setB] = createSignal(0);

  // The wedge target. Deliberately arranged so neither scheduler bug can
  // mask it: this is the ONLY user effect subscribed to `n` (nothing in its
  // batch for #2762 to abandon), and it writes no signals (nothing to strand
  // the scheduler, so no #2761 stall).
  createEffect(n, (v) => {
    console.log('body ran with n =', v);
    return () => {
      console.log('cleanup ran (registered by n =', v, ')');
      if (v === 0) throw new Error('cleanup boom');
    };
  });

  // Control effect on a DIFFERENT signal, driven by its own button so it is
  // never in the same batch as the thrower. It keeps working throughout,
  // proving the app is healthy and the wedge is per-effect.
  createEffect(b, (v) => {
    console.log('control effect saw b =', v);
  });

  return (
    <div>
      <p>n = {n()}</p>
      <p>b = {b()}</p>
      <button onClick={() => setN(n() + 1)}>bump n (watch the console)</button>
      <button onClick={() => setB(b() + 1)}>
        bump b (control — always works)
      </button>
    </div>
  );
}

Steps to Reproduce the Bug or Issue

1. Open the repro and the browser console. On mount: body ran with n = 0 and control effect saw b = 0.
2. Click "bump n". The console shows cleanup ran (registered by n = 0) then an uncaught Error: cleanup boom. Note body ran with n = 1 never appears.
3. Click "bump b". control effect saw b = 1 logs — the scheduler and the rest of the app are healthy.
4. Click "bump n" again. Actual: cleanup ran (registered by n = 0) and cleanup boom again — the cleanup registered by the n=0 run fires on every update forever, and the effect body never executes again.
5. Also note that from step 2 onward, every click emits false [STRICT_READ_UNTRACKED] warnings for the n() / b() reads in the click handlers, which warned nothing before the throw.

Expected behavior

A throwing cleanup should surface its error once and only cost that round: the stale cleanup is detached (never re-invoked), the effect body runs again on the next update, and dev diagnostics are unaffected. Expected console for the steps above:

cleanup ran (registered by n = 0)
Error: cleanup boom            ← once
control effect saw b = 1
body ran with n = 2            ← effect recovered
(no STRICT_READ_UNTRACKED warnings)

Screenshots or Videos

No response

Platform

• OS: macOs
• Browser: Chrome
• Version: 2.0.0-beta.15

Additional context

Root cause

In runEffect (packages/solid-signals/src/core/effect.ts:128–129), the previous cleanup is invoked before its reference is cleared, and before the try block:

prevStrictRead = setStrictRead("an effect callback"); // effect.ts:126
node._cleanup?.();          // effect.ts:128 ← throws here
node._cleanup = undefined;  // effect.ts:129 ← never reached → symptoms 1+2
try {
  ...
} catch (error) {
  ...notify error boundary...   // effect.ts:142 ← never reached → symptom 3
} finally {
  setStrictRead(prevStrictRead); // effect.ts:147 ← never reached → symptom 4
}

Because the throw happens before the try, the stale _cleanup survives (so every later run re-enters the dead cleanup and aborts before the body), and the strict-read flag leaks globally (each later effect run captures the leaked value as its "previous" and restores it, so the corruption is permanent).

The same invoke-before-detach pattern exists in trackedEffect's compute (effect.ts:181–182), so createTrackedEffect / onSettled cleanups wedge the same way.

Why the repro isn't masked by #2761 / #2762

A throwing cleanup normally also triggers the scheduler bugs, which hide this one behind an app-wide failure. The repro avoids both: the throwing effect is the only user effect subscribed to its signal (nothing for #2762 to abandon in its batch), and it writes no signals (nothing to strand scheduled, so no #2761 stall). That is why this needs tracking separately — fixing #2761 / #2762 alone leaves this bug fully intact, just easier to see.

Suggested fix direction

Detach the cleanup before invoking it, and invoke it inside the try so its error takes the same path as an effect-body error (error boundary notify, strict-read guard restored in the finally). The failed round is skipped like any errored run; the next update recovers.

[ryansolid]

Fixed in 90238e79. Cleanups are now detached before invocation and run inside the effect's try, so a throwing cleanup is never re-invoked, error boundaries can catch it, and the dev strict-read guard is restored. Your repro's isolation of the wedge from the scheduler issues made this easy to pin down — thanks, and thanks for the matching fix in #2814, which is essentially what landed.