Fix nested-effect cleanup order and add LIFO disposal

src/index.ts

@@ -18,6 +18,12 @@ interface SignalNode<T = any> extends ReactiveNode {
 	pendingValue: T;
 }
 
+// Marks a parent (effect or scope) whose deps include at least one child
+// effect. Used to gate the dispose-children-first slow path in run() so
+// leaf effects (no children, no own cleanup) avoid the extra deps walk.
+// The bit is outside ReactiveFlags' range and never touched by system.ts.
+const HasChildEffect = 64;
+
 let cycle = 0;
 let runDepth = 0;
 let batchDepth = 0;
@@ -64,13 +70,21 @@ const {
 			queued[insertIndex] = left;
 		}
 	},
-	unwatched(node) {
-		if (!(node.flags & ReactiveFlags.Mutable)) {
+	unwatched(node: SignalNode | ComputedNode | EffectNode | EffectScopeNode) {
+		if ('getter' in node) {
+			if (node.depsTail !== undefined) {
+				node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty;
+				disposeAllDepsInReverse(node);
+			}
+		}
+		else if ('currentValue' in node) {
+			// Nothing to do for signals, they are always mutable and never dirty until pendingValue changes
+		}
+		else if ('fn' in node) {
+			effectOper.call(node);
+		}
+		else {
 			effectScopeOper.call(node);
-		} else if (node.depsTail !== undefined) {
-			node.depsTail = undefined;
-			node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty;
-			purgeDeps(node);
 		}
 	},
 });
@@ -161,6 +175,7 @@ export function effect(fn: () => void | (() => void)): () => void {
 	const prevSub = setActiveSub(e);
 	if (prevSub !== undefined) {
 		link(e, prevSub, 0);
+		prevSub.flags |= HasChildEffect;
 	}
 	try {
 		++runDepth;
@@ -184,6 +199,7 @@ export function effectScope(fn: () => void): () => void {
 	const prevSub = setActiveSub(e);
 	if (prevSub !== undefined) {
 		link(e, prevSub, 0);
+		prevSub.flags |= HasChildEffect;
 	}
 	try {
 		fn();
@@ -222,6 +238,17 @@ export function trigger(fn: () => void) {
 }
 
 function updateComputed(c: ComputedNode): boolean {
+	if (c.flags & HasChildEffect) {
+		let link = c.depsTail;
+		while (link !== undefined) {
+			const prev = link.prevDep;
+			const dep = link.dep;
+			if (!('getter' in dep) && !('currentValue' in dep)) {
+				unlink(link, c);
+			}
+			link = prev;
+		}
+	}
 	c.depsTail = undefined;
 	c.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck;
 	const prevSub = setActiveSub(c);
@@ -250,6 +277,17 @@ function run(e: EffectNode): void {
 			&& checkDirty(e.deps!, e)
 		)
 	) {
+		if (flags & HasChildEffect) {
+			let link = e.depsTail;
+			while (link !== undefined) {
+				const prev = link.prevDep;
+				const dep = link.dep;
+				if (!('getter' in dep) && !('currentValue' in dep)) {
+					unlink(link, e);
+				}
+				link = prev;
+			}
+		}
 		if (e.cleanup) {
 			runCleanup(e);
 			if (!e.flags) {
@@ -270,7 +308,7 @@ function run(e: EffectNode): void {
 			purgeDeps(e);
 		}
 	} else if (e.deps !== undefined) {
-		e.flags = ReactiveFlags.Watching;
+		e.flags = ReactiveFlags.Watching | (flags & HasChildEffect);
 	}
 }
 
@@ -369,22 +407,30 @@ function runCleanup(e: EffectNode): void {
 }
 
 function effectOper(this: EffectNode): void {
+	effectScopeOper.call(this);
 	if (this.cleanup) {
 		runCleanup(this);
 	}
-	effectScopeOper.call(this);
 }
 
 function effectScopeOper(this: EffectScopeNode): void {
-	this.depsTail = undefined;
 	this.flags = ReactiveFlags.None;
-	purgeDeps(this);
+	disposeAllDepsInReverse(this);
 	const sub = this.subs;
 	if (sub !== undefined) {
 		unlink(sub);
 	}
 }
 
+function disposeAllDepsInReverse(sub: ReactiveNode): void {
+	let link = sub.depsTail;
+	while (link !== undefined) {
+		const prev = link.prevDep;
+		unlink(link, sub);
+		link = prev;
+	}
+}
+
 function purgeDeps(sub: ReactiveNode) {
 	const depsTail = sub.depsTail;
 	let dep = depsTail !== undefined ? depsTail.nextDep : sub.deps;

tests/effect.spec.ts

@@ -1,5 +1,5 @@
 import { expect, test } from 'vitest';
-import { effect, getActiveSub, signal } from '../src';
+import { computed, effect, getActiveSub, signal } from '../src';
 import { ReactiveFlags } from '../src/system';
 
 test('should support custom recurse effect', () => {
@@ -16,6 +16,197 @@ test('should support custom recurse effect', () => {
 	expect(triggers).toBe(6);
 });
 
+test('cleanup order on outer re-run: inner before outer, before new run', () => {
+	const log: string[] = [];
+	const a = signal(0);
+
+	effect(() => {
+		a();
+		log.push('outer:run');
+		effect(() => {
+			log.push('inner:run');
+			return () => log.push('inner:cleanup');
+		});
+		return () => log.push('outer:cleanup');
+	});
+	expect(log).toEqual(['outer:run', 'inner:run']);
+
+	log.length = 0;
+	a(1);
+	expect(log).toEqual([
+		'inner:cleanup',
+		'outer:cleanup',
+		'outer:run',
+		'inner:run',
+	]);
+});
+
+test('cleanup order on dispose: inner before outer', () => {
+	const log: string[] = [];
+
+	const dispose = effect(() => {
+		log.push('outer:run');
+		effect(() => {
+			log.push('inner:run');
+			return () => log.push('inner:cleanup');
+		});
+		return () => log.push('outer:cleanup');
+	});
+	log.length = 0;
+	dispose();
+	expect(log).toEqual(['inner:cleanup', 'outer:cleanup']);
+});
+
+test('sibling cleanup order on dispose: reverse creation (LIFO)', () => {
+	const log: string[] = [];
+
+	const dispose = effect(() => {
+		effect(() => {
+			return () => log.push('inner1:cleanup');
+		});
+		effect(() => {
+			return () => log.push('inner2:cleanup');
+		});
+		effect(() => {
+			return () => log.push('inner3:cleanup');
+		});
+		return () => log.push('outer:cleanup');
+	});
+	dispose();
+	expect(log).toEqual([
+		'inner3:cleanup',
+		'inner2:cleanup',
+		'inner1:cleanup',
+		'outer:cleanup',
+	]);
+});
+
+test('sibling cleanup order on outer re-run: reverse creation (LIFO)', () => {
+	const log: string[] = [];
+	const a = signal(0);
+
+	effect(() => {
+		a();
+		effect(() => {
+			return () => log.push('inner1:cleanup');
+		});
+		effect(() => {
+			return () => log.push('inner2:cleanup');
+		});
+		effect(() => {
+			return () => log.push('inner3:cleanup');
+		});
+		return () => log.push('outer:cleanup');
+	});
+	log.length = 0;
+
+	a(1);
+	expect(log.slice(0, 4)).toEqual([
+		'inner3:cleanup',
+		'inner2:cleanup',
+		'inner1:cleanup',
+		'outer:cleanup',
+	]);
+});
+
+test('three-level nested cleanup on dispose: deepest first (depth-first reverse)', () => {
+	const log: string[] = [];
+
+	const dispose = effect(() => {
+		effect(() => {
+			effect(() => {
+				return () => log.push('grandchild:cleanup');
+			});
+			return () => log.push('child:cleanup');
+		});
+		return () => log.push('outer:cleanup');
+	});
+	dispose();
+	expect(log).toEqual([
+		'grandchild:cleanup',
+		'child:cleanup',
+		'outer:cleanup',
+	]);
+});
+
+test('computed unwatched: child effect cleanups run in reverse creation (LIFO)', () => {
+	// When the computed loses its last subscriber and gets unwatched, any
+	// effects it created during its getter must be cleaned up LIFO, not FIFO.
+	const log: string[] = [];
+	const c = computed(() => {
+		effect(() => () => log.push('e1'));
+		effect(() => () => log.push('e2'));
+		effect(() => () => log.push('e3'));
+		return 0;
+	});
+	const dispose = effect(() => { c(); });
+	log.length = 0;
+	dispose();
+	expect(log).toEqual(['e3', 'e2', 'e1']);
+});
+
+test('effect created inside computed: old inner cleanup runs before new inner setup', () => {
+	// When a computed re-evaluates, any effects its getter created on the
+	// previous run must be disposed (with cleanup) before the getter runs
+	// again. Otherwise the old inner's cleanup ends up running after the
+	// new inner has already been set up.
+	const a = signal(0);
+	const log: string[] = [];
+
+	const c = computed(() => {
+		log.push('computed:eval');
+		effect(() => {
+			log.push('inner:run');
+			return () => log.push('inner:cleanup');
+		});
+		return a();
+	});
+
+	effect(() => {
+		c();
+	});
+	log.length = 0;
+
+	a(1);
+	expect(log).toEqual([
+		'inner:cleanup',
+		'computed:eval',
+		'inner:run',
+	]);
+});
+
+test('cleanup order is correct on outer re-run after a prior inner-only re-run', () => {
+	// Regression: inner re-running alone routes outer through run()'s
+	// not-dirty branch (restore Watching), which must preserve any
+	// "has child effect" tracking so the next real outer re-run still
+	// disposes children before its own cleanup.
+	const a = signal(0);
+	const b = signal(0);
+	const log: string[] = [];
+
+	effect(() => {
+		a();
+		log.push('outer:run');
+		effect(() => {
+			b();
+			log.push('inner:run');
+			return () => log.push('inner:cleanup');
+		});
+		return () => log.push('outer:cleanup');
+	});
+
+	b(1); // inner re-runs alone; outer is touched via notify chain
+	log.length = 0;
+
+	a(1);
+	expect(log).toEqual([
+		'inner:cleanup',
+		'outer:cleanup',
+		'outer:run',
+		'inner:run',
+	]);
+});
+
 // https://github.com/stackblitz/alien-signals/issues/115
 test('outer effect keeps responding to its own dep after inner re-runs', () => {
 	const a = signal(0);