From 790f180c53e2e3ed5b0520a8004264de890b22ca Mon Sep 17 00:00:00 2001 From: DerpyCrabs Date: Tue, 8 Sep 2026 23:02:29 +0300 Subject: [PATCH] fix(signals): preserve transactions reentered during finalization --- .changeset/preserve-reentered-transitions.md | 5 + packages/signals/src/core/scheduler.ts | 10 ++ .../signals/tests/finalize-reentry.test.ts | 121 ++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 .changeset/preserve-reentered-transitions.md create mode 100644 packages/signals/tests/finalize-reentry.test.ts diff --git a/.changeset/preserve-reentered-transitions.md b/.changeset/preserve-reentered-transitions.md new file mode 100644 index 000000000..0e342a647 --- /dev/null +++ b/.changeset/preserve-reentered-transitions.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Preserve pending transaction state when boundary checks re-enter a transaction during finalization. diff --git a/packages/signals/src/core/scheduler.ts b/packages/signals/src/core/scheduler.ts index 525e2c9ad..72e3f7856 100644 --- a/packages/signals/src/core/scheduler.ts +++ b/packages/signals/src/core/scheduler.ts @@ -713,6 +713,12 @@ export class GlobalQueue extends Queue { } } clock++; + // Finalization can re-enter a pending transaction. Its effects must + // return through the transition gate before any apply runs. + if (activeTransition) { + scheduled = true; + return; + } // Check if finalization added items to the heap (from optimistic reversion) scheduled = dirtyQueue._max >= dirtyQueue._min; // Run lane effects first (for ready lanes), then regular effects @@ -1032,12 +1038,16 @@ export function finalizePureQueue( ) { // For incomplete transitions, skip pending resolution and optimistic reversion // For completing transitions or no-transition, resolve pending and revert optimistic + const finalizingBatch = currentBatch; const resolvePending = !incomplete; if (resolvePending) commitPendingNodes(); if (!incomplete && globalQueue._children.length) checkBoundaryChildren(globalQueue); const ranHeap = dirtyQueue._max >= dirtyQueue._min; if (ranHeap) runHeap(dirtyQueue, GlobalQueue._update); if (resolvePending) { + // Boundary checks and recomputes can adopt another transaction. The + // current finalize must not commit or revert that transaction’s state. + if (currentBatch !== finalizingBatch) return; if (ranHeap) commitPendingNodes(); // The settling batch: the completing transaction's, or the ambient one. const batch = completingTransition ?? globalQueue._batch; diff --git a/packages/signals/tests/finalize-reentry.test.ts b/packages/signals/tests/finalize-reentry.test.ts new file mode 100644 index 000000000..45b51129f --- /dev/null +++ b/packages/signals/tests/finalize-reentry.test.ts @@ -0,0 +1,121 @@ +import { expect, it } from "vitest"; +import { + action, + createMemo, + createProjection, + createRenderEffect, + createRoot, + createSignal, + deep, + flush, + snapshot +} from "../src/index.js"; +import { Queue, globalQueue } from "../src/core/scheduler.js"; + +it("keeps writes and effects held when a boundary check re-enters an action", async () => { + const gate = Promise.withResolvers(); + const rendered: number[] = []; + const [value, setValue] = createSignal(0); + const [, setTick] = createSignal(0); + const dispose = createRoot(dispose => { + createRenderEffect(value, v => { + rendered.push(v); + }); + return dispose; + }); + flush(); + + const start = action(function* () { + setValue(1); + yield gate.promise; + }); + // Park the action with value=1 staged but uncommitted. + const done = start(); + flush(); + + let checked = false; + const boundary = Object.assign(new Queue(), { + _checkSources() { + if (checked) return; + checked = true; + // Writing a signal owned by the parked action re-enters its transaction. + setValue(2); + } + }); + globalQueue.addChild(boundary); + try { + // Unrelated work starts an ambient flush that checks boundary sources. + setTick(1); + flush(); + expect.soft(value()).toBe(0); + expect.soft(rendered).toEqual([0]); + } finally { + globalQueue.removeChild(boundary); + gate.resolve(); + await done; + flush(); + dispose(); + flush(); + } + expect(rendered).toEqual([0, 2]); +}); + +it("releases a deep projection reader after store-commit re-entry during a refetch", async () => { + const settingsResponse = Promise.withResolvers<{ pins: string[] }>(); + const configResponse = Promise.withResolvers<{ ready: boolean }>(); + const [refreshRequested, setRefreshRequested] = createSignal(false); + const [, setTick] = createSignal(0); + const rendered: string[][] = []; + + const dispose = createRoot(dispose => { + const settings = createProjection<{ pins: string[] }>( + () => (refreshRequested() ? settingsResponse.promise : { pins: [] }), + { pins: [] } + ); + const config = createProjection( + () => (refreshRequested() ? configResponse.promise : { ready: false }), + { ready: false } + ); + const pins = createMemo(() => snapshot(deep(settings.pins))); + createRenderEffect(pins, value => { + rendered.push([...value]); + }); + createRenderEffect( + () => config.ready, + () => {} + ); + return dispose; + }); + flush(); + + try { + // Both refetches join one transition. + setRefreshRequested(true); + flush(); + + // Settings settles first; the config response still holds the render. + settingsResponse.resolve({ pins: ["new pin"] }); + await Promise.resolve(); + flush(); + expect.soft(rendered).toEqual([[]]); + + // An unrelated flush commits the settings store backing. Its deep() + // notification re-enters the held transaction from commitPendingNodes(). + setTick(1); + flush(); + expect.soft(rendered).toEqual([[]]); + + // Completing the last refetch must release the parked render. + configResponse.resolve({ ready: true }); + await Promise.resolve(); + flush(); + expect(rendered).toEqual([[], ["new pin"]]); + } finally { + settingsResponse.resolve({ pins: ["new pin"] }); + configResponse.resolve({ ready: true }); + await Promise.resolve(); + flush(); + dispose(); + flush(); + } +});