Skip to content

Commit 1c9e9e7

Browse files
ryansolidDerpyCrabscursoragent
committed
fix(signals): preserve transactions entered during finalization; effects follow ownership (#3319)
An ambient flush can enter a held transaction while finalizing (store commit hook, boundary check, or a stamped recompute) and then kept committing state and applying effects as though it still owned the batch, leaving the UI stale once the transaction settled. finalizePureQueue captures the batch it started with and settles nothing an entered transaction adopted; a completing transaction whose ambient batch was separate still settles its own containers. Effects follow the #3322 owner stamp: in a flush whose finalize entered a transaction, runEffect leaves runs owned by a still-held transaction queued for the next gate to park, and applies everything computed mainline — the write that caused the flush reads and renders together. Lanes are exempt by construction (they never enter the ordinary queue). The finalize guard and the two regression tests come from PR #3319 by DerpyCrabs; the whole-flush park it proposed is replaced with per-effect ownership, which the two added tests distinguish. Closes #3319 Co-authored-by: DerpyCrabs <derpycrabs@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 1354a53 commit 1c9e9e7

7 files changed

Lines changed: 343 additions & 10 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@solidjs/signals": patch
3+
---
4+
5+
Fix a held transaction being re-entered while an unrelated flush finalizes — through a store commit hook (`deep()` readers of a projection), a boundary check, or a recompute — and that flush then committing the transaction's state and running its effects as if it still owned the batch, leaving the UI permanently stale once the transaction settled (#3319). Finalization now captures the batch it started with and settles nothing an entered transaction adopted (a completing transaction still settles its own separate containers). Effects follow ownership: a run is applied by the commit of the transaction that computed its value, so the entering flush still applies everything it computed mainline — the write that caused it reads and renders together — while runs owned by the still-held transaction park with it and release when it completes. Optimistic lanes are unaffected; they apply their own effects ahead of their transaction by design.

packages/signals/docs/INTERNALS-ASYNC-STATE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ Semantics of the `(_pendingValue, _overrideValue)` pair for an optimistic node
5757
- `_optimisticNodes` — nodes whose override reverts at completion (`resolveOptimisticNodes`).
5858
- Incomplete-transition flush stashes queues (`stashQueues`) and continues with a fresh view; completion restores them, commits pending, reverts optimistic, replays `_gatedSubs`, cleans lanes.
5959
- `_contested` — effects whose single value slot was written under this transaction and then overwritten by another live transaction or by mainline (#3322). Effects are not shared state, so a shared effect never merges transactions (memos do, via their `_transition` stamp); instead `Effect._valueTransition` records which view produced `_value`, `contestEffect` (called from `recompute` when that owner changes) registers the effect on every owed live transaction, and `finalizePureQueue` re-dirties them **before** its heap run so the re-derive and the effect phase land in the same pass — the other view's value is never published. Rules that fall out: a stale (render) reader with no transaction active is mainline and sees a foreign transaction's staged signal as committed (`read`'s fast path and `readNodeFast` apply `stale && el._transition !== null`, matching the slow path); a value computed mainline needs no protection (mainline publishes what it computes, and a transaction whose writes never touched the effect finds it still correct at commit).
60+
- Finalize re-entry (#3319): `finalizePureQueue` can *enter* a held transaction partway through — a store commit hook (`bumpDeep` on a node the transaction owns), a boundary `_checkSources` write, or a stamped recompute in its heap — and `initTransition` then adopts the batch being finalized. Two rules keep that consistent. **State:** finalize captures the batch it started with and, if `currentBatch` changed, commits/reverts nothing batch-derived (the entered transaction owns it now); a *completing* transaction whose ambient batch was separate (the #2916 shape) still settles its own containers, since adoption never touched them. **Effects:** ownership. A run applies with the commit of the transaction that computed its value (`Effect._valueTransition`). In a flush whose finalize entered a transaction, `flush()` sets `parkHeldOwners` around the ordinary `run()` calls and `runEffect` leaves runs owned by a still-held transaction queued — `_modified` stays set — for the next gate to stash with the owner, while everything computed mainline (the write that caused the flush) applies now. Lanes are exempt by construction: they apply their own effects ahead of their transaction (the optimistic view) and never enter the ordinary queue. Known residue: writes staged by a hook *before* the entry are adopted (held) and, because finalize's heap runs after its hooks, their dependents recompute owner-stamped and park with them; an entry that happens *inside* that heap can leave an earlier mainline-computed effect applied over an adopted source — narrow, and inherited from adoption rather than from this rule.
6061
- `transitionComplete`: prunes dead reporters (`reporterBlocksSource`), transition is done when no live reporter still blocks a pending source and no active-override node is blocked on someone else's async.
6162

6263
## 4. Write paths (all must stay equivalent)

packages/signals/src/core/effect.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@ import { StatusError, unwrapStatusError } from "./error.js";
2323
import { enqueueSub } from "./heap.js";
2424
import {
2525
_hitUnhandledAsync,
26+
currentTransition,
2627
GlobalQueue,
2728
haltReactivity,
29+
parkHeldOwners,
2830
resetUnhandledAsync,
2931
schedule,
3032
setTrackedQueueCallback,
@@ -148,6 +150,17 @@ function notifyEffectStatus(this: Effect<any>, status?: number, error?: any): vo
148150

149151
function runEffect(node: Effect<any>): void {
150152
if (!node._modified || node._flags & REACTIVE_DISPOSED) return;
153+
// Ownership (#3319): a value computed under a transaction is applied by that
154+
// transaction's commit. Only a flush whose finalize entered a transaction
155+
// can reach here with a still-held owner (every other path parks or settles
156+
// first); leave the run queued — `_modified` stays set — and the next gate
157+
// stashes it with the owner. Mainline-owned runs (null) apply now.
158+
if (parkHeldOwners && node._valueTransition !== null) {
159+
if (currentTransition(node._valueTransition)._done !== true) {
160+
node._queue.enqueue(node._type, node._boundRunEffect!);
161+
return;
162+
}
163+
}
151164
// Error arm (#2840), user effects only: a compute-phase error that is still
152165
// the node's settled state at effect time runs the bundle's error handler in
153166
// this same imperative, writable scope. Unwrap the StatusError used for

packages/signals/src/core/scheduler.ts

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ function cancelZombieRecompute(el: Computed<unknown>): void {
8282
export let clock = 0;
8383
export let activeTransition: Transition | null = null;
8484
let scheduled = false;
85+
/** Set only while the ordinary effect phase runs in a flush whose finalize
86+
* entered a transaction (#3319): runEffect leaves runs owned by a still-held
87+
* transaction queued for the next gate to park with their owner. */
88+
export let parkHeldOwners = false;
8589
let halted = false;
8690
let haltNotified = false;
8791
let syncDepth = 0;
@@ -744,11 +748,25 @@ export class GlobalQueue extends Queue {
744748
clock++;
745749
// Check if finalization added items to the heap (from optimistic reversion)
746750
scheduled = dirtyQueue._max >= dirtyQueue._min;
751+
// Finalization entered a transaction (a commit hook, boundary sweep or
752+
// recompute wrote a node it owns). Effects computed under it since are
753+
// its to apply, not this flush's: runEffect leaves them queued and the
754+
// next gate parks them with it. Everything computed mainline — the work
755+
// this flush already committed — applies now (#3319).
756+
// Lanes are exempt: a lane applies its own effects ahead of its
757+
// transaction by design (the optimistic view), so the flag wraps only
758+
// the ordinary runs.
759+
const entered = activeTransition !== null;
760+
if (entered) scheduled = true;
747761
// Run lane effects first (for ready lanes), then regular effects
748762
activeLanes.size && GlobalQueue._runLaneEffects!(EFFECT_RENDER);
763+
parkHeldOwners = entered;
749764
this.run(EFFECT_RENDER);
765+
parkHeldOwners = false;
750766
activeLanes.size && GlobalQueue._runLaneEffects!(EFFECT_USER);
767+
parkHeldOwners = entered;
751768
this.run(EFFECT_USER);
769+
parkHeldOwners = false;
752770
if (__DEV__) {
753771
devCheckActiveOverrides(n => {
754772
if (this._batch._optimisticNodes.includes(n as OptimisticNode)) return true;
@@ -1061,6 +1079,7 @@ export function finalizePureQueue(
10611079
) {
10621080
// For incomplete transitions, skip pending resolution and optimistic reversion
10631081
// For completing transitions or no-transition, resolve pending and revert optimistic
1082+
const finalizingBatch = currentBatch;
10641083
const resolvePending = !incomplete;
10651084
if (resolvePending) commitPendingNodes();
10661085
if (!incomplete && globalQueue._children.length) checkBoundaryChildren(globalQueue);
@@ -1075,9 +1094,19 @@ export function finalizePureQueue(
10751094
const ranHeap = dirtyQueue._max >= dirtyQueue._min;
10761095
if (ranHeap) runHeap(dirtyQueue, GlobalQueue._update);
10771096
if (resolvePending) {
1078-
if (ranHeap) commitPendingNodes();
1097+
// Boundary checks, commit hooks and recomputes can enter a transaction,
1098+
// which adopts the batch this finalize was settling: nothing batch-derived
1099+
// may be committed or reverted here — the entered transaction owns it now
1100+
// (#3319). A completing transaction's OWN containers are a different
1101+
// matter: when the ambient batch was separate from it (the #2916 shape),
1102+
// adoption never touched them and it must still settle them; when the
1103+
// batch WAS the completing transaction, adoption re-stamped its contents
1104+
// into the entered one and there is nothing left to settle.
1105+
if (currentBatch !== finalizingBatch) {
1106+
if (completingTransition === null || completingTransition === finalizingBatch) return;
1107+
} else if (ranHeap) commitPendingNodes();
10791108
// The settling batch: the completing transaction's, or the ambient one.
1080-
const batch = completingTransition ?? globalQueue._batch;
1109+
const batch = completingTransition ?? finalizingBatch;
10811110
// Optimistic reversion: a non-empty batch means _optimisticWrite ran,
10821111
// which installed the engine's hooks.
10831112
if (batch._optimisticNodes.length) GlobalQueue._resolveOptimistic!(batch._optimisticNodes);
Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
import { expect, it } from "vitest";
2+
import {
3+
action,
4+
createMemo,
5+
createProjection,
6+
createRenderEffect,
7+
createRoot,
8+
createSignal,
9+
deep,
10+
flush,
11+
snapshot
12+
} from "../src/index.js";
13+
import { Queue, globalQueue } from "../src/core/scheduler.js";
14+
15+
it("keeps writes and effects held when a boundary check re-enters an action", async () => {
16+
const gate = Promise.withResolvers<void>();
17+
const rendered: number[] = [];
18+
const [value, setValue] = createSignal(0);
19+
const [, setTick] = createSignal(0);
20+
const dispose = createRoot(dispose => {
21+
createRenderEffect(value, v => {
22+
rendered.push(v);
23+
});
24+
return dispose;
25+
});
26+
flush();
27+
28+
const start = action(function* () {
29+
setValue(1);
30+
yield gate.promise;
31+
});
32+
// Park the action with value=1 staged but uncommitted.
33+
const done = start();
34+
flush();
35+
36+
let checked = false;
37+
const boundary = Object.assign(new Queue(), {
38+
_checkSources() {
39+
if (checked) return;
40+
checked = true;
41+
// Writing a signal owned by the parked action re-enters its transaction.
42+
setValue(2);
43+
}
44+
});
45+
globalQueue.addChild(boundary);
46+
try {
47+
// Unrelated work starts an ambient flush that checks boundary sources.
48+
setTick(1);
49+
flush();
50+
expect.soft(value()).toBe(0);
51+
expect.soft(rendered).toEqual([0]);
52+
} finally {
53+
globalQueue.removeChild(boundary);
54+
gate.resolve();
55+
await done;
56+
flush();
57+
dispose();
58+
flush();
59+
}
60+
expect(rendered).toEqual([0, 2]);
61+
});
62+
63+
it("releases a deep projection reader after store-commit re-entry during a refetch", async () => {
64+
const settingsResponse = Promise.withResolvers<{ pins: string[] }>();
65+
const configResponse = Promise.withResolvers<{ ready: boolean }>();
66+
const [refreshRequested, setRefreshRequested] = createSignal(false);
67+
const [, setTick] = createSignal(0);
68+
const rendered: string[][] = [];
69+
70+
const dispose = createRoot(dispose => {
71+
const settings = createProjection<{ pins: string[] }>(
72+
() => (refreshRequested() ? settingsResponse.promise : { pins: [] }),
73+
{ pins: [] }
74+
);
75+
const config = createProjection(
76+
() => (refreshRequested() ? configResponse.promise : { ready: false }),
77+
{ ready: false }
78+
);
79+
const pins = createMemo(() => snapshot(deep(settings.pins)));
80+
createRenderEffect(pins, value => {
81+
rendered.push([...value]);
82+
});
83+
createRenderEffect(
84+
() => config.ready,
85+
() => {}
86+
);
87+
return dispose;
88+
});
89+
flush();
90+
91+
try {
92+
// Both refetches join one transition.
93+
setRefreshRequested(true);
94+
flush();
95+
96+
// Settings settles first; the config response still holds the render.
97+
settingsResponse.resolve({ pins: ["new pin"] });
98+
await Promise.resolve();
99+
flush();
100+
expect.soft(rendered).toEqual([[]]);
101+
102+
// An unrelated flush commits the settings store backing. Its deep()
103+
// notification re-enters the held transaction from commitPendingNodes().
104+
setTick(1);
105+
flush();
106+
expect.soft(rendered).toEqual([[]]);
107+
108+
// Completing the last refetch must release the parked render.
109+
configResponse.resolve({ ready: true });
110+
await Promise.resolve();
111+
flush();
112+
expect(rendered).toEqual([[], ["new pin"]]);
113+
} finally {
114+
settingsResponse.resolve({ pins: ["new pin"] });
115+
configResponse.resolve({ ready: true });
116+
await Promise.resolve();
117+
flush();
118+
dispose();
119+
flush();
120+
}
121+
});
122+
123+
// Effect ownership: a run applies with the commit of the transaction that
124+
// computed its value. The flush that entered a transaction mid-finalize still
125+
// applies everything it computed mainline — otherwise the write that caused the
126+
// flush reads committed while its own render stays stale until the entered
127+
// transaction settles.
128+
it("applies mainline-computed effects in the flush that entered a transaction", async () => {
129+
const settingsResponse = Promise.withResolvers<{ pins: string[] }>();
130+
const configResponse = Promise.withResolvers<{ ready: boolean }>();
131+
const [refreshRequested, setRefreshRequested] = createSignal(false);
132+
const [tick, setTick] = createSignal(0);
133+
const rendered: string[][] = [];
134+
const ticks: number[] = [];
135+
136+
const dispose = createRoot(dispose => {
137+
const settings = createProjection<{ pins: string[] }>(
138+
() => (refreshRequested() ? settingsResponse.promise : { pins: [] }),
139+
{ pins: [] }
140+
);
141+
const config = createProjection(
142+
() => (refreshRequested() ? configResponse.promise : { ready: false }),
143+
{ ready: false }
144+
);
145+
const pins = createMemo(() => snapshot(deep(settings.pins)));
146+
createRenderEffect(pins, value => {
147+
rendered.push([...value]);
148+
});
149+
createRenderEffect(
150+
() => config.ready,
151+
() => {}
152+
);
153+
createRenderEffect(tick, t => {
154+
ticks.push(t);
155+
});
156+
return dispose;
157+
});
158+
flush();
159+
160+
try {
161+
setRefreshRequested(true);
162+
flush();
163+
settingsResponse.resolve({ pins: ["new pin"] });
164+
await Promise.resolve();
165+
flush();
166+
expect.soft(rendered).toEqual([[]]);
167+
168+
// The tick flush commits the settings backing; its deep() notification
169+
// enters the held transaction from commitPendingNodes().
170+
setTick(1);
171+
flush();
172+
expect.soft(tick()).toBe(1);
173+
// Computed mainline before finalize → applied by this flush (no read/DOM split).
174+
expect.soft(ticks).toEqual([0, 1]);
175+
// Computed under the entered transaction → parked with it.
176+
expect.soft(rendered).toEqual([[]]);
177+
178+
configResponse.resolve({ ready: true });
179+
await Promise.resolve();
180+
flush();
181+
expect(rendered).toEqual([[], ["new pin"]]);
182+
expect(ticks).toEqual([0, 1]);
183+
} finally {
184+
settingsResponse.resolve({ pins: ["new pin"] });
185+
configResponse.resolve({ ready: true });
186+
await Promise.resolve();
187+
flush();
188+
dispose();
189+
flush();
190+
}
191+
});
192+
193+
// A write staged during finalize BEFORE the entry is adopted by the entered
194+
// transaction (held). Finalize's heap runs after its hooks, so the dependent
195+
// effect recomputes after the entry, owner-stamped, and parks with it: state
196+
// and DOM stay consistent and release together.
197+
it("holds a pre-entry hook write and its effect together with the entered transaction", async () => {
198+
const gate = Promise.withResolvers<void>();
199+
const [value, setValue] = createSignal(0);
200+
const [other, setOther] = createSignal(0);
201+
const [tick, setTick] = createSignal(0);
202+
const renderedValue: number[] = [];
203+
const renderedOther: number[] = [];
204+
const renderedTick: number[] = [];
205+
const dispose = createRoot(dispose => {
206+
createRenderEffect(value, v => void renderedValue.push(v));
207+
createRenderEffect(other, v => void renderedOther.push(v));
208+
createRenderEffect(tick, v => void renderedTick.push(v));
209+
return dispose;
210+
});
211+
flush();
212+
213+
const start = action(function* () {
214+
setValue(1);
215+
yield gate.promise;
216+
});
217+
const done = start();
218+
flush();
219+
220+
let checked = false;
221+
const boundary = Object.assign(new Queue(), {
222+
_checkSources() {
223+
if (checked) return;
224+
checked = true;
225+
setOther(1); // ambient, staged before the entry
226+
setValue(2); // enters the parked action
227+
}
228+
});
229+
globalQueue.addChild(boundary);
230+
try {
231+
setTick(1);
232+
flush();
233+
expect.soft(tick()).toBe(1);
234+
expect.soft(renderedTick).toEqual([0, 1]);
235+
expect.soft(value()).toBe(0);
236+
expect.soft(renderedValue).toEqual([0]);
237+
expect.soft(other()).toBe(0);
238+
expect.soft(renderedOther).toEqual([0]);
239+
} finally {
240+
globalQueue.removeChild(boundary);
241+
gate.resolve();
242+
await done;
243+
flush();
244+
}
245+
expect(value()).toBe(2);
246+
expect(other()).toBe(1);
247+
expect(renderedValue).toEqual([0, 2]);
248+
expect(renderedOther).toEqual([0, 1]);
249+
dispose();
250+
flush();
251+
});

packages/signals/tests/treeshake.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,16 @@ describe("pay-for-use tree-shaking (#2883)", () => {
158158
// writes the slow path already had. Core-retained by necessity: the
159159
// clobber happens in recompute and the fix is the commit ordering itself.
160160
// Measured at 21,765 post-change.
161-
expect(minifiedBytes).toBeLessThan(21_900);
161+
// CONSCIOUS BUMP (2026-09-09): +~166B for effect ownership on finalize
162+
// re-entry (#3319) — finalizePureQueue captures the batch it started
163+
// with and does not commit/revert one an entered transaction adopted
164+
// (while a completing transaction still settles its own separate
165+
// containers), and runEffect leaves runs owned by a still-held
166+
// transaction queued for the next gate when the flush's finalize entered
167+
// one, applying only what was computed mainline. The coarse alternative
168+
// (park the whole flush) is ~70B but splits reads from the DOM for the
169+
// write that caused the flush. Measured at 21,931 post-change.
170+
expect(minifiedBytes).toBeLessThan(22_050);
162171
});
163172

164173
it("plain stores shed the verdict layer, affects, boundaries, and map", async () => {

0 commit comments

Comments
 (0)