fix(signals): a removed row's cleanup can write during teardown (#4165)

.changeset/cleanup-write-during-teardown.md

@@ -0,0 +1,5 @@
+---
+"@solidjs/signals": patch
+---
+
+Allow writes in cleanups during teardown (#4165). Removing a keyed `<For>` row ran the row's cleanups inside the list's pass, so a setter in an `onCleanup` or `onSettled` cleanup was rejected in development as a write in an owned scope, which halted reactivity. A cleanup is teardown, not part of the computation disposing it, so the dev-only owned-scope write check now stands down while cleanups run. Cleanups keep their owner and context, and production builds are unchanged.

packages/signals/src/core/core.ts

@@ -1489,7 +1489,11 @@ export function exitCallback(): void {
 }
 /**
  * Dev-only: > 0 while owner teardown runs cleanups (`_disposal` entries and
- * effect-returned cleanups — owner.ts), for the same reason. Kept apart from
+ * effect-returned cleanups — owner.ts), for the same reason. The owned-scope
+ * write guard (setSignal, markRefresh, the store setter) stands down here
+ * too: a cleanup is teardown, not part of whichever computation is disposing
+ * its owner (a list pass removing a row, a branch switching away), so its
+ * writes are imperative like an effect callback's (#4165). Kept apart from
  * `callbackDepth` because its sites cannot use try/finally (the frame would
  * survive into prod): a throwing cleanup leaves it raised, and dev.ts resets
  * it on the next microtask, which teardown — synchronous — never spans.
@@ -2106,7 +2110,8 @@ export function setSignal<T>(el: Signal<T> | Computed<T>, v: T | ((prev: T) => T
     __DEV__ &&
     !(el._config & CONFIG_OWNED_WRITE) &&
     !(context && context._config & CONFIG_CHILDREN_FORBIDDEN) &&
-    context
+    context &&
+    !disposalDepth
   ) {
     emitDiagnostic({
       code: "REACTIVE_WRITE_IN_OWNED_SCOPE",
@@ -2304,7 +2309,8 @@ export function markRefresh(node: Computed<any>): void {
     __DEV__ &&
     context &&
     !((node._config ?? 0) & CONFIG_OWNED_WRITE) &&
-    !(context._config & CONFIG_CHILDREN_FORBIDDEN)
+    !(context._config & CONFIG_CHILDREN_FORBIDDEN) &&
+    !disposalDepth
   ) {
     emitDiagnostic({
       code: "REACTIVE_WRITE_IN_OWNED_SCOPE",

packages/signals/src/store/store.ts

@@ -49,6 +49,7 @@ import {
 import {
   captureWriteSnapshot,
   context,
+  disposalDepth,
   isEqual,
   notePromotedWrite,
   ownedScopeWriteMessage,
@@ -2293,7 +2294,7 @@ const ASYNC_STORE_SETTER_MESSAGE =
  * write violation — the user guard (`mapArray`'s pattern: the node-level
  * writes carry `ownedWrite`; the setter asks). */
 function devGuardStoreSetterWrite(): void {
-  if (context && !(context._config & CONFIG_CHILDREN_FORBIDDEN)) {
+  if (context && !(context._config & CONFIG_CHILDREN_FORBIDDEN) && !disposalDepth) {
     emitDiagnostic({
       code: "REACTIVE_WRITE_IN_OWNED_SCOPE",
       kind: "write",

packages/signals/tests/for-row-cleanup-write-4165.test.ts

@@ -0,0 +1,100 @@
+/**
+ * A cleanup is teardown, so a setter in it is not a write in the owned scope
+ * of whichever computation is tearing the owner down: a list pass removing a
+ * row, or a branch memo switching away (#4165). The cleanup still runs under
+ * its owner, so `getOwner` and context lookups keep working.
+ *
+ * @see https://github.com/solidjs/solid/issues/4165
+ */
+import { expect, it } from "vitest";
+import {
+  createContext,
+  createMemo,
+  createRenderEffect,
+  createRoot,
+  createSignal,
+  createStore,
+  flush,
+  getContext,
+  getOwner,
+  mapArray,
+  onCleanup,
+  setContext,
+  untrack
+} from "../src/index.js";
+
+it("a removed row's cleanup can write a signal", () => {
+  const [rows, setRows] = createSignal([1, 2]);
+  const [count, setCount] = createSignal(0);
+  const seen: unknown[] = [];
+  createRoot(() => {
+    const mapped = mapArray(rows, r => {
+      onCleanup(() => setCount(n => n + 1));
+      return r;
+    });
+    createRenderEffect(
+      () => mapped(),
+      v => void seen.push([...v])
+    );
+  });
+  flush();
+  setRows([2]);
+  flush();
+  setRows([2, 3]);
+  flush();
+  expect(seen).toEqual([[1, 2], [2], [2, 3]]);
+  expect(count()).toBe(1);
+});
+
+it("a branch's cleanup can write a signal when the branch switches away", () => {
+  const [on, setOn] = createSignal(true);
+  const [count, setCount] = createSignal(0);
+  createRoot(() => {
+    const branch = createMemo(() => {
+      if (!on()) return null;
+      return untrack(() => {
+        onCleanup(() => setCount(n => n + 1));
+        return 1;
+      });
+    });
+    createRenderEffect(branch, () => {});
+  });
+  flush();
+  setOn(false);
+  flush();
+  expect(count()).toBe(1);
+});
+
+it("a removed row's cleanup can write a store", () => {
+  const [rows, setRows] = createSignal([1, 2]);
+  const [state, setState] = createStore({ removed: [] as number[] });
+  createRoot(() => {
+    const mapped = mapArray(rows, r => {
+      onCleanup(() => setState(s => void s.removed.push(r)));
+      return r;
+    });
+    createRenderEffect(mapped, () => {});
+  });
+  flush();
+  setRows([2]);
+  flush();
+  expect(state.removed).toEqual([1]);
+});
+
+it("a removed row's cleanup still sees its owner and context", () => {
+  const Ctx = createContext("default");
+  const [rows, setRows] = createSignal([1, 2]);
+  const seen: unknown[] = [];
+  createRoot(() => {
+    setContext(Ctx, "provided");
+    const mapped = mapArray(rows, r => {
+      onCleanup(() => seen.push([r, !!getOwner(), getContext(Ctx)]));
+      return r;
+    });
+    createRenderEffect(mapped, () => {});
+  });
+  flush();
+  setRows([2]);
+  flush();
+  expect(seen).toEqual([[1, true, "provided"]]);
+});

packages/web/test/for-row-cleanup-write-4165.spec.tsx

@@ -0,0 +1,37 @@
+/**
+ * @jsxImportSource @solidjs/web
+ * @vitest-environment jsdom
+ */
+// #4165: removing a keyed <For> row ran the row's cleanups inside the list's
+// pass, so a setter in a cleanup was rejected as a write in an owned scope and
+// halted reactivity in development. Cleanups now run with no owner.
+import { afterEach, expect, test } from "vitest";
+import { createSignal, flush, For, onCleanup, onSettled, resetErrorHalt } from "solid-js";
+import { render } from "../src/index.js";
+
+afterEach(() => resetErrorHalt());
+
+for (const [label, register] of [
+  ["onSettled cleanup", (fn: () => void) => onSettled(() => fn)],
+  ["onCleanup", (fn: () => void) => onCleanup(fn)]
+] as const)
+  test(`a row's ${label} can write a signal when the row is removed`, () => {
+    const [rows, setRows] = createSignal([1, 2]);
+    const [count, setCount] = createSignal(0);
+    function Row(props: { n: number }) {
+      register(() => setCount(n => n + 1));
+      return <i>{props.n}</i>;
+    }
+    const div = document.createElement("div");
+    const dispose = render(() => <For each={rows()}>{n => <Row n={n} />}</For>, div);
+    flush();
+    expect(div.textContent).toBe("12");
+    setRows([2]);
+    flush();
+    expect(div.textContent).toBe("2");
+    expect(count()).toBe(1);
+    setRows([2, 3]);
+    flush();
+    expect(div.textContent).toBe("23");
+    dispose();
+  });