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 <randalmurphal@users.noreply.github.com>

.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

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

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