fix: ensure effects destroy owned deriveds upon teardown (#13563)

* fix: ensure effects destroy owned deriveds upon teardown

* add test

* make old test work

* tune

* tune

* Update packages/svelte/src/internal/client/runtime.js

---------

Co-authored-by: Rich Harris <[email protected]>

Author: trueadm

.changeset/ninety-months-laugh.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure effects destroy owned deriveds upon teardown

packages/svelte/src/internal/client/reactivity/deriveds.js

@@ -58,6 +58,9 @@ export function derived(fn) {
 		var derived = /** @type {Derived} */ (active_reaction);
 		(derived.children ??= []).push(signal);
 	}
+	if (active_effect !== null) {
+		(active_effect.deriveds ??= []).push(signal);
+	}
 
 	return signal;
 }
@@ -103,10 +106,11 @@ function destroy_derived_children(derived) {
 let stack = [];
 
 /**
+ * @template T
  * @param {Derived} derived
- * @returns {void}
+ * @returns {T}
  */
-export function update_derived(derived) {
+export function execute_derived(derived) {
 	var value;
 	var prev_active_effect = active_effect;
 
@@ -138,6 +142,15 @@ export function update_derived(derived) {
 		}
 	}
 
+	return value;
+}
+
+/**
+ * @param {Derived} derived
+ * @returns {void}
+ */
+export function update_derived(derived) {
+	var value = execute_derived(derived);
 	var status =
 		(skip_reaction || (derived.f & UNOWNED) !== 0) && derived.deps !== null ? MAYBE_DIRTY : CLEAN;
 
@@ -153,17 +166,11 @@ export function update_derived(derived) {
  * @param {Derived} signal
  * @returns {void}
  */
-function destroy_derived(signal) {
+export function destroy_derived(signal) {
 	destroy_derived_children(signal);
 	remove_reactions(signal, 0);
 	set_signal_status(signal, DESTROYED);
 
 	// TODO we need to ensure we remove the derived from any parent derives
-
-	signal.children =
-		signal.deps =
-		signal.reactions =
-		// @ts-expect-error `signal.fn` cannot be `null` while the signal is alive
-		signal.fn =
-			null;
+	signal.v = signal.children = signal.deps = signal.reactions = null;
 }

packages/svelte/src/internal/client/reactivity/effects.js

@@ -97,6 +97,7 @@ function create_effect(type, fn, sync, push = true) {
 	var effect = {
 		ctx: component_context,
 		deps: null,
+		deriveds: null,
 		nodes_start: null,
 		nodes_end: null,
 		f: type | DIRTY,

packages/svelte/src/internal/client/reactivity/props.js

@@ -10,10 +10,17 @@ import {
 import { get_descriptor, is_function } from '../../shared/utils.js';
 import { mutable_source, set, source } from './sources.js';
 import { derived, derived_safe_equal } from './deriveds.js';
-import { get, is_signals_recorded, untrack, update } from '../runtime.js';
+import {
+	active_effect,
+	get,
+	is_signals_recorded,
+	set_active_effect,
+	untrack,
+	update
+} from '../runtime.js';
 import { safe_equals } from './equality.js';
 import * as e from '../errors.js';
-import { LEGACY_DERIVED_PROP } from '../constants.js';
+import { BRANCH_EFFECT, LEGACY_DERIVED_PROP, ROOT_EFFECT } from '../constants.js';
 import { proxy } from '../proxy.js';
 
 /**
@@ -217,6 +224,26 @@ export function spread_props(...props) {
 	return new Proxy({ props }, spread_props_handler);
 }
 
+/**
+ * @template T
+ * @param {() => T} fn
+ * @returns {T}
+ */
+function with_parent_branch(fn) {
+	var effect = active_effect;
+	var previous_effect = active_effect;
+
+	while (effect !== null && (effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0) {
+		effect = effect.parent;
+	}
+	try {
+		set_active_effect(effect);
+		return fn();
+	} finally {
+		set_active_effect(previous_effect);
+	}
+}
+
 /**
  * This function is responsible for synchronizing a possibly bound prop with the inner component state.
  * It is used whenever the compiler sees that the component writes to the prop, or when it has a default prop_value.
@@ -276,8 +303,8 @@ export function prop(props, key, flags, fallback) {
 	} else {
 		// Svelte 4 did not trigger updates when a primitive value was updated to the same value.
 		// Replicate that behavior through using a derived
-		var derived_getter = (immutable ? derived : derived_safe_equal)(
-			() => /** @type {V} */ (props[key])
+		var derived_getter = with_parent_branch(() =>
+			(immutable ? derived : derived_safe_equal)(() => /** @type {V} */ (props[key]))
 		);
 		derived_getter.f |= LEGACY_DERIVED_PROP;
 		getter = () => {
@@ -321,19 +348,21 @@ export function prop(props, key, flags, fallback) {
 	// The derived returns the current value. The underlying mutable
 	// source is written to from various places to persist this value.
 	var inner_current_value = mutable_source(prop_value);
-	var current_value = derived(() => {
-		var parent_value = getter();
-		var child_value = get(inner_current_value);
-
-		if (from_child) {
-			from_child = false;
-			was_from_child = true;
-			return child_value;
-		}
+	var current_value = with_parent_branch(() =>
+		derived(() => {
+			var parent_value = getter();
+			var child_value = get(inner_current_value);
+
+			if (from_child) {
+				from_child = false;
+				was_from_child = true;
+				return child_value;
+			}
 
-		was_from_child = false;
-		return (inner_current_value.v = parent_value);
-	});
+			was_from_child = false;
+			return (inner_current_value.v = parent_value);
+		})
+	);
 
 	if (!immutable) current_value.equals = safe_equals;
 

packages/svelte/src/internal/client/reactivity/types.d.ts

@@ -42,6 +42,8 @@ export interface Effect extends Reaction {
 	nodes_end: null | TemplateNode;
 	/** The associated component context */
 	ctx: null | ComponentContext;
+	/** Reactions created inside this signal */
+	deriveds: null | Derived[];
 	/** The effect function */
 	fn: null | (() => void | (() => void));
 	/** The teardown function returned from the effect function */

packages/svelte/src/internal/client/runtime.js

@@ -27,7 +27,7 @@ import {
 import { flush_tasks } from './dom/task.js';
 import { add_owner } from './dev/ownership.js';
 import { mutate, set, source } from './reactivity/sources.js';
-import { update_derived } from './reactivity/deriveds.js';
+import { destroy_derived, execute_derived, update_derived } from './reactivity/deriveds.js';
 import * as e from './errors.js';
 import { lifecycle_outside_component } from '../shared/errors.js';
 import { FILENAME } from '../../constants.js';
@@ -300,12 +300,13 @@ export function update_reaction(reaction) {
 	var previous_reaction = active_reaction;
 	var previous_skip_reaction = skip_reaction;
 	var prev_derived_sources = derived_sources;
+	var flags = reaction.f;
 
 	new_deps = /** @type {null | Value[]} */ (null);
 	skipped_deps = 0;
 	untracked_writes = null;
-	active_reaction = (reaction.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null;
-	skip_reaction = !is_flushing_effect && (reaction.f & UNOWNED) !== 0;
+	active_reaction = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null;
+	skip_reaction = !is_flushing_effect && (flags & UNOWNED) !== 0;
 	derived_sources = null;
 
 	try {
@@ -408,6 +409,16 @@ export function remove_reactions(signal, start_index) {
  * @returns {void}
  */
 export function destroy_effect_children(signal, remove_dom = false) {
+	var deriveds = signal.deriveds;
+
+	if (deriveds !== null) {
+		signal.deriveds = null;
+
+		for (var i = 0; i < deriveds.length; i += 1) {
+			destroy_derived(deriveds[i]);
+		}
+	}
+
 	var effect = signal.first;
 	signal.first = signal.last = null;
 
@@ -729,9 +740,15 @@ export async function tick() {
  */
 export function get(signal) {
 	var flags = signal.f;
-
-	if ((flags & DESTROYED) !== 0) {
-		return signal.v;
+	var is_derived = (flags & DERIVED) !== 0;
+
+	// If the derived is destroyed, just execute it again without retaining
+	// its memoisation properties as the derived is stale
+	if (is_derived && (flags & DESTROYED) !== 0) {
+		var value = execute_derived(/** @type {Derived} */ (signal));
+		// Ensure the derived remains destroyed
+		destroy_derived(/** @type {Derived} */ (signal));
+		return value;
 	}
 
 	if (is_signals_recorded) {
@@ -768,7 +785,7 @@ export function get(signal) {
 		}
 	}
 
-	if ((flags & DERIVED) !== 0) {
+	if (is_derived) {
 		var derived = /** @type {Derived} */ (signal);
 
 		if (check_dirtiness(derived)) {

packages/svelte/tests/runtime-runes/samples/derived-unowned-3/main.svelte

@@ -1,6 +1,4 @@
 <script>
-  import { untrack } from 'svelte';
-
   class Model {
     data = $state();
 
@@ -28,9 +26,7 @@
   $effect(() => {
     if(needsSet) {
       setModel('effect');
-      untrack(() => {
-        needsSet = false;
-      });
+      needsSet = false;
     }
   });
 

packages/svelte/tests/signals/test.ts

@@ -488,7 +488,7 @@ describe('signals', () => {
 			assert.equal(a?.deps?.length, 1);
 			assert.equal(s?.reactions?.length, 1);
 			destroy();
-			assert.equal(a?.deps?.length, 1);
+			assert.equal(a?.deps, null);
 			assert.equal(s?.reactions, null);
 		};
 	});
@@ -725,4 +725,19 @@ describe('signals', () => {
 			assert.equal($.get(d), true);
 		};
 	});
+
+	test('deriveds read inside the root/branches are cleaned up', () => {
+		return () => {
+			const a = state(0);
+
+			const destroy = effect_root(() => {
+				const b = derived(() => $.get(a));
+				$.get(b);
+			});
+
+			destroy();
+
+			assert.deepEqual(a.reactions, null);
+		};
+	});
 });