From 2b740e496e857ae9e53bd15f5d5d7a2d4398139b Mon Sep 17 00:00:00 2001 From: Nic Polumeyv Date: Tue, 25 Aug 2026 04:14:43 -0400 Subject: [PATCH] fix: commit a reaction's dependencies even when it throws (#18703) Fixes #18414 and a second bug with the same cause. `update_reaction` in `runtime.js` commits a reaction's dependencies after `fn()` returns: it swaps in `new_deps`, removes stale reactions and registers the reaction into each new dep's `reactions`. When `fn()` throws, the whole block is skipped. Two things go wrong from that. A derived that the run read for the first time was already set `CONNECTED` in `get` and registered itself into its own deps' `reactions` during `update_derived`, but the throwing reaction never registers as its reader. It sits in its deps' `reactions` with `reactions === null` of its own, `remove_reaction`'s disconnect cascade can never reach it, and it retains its `ctx`, closures and DOM past component destruction and `unmount()`. A derived that throws during its own run ends up with no dependencies at all. It is not in its sources' `reactions`, so a later change to them never re-runs it, and for an unowned derived `update_derived` then marks the error value CLEAN because the derived has no deps. Anything reading it keeps getting the first error forever. The commit block is now `update_dependencies` and the `catch` calls it before `handle_error`, so a failing reaction keeps the deps it read up to the throw. That subscribes it to the inputs that produced the error, and lets the normal disconnect cascade clean up when it is destroyed (`Boundary.#handle_error` destroys the failed effect). Co-authored-by: Randy Murphy --- .changeset/quiet-deriveds-unwind.md | 5 ++ .../svelte/src/internal/client/runtime.js | 76 +++++++++++-------- packages/svelte/tests/signals/test.ts | 70 +++++++++++++++++ 3 files changed, 119 insertions(+), 32 deletions(-) create mode 100644 .changeset/quiet-deriveds-unwind.md diff --git a/.changeset/quiet-deriveds-unwind.md b/.changeset/quiet-deriveds-unwind.md new file mode 100644 index 0000000000..8036bdb781 --- /dev/null +++ b/.changeset/quiet-deriveds-unwind.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: keep the dependencies of a reaction that throws, so deriveds it read are neither leaked nor stuck in their error diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index e0914b8f70..a881403f58 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -258,37 +258,7 @@ export function update_reaction(reaction) { var fn = /** @type {Function} */ (reaction.fn); var result = fn(); reaction.f |= REACTION_RAN; - var deps = reaction.deps; - - // Don't remove reactions during fork; - // they must remain for when fork is discarded - var is_fork = current_batch?.is_fork; - - if (new_deps !== null) { - var i; - - if (!is_fork) { - remove_reactions(reaction, skipped_deps); - } - - if (deps !== null && skipped_deps > 0) { - deps.length = skipped_deps + new_deps.length; - for (i = 0; i < new_deps.length; i++) { - deps[skipped_deps + i] = new_deps[i]; - } - } else { - reaction.deps = deps = new_deps; - } - - if (effect_tracking() && (reaction.f & CONNECTED) !== 0) { - for (i = skipped_deps; i < deps.length; i++) { - (deps[i].reactions ??= []).push(reaction); - } - } - } else if (!is_fork && deps !== null && skipped_deps < deps.length) { - remove_reactions(reaction, skipped_deps); - deps.length = skipped_deps; - } + var deps = update_dependencies(reaction); // If we're inside an effect and we have untracked writes, then we need to // ensure that if any of those untracked writes result in re-invalidation @@ -300,7 +270,7 @@ export function update_reaction(reaction) { deps !== null && (reaction.f & (DERIVED | MAYBE_DIRTY | DIRTY)) === 0 ) { - for (i = 0; i < /** @type {Source[]} */ (untracked_writes).length; i++) { + for (var i = 0; i < /** @type {Source[]} */ (untracked_writes).length; i++) { schedule_possible_effect_self_invalidation( untracked_writes[i], /** @type {Effect} */ (reaction) @@ -344,6 +314,9 @@ export function update_reaction(reaction) { return result; } catch (error) { + // still commit the deps read before the throw, otherwise deriveds connected by this run keep no reader and the reaction never re-runs when they change + update_dependencies(reaction); + return handle_error(error); } finally { reaction.f ^= REACTION_IS_UPDATING; @@ -358,6 +331,45 @@ export function update_reaction(reaction) { } } +/** + * @param {Reaction} reaction + */ +function update_dependencies(reaction) { + var deps = reaction.deps; + + // Don't remove reactions during fork; + // they must remain for when fork is discarded + var is_fork = current_batch?.is_fork; + + if (new_deps !== null) { + var i; + + if (!is_fork) { + remove_reactions(reaction, skipped_deps); + } + + if (deps !== null && skipped_deps > 0) { + deps.length = skipped_deps + new_deps.length; + for (i = 0; i < new_deps.length; i++) { + deps[skipped_deps + i] = new_deps[i]; + } + } else { + reaction.deps = deps = new_deps; + } + + if (effect_tracking() && (reaction.f & CONNECTED) !== 0) { + for (i = skipped_deps; i < deps.length; i++) { + (deps[i].reactions ??= []).push(reaction); + } + } + } else if (!is_fork && deps !== null && skipped_deps < deps.length) { + remove_reactions(reaction, skipped_deps); + deps.length = skipped_deps; + } + + return deps; +} + /** * @template V * @param {Reaction} signal diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 07f79bd395..c1ebc3ec75 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -1521,4 +1521,74 @@ describe('signals', () => { assert.equal(s.reactions, null); }; }); + + // https://github.com/sveltejs/svelte/issues/18414 + test('a reaction that throws after first-reading a fresh derived does not leak it', () => { + const src = state(0); + const pane = derived(() => $.get(src)); + const base = derived(() => $.get(pane) + ':base'); + const extra = derived(() => $.get(pane) + ':extra'); + const flag = state(false); + + const destroy = effect_root(() => { + render_effect(() => { + if ($.get(flag)) { + $.get(extra); + throw new Error('render boom'); + } else { + $.get(base); + } + }); + }); + + return () => { + try { + flushSync(() => set(flag, true)); + } catch {} + + destroy(); + + assert.equal(src.reactions, null); + }; + }); + + test('a derived that throws on its first run re-runs when its dependencies change', () => { + const s = state(0); + const fn = () => { + if ($.get(s) === 0) throw new Error('boom'); + return $.get(s); + }; + const owned = derived(fn); + const previous_effect = $.active_effect; + $.set_active_effect(null); + const unowned = derived(fn); + $.set_active_effect(previous_effect); + const log: any[] = []; + + const destroy = effect_root(() => { + render_effect(() => { + for (const d of [owned, unowned]) { + try { + log.push($.get(d)); + } catch { + log.push('error'); + } + } + }); + }); + + return () => { + assert.notEqual(owned.parent, null); + assert.equal(unowned.parent, null); + + flushSync(); + assert.deepEqual(log, ['error', 'error']); + + flushSync(() => set(s, 1)); + assert.deepEqual(log, ['error', 'error', 1, 1]); + + destroy(); + assert.equal(s.reactions, null); + }; + }); });