fix(web): surface a re-asked frame's error instead of re-asking forever

.changeset/frames-reask-error-by-version.md

@@ -0,0 +1,5 @@
+---
+"@solidjs/web": patch
+---
+
+Fix an unbounded request loop when a server component's re-asked flight fails the same way as the error it replaces. The server writes an escaped render failure as its message string, so two flights that fail alike carry equal payloads; a mount's content node told "already surfaced" from "the next flight's error" by the payload and re-asked on every flight instead of surfacing the error. It now keys a surfaced error by the response version that carried it: `reset()`, and a fresh mount over an errored address (e.g. after a hover preload that failed), make one request and the error reaches the nearest `<Errored>` again.

packages/web/frames/src/client.ts

@@ -292,8 +292,9 @@ function followAddress(host: any, frame: { rebind(address: string): void }, bind
  * error). The error is announced to this node by the mount's frame
  * (`failed`, a tick written from its `onApply`); the node reads the record
  * off the frame bound to the address and surfaces each record once — the
- * applied state of 2.1, keyed by record identity, so a re-read of an error
- * this node already surfaced is not a re-throw but a RE-ASK.
+ * applied state of 2.1, keyed by the response version that carried it, so
+ * a re-read of an error this node already surfaced is not a re-throw but a
+ * RE-ASK, and a re-asked flight's error is a new one however alike.
  *
  * `reset` re-asks: the `<Errored>`'s `reset` recomputes the node that
  * threw — this one — and an errored landing is not a landing for a fresh
@@ -318,22 +319,27 @@ function followAddress(host: any, frame: { rebind(address: string): void }, bind
  * never sees the node go async.
  */
 function landing<T>(host: any, address: string, value: T, failed: () => unknown): () => T {
-  // What the address's store holds at creation is applied: a fresh
-  // consumer of an errored address re-asks, it does not re-throw.
-  let thrown = host.get(address)?.error;
+  // A record is its response's: the version that carried the error names
+  // it, not the payload — the server's is a message string, and two flights
+  // that fail alike carry equal ones. `errored` reads the address: false
+  // when it holds no error, 1 for the error already surfaced, 2 for a new
+  // one (now marked surfaced). What the address's store holds at creation
+  // is applied: a fresh consumer of an errored address re-asks, it does not
+  // re-throw.
+  let frame: any, thrown: number | undefined;
+  const errored = () =>
+    (frame = host.get(address))?.error !== undefined &&
+    (thrown === (thrown = frame.version) ? 1 : 2);
+  errored();
   return createMemo(() => {
     failed();
-    const error = host.get(address)?.error;
-    if (error !== undefined && error !== thrown) {
-      thrown = error;
-      throw error;
-    }
+    const state = errored();
+    if ((state as number) > 1) throw frame.error;
     const wait = host.landing(address);
     if (!wait) return value;
     // An errored address with no flight open (a flight's `start` clears
     // the mounts' error): this read is the re-ask.
-    const asked = error !== undefined ? reask(address) : undefined;
-    return (asked ? asked.then(() => wait) : wait).then(() => value);
+    return (state ? reask(address, wait) : wait).then(() => value);
   });
 }
 
@@ -343,18 +349,18 @@ function landing<T>(host: any, address: string, value: T, failed: () => unknown)
  * server-function client hands its response handler the call it dispatched
  * (or answered locally) as a thunk: the same reference, arguments, declared
  * shape and per-call options, so a `GET`-declared read stays a GET by
- * construction. Resolves when the call's response has been handled: the
- * flight is open and lands through the host. `undefined` for an address no
- * call is recorded for.
+ * construction. Resolves to `wait` (the address's next landing) once the
+ * call's response has been handled: the flight is open and lands through
+ * the host. `wait` itself for an address no call is recorded for.
  */
-function reask(address: string): Promise<unknown> | undefined {
+function reask<T>(address: string, wait: Promise<T>): Promise<T> {
   const call = callFor(address);
   if (IS_DEV && !call)
     console.error(
       `Server component boundary "${address}" errored, but no call is recorded for it; ` +
         `reset() cannot re-ask the server. (The address was written by hand, not by a call.)`
     );
-  return call && call.retry();
+  return call ? call.retry().then(() => wait) : wait;
 }
 
 /**

packages/web/test/frames-reask-loop.spec.tsx

@@ -0,0 +1,175 @@
+/**
+ * @jsxImportSource @solidjs/web
+ * @vitest-environment jsdom
+ */
+// A re-ask is one request (see frames-errored-reset-refetch.spec.tsx for
+// the contract): `reset`, or a fresh consumer of an errored address, opens
+// ONE new flight, and that flight's error surfaces like any other — it is
+// not the error the node already surfaced, whatever its payload.
+//
+// The server writes an escaped render failure as an unkeyed error whose
+// payload is the sanitized MESSAGE (`sink.error("", message)`), a string.
+// Two flights that fail the same way carry equal strings; telling "the
+// error already surfaced" from "the next flight's error" by the payload
+// read every re-asked flight's error as the old one and re-asked again, a
+// request per flight with no end.
+import { afterEach, describe, expect, test, vi } from "vitest";
+import { createRoot, Errored, Loading, resetErrorHalt, type Component } from "solid-js";
+import { dynamic, dynamicComponent } from "../src/index.js";
+import { installServerComponents } from "../frames/src/client.js";
+import { createServerReference, GET } from "../server-functions/src/client.js";
+import { makeHost, frameResponse, pump } from "./lifecycle-matrix/harness.js";
+
+/** The server's shape for a component that threw: start, the message, complete. */
+const errored = (id: string, message: string) =>
+  frameResponse(id, [
+    { type: "start", id, version: 1 },
+    { type: "error", id, version: 1, error: message },
+    { type: "complete", id, version: 1 }
+  ]);
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  vi.restoreAllMocks();
+  resetErrorHalt();
+});
+
+function mount(code: () => any) {
+  const container = document.createElement("div");
+  document.body.appendChild(container);
+  let div!: HTMLDivElement;
+  const dispose = createRoot(d => {
+    <div ref={div}>{code()}</div>;
+    container.appendChild(div);
+    return d;
+  });
+  return {
+    div,
+    cleanup() {
+      dispose();
+      container.remove();
+    }
+  };
+}
+
+/**
+ * Every request fails the same way. Past `cap` the stub never answers, so
+ * a runaway re-ask stops at a count the assertions can read.
+ */
+function failingServer(message: string, cap = 25) {
+  const counter = { calls: 0 };
+  vi.stubGlobal("fetch", async () => {
+    counter.calls++;
+    if (counter.calls > cap) return new Promise<Response>(() => {});
+    return errored("srv", message);
+  });
+  return counter;
+}
+
+const VIA: ReadonlyArray<[string, (source: () => any) => Component<any>]> = [
+  ["dynamic", dynamic],
+  ["dynamicComponent", dynamicComponent]
+];
+
+describe.each(VIA)("a re-ask is one request — via %s", (_via, dyn) => {
+  test("reset() re-asks once, and the re-asked flight's equal error surfaces again", async () => {
+    const { host } = makeHost();
+    installServerComponents(host);
+    const server = failingServer("boom");
+    const getUser = createServerReference("reask-loop/reset");
+    const Page = dyn(() => getUser() as any);
+    let reset: (() => void) | undefined;
+    const m = mount(() => (
+      <Errored
+        fallback={(err, r) => {
+          reset = r;
+          return <span class="err">failed: {String(err())}</span>;
+        }}
+      >
+        <Loading fallback={<span class="shell">loading</span>}>
+          <Page />
+        </Loading>
+      </Errored>
+    ));
+    await pump();
+    expect(server.calls).toBe(1);
+    expect(m.div.querySelector(".err")!.textContent).toBe("failed: boom");
+
+    for (const asked of [2, 3]) {
+      reset!();
+      await pump(20);
+      expect(server.calls).toBe(asked);
+      // Surfaced: the boundary is back on its fallback, not pending on the
+      // re-asked flight behind the <Loading>.
+      expect(m.div.querySelector(".err")!.textContent).toBe("failed: boom");
+      expect(m.div.querySelector(".shell")).toBeNull();
+      await pump(20);
+      expect(server.calls).toBe(asked);
+    }
+    m.cleanup();
+  });
+
+  test("a mount over an errored preload surfaces the next flight's equal error, then stops asking", async () => {
+    const { host } = makeHost();
+    installServerComponents(host);
+    const server = failingServer("boom");
+    const getUser = GET(createServerReference("reask-loop/preload"));
+    // The router's hover preload: the call made ahead of the mount.
+    void Promise.resolve(getUser()).catch(() => {});
+    await pump();
+    expect(server.calls).toBe(1);
+
+    const Page = dyn(() => getUser() as any);
+    const m = mount(() => (
+      <Errored fallback={err => <span class="err">failed: {String(err())}</span>}>
+        <Loading fallback={<span class="shell">loading</span>}>
+          <Page />
+        </Loading>
+      </Errored>
+    ));
+    await pump(20);
+    expect(m.div.querySelector(".err")!.textContent).toBe("failed: boom");
+    const surfaced = server.calls;
+    // The mount's call and at most one re-ask behind it.
+    expect(surfaced).toBeLessThanOrEqual(3);
+    await pump(40);
+    expect(server.calls).toBe(surfaced);
+    m.cleanup();
+  });
+});
+
+test("with no <Errored>, a mount over an errored preload halts on the next flight's equal error instead of re-asking", async () => {
+  // The halt's rethrow out of scheduled flushes (see the reset spec's (d)).
+  const queue = queueMicrotask;
+  vi.stubGlobal("queueMicrotask", (fn: () => void) =>
+    queue(() => {
+      try {
+        fn();
+      } catch {}
+    })
+  );
+  vi.stubGlobal("reportError", () => {});
+  const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
+  const { host } = makeHost();
+  installServerComponents(host);
+  const server = failingServer("boom");
+  const getUser = GET(createServerReference("reask-loop/uncaught"));
+  void Promise.resolve(getUser()).catch(() => {});
+  await pump();
+
+  const Page = dynamicComponent(() => getUser() as any);
+  const m = mount(() => (
+    <Loading fallback={<span class="shell">loading</span>}>
+      <Page />
+    </Loading>
+  ));
+  await pump(20);
+  const surfaced = server.calls;
+  expect(surfaced).toBeLessThanOrEqual(3);
+  expect(consoleError.mock.calls.some(args => /REACTIVITY_HALTED/.test(String(args[0])))).toBe(
+    true
+  );
+  await pump(40);
+  expect(server.calls).toBe(surfaced);
+  m.cleanup();
+});