fix(web/frames): classify an adopted occurrence only after every delivered record has drained (C18)

.changeset/frames-c18-classify-after-drain.md

@@ -0,0 +1,5 @@
+---
+"@solidjs/web": patch
+---
+
+Frames client: a recordless adopted occurrence is classified only after every delivered document record has drained (frames-rulings 3.5, proposed; consistency contract C18 / red R9). `adoptBoundary.recordsPending` gains a third term — `_$HY.r` still holds a slot or region record for the boundary that `drainRecords` has not applied — so "pending" is the drain's state, not only the parser's. Before, two records drained after the parser finished classified each other: the deferred drain applies one record per `host.apply`, each a synchronous frame sync, and that first sync found the second occurrence recordless with `document.readyState` no longer "loading", classified it direct-insert, and evaluated its render prop as a zero-arg accessor — a `TypeError` on the props read halted the reactive system. The same window was reachable from a live-hole op and from the live pump's catch-up read. Now such a sync defers the occurrence and the drain's next apply mounts it with its args.

packages/web/frames/src/client.ts

@@ -1331,6 +1331,11 @@ function adoptBoundary(
   // recordless occurrence it deferred (#2968 — the frame's recordsPending/
   // drainRecords seam below).
   const appliedRecords = new Set<string>();
+  // The document keys this boundary's records under the wire name: slot
+  // records as `sc:slot:<id>:<occurrence>`, nested regions as
+  // `sc:region:<id>.<occurrence>.<key>`.
+  const slotPrefix = `sc:slot:${id}:`;
+  const regionPrefix = `sc:region:${id}.`;
   // Deferred fragments in the adopted markup (#2978): a <Loading> that
   // suspended inside the server component during document SSR left a `pl-*`
   // placeholder here, but its producer ran on the SERVER — no client
@@ -1361,7 +1366,9 @@ function adoptBoundary(
     // drain normally starts the pump; attempted on every re-drain anyway —
     // idempotent, and a defensive catch for a record that lands late.
     pumpLiveChannel();
-    const slotPrefix = `sc:slot:${id}:`;
+    // Each record is marked applied BEFORE its `host.apply`: that apply
+    // syncs the frame, and the sync's `recordsPending` must read the other
+    // delivered records as still pending while this one is no longer (3.5).
     for (const key of Object.keys(hy.r)) {
       if (appliedRecords.has(key)) continue;
       if (key.startsWith(slotPrefix)) {
@@ -1375,19 +1382,17 @@ function adoptBoundary(
           key: key.slice(slotPrefix.length),
           args: hy.r[key]
         });
-      } else if (key.startsWith("sc:region:")) {
+      } else if (key.startsWith(regionPrefix)) {
+        appliedRecords.add(key);
+        // Async-occluded regions arrive as promises (the producer held
+        // the stream on them); regions keep their producer-relative ids
+        // (the records reference them by those), and the store warms per
+        // id either way, so a late apply still lands before the region
+        // binds on expand.
         const childId = key.slice("sc:region:".length);
-        if (childId.startsWith(id + ".")) {
-          appliedRecords.add(key);
-          // Async-occluded regions arrive as promises (the producer held
-          // the stream on them); regions keep their producer-relative ids
-          // (the records reference them by those), and the store warms per
-          // id either way, so a late apply still lands before the region
-          // binds on expand.
-          const val = hy.r[key];
-          const apply = (html: any) => host.apply({ type: "html", id: childId, version: 0, html });
-          val && typeof val.then === "function" ? val.then(apply) : apply(val);
-        }
+        const val = hy.r[key];
+        const apply = (html: any) => host.apply({ type: "html", id: childId, version: 0, html });
+        val && typeof val.then === "function" ? val.then(apply) : apply(val);
       }
     }
   };
@@ -1473,23 +1478,44 @@ function adoptBoundary(
     ownerScope: boundaryScope(owner),
     reveal: revealSeam(owner),
     onApply: settle,
-    // May the document still run scripts that assign records? While the
-    // parser is running the answer is yes, and a held fragment's replay can
-    // still deliver one — so a recordless occurrence defers instead of
-    // misclassifying as content (the runtime re-checks until this flips
-    // false). Deliberately NOT boundaryMayArrive(): its `!_$HY.done` term
-    // answers a different question (can this boundary's ELEMENT still
-    // appear), and holding classification until client hydration completes
-    // pushes the adopted mount past the hydrate window — the claim then
-    // adopts markup the client's state has already moved past (the
-    // adopted-slot-live spec pins the working ordering).
+    // Is a record for this boundary still to come — or here and not yet
+    // applied? While the parser is running the document can still run a
+    // data script, a held fragment's replay can still deliver one, and a
+    // record whose script already ran sits in `_$HY.r` until the drain
+    // moves it into the store — so a recordless occurrence defers instead
+    // of misclassifying as content (the runtime re-checks until this flips
+    // false). The third term is frames-rulings 3.5 (contract C18/R9): an
+    // occurrence is classified only after every delivered record has
+    // drained — "pending" is the drain's state, not the parser's. Without
+    // it, two records drained after the parser finished classify each
+    // other: the first apply's sync finds the second recordless with the
+    // parser done, evaluates its render prop argless, and the `TypeError`
+    // halts the reactive system. Deliberately NOT boundaryMayArrive(): its
+    // `!_$HY.done` term answers a different question (can this boundary's
+    // ELEMENT still appear), and holding classification until client
+    // hydration completes pushes the adopted mount past the hydrate window
+    // — the claim then adopts markup the client's state has already moved
+    // past (the adopted-slot-live spec pins the working ordering).
     // Spread-cast: the published FrameOptions predates this seam; a runtime
     // without it simply never calls the hooks (drop once the pin catches up).
     ...({
       recordsPending: () => {
         if (document.readyState === "loading") return true;
         const hy = (globalThis as any)._$HY;
-        return !!(hy && hy.fr && hy.fr.pending());
+        if (!hy) return false;
+        if (hy.fr && hy.fr.pending()) return true;
+        // Delivered and undrained: a record for this boundary whose data
+        // script ran (`_$HY.r` has the key) that the drain has not moved
+        // into the store yet. "Recordless" reads the store, and the parser's
+        // state says nothing about this gap — the record sits one loop
+        // iteration from applying while a sync runs.
+        for (const key in hy.r)
+          if (
+            !appliedRecords.has(key) &&
+            (key.startsWith(slotPrefix) || key.startsWith(regionPrefix))
+          )
+            return true;
+        return false;
       },
       drainRecords,
       // The identity split binds the frame to the call ADDRESS (id + args

packages/web/frames/src/frame-client.ts

@@ -347,7 +347,10 @@ export interface FrameOptions {
    * occurrence is ambiguous while this returns true: the frame defers its
    * mount one macrotask (all currently parsed scripts run first), calls
    * `drainRecords`, and classifies with whatever is then resolvable. Return
-   * false once the document can run no further data scripts.
+   * false once the document can run no further data scripts AND every
+   * record it has already delivered has been drained (frames-rulings 3.5):
+   * a record that executed but has not been applied yet is pending too — a
+   * sync in that window must defer, not classify.
    */
   recordsPending?(): boolean;
   /** Re-absorb the document's arrived-by-now records (idempotent per key). */
@@ -1312,7 +1315,11 @@ class FrameImpl {
         // bounded by the same contract as everything else here:
         // recordsPending flips false when the document completes with no
         // fragment left to reveal (truncation included — the ledger rejects
-        // stragglers). Deferral is invisible on screen: an adopted
+        // stragglers) and no delivered record left undrained (frames-
+        // rulings 3.5: the drain itself syncs once per record it applies,
+        // and that sync must not classify the records still in its loop —
+        // nor may a live op's sync in the same window). Deferral is
+        // invisible on screen: an adopted
         // occurrence's server-rendered interior is already in the DOM; the
         // mount is the hydration attach. Full syncs only: a scoped segment
         // fill renders into a detached fragment a later full sync can't

packages/web/test/consistency/c18-classify-after-drain.spec.tsx

@@ -0,0 +1,253 @@
+/**
+ * @jsxImportSource @solidjs/web
+ * @vitest-environment jsdom
+ *
+ * C18 — classification waits for the drain.
+ *
+ * "A recordless adopted occurrence is classified (direct-insert vs invoked)
+ * only after every record the document already holds for the boundary has
+ * been applied: no sync that runs between the parser's end and the deferred
+ * drain — the drain's own first `host.apply`, a live op, the live pump's
+ * catch-up read — may evaluate a render prop as a zero-arg accessor."
+ *
+ * Ruling (frames-rulings 3.5, proposed): "an occurrence is classified only
+ * after every delivered record has drained" — "pending" is the DRAIN's
+ * state, not the parser's. The #2968 defer's bound was `recordsPending()` =
+ * parser running or a fragment pending; a record whose data script already
+ * ran sat in `_$HY.r` until the deferred `drainRecords` moved it into the
+ * store, and nothing read that gap (contract §Red R9).
+ *
+ * Mechanism meant to carry it: frames/src/client.ts
+ * `adoptBoundary.recordsPending`'s third term — `_$HY.r` holds a key under
+ * the boundary's `sc:slot:<id>:` / `sc:region:<id>.` prefix that is not yet
+ * in `appliedRecords` — read by frames/src/frame-client.ts `#syncSlots`'
+ * defer arm. `drainRecords` marks a key applied BEFORE its `host.apply`, so
+ * the sync that apply runs sees the other delivered records as pending and
+ * its own as drained.
+ *
+ * Observation: the fills here are REAL — `p => <li>{p.text}{tick()}</li>`,
+ * the props read unguarded (plus one untracked identification read, as a
+ * fill's top-level read is otherwise a STRICT_READ diagnostic). Classified
+ * direct-insert, the render prop is evaluated as a zero-arg accessor inside
+ * the insert effect: `p.text` is a `TypeError` and the reactive system
+ * halts (`REACTIVITY_HALTED`). The pin
+ * asserts the opposite: every occurrence invoked once with its args, the
+ * server `<li>` claimed in place, no error, and the page still reactive
+ * after the `tick` bump. The harness's replay pins (`harness/replay.spec.tsx`
+ * C18 ×3) hold the same three orders through the oracle's tolerant fill;
+ * this file pins the consequence for a real one.
+ */
+import { afterEach, describe, expect, test, vi } from "vitest";
+import { createSignal, flush, untrack } from "solid-js";
+import { hydrate } from "@solidjs/web";
+import {
+  bootPage,
+  fillHtml2,
+  frameHtml,
+  freshFid,
+  holeHtml,
+  microtasks,
+  quiesce,
+  slotRange,
+  type Page
+} from "./support.js";
+
+let page: Page | undefined;
+afterEach(async () => {
+  await page?.cleanup();
+  page = undefined;
+});
+
+/** The server render of `p => <li>{p.text}{tick()}</li>` at tick 0. */
+const liveFillHtml = (fid: string, occ: string, text: string) => fillHtml2(fid, occ, text, "0");
+
+/**
+ * The parser's clock: `document.readyState` reads "loading" until `done()`
+ * — the records the document still owes execute while it is running, and
+ * the response's tail (the last data script, then the end) parses in one
+ * go before any timer fires, so the restore is synchronous with the last
+ * record.
+ */
+function parserRunning() {
+  const spy = vi.spyOn(document, "readyState", "get").mockReturnValue("loading");
+  return () => spy.mockRestore();
+}
+
+describe("C18 — classification waits for the drain", () => {
+  // Arm (a): the drain's own first apply. Two render-prop occurrences; both
+  // records owed when the boundary adopts (the #2968 defer arms); both
+  // execute, the parser finishes, THEN the deferred drain fires. Its first
+  // `host.apply` syncs the frame while the second record is still one loop
+  // iteration away in `_$HY.r`: that sync must defer item#1, not classify
+  // it — the loop's next apply mounts it with its args.
+  test("(a) two records drained after the parser finished: each occurrence claims with its args; nothing is evaluated argless", async () => {
+    const fid = freshFid("c18a");
+    const parserDone = parserRunning();
+    page = bootPage(
+      frameHtml(
+        fid,
+        `<ul>${slotRange("item#0", liveFillHtml(fid, "item#0", "p0"))}${slotRange(
+          "item#1",
+          liveFillHtml(fid, "item#1", "p1")
+        )}</ul>`
+      )
+    );
+    const serverLis = [...page.container.querySelectorAll("li")];
+    const Comp = (globalThis as any)._$SC.r(fid);
+    const [tick, setTick] = createSignal(0);
+    const invoked: string[] = [];
+    const dispose = hydrate(
+      () => (
+        <Comp
+          item={(p: { text: string }) => {
+            invoked.push(untrack(() => p.text));
+            return (
+              <li>
+                {p.text}
+                {tick()}
+              </li>
+            );
+          }}
+        />
+      ),
+      page.container
+    );
+    // Adopted with the parser running: both occurrences deferred, nothing
+    // invoked, the server markup untouched.
+    expect(invoked).toEqual([]);
+    expect(page.container.textContent).toBe("p0" + "0" + "p1" + "0");
+
+    // The document's tail: both data scripts, then the end of the response
+    // — before the deferred drain's macrotask.
+    page.slotRecord(fid, "item#0", { text: "p0" });
+    page.slotRecord(fid, "item#1", { text: "p1" });
+    parserDone();
+    expect(invoked).toEqual([]);
+
+    await quiesce();
+    await quiesce();
+    expect(invoked.sort()).toEqual(["p0", "p1"]);
+    expect([...page.container.querySelectorAll("li")]).toEqual(serverLis);
+    expect(page.container.textContent).toBe("p0" + "0" + "p1" + "0");
+    setTick(1);
+    flush();
+    expect(page.container.textContent).toBe("p0" + "1" + "p1" + "1");
+    expect(page.warnings).toEqual([]);
+    expect(page.errors).toEqual([]);
+    dispose();
+  });
+
+  // Arm (b): a sync the drain did not trigger, in the same window. One
+  // occurrence and a live hole; the record executes and the parser finishes
+  // while the defer is armed; a live hole op then lands — the pump's read is
+  // a microtask, the drain a macrotask — and its `host.apply` syncs the frame
+  // over the undrained record. That sync must defer the occurrence; the
+  // drain's apply mounts it. (The pump's catch-up read over ops logged
+  // before adoption is the same sync from the other side of adoption —
+  // `harness/replay.spec.tsx`'s third C18 pin.)
+  test("(b) a live op syncs the frame between the parser's end and the drain: the occurrence defers, then claims with its args", async () => {
+    const fid = freshFid("c18b");
+    const parserDone = parserRunning();
+    page = bootPage(
+      frameHtml(
+        fid,
+        `<ul>${slotRange("item#0", liveFillHtml(fid, "item#0", "p0"))}</ul><p>${holeHtml(
+          18,
+          "hole-v0"
+        )}</p>`
+      )
+    );
+    const serverLi = page.container.querySelector("li")!;
+    const Comp = (globalThis as any)._$SC.r(fid);
+    const [tick, setTick] = createSignal(0);
+    const invoked: string[] = [];
+    const dispose = hydrate(
+      () => (
+        <Comp
+          item={(p: { text: string }) => {
+            invoked.push(untrack(() => p.text));
+            return (
+              <li>
+                {p.text}
+                {tick()}
+              </li>
+            );
+          }}
+        />
+      ),
+      page.container
+    );
+    expect(invoked).toEqual([]);
+
+    page.slotRecord(fid, "item#0", { text: "p0" });
+    parserDone();
+    // The live op's sync lands on the pump's microtask read — before the
+    // deferred drain's macrotask.
+    page.live.push({ type: "hole", key: "lh:18", html: "hole-v1" });
+    await microtasks(4);
+    expect(page.container.querySelector("p")!.textContent).toBe("hole-v1");
+    expect(invoked).toEqual([]);
+
+    await quiesce();
+    await quiesce();
+    expect(invoked).toEqual(["p0"]);
+    expect(page.container.querySelector("li")).toBe(serverLi);
+    expect(page.container.textContent).toBe("p0" + "0" + "hole-v1");
+    setTick(1);
+    flush();
+    expect(page.container.textContent).toBe("p0" + "1" + "hole-v1");
+    expect(page.warnings).toEqual([]);
+    expect(page.errors).toEqual([]);
+    dispose();
+  });
+
+  // Control: a tick between the two records — the drain runs while the
+  // parser is still owed the second, every sync reads `recordsPending()`
+  // true through the parser's term alone. Green before and after 3.5.
+  test("control: a drain per record while the parser is still running classifies nothing early", async () => {
+    const fid = freshFid("c18c");
+    const parserDone = parserRunning();
+    page = bootPage(
+      frameHtml(
+        fid,
+        `<ul>${slotRange("item#0", liveFillHtml(fid, "item#0", "p0"))}${slotRange(
+          "item#1",
+          liveFillHtml(fid, "item#1", "p1")
+        )}</ul>`
+      )
+    );
+    const Comp = (globalThis as any)._$SC.r(fid);
+    const [tick, setTick] = createSignal(0);
+    const invoked: string[] = [];
+    const dispose = hydrate(
+      () => (
+        <Comp
+          item={(p: { text: string }) => {
+            invoked.push(untrack(() => p.text));
+            return (
+              <li>
+                {p.text}
+                {tick()}
+              </li>
+            );
+          }}
+        />
+      ),
+      page.container
+    );
+    page.slotRecord(fid, "item#0", { text: "p0" });
+    await quiesce();
+    expect(invoked).toEqual(["p0"]);
+    page.slotRecord(fid, "item#1", { text: "p1" });
+    parserDone();
+    await quiesce();
+    await quiesce();
+    expect(invoked).toEqual(["p0", "p1"]);
+    setTick(1);
+    flush();
+    expect(page.container.textContent).toBe("p0" + "1" + "p1" + "1");
+    expect(page.warnings).toEqual([]);
+    expect(page.errors).toEqual([]);
+    dispose();
+  });
+});