fix(core): prevent model effect capture in untracked

.changeset/tender-carrots-hunt.md

@@ -0,0 +1,7 @@
+---
+"@preact/signals-core": patch
+---
+
+Prevent model effect capture while creating effects inside `untracked()` and `action()` callbacks.
+
+If you create an `effect()` inside an `untracked()` callback within a `createModel()` factory, that effect is no longer disposed when the model is disposed. Use the disposer returned by `effect()` to clean it up manually.

.husky/pre-commit

@@ -1,4 +1,4 @@
 #!/usr/bin/env sh
 . "$(dirname -- "$0")/_/husky.sh"
 
-npm run nano-staged
+pnpm run nano-staged

packages/core/src/index.ts

@@ -108,20 +108,35 @@ function batch<T>(fn: () => T): T {
 // Currently evaluated computed or effect.
 let evalContext: Computed | Effect | undefined = undefined;
 
+// Effects captured while constructing a model instance.
+let capturedEffects: Effect[] | undefined;
+
 /**
  * Run a callback function that can access signal values without
  * subscribing to the signal updates.
  *
+ * When called inside a `createModel` factory, this also suppresses
+ * model-owned effect capture. Effects created inside the callback will not
+ * be owned by the surrounding model and must be disposed manually. Nested
+ * `createModel` calls inside the callback still capture their own effects.
+ *
  * @param fn The callback function.
  * @returns The value returned by the callback.
  */
 function untracked<T>(fn: () => T): T {
 	const prevContext = evalContext;
+	const prevCapturedEffects = capturedEffects;
+
 	evalContext = undefined;
+	// Model effect capture is another kind of ambient tracking. Suppress it in
+	// untracked callbacks while still allowing nested createModel() calls to
+	// establish their own capture scope.
+	capturedEffects = undefined;
 	try {
 		return fn();
 	} finally {
 		evalContext = prevContext;
+		capturedEffects = prevCapturedEffects;
 	}
 }
 
@@ -859,8 +874,6 @@ export interface EffectOptions {
 	name?: string;
 }
 
-let capturedEffects: Effect[] | undefined;
-
 /** @internal */
 function Effect(this: Effect, fn: EffectFn, options?: EffectOptions) {
 	this._fn = fn;
@@ -1027,6 +1040,9 @@ interface InternalModelConstructor<
 
 function startCapturingEffects(): () => Effect[] | undefined {
 	let prevCapturedEffects = capturedEffects;
+	// Always establish a fresh capture scope, even when `untracked()` has
+	// temporarily cleared the parent scope. This lets nested models own their
+	// effects without promoting them to a suppressed outer scope.
 	capturedEffects = [];
 
 	return function stopCapturingEffects() {

packages/core/test/signal.test.tsx

@@ -5,6 +5,7 @@ import {
 	effect,
 	batch,
 	createModel,
+	action,
 	Signal,
 	untracked,
 	ReadonlySignal,
@@ -2680,6 +2681,157 @@ describe("createModel", () => {
 		expect(effectRan).to.equal(true); // Effect runs before error is thrown
 	});
 
+	it("does not capture effects created inside untracked callbacks", () => {
+		let runs = 0;
+		const cleanup = vi.fn();
+		let disposeEffect!: () => void;
+
+		const TestModel = createModel(() => {
+			const count = signal(0);
+
+			untracked(() => {
+				disposeEffect = effect(() => {
+					count.value;
+					runs++;
+					return cleanup;
+				});
+			});
+
+			return { count };
+		});
+
+		const model = new TestModel();
+		expect(runs).to.equal(1);
+
+		model[Symbol.dispose]();
+		expect(cleanup).not.toHaveBeenCalled();
+
+		model.count.value++;
+		expect(runs).to.equal(2);
+		expect(cleanup).toHaveBeenCalledTimes(1);
+
+		disposeEffect();
+		expect(cleanup).toHaveBeenCalledTimes(2);
+	});
+
+	it("does not capture effects created inside action callbacks", () => {
+		let runs = 0;
+		const cleanup = vi.fn();
+		let disposeEffect!: () => void;
+
+		const TestModel = createModel(() => {
+			const count = signal(0);
+
+			const createExternalEffect = action(() => {
+				disposeEffect = effect(() => {
+					count.value;
+					runs++;
+					return cleanup;
+				});
+			});
+
+			createExternalEffect();
+
+			return { count };
+		});
+
+		const model = new TestModel();
+		expect(runs).to.equal(1);
+
+		model[Symbol.dispose]();
+		expect(cleanup).not.toHaveBeenCalled();
+
+		model.count.value++;
+		expect(runs).to.equal(2);
+		expect(cleanup).toHaveBeenCalledTimes(1);
+
+		disposeEffect();
+		expect(cleanup).toHaveBeenCalledTimes(2);
+	});
+
+	it("lets nested models created inside untracked capture their own effects without promoting them to the outer model", () => {
+		let outerRuns = 0;
+		let innerRuns = 0;
+		const outerCleanup = vi.fn();
+		const innerCleanup = vi.fn();
+
+		const InnerModel = createModel(() => {
+			const count = signal(0);
+
+			effect(() => {
+				count.value;
+				innerRuns++;
+				return innerCleanup;
+			});
+
+			return { count };
+		});
+
+		const OuterModel = createModel(() => {
+			const outerCount = signal(0);
+
+			effect(() => {
+				outerCount.value;
+				outerRuns++;
+				return outerCleanup;
+			});
+
+			const inner = untracked(() => new InnerModel());
+			return { outerCount, inner };
+		});
+
+		const outer = new OuterModel();
+		expect(outerRuns).to.equal(1);
+		expect(innerRuns).to.equal(1);
+
+		outer[Symbol.dispose]();
+		expect(outerCleanup).toHaveBeenCalledTimes(1);
+		expect(innerCleanup).not.toHaveBeenCalled();
+
+		outer.outerCount.value++;
+		expect(outerRuns).to.equal(1);
+
+		outer.inner.count.value++;
+		expect(innerRuns).to.equal(2);
+		expect(innerCleanup).toHaveBeenCalledTimes(1);
+
+		outer.inner[Symbol.dispose]();
+		expect(innerCleanup).toHaveBeenCalledTimes(2);
+	});
+
+	it("restores model effect capture when untracked callbacks throw inside model factories", () => {
+		let runs = 0;
+		const cleanup = vi.fn();
+
+		const TestModel = createModel(() => {
+			const count = signal(0);
+
+			try {
+				untracked(() => {
+					throw new Error("transient");
+				});
+			} catch {}
+
+			effect(() => {
+				count.value;
+				runs++;
+				return cleanup;
+			});
+
+			return { count };
+		});
+
+		const model = new TestModel();
+		expect(runs).to.equal(1);
+
+		model[Symbol.dispose]();
+		expect(cleanup).toHaveBeenCalledTimes(1);
+
+		model.count.value++;
+		expect(runs).to.equal(1);
+		expect(cleanup).toHaveBeenCalledTimes(1);
+	});
+
 	describe("model composition", () => {
 		it("allows instantiating a model within another model", () => {
 			const InnerModel = createModel(() => ({