fix: effect teardown on failed setup, computed throw recovery

src/index.ts

@@ -11,6 +11,17 @@ interface EffectNode extends ReactiveNode {
 interface ComputedNode<T = any> extends ReactiveNode {
 	value: T | undefined;
 	getter: (previousValue?: T) => T;
+	/**
+	 * The error thrown by the most recent getter evaluation, if any.
+	 *
+	 * When a computed's getter throws, the computed is left in a cached
+	 * `Mutable` state with `value` unset. Without this field, a subsequent
+	 * read (e.g. from an unrelated re-run of the subscriber) would fall
+	 * through all branches and return the stale `undefined` instead of
+	 * surfacing the error. Storing it lets `computedOper` re-throw the same
+	 * error on reads until the dependencies change and the getter re-runs.
+	 */
+	error: unknown;
 }
 
 interface SignalNode<T = any> extends ReactiveNode {
@@ -99,6 +110,10 @@ export function setActiveSub(sub?: ReactiveNode) {
 	return prevSub;
 }
 
+function shouldTrack(sub: ReactiveNode): boolean {
+	return !!(sub.flags & (ReactiveFlags.Mutable | ReactiveFlags.Watching));
+}
+
 export function getBatchDepth(): number {
 	return batchDepth;
 }
@@ -153,6 +168,7 @@ export function signal<T>(initialValue?: T): {
 export function computed<T>(getter: (previousValue?: T) => T): () => T {
 	return computedOper.bind({
 		value: undefined,
+		error: undefined,
 		subs: undefined,
 		subsTail: undefined,
 		deps: undefined,
@@ -173,13 +189,16 @@ export function effect(fn: () => void | (() => void)): () => void {
 		flags: ReactiveFlags.Watching | ReactiveFlags.RecursedCheck,
 	};
 	const prevSub = setActiveSub(e);
-	if (prevSub !== undefined) {
+	if (prevSub !== undefined && shouldTrack(prevSub)) {
 		link(e, prevSub, 0);
 		prevSub.flags |= HasChildEffect;
 	}
 	try {
 		++runDepth;
 		e.cleanup = e.fn();
+	} catch (error) {
+		effectOper.call(e);
+		throw error;
 	} finally {
 		--runDepth;
 		activeSub = prevSub;
@@ -197,12 +216,15 @@ export function effectScope(fn: () => void): () => void {
 		flags: ReactiveFlags.Mutable,
 	};
 	const prevSub = setActiveSub(e);
-	if (prevSub !== undefined) {
+	if (prevSub !== undefined && shouldTrack(prevSub)) {
 		link(e, prevSub, 0);
 		prevSub.flags |= HasChildEffect;
 	}
 	try {
 		fn();
+	} catch (error) {
+		effectScopeOper.call(e);
+		throw error;
 	} finally {
 		activeSub = prevSub;
 	}
@@ -256,7 +278,14 @@ function updateComputed(c: ComputedNode): boolean {
 	try {
 		++cycle;
 		const oldValue = c.value;
-		return oldValue !== (c.value = c.getter(oldValue));
+		const newValue = c.getter(oldValue);
+		c.error = undefined;
+		return oldValue !== (c.value = newValue);
+	} catch (error) {
+		// Store the error so subsequent reads (before the deps change) can
+		// re-throw it instead of returning the stale cached value.
+		c.error = error;
+		throw error;
 	} finally {
 		activeSub = prevSub;
 		c.flags &= ~ReactiveFlags.RecursedCheck;
@@ -333,36 +362,52 @@ function flush(): void {
 
 function computedOper<T>(this: ComputedNode<T>): T {
 	const flags = this.flags;
-	if (
-		flags & ReactiveFlags.Dirty
-		|| (
-			flags & ReactiveFlags.Pending
-			&& (
-				checkDirty(this.deps!, this)
-				|| (this.flags = flags & ~ReactiveFlags.Pending, false)
+	const sub = activeSub;
+	const shouldLink = sub !== undefined && shouldTrack(sub);
+	try {
+		if (
+			flags & ReactiveFlags.Dirty
+			|| (
+				flags & ReactiveFlags.Pending
+				&& (
+					checkDirty(this.deps!, this)
+					|| (this.flags = flags & ~ReactiveFlags.Pending, false)
+				)
 			)
-		)
-	) {
-		if (updateComputed(this)) {
-			const subs = this.subs;
-			if (subs !== undefined) {
-				shallowPropagate(subs);
+		) {
+			if (updateComputed(this)) {
+				const subs = this.subs;
+				if (subs !== undefined) {
+					shallowPropagate(subs);
+				}
 			}
+		} else if (!flags) {
+			this.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck;
+			const prevSub = setActiveSub(this);
+			try {
+				this.value = this.getter();
+				this.error = undefined;
+			} catch (error) {
+				// Store the error so subsequent reads (before the deps
+				// change) can re-throw it instead of returning the stale
+				// cached value.
+				this.error = error;
+				throw error;
+			} finally {
+				activeSub = prevSub;
+				this.flags &= ~ReactiveFlags.RecursedCheck;
+			}
+		} else if (this.error !== undefined) {
+			// Cached but the last evaluation threw, and the deps haven't
+			// changed (not Dirty/Pending): re-throw the stored error rather
+			// than returning the stale `undefined` value.
+			throw this.error;
 		}
-	} else if (!flags) {
-		this.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck;
-		const prevSub = setActiveSub(this);
-		try {
-			this.value = this.getter();
-		} finally {
-			activeSub = prevSub;
-			this.flags &= ~ReactiveFlags.RecursedCheck;
+	} finally {
+		if (shouldLink) {
+			link(this, sub, cycle);
 		}
 	}
-	const sub = activeSub;
-	if (sub !== undefined) {
-		link(this, sub, cycle);
-	}
 	return this.value!;
 }
 
@@ -388,7 +433,7 @@ function signalOper<T>(this: SignalNode<T>, ...value: [T]): T | void {
 			}
 		}
 		const sub = activeSub;
-		if (sub !== undefined) {
+		if (sub !== undefined && shouldTrack(sub)) {
 			link(this, sub, cycle);
 		}
 		return this.currentValue;

tests/computed-throw.spec.ts

@@ -0,0 +1,72 @@
+import { expect, test } from 'vitest';
+import { computed, effect, signal } from '../src';
+
+/**
+ * Regression test: after a computed's getter throws on first evaluation, the
+ * computed is left in a cached `Mutable` state with `value` unset. A
+ * subsequent read (e.g. from an unrelated re-run of the subscriber, or a
+ * direct re-read) must re-throw the stored error rather than return the stale
+ * `undefined` value.
+ */
+test('computed that throws on first eval re-throws on unrelated re-runs (not stale undefined)', () => {
+	const s = signal(0);
+	const other = signal(0);
+	let getterCalls = 0;
+	const c = computed(() => {
+		getterCalls++;
+		if (s() === 0) {
+			throw new Error('PENDING');
+		}
+		return s() * 2;
+	});
+
+	let effectRuns = 0;
+	let effectValue: unknown = 'unset';
+	const stop = effect(() => {
+		effectRuns++;
+		other(); // subscribe to an unrelated signal
+		try {
+			effectValue = c();
+		} catch (e) {
+			effectValue = 'threw';
+		}
+	});
+
+	expect(effectRuns).toBe(1);
+	expect(effectValue).toBe('threw');
+
+	// Writing an unrelated signal re-runs the effect, but the computed's own
+	// deps haven't changed, so the getter is NOT re-invoked. The stored error
+	// must be re-thrown (not a stale `undefined` returned).
+	other(1);
+	expect(effectRuns).toBe(2);
+	expect(getterCalls).toBe(1);
+	expect(effectValue).toBe('threw');
+	stop();
+});
+
+test('computed that throws on first eval re-throws on direct re-read (not stale undefined)', () => {
+	const s = signal(0);
+	let getterCalls = 0;
+	const c = computed(() => {
+		getterCalls++;
+		if (s() === 0) {
+			throw new Error('PENDING');
+		}
+		return s() * 2;
+	});
+
+	// First read throws.
+	expect(() => c()).toThrow('PENDING');
+	expect(getterCalls).toBe(1);
+
+	// A direct re-read with unchanged deps must re-throw the stored error,
+	// not return the stale cached `undefined`.
+	expect(() => c()).toThrow('PENDING');
+	expect(getterCalls).toBe(1);
+
+	// Once the dep changes, the getter re-runs and succeeds.
+	s(5);
+	expect(c()).toBe(10);
+	expect(getterCalls).toBe(2);
+});

tests/effect-teardown-repro.spec.ts

@@ -0,0 +1,67 @@
+import { expect, test } from 'vitest';
+import { effect, effectScope, getActiveSub, signal } from '../src';
+import { ReactiveFlags, type ReactiveNode } from '../src/system';
+
+test('stopped effect does not subscribe to signals read later in the same run', () => {
+	const rerun = signal(0);
+	const readAfterStop = signal(0);
+	let stop!: () => void;
+	let node: ReactiveNode | undefined;
+	let stopDuringRun = false;
+	let runs = 0;
+
+	stop = effect(() => {
+		node ??= getActiveSub();
+		runs++;
+		rerun();
+		if (stopDuringRun) {
+			stop();
+			readAfterStop();
+		}
+	});
+
+	expect(runs).toBe(1);
+
+	stopDuringRun = true;
+	rerun(1);
+
+	expect(runs).toBe(2);
+	expect(node!.flags).toBe(ReactiveFlags.None);
+	expect(node!.deps).toBeUndefined();
+});
+
+test('failed effect setup does not leave a live subscription behind', () => {
+	const source = signal(0);
+	let runs = 0;
+
+	expect(() =>
+		effect(() => {
+			runs++;
+			source();
+			throw new Error('setup failed');
+		})
+	).toThrow('setup failed');
+
+	expect(runs).toBe(1);
+	expect(() => source(1)).not.toThrow();
+	expect(runs).toBe(1);
+});
+
+test('failed effect scope setup disposes child effects created before throw', () => {
+	const source = signal(0);
+	let childRuns = 0;
+
+	expect(() =>
+		effectScope(() => {
+			effect(() => {
+				childRuns++;
+				source();
+			});
+			throw new Error('scope setup failed');
+		})
+	).toThrow('scope setup failed');
+
+	expect(childRuns).toBe(1);
+	source(1);
+	expect(childRuns).toBe(1);
+});