Close the ambient transaction window when a transaction opens without writes (#3141)

.changeset/fix-lingering-ambient-transaction-capture.md

@@ -0,0 +1,5 @@
+---
+"@solidjs/signals": patch
+---
+
+Fix unrelated async work being captured by a lingering ambient transaction (#3141). Parking is flush-driven, but a transaction opened without any writes — an action whose first statements only await — scheduled nothing, so `activeTransition` and the adopted batch stayed armed across the async gap. The next unrelated work to arrive was adopted into a transaction it had nothing to do with: an optimistic store's authoritative landing would not render until the stranger action settled, an unowned optimistic write rode that transaction instead of reverting at the flush, and `deep()`/per-key readers disagreed about the committed value in the meantime. `initTransition` now guarantees a flush, so the ambient window closes in one flush regardless of whether the transaction wrote anything — enforcing the A26 containment ruling.

packages/signals/INTERNALS-ASYNC-STATE.md

@@ -50,6 +50,7 @@ Semantics of the `(_pendingValue, _overrideValue)` pair for an optimistic node
 ## 3. Transitions (`scheduler.ts`)
 
 - Created by `initTransition` on the first transition-worthy write; at most one `activeTransition` per flush; concurrent ones merge (`mergeTransitionState`, `_done` forwarding pointer).
+- `initTransition` ends by scheduling a flush: the ambient window is one flush by definition, but parking is flush-driven, so a transaction opened with no writes (an action that only awaits) would otherwise leave `activeTransition` and the adopted batch armed across the async gap, capturing the next unrelated work to arrive — the A26-rejected behavior (#3141).
 - `_asyncReporters: Map<source, Set<reporter>>` — which computeds are blocked on which async sources. **Populated only from `GlobalQueue.notify` during render-effect status notification** `[ruled — async-registration-invariants rule]`.
 - `_pendingNodes` — nodes whose `_pendingValue` commits when the transition completes (`commitPendingNodes` → `commitPendingNode`).
 - `_optimisticNodes` — nodes whose override reverts at completion (`resolveOptimisticNodes`).

packages/signals/src/core/scheduler.ts

@@ -717,6 +717,15 @@ export class GlobalQueue extends Queue {
     for (const lane of activeLanes) {
       if (!lane._transition) lane._transition = activeTransition;
     }
+    // A transaction's ambient window is one flush. Entering must therefore
+    // guarantee a flush: a transaction opened with no writes (an action whose
+    // first statements only await) otherwise leaves activeTransition and the
+    // adopted batch armed across the async gap, and the next unrelated work
+    // to arrive — an optimistic store's authoritative landing, a plain async
+    // settle — is adopted into a transaction it has nothing to do with
+    // (#3141). The scheduled flush parks the incomplete transaction through
+    // the normal machinery and detaches the ambient slots first.
+    schedule();
   }
 }
 

packages/signals/tests/router-transition-error-commit.test.ts

@@ -0,0 +1,61 @@
+import {
+  createEffect,
+  createErrorBoundary,
+  createMemo,
+  createRenderEffect,
+  createRoot,
+  createSignal,
+  flush
+} from "../src/index.js";
+
+afterEach(() => flush());
+
+it("commits a source write and its effect when a downstream async error is handled", async () => {
+  const error = new Error("lazy route failed");
+  let reject!: (error: unknown) => void;
+  const [route, setRoute] = createSignal("/", { ownedWrite: true });
+  let rendered = "";
+  let external = "/";
+
+  createRoot(() => {
+    const lazyRoute = createMemo(() => {
+      if (route() === "/") return "home";
+      return new Promise<string>((_, fail) => (reject = fail));
+    });
+    const boundary = createErrorBoundary(
+      () => lazyRoute(),
+      caught => {
+        expect(caught()).toBe(error);
+        return "error";
+      }
+    );
+    createRenderEffect(boundary, value => {
+      rendered = value;
+    });
+    createEffect(
+      route,
+      value => {
+        external = value;
+      },
+      { defer: true }
+    );
+  });
+
+  flush();
+  expect(rendered).toBe("home");
+
+  setRoute("/plugins");
+  flush();
+  expect(route()).toBe("/");
+  expect(rendered).toBe("home");
+  expect(external).toBe("/");
+
+  reject(error);
+  await Promise.resolve();
+  await Promise.resolve();
+  flush();
+
+  expect(rendered).toBe("error");
+  expect(route()).toBe("/plugins");
+  expect(external).toBe("/plugins");
+});

packages/signals/tests/store/optimistic-ambient-capture.test.ts

@@ -0,0 +1,136 @@
+/**
+ * #3141: a transaction's ambient window must be one flush.
+ *
+ * `initTransition` used to leave `activeTransition` (and the adopted batch)
+ * armed indefinitely when the transaction was opened without any writes — an
+ * action whose first statements only await schedules nothing, so no flush
+ * ever parked it. The next unrelated work to arrive was adopted into a
+ * transaction it had nothing to do with: an optimistic store's authoritative
+ * landing (with its flight still pending, so its live lane was entangled)
+ * would not render until the stranger action settled seconds later, an
+ * unowned optimistic write rode the same transaction instead of reverting,
+ * and deep()/per-key readers split-brained over what "committed" meant
+ * meanwhile.
+ *
+ * The construction here is exact and deterministic:
+ *  - the action is invoked in a bare macrotask and performs no writes before
+ *    its first await, so pre-fix nothing scheduled a flush and the
+ *    transaction stayed ambient;
+ *  - the store generator keeps running after the yield (awaiting a further
+ *    gate), so the landing arrives on a still-live lane — the entanglement
+ *    target. (A generator that ends at the yield resolves its flight and
+ *    escapes the capture, which is why simpler distillations don't fail.)
+ * No manual flush() between steps — a manual flush parks the transaction and
+ * masks the window. The scheduler's own flushing must do the right thing.
+ */
+import { describe, expect, it } from "vitest";
+import {
+  action,
+  createOptimisticStore,
+  createRenderEffect,
+  createRoot,
+  deep,
+  flush
+} from "../../src/index.js";
+
+function deferred<T = void>() {
+  let resolve!: (v: T) => void;
+  const promise = new Promise<T>(r => (resolve = r));
+  return { promise, resolve };
+}
+
+// A macrotask hop: lets the scheduler's own flush run without forcing one.
+const hop = () => new Promise(r => setTimeout(r, 0));
+
+describe("#3141: lingering ambient transaction capture", () => {
+  it("an in-flight store landing renders immediately despite an unrelated open action", async () => {
+    const stepGate = deferred();
+    const endGate = deferred();
+    const actGate = deferred();
+    const domLog: string[] = [];
+    const obsLog: string[] = [];
+    let setState!: (fn: (s: number[]) => void) => void;
+    let dispose!: () => void;
+
+    const act = action(function* (p: Promise<void>) {
+      yield p;
+    });
+
+    createRoot(d => {
+      dispose = d;
+      const [s, ss] = createOptimisticStore<number[]>(
+        async function* () {
+          yield [1, 2, 3];
+          await stepGate.promise;
+          yield [3, 2, 1];
+          // The flight stays pending past the yield: the landing arrives on a
+          // live lane, which is what the lingering transaction entangled.
+          await endGate.promise;
+        },
+        [1, 2]
+      );
+      setState = ss;
+
+      createRenderEffect(
+        () => JSON.stringify(s),
+        v => {
+          domLog.push(v);
+        }
+      );
+      createRenderEffect(
+        () => deep(s),
+        v => {
+          obsLog.push(JSON.stringify(v));
+        }
+      );
+    });
+
+    await hop();
+    expect(domLog.at(-1)).toBe("[1,2,3]");
+
+    // Open the transaction in a bare macrotask: no writes precede its first
+    // await, so nothing else will schedule the flush that parks it.
+    let acting!: Promise<unknown>;
+    setTimeout(() => {
+      acting = act(actGate.promise);
+    }, 0);
+    await hop();
+    await hop();
+
+    // The store's own truth lands mid-action: it must render now — not when
+    // the unrelated action settles — and both reader families must agree.
+    stepGate.resolve();
+    await hop();
+    await hop();
+    expect(domLog.at(-1)).toBe("[3,2,1]");
+    expect(obsLog.at(-1)).toBe("[3,2,1]");
+
+    // An unowned optimistic push composes over the CURRENT base and, with no
+    // transaction of its own, reverts at the flush — it must not be adopted
+    // by the open action and persist until that action settles.
+    setState(draft => {
+      draft.push(1111);
+    });
+    await hop();
+    expect(domLog.at(-1)).toBe("[3,2,1]");
+    expect(obsLog.at(-1)).toBe("[3,2,1]");
+    expect(domLog).not.toContain("[1,2,3,1111]"); // the split-brain composition
+
+    // At no point may the two reader families disagree about the value. The
+    // observer may legitimately fire more often (its initial run doubles), so
+    // compare deduped value sequences: pre-fix the DOM showed [1,2,3,1111]
+    // while deep() reported [3,2,1,1111].
+    const dedupe = (log: string[]) => log.filter((v, i) => v !== log[i - 1]);
+    expect(dedupe(obsLog)).toEqual(dedupe(domLog));
+
+    actGate.resolve();
+    await acting;
+    await hop();
+    expect(domLog.at(-1)).toBe("[3,2,1]");
+
+    endGate.resolve();
+    await hop();
+    dispose();
+    flush();
+  });
+});