Merge pull request #2950 from brenelz/fix/action-done-window-batch-adoption

fix(signals): re-adopt the queue batch when an action completes

Author: ryansolid

.changeset/action-done-window-batch-adoption.md

@@ -0,0 +1,14 @@
+---
+"@solidjs/signals": patch
+---
+
+Re-adopt the queue batch when an action completes. `done()` restored the
+active transition with a bare `setActiveTransition`, leaving the global
+queue's batch as a detached ambient batch until the scheduled flush. Anything
+registered in that microtask window was stranded with nothing to finalize it:
+a completed action's held writes were silently lost when another action
+resumed in the window (its transition merge never transferred them), a bare
+optimistic write never reverted, and an `affects()` mark could leak — leaving
+`isPending` stuck true. Completing an action now goes through
+`initTransition`, the same batch-adoption path every other
+transition-resumption site already uses.

packages/solid-signals/src/core/action.ts

@@ -4,7 +4,6 @@ import {
   flush,
   globalQueue,
   schedule,
-  setActiveTransition,
   type Transition
 } from "./scheduler.js";
 import { isThenable } from "./async.js";
@@ -122,7 +121,12 @@ export function action<Args extends any[], Y, R>(
         ctx = currentTransition(ctx);
         const i = ctx._actions.indexOf(it);
         if (i >= 0) ctx._actions.splice(i, 1);
-        setActiveTransition(ctx);
+        // Re-adopt through initTransition like every other resumption site:
+        // a bare setActiveTransition leaves globalQueue._batch as a detached
+        // ambient batch, and anything registered before the scheduled flush
+        // (held writes on a merging transition, optimistic overrides,
+        // affects() marks) lands there with nothing to ever finalize it.
+        globalQueue.initTransition(ctx);
         schedule();
         failed ? reject(e) : resolve(v!);
       };

packages/solid-signals/tests/action-done-window.test.ts

@@ -0,0 +1,170 @@
+/**
+ * The post-action done() window (#2916 shape): an async-generator action's
+ * done() runs from an iterator-result microtask, restoring activeTransition
+ * with no synchronous flush after it. Until the scheduled flush runs,
+ * globalQueue._batch was a detached ambient batch — so anything registered in
+ * that window (ordinary writes held by a merged transition, optimistic
+ * overrides, affects() marks) landed in a batch that nothing ever finalized.
+ * done() now re-adopts the batch through initTransition, the same path every
+ * other transition-resumption site uses.
+ *
+ * Each test polls microtasks until it observes the restored transition
+ * (scheduler.activeTransition !== null) and injects its work exactly there.
+ */
+import { describe, expect, it } from "vitest";
+import {
+  action,
+  affects,
+  createMemo,
+  createOptimistic,
+  createRenderEffect,
+  createRoot,
+  createSignal,
+  flush,
+  isPending
+} from "../src/index.js";
+import * as scheduler from "../src/core/scheduler.js";
+
+const tick = () => Promise.resolve();
+
+describe("post-action done() window", () => {
+  it("a completed action's write survives another action resuming in its done-window", async () => {
+    const [x, setX] = createSignal(0);
+
+    let resolveA!: () => void;
+    const pA = new Promise<void>(r => (resolveA = r));
+
+    const A = action(async function* () {
+      setX(1);
+      yield pA;
+    });
+
+    // B yields a custom thenable so the test controls exactly when B resumes.
+    let resumeB!: (v?: any) => void;
+    let hasResumeB = false;
+    const thenB = {
+      then(onFulfilled: (v: any) => void) {
+        resumeB = onFulfilled;
+        hasResumeB = true;
+      }
+    };
+    const B = action(function* () {
+      yield thenB as any;
+    });
+
+    const aDone = A();
+    flush(); // stash T_A
+    await tick();
+    const bDone = B(); // fresh transition T_B (T_A stashed, activeTransition null)
+    flush(); // stash T_B
+
+    resolveA();
+
+    // Land in A's done-window and resume B there, so initTransition(T_B)
+    // merges the restored T_A into T_B. T_A's held write must survive the
+    // merge and commit when T_B settles.
+    let resumed = false;
+    for (let i = 0; i < 16; i++) {
+      await tick();
+      if (!resumed && scheduler.activeTransition !== null && hasResumeB) {
+        resumed = true;
+        resumeB(undefined);
+      }
+    }
+    expect(resumed).toBe(true);
+
+    await Promise.all([aDone, bDone]);
+    await new Promise(r => setTimeout(r, 0));
+    flush();
+
+    expect(x()).toBe(1);
+
+    // And the signal must remain writable afterwards.
+    setX(9);
+    flush();
+    expect(x()).toBe(9);
+  });
+
+  it("a bare optimistic write in the done-window still reverts", async () => {
+    const [opt, setOpt] = createOptimistic(0);
+    let dispose!: () => void;
+    createRoot(d => {
+      dispose = d;
+      const m = createMemo(() => opt() * 2);
+      createRenderEffect(m, () => {});
+    });
+    flush();
+
+    let resolveA!: () => void;
+    const pA = new Promise<void>(r => (resolveA = r));
+    const A = action(async function* () {
+      yield pA;
+    });
+    const aDone = A();
+    flush();
+
+    resolveA();
+
+    let wrote = false;
+    for (let i = 0; i < 16; i++) {
+      await tick();
+      if (!wrote && scheduler.activeTransition !== null) {
+        wrote = true;
+        setOpt(5);
+      }
+    }
+    expect(wrote).toBe(true);
+
+    await aDone;
+    await new Promise(r => setTimeout(r, 0));
+    flush();
+    flush();
+
+    // Every batch that could own the write has settled: it must have reverted.
+    expect(opt()).toBe(0);
+    dispose();
+  });
+
+  it("an affects() mark in the done-window is released by the settling flush", async () => {
+    const [count] = createSignal(1);
+    let dispose!: () => void;
+    createRoot(d => {
+      dispose = d;
+      const m = createMemo(() => count() * 2);
+      createRenderEffect(m, () => {});
+    });
+    flush();
+
+    let resolveA!: () => void;
+    const pA = new Promise<void>(r => (resolveA = r));
+    const A = action(async function* () {
+      yield pA;
+    });
+    const aDone = A();
+    flush();
+
+    resolveA();
+
+    let marked = false;
+    for (let i = 0; i < 16; i++) {
+      await tick();
+      if (!marked && scheduler.activeTransition !== null) {
+        marked = true;
+        affects(count);
+      }
+    }
+    expect(marked).toBe(true);
+
+    await aDone;
+    await new Promise(r => setTimeout(r, 0));
+    flush();
+    flush();
+
+    // The mark now belongs to the restored transaction and releases at its
+    // settle; before the fix it landed in the detached ambient batch, where
+    // (in combination with other pending work) it could leak forever
+    // (isPending stuck true, INV-10 on the next quiescent flush).
+    expect(isPending(() => count())).toBe(false);
+    dispose();
+  });
+});