fix: commit a reaction's dependencies even when it throws

Fixes #18414

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