Skip to content

Stopped effects can be re-subscribed and failed effect/effectScope setup can leak nodes #118

@medz

Description

@medz

Summary

I found two related lifecycle regressions in alien-signals@3.2.1
(8734d386d925025d0e99419bd9161c17b112c5ee):

  1. If an effect calls its own disposer and then reads a signal before returning,
    the stopped effect is linked back into that signal's subscriber list.
  2. If the initial effect() callback throws, the partially initialized effect
    remains subscribed and can run again on later signal writes.
  3. If an effectScope() body throws after creating child effects, those child
    effects remain alive even though the scope was never returned to the caller.

These leave orphaned reactive nodes in the graph, and can keep throwing effects
or child effects alive with no disposer available to user code.

Reproduction

This was verified against upstream v3.2.1 with:

npx vitest run tests/effect-teardown.spec.ts

Add this test file:

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);
});

Actual behavior

All three tests fail on v3.2.1.

Observed failures:

  • The stopped effect has flags === ReactiveFlags.None, but still gains a new
    dependency after reading a signal later in the same callback.
  • A throwing effect() setup remains subscribed, so writing to the source
    signal throws setup failed again.
  • A throwing effectScope() setup leaves child effects alive, so writing to the
    source signal re-runs the child effect.

Expected behavior

  • A stopped effect/scope should not be eligible for dependency tracking later in
    the same callback.
  • If initial effect() setup throws, the partially initialized effect should be
    disposed before the error is rethrown.
  • If initial effectScope() setup throws, the partially initialized scope and
    any children created before the throw should be disposed before the error is
    rethrown.

Root cause

signalOper() and computedOper() link to activeSub whenever it is defined:

const sub = activeSub;
if (sub !== undefined) {
	link(this, sub, cycle);
}

However, activeSub can still point at a node that has already been stopped
inside its own callback. In that state flags is already
ReactiveFlags.None, but a later signal read still creates a new dependency.

Separately, the initial setup paths for effect() and effectScope() restore
global state in finally, but they do not tear down the partially initialized
node on error.

Suggested fix

Use an eligibility check before linking a dependency to activeSub, and tear
down partially initialized nodes in catch blocks.

Prototype patch:

 export function setActiveSub(sub?: ReactiveNode) {
 	const prevSub = activeSub;
 	activeSub = sub;
 	return prevSub;
 }

+function shouldTrack(sub: ReactiveNode): boolean {
+	return !!(sub.flags & (ReactiveFlags.Mutable | ReactiveFlags.Watching));
+}
+
 export function effect(fn: () => void | (() => void)): () => void {
 	...
 	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;
 		e.flags &= ~ReactiveFlags.RecursedCheck;
 	}
 	return effectOper.bind(e);
 }

 export function effectScope(fn: () => void): () => void {
 	...
 	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;
 	}
 	return effectScopeOper.bind(e);
 }

 function computedOper<T>(this: ComputedNode<T>): T {
 	...
 	const sub = activeSub;
-	if (sub !== undefined) {
+	if (sub !== undefined && shouldTrack(sub)) {
 		link(this, sub, cycle);
 	}
 	return this.value!;
 }

 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;
 }

Validation

With the prototype patch applied locally to upstream v3.2.1:

npx vitest run tests/effect-teardown.spec.ts
npm run check

Both commands pass.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions