fix: dispose nested effects in reverse-creation order, before own cleanup

Two bugs in cleanup of nested effects:

1. When an inner effect is auto-disposed (parent re-run / parent
   dispose / scope dispose), its cleanup was never called. The
   unwatched callback routed straight to effectScopeOper, which
   clears state but never reads .cleanup. Inner cleanup was
   silently dropped on every auto-dispose path.

2. Even after wiring cleanup into the auto-dispose path, the order
   was inverted: the parent's own cleanup ran first (in effectOper
   / run() / updateComputed), then children were disposed via
   purgeDeps at the end. Standard reverse-of-creation order needs
   the inner cleanup to run BEFORE the outer cleanup, so the inner
   can still rely on whatever state the outer set up; and BEFORE
   the body re-runs (in the re-run case), so the old inner doesn't
   end up cleaning up state that the new inner already set up.

Fixes:

- unwatched() dispatches by node shape: EffectNode → effectOper
  (full dispose including cleanup), ComputedNode → its existing
  stale-clear path, EffectScopeNode → effectScopeOper.

- effectOper / effectScopeOper / run() / updateComputed all walk
  this.depsTail backward and unlink child EffectNode deps before
  the node's own cleanup runs. unlink triggers unwatched → recursive
  effectOper on each child, so grandchildren get cleaned up
  depth-first reverse before their parent.

- A per-effect flag (HasChildEffect, defined locally in index.ts —
  outside ReactiveFlags' range so system.ts never touches it) gates
  the pre-walk in run() and updateComputed. Leaf effects with no
  child effects skip the walk entirely; the flag is set by effect()
  when it links to a parent and is naturally cleared at the start
  of a body re-run.

- The not-dirty branch of run() (restore Watching after notify
  chain) explicitly preserves HasChildEffect: e.flags = Watching |
  (flags & HasChildEffect). Otherwise an inner-only re-run would
  clobber the outer's bit and the next real re-run would skip the
  pre-walk.

Tests:

- effect.spec.ts:
  - cleanup order on outer re-run
  - cleanup order on dispose
  - sibling cleanup on dispose / re-run (LIFO)
  - three-level nested cleanup
  - cleanup order after a prior inner-only re-run
  - effect created inside computed (parent is ComputedNode)
- effectScope.spec.ts:
  - scope dispose runs child cleanup
  - sibling LIFO on scope dispose
  - depth-first reverse for nested in scope

All 198 pass.

Author: johnsoncodehk

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,22 @@ const {
 			queued[insertIndex] = left;
 		}
 	},
-	unwatched(node) {
-		if (!(node.flags & ReactiveFlags.Mutable)) {
+	unwatched(node: SignalNode | ComputedNode | EffectNode | EffectScopeNode) {
+		if ('fn' in node) {
+			effectOper.call(node);
+		}
+		else if ('getter' in node) {
+			if (node.depsTail !== undefined) {
+				node.depsTail = undefined;
+				node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty;
+				purgeDeps(node);
+			}
+		}
+		else if ('currentValue' in node) {
+			// Nothing to do for signals, they are always mutable and never dirty until pendingValue changes
+		}
+		else {
 			effectScopeOper.call(node);
-		} else if (node.depsTail !== undefined) {
-			node.depsTail = undefined;
-			node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty;
-			purgeDeps(node);
 		}
 	},
 });
@@ -161,6 +176,7 @@ export function effect(fn: () => void | (() => void)): () => void {
 	const prevSub = setActiveSub(e);
 	if (prevSub !== undefined) {
 		link(e, prevSub, 0);
+		prevSub.flags |= HasChildEffect;
 	}
 	try {
 		++runDepth;
@@ -222,6 +238,16 @@ 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;
+			if ('fn' in link.dep) {
+				unlink(link, c);
+			}
+			link = prev;
+		}
+	}
 	c.depsTail = undefined;
 	c.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck;
 	const prevSub = setActiveSub(c);
@@ -250,6 +276,16 @@ function run(e: EffectNode): void {
 			&& checkDirty(e.deps!, e)
 		)
 	) {
+		if (flags & HasChildEffect) {
+			let link = e.depsTail;
+			while (link !== undefined) {
+				const prev = link.prevDep;
+				if ('fn' in link.dep) {
+					unlink(link, e);
+				}
+				link = prev;
+			}
+		}
 		if (e.cleanup) {
 			runCleanup(e);
 			if (!e.flags) {
@@ -270,7 +306,7 @@ function run(e: EffectNode): void {
 			purgeDeps(e);
 		}
 	} else if (e.deps !== undefined) {
-		e.flags = ReactiveFlags.Watching;
+		e.flags = ReactiveFlags.Watching | (flags & HasChildEffect);
 	}
 }
 
@@ -369,16 +405,20 @@ 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);
+	let link = this.depsTail;
+	while (link !== undefined) {
+		const prev = link.prevDep;
+		unlink(link, this);
+		link = prev;
+	}
 	const sub = this.subs;
 	if (sub !== undefined) {
 		unlink(sub);

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,181 @@ 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('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);

tests/effectScope.spec.ts

@@ -0,0 +1,44 @@
+import { expect, test } from 'vitest';
+import { effect, effectScope } from '../src';
+
+test('scope dispose runs child effect cleanup', () => {
+	const log: string[] = [];
+	const dispose = effectScope(() => {
+		effect(() => {
+			return () => log.push('inner:cleanup');
+		});
+	});
+	dispose();
+	expect(log).toEqual(['inner:cleanup']);
+});
+
+test('scope dispose: sibling effects clean up in reverse creation (LIFO)', () => {
+	const log: string[] = [];
+	const dispose = effectScope(() => {
+		effect(() => {
+			return () => log.push('e1:cleanup');
+		});
+		effect(() => {
+			return () => log.push('e2:cleanup');
+		});
+		effect(() => {
+			return () => log.push('e3:cleanup');
+		});
+	});
+	dispose();
+	expect(log).toEqual(['e3:cleanup', 'e2:cleanup', 'e1:cleanup']);
+});
+
+test('scope dispose: nested effect cleanup runs depth-first reverse', () => {
+	const log: string[] = [];
+	const dispose = effectScope(() => {
+		effect(() => {
+			effect(() => {
+				return () => log.push('grandchild:cleanup');
+			});
+			return () => log.push('child:cleanup');
+		});
+	});
+	dispose();
+	expect(log).toEqual(['grandchild:cleanup', 'child:cleanup']);
+});