fix: address derived memory leak on disconnection from reactive graph (#11819)

Fixes #11817.

It turns out that we weren't fully handling the disconnection of derived signals from the reactivity graph – resulting in cases where they could leak memory because they were not being removed from their dependency reactions array. However, removing ourselves from this array meant that any future changes might mean we need to reconnect the derived to the graph again – so we have to do some additional bookkeeping to make this work. Thankfully, we already have versioning because of unowned deriveds, we can use the versions to understand if the owned derived signal is dirty after being connected to the reactivity graph again – something we couldn't do in early permutations of the signal architecture – and likely why we hadn't addressed this in the same sense.

Author: trueadm

.changeset/afraid-worms-drum.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: address derived memory leak on disconnection from reactive graph

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

@@ -5,16 +5,17 @@ export const BLOCK_EFFECT = 1 << 4;
 export const BRANCH_EFFECT = 1 << 5;
 export const ROOT_EFFECT = 1 << 6;
 export const UNOWNED = 1 << 7;
-export const CLEAN = 1 << 8;
-export const DIRTY = 1 << 9;
-export const MAYBE_DIRTY = 1 << 10;
-export const INERT = 1 << 11;
-export const DESTROYED = 1 << 12;
-export const EFFECT_RAN = 1 << 13;
+export const DISCONNECTED = 1 << 8;
+export const CLEAN = 1 << 9;
+export const DIRTY = 1 << 10;
+export const MAYBE_DIRTY = 1 << 11;
+export const INERT = 1 << 12;
+export const DESTROYED = 1 << 13;
+export const EFFECT_RAN = 1 << 14;
 /** 'Transparent' effects do not create a transition boundary */
-export const EFFECT_TRANSPARENT = 1 << 14;
+export const EFFECT_TRANSPARENT = 1 << 15;
 /** Svelte 4 legacy mode props need to be handled with deriveds and be recognized elsewhere, hence the dedicated flag */
-export const LEGACY_DERIVED_PROP = 1 << 15;
+export const LEGACY_DERIVED_PROP = 1 << 16;
 
 export const STATE_SYMBOL = Symbol('$state');
 export const LOADING_ATTR_SYMBOL = Symbol('');

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

@@ -323,6 +323,8 @@ export function prop(props, key, flags, fallback) {
 				from_child = true;
 				set(inner_current_value, value);
 				get(current_value); // force a synchronisation immediately
+				// Increment the value so that unowned/disconnected nodes can validate dirtiness again.
+				current_value.version++;
 			}
 
 			return value;

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

@@ -16,7 +16,8 @@ import {
 	STATE_SYMBOL,
 	BLOCK_EFFECT,
 	ROOT_EFFECT,
-	LEGACY_DERIVED_PROP
+	LEGACY_DERIVED_PROP,
+	DISCONNECTED
 } from './constants.js';
 import { flush_tasks } from './dom/task.js';
 import { add_owner } from './dev/ownership.js';
@@ -198,26 +199,29 @@ export function check_dirtiness(reaction) {
 		return true;
 	}
 
+	var is_disconnected = (flags & DISCONNECTED) !== 0;
+
 	if ((flags & MAYBE_DIRTY) !== 0 || (is_dirty && is_unowned)) {
 		var dependencies = reaction.deps;
 
 		if (dependencies !== null) {
 			var length = dependencies.length;
 			var is_equal;
+			var reactions;
 
 			for (var i = 0; i < length; i++) {
 				var dependency = dependencies[i];
 
 				if (!is_dirty && check_dirtiness(/** @type {import('#client').Derived} */ (dependency))) {
 					is_equal = update_derived(/** @type {import('#client').Derived} **/ (dependency), true);
 				}
+				var version = dependency.version;
 
 				if (is_unowned) {
 					// If we're working with an unowned derived signal, then we need to check
 					// if our dependency write version is higher. If it is then we can assume
 					// that state has changed to a newer version and thus this unowned signal
 					// is also dirty.
-					var version = dependency.version;
 
 					if (version > /** @type {import('#client').Derived} */ (reaction).version) {
 						/** @type {import('#client').Derived} */ (reaction).version = version;
@@ -228,7 +232,7 @@ export function check_dirtiness(reaction) {
 						// If we are working with an unowned signal as part of an effect (due to !current_skip_reaction)
 						// and the version hasn't changed, we still need to check that this reaction
 						// if linked to the dependency source – otherwise future updates will not be caught.
-						var reactions = dependency.reactions;
+						reactions = dependency.reactions;
 						if (reactions === null) {
 							dependency.reactions = [reaction];
 						} else {
@@ -238,6 +242,20 @@ export function check_dirtiness(reaction) {
 				} else if ((reaction.f & DIRTY) !== 0) {
 					// `signal` might now be dirty, as a result of calling `check_dirtiness` and/or `update_derived`
 					return true;
+				} else if (is_disconnected) {
+					// It might be that the derived was was dereferenced from its dependencies but has now come alive again.
+					// In thise case, we need to re-attach it to the graph and mark it dirty if any of its dependencies have
+					// changed since.
+					if (version > /** @type {import('#client').Derived} */ (reaction).version) {
+						/** @type {import('#client').Derived} */ (reaction).version = version;
+						is_dirty = true;
+					}
+					reactions = dependency.reactions;
+					if (reactions === null) {
+						dependency.reactions = [reaction];
+					} else if (!reactions.includes(reaction)) {
+						reactions.push(reaction);
+					}
 				}
 			}
 		}
@@ -246,6 +264,9 @@ export function check_dirtiness(reaction) {
 		if (!is_unowned) {
 			set_signal_status(reaction, CLEAN);
 		}
+		if (is_disconnected) {
+			reaction.f ^= DISCONNECTED;
+		}
 	}
 
 	return is_dirty;
@@ -422,9 +443,15 @@ function remove_reaction(signal, dependency) {
 			}
 		}
 	}
-	if (reactions_length === 0 && (dependency.f & UNOWNED) !== 0) {
-		// If the signal is unowned then we need to make sure to change it to maybe dirty.
+	// If the derived has no reactions, then we can disconnect it from the graph,
+	// allowing it to either reconnect in the future, or be GC'd by the VM.
+	if (reactions_length === 0 && (dependency.f & DERIVED) !== 0) {
 		set_signal_status(dependency, MAYBE_DIRTY);
+		// If we are working with a derived that is owned by an effect, then mark it as being
+		// disconnected.
+		if ((dependency.f & (UNOWNED | DISCONNECTED)) === 0) {
+			dependency.f ^= DISCONNECTED;
+		}
 		remove_reactions(/** @type {import('#client').Derived} **/ (dependency), 0);
 	}
 }
@@ -815,7 +842,7 @@ export function get(signal) {
 		) {
 			if (current_dependencies === null) {
 				current_dependencies = [signal];
-			} else {
+			} else if (current_dependencies[current_dependencies.length - 1] !== signal) {
 				current_dependencies.push(signal);
 			}
 		}

packages/svelte/tests/signals/test.ts

@@ -2,13 +2,14 @@ import { describe, assert, it } from 'vitest';
 import { flushSync } from '../../src/index-client';
 import * as $ from '../../src/internal/client/runtime';
 import {
+	destroy_effect,
 	effect,
 	effect_root,
 	render_effect,
 	user_effect
 } from '../../src/internal/client/reactivity/effects';
 import { source, set } from '../../src/internal/client/reactivity/sources';
-import type { Derived, Value } from '../../src/internal/client/types';
+import type { Derived, Effect, Value } from '../../src/internal/client/types';
 import { proxy } from '../../src/internal/client/proxy';
 import { derived } from '../../src/internal/client/reactivity/deriveds';
 
@@ -423,4 +424,27 @@ describe('signals', () => {
 			destroy();
 		};
 	});
+
+	test('owned deriveds correctly cleanup when no longer connected to graph', () => {
+		let a: Derived<unknown>;
+		let state = source(0);
+
+		const destroy = effect_root(() => {
+			render_effect(() => {
+				a = derived(() => {
+					$.get(state);
+				});
+				$.get(a);
+			});
+		});
+
+		return () => {
+			flushSync();
+			assert.equal(a?.deps?.length, 1);
+			assert.equal(state?.reactions?.length, 1);
+			destroy();
+			assert.equal(a?.deps?.length, 1);
+			assert.equal(state?.reactions, null);
+		};
+	});
 });