Skip to content

Commit 82d61b4

Browse files
brenelzclaude
andcommitted
fix(signals): re-adopt the queue batch when an action completes
done() restored the active transition with a bare setActiveTransition, leaving globalQueue._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 — the transition merge moves every list except _pendingNodes, and the batch-adoption pass only sees the queue's batch (INV-7, #2827 class) - a bare optimistic write never reverted (INV-6) - an affects() mark could leak, leaving isPending stuck true (INV-10) Completing an action now goes through initTransition, the same merge-and-adopt path every other transition-resumption site already uses. With the batch adopted, the next action's initTransition also transfers and re-stamps the restored transition's pending nodes, closing the merge orphan without touching mergeTransitionState. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 63cd066 commit 82d61b4

3 files changed

Lines changed: 190 additions & 2 deletions

File tree

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

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import {
44
flush,
55
globalQueue,
66
schedule,
7-
setActiveTransition,
87
type Transition
98
} from "./scheduler.js";
109
import { isThenable } from "./async.js";
@@ -122,7 +121,12 @@ export function action<Args extends any[], Y, R>(
122121
ctx = currentTransition(ctx);
123122
const i = ctx._actions.indexOf(it);
124123
if (i >= 0) ctx._actions.splice(i, 1);
125-
setActiveTransition(ctx);
124+
// Re-adopt through initTransition like every other resumption site:
125+
// a bare setActiveTransition leaves globalQueue._batch as a detached
126+
// ambient batch, and anything registered before the scheduled flush
127+
// (held writes on a merging transition, optimistic overrides,
128+
// affects() marks) lands there with nothing to ever finalize it.
129+
globalQueue.initTransition(ctx);
126130
schedule();
127131
failed ? reject(e) : resolve(v!);
128132
};
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/**
2+
* The post-action done() window (#2916 shape): an async-generator action's
3+
* done() runs from an iterator-result microtask, restoring activeTransition
4+
* with no synchronous flush after it. Until the scheduled flush runs,
5+
* globalQueue._batch was a detached ambient batch — so anything registered in
6+
* that window (ordinary writes held by a merged transition, optimistic
7+
* overrides, affects() marks) landed in a batch that nothing ever finalized.
8+
* done() now re-adopts the batch through initTransition, the same path every
9+
* other transition-resumption site uses.
10+
*
11+
* Each test polls microtasks until it observes the restored transition
12+
* (scheduler.activeTransition !== null) and injects its work exactly there.
13+
*/
14+
import { describe, expect, it } from "vitest";
15+
import {
16+
action,
17+
affects,
18+
createMemo,
19+
createOptimistic,
20+
createRenderEffect,
21+
createRoot,
22+
createSignal,
23+
flush,
24+
isPending
25+
} from "../src/index.js";
26+
import * as scheduler from "../src/core/scheduler.js";
27+
28+
const tick = () => Promise.resolve();
29+
30+
describe("post-action done() window", () => {
31+
it("a completed action's write survives another action resuming in its done-window", async () => {
32+
const [x, setX] = createSignal(0);
33+
34+
let resolveA!: () => void;
35+
const pA = new Promise<void>(r => (resolveA = r));
36+
37+
const A = action(async function* () {
38+
setX(1);
39+
yield pA;
40+
});
41+
42+
// B yields a custom thenable so the test controls exactly when B resumes.
43+
let resumeB!: (v?: any) => void;
44+
let hasResumeB = false;
45+
const thenB = {
46+
then(onFulfilled: (v: any) => void) {
47+
resumeB = onFulfilled;
48+
hasResumeB = true;
49+
}
50+
};
51+
const B = action(function* () {
52+
yield thenB as any;
53+
});
54+
55+
const aDone = A();
56+
flush(); // stash T_A
57+
await tick();
58+
const bDone = B(); // fresh transition T_B (T_A stashed, activeTransition null)
59+
flush(); // stash T_B
60+
61+
resolveA();
62+
63+
// Land in A's done-window and resume B there, so initTransition(T_B)
64+
// merges the restored T_A into T_B. T_A's held write must survive the
65+
// merge and commit when T_B settles.
66+
let resumed = false;
67+
for (let i = 0; i < 16; i++) {
68+
await tick();
69+
if (!resumed && scheduler.activeTransition !== null && hasResumeB) {
70+
resumed = true;
71+
resumeB(undefined);
72+
}
73+
}
74+
expect(resumed).toBe(true);
75+
76+
await Promise.all([aDone, bDone]);
77+
await new Promise(r => setTimeout(r, 0));
78+
flush();
79+
80+
expect(x()).toBe(1);
81+
82+
// And the signal must remain writable afterwards.
83+
setX(9);
84+
flush();
85+
expect(x()).toBe(9);
86+
});
87+
88+
it("a bare optimistic write in the done-window still reverts", async () => {
89+
const [opt, setOpt] = createOptimistic(0);
90+
let dispose!: () => void;
91+
createRoot(d => {
92+
dispose = d;
93+
const m = createMemo(() => opt() * 2);
94+
createRenderEffect(m, () => {});
95+
});
96+
flush();
97+
98+
let resolveA!: () => void;
99+
const pA = new Promise<void>(r => (resolveA = r));
100+
const A = action(async function* () {
101+
yield pA;
102+
});
103+
const aDone = A();
104+
flush();
105+
106+
resolveA();
107+
108+
let wrote = false;
109+
for (let i = 0; i < 16; i++) {
110+
await tick();
111+
if (!wrote && scheduler.activeTransition !== null) {
112+
wrote = true;
113+
setOpt(5);
114+
}
115+
}
116+
expect(wrote).toBe(true);
117+
118+
await aDone;
119+
await new Promise(r => setTimeout(r, 0));
120+
flush();
121+
flush();
122+
123+
// Every batch that could own the write has settled: it must have reverted.
124+
expect(opt()).toBe(0);
125+
dispose();
126+
});
127+
128+
it("an affects() mark in the done-window is released by the settling flush", async () => {
129+
const [count] = createSignal(1);
130+
let dispose!: () => void;
131+
createRoot(d => {
132+
dispose = d;
133+
const m = createMemo(() => count() * 2);
134+
createRenderEffect(m, () => {});
135+
});
136+
flush();
137+
138+
let resolveA!: () => void;
139+
const pA = new Promise<void>(r => (resolveA = r));
140+
const A = action(async function* () {
141+
yield pA;
142+
});
143+
const aDone = A();
144+
flush();
145+
146+
resolveA();
147+
148+
let marked = false;
149+
for (let i = 0; i < 16; i++) {
150+
await tick();
151+
if (!marked && scheduler.activeTransition !== null) {
152+
marked = true;
153+
affects(count);
154+
}
155+
}
156+
expect(marked).toBe(true);
157+
158+
await aDone;
159+
await new Promise(r => setTimeout(r, 0));
160+
flush();
161+
flush();
162+
163+
// The mark now belongs to the restored transaction and releases at its
164+
// settle; before the fix it landed in the detached ambient batch, where
165+
// (in combination with other pending work) it could leak forever
166+
// (isPending stuck true, INV-10 on the next quiescent flush).
167+
expect(isPending(() => count())).toBe(false);
168+
dispose();
169+
});
170+
});

0 commit comments

Comments
 (0)