fix(signals): contain throwing effects and detach cleanups (closes #2761, #2762, #2813)#2814
fix(signals): contain throwing effects and detach cleanups (closes #2761, #2762, #2813)#2814yumemi-thomas wants to merge 2 commits into
Conversation
…e flush (closes solidjs#2761, solidjs#2762) A throwing effect unwound the flush mid-queue: sibling effects in the already-detached queue array were lost forever (solidjs#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() (solidjs#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 <noreply@anthropic.com>
…idjs#2813) 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 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 8877bd7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
yumemi-thomas
force-pushed
the
fix/effect-error-handling
branch
from
6c2ca14 to
8877bd7
Compare
|
Thanks for this — the diagnosis of all three bugs is excellent, and the #2813 portion (detaching cleanups before invocation, inside the try) is essentially what we landed in 90238e79. We're declining the containment half, though, on philosophy rather than execution. Collecting unhandled errors and draining the remaining queue keeps the app running after its state is already undefined — the flush that threw had writes half-applied, so the drained siblings run against state no one can reason about, and the judgment calls you flagged (first-error-wins, end-of-drain surfacing) are symptoms of that. We went the other way: an error that escapes every boundary now permanently halts the scheduler, loudly. Unhandled errors are a crash, not a condition to route around; recovery belongs to error boundaries, which do contain errors without halting anything. Appreciate the rigor here — red-first tests and verifying against the published package is exactly how we'd want it done. |
2 participants
Summary
Fixes #2761, #2762 and #2813 — three ways a single throwing effect (or cleanup) can break an app on 2.0.0-beta.15.
The first two share a root cause: when an effect throws and nothing handles the error, the exception unwinds
GlobalQueue.flushmid-queue. The queue array is already detached at that point, so sibling effects queued behind the thrower are lost (#2762). And if the effect wrote a signal earlier in the same flush,scheduledstays true with no microtask armed, so every later write short-circuits inschedule()and the app is dead until something callsflush()manually (#2761).Rather than patching the individual unwind paths, this removes the mechanism: unhandled effect errors no longer throw through the scheduler at all. They go through a small funnel (
reportUncaughtError) that collects them during a drain and rethrows the first one fromflush()once the drain completes (extra ones areconsole.error'd so they don't disappear). Outside a drain it throws in place, so creation-time behavior is unchanged. I also split "work is pending" from "a drain microtask is armed" (newdrainArmedflag), so even if a drain dies for some unforeseen reason, the next write can always arm a fresh one instead of trusting a flag that lied.#2813 is a separate bug the scheduler stall was hiding:
runEffectinvokes the previous cleanup before clearingnode._cleanupand before thetry. A throwing cleanup is therefore never detached (every later run re-invokes the dead cleanup and the effect body never runs again), never reaches thecatch(error boundaries can't see cleanup errors), and never reaches thefinally(the dev strict-read guard leaks, and every untracked read in the app starts warningSTRICT_READ_UNTRACKED). The fix is to detach the cleanup to a local before calling it, inside thetry. Same reorder intrackedEffectand the error handler's cleanup callback.The observable changes, in one list:
flush()after the drain completes, instead of aborting it mid-queuerunEffect's catch)STRICT_READ_UNTRACKEDfalse positives after a cleanup throw is goneTwo commits, one per root cause, each green on its own. One existing test changed (
onSettledreentrant-flush): it pinned the old abort behavior where the nested callback and a pending write were silently dropped; the guardrail error still throws, but queued work now drains.Why that test's expectations changed (the only existing-behavior change in the suite)
The test's purpose — the reentrant-flush guardrail throws — is untouched, as are the two writes after the
flush()call (the throw still aborts the user callback at that statement, sovalues.at(-1)is5, not15). What the old secondary assertions pinned was the #2762 unwind itself, observed from anonSettledcallback: the nestedonSettled, registered before the violation, was silently discarded (log = ["outer"]), and the effect stayed permanently stale against a write that had already committed —count()read5while the effect had seen0and would never be told otherwise (values.at(-1) = 0). That's an internally inconsistent reactive graph frozen into a test.This change is forced by any fix to #2762, not by this particular design: even the most minimal per-callback containment in
runQueueproduces the new values. Keeping the old expectations would require special-casing the guardrail error to still unwind the drain — i.e. deliberately preserving one live instance of the bug.Two judgment calls reviewers may want to weigh in on: when several effects throw in one drain, the first is thrown and the rest are
console.error'd (anAggregateErroror anonUncaughtErrorhook would be alternatives, but both change public API), and errors now surface at the end of the drain rather than mid-flush (no existing test cared, but it is observable).Full disclosure: I wrote this with the help of an AI assistant (Claude Fable 5) and reviewed every line before pushing. All new tests were written first and confirmed failing on the unfixed code, and the bugs and fixes were both verified against the published beta.15 npm package, not just this repo.
How did you test this change?
flush/strictRead/createTrackedEffect/createErrorBoundary, each verified red on the pre-fix code before applying the fix. The strict-read one is telling: reverting the fix fails it plus 11 unrelated tests in the same file, because the leaked flag poisons everything that runs after it.pnpm build, thenturbo run test test-types --force(uncached): all 19 tasks green — signals 782, solid-js 416, web 409. Commit 1 was also tested in isolation (778/778).flush()), replicating the StackBlitz repros from the three issues: the error surfaces once, siblings survive, later writes keep flushing, the wedged effect recovers, and noSTRICT_READ_UNTRACKEDwarnings appear. The same scripts run against the published@solidjs/signals@2.0.0-beta.15package reproduce all three failures — the fixes target what is actually shipped.