Stop discarding the render failure's cause, and refuse startingLakePositions (#227)

src/components/ElevationPreviewPanel.vue

@@ -248,8 +248,13 @@ async function generate() {
       // blit) - that is the latency a user feels, and what tiling had to move.
       elapsedMs.value = Math.round(performance.now() - startedAt);
     }
-  } catch {
-    error.value = "Preview failed.";
+  } catch (e) {
+    // The real cause, not a constant. A bare `catch` here discarded every
+    // render failure's message - issue #341 - and produced the same
+    // "Preview failed." that `PreviewPanel.vue` produces from the unrelated
+    // Docker preview service, which is what made the two hard to tell apart.
+    // Same idiom as `IslandFinderPanel.vue`.
+    error.value = e instanceof Error ? e.message : "Preview failed.";
   } finally {
     loading.value = false;
   }

src/components/useElevationPreview.ts

@@ -128,6 +128,25 @@ export function createWorkerHost(
     }
   }
 
+  /**
+   * The cause out of a worker `error` event.
+   *
+   * This used to take no argument at all and substitute the constant below, so
+   * a worker that died of a bad import, a syntax error or an out-of-memory
+   * reported the same six words as one that died of anything else. `ErrorEvent`
+   * carries `message`; a worker whose script failed to LOAD reports an empty
+   * one, and a test's fake carries whatever it likes - both fall back to the
+   * bare constant rather than to an empty string, which would read as no error
+   * at all.
+   */
+  function workerErrorMessage(e: unknown): string {
+    const raw =
+      typeof e === "object" && e !== null && "message" in e
+        ? String((e as { message: unknown }).message)
+        : "";
+    return raw === "" ? "Elevation render worker error" : `Elevation render worker error: ${raw}`;
+  }
+
   function dropWorker(slot: number) {
     const w = workers[slot];
     workers[slot] = null;
@@ -157,9 +176,9 @@ export function createWorkerHost(
     };
     // A worker crash would otherwise leave its tiles' promises pending forever.
     // Every tile this slot holds fails, not just the newest.
-    w.onerror = () => {
+    w.onerror = (e: unknown) => {
       dropWorker(slot);
-      rejectSlot(slot, "Elevation render worker error");
+      rejectSlot(slot, workerErrorMessage(e));
     };
     workers[slot] = w;
     return w;

src/noise/preview/elevationRender.worker.ts

@@ -82,8 +82,27 @@ function serve(req: ElevationRenderRequest): void {
     self.postMessage(message);
     return;
   }
-  const result = runRenderRequest(req, engine);
-  self.postMessage(result, [result.buffer]);
+  // A render that throws must be REPORTED, not allowed to escape.
+  //
+  // Letting it escape `onmessage` fires the worker's `error` event, and the
+  // host treats that as a crashed worker: it terminates the slot and rejects
+  // every tile the slot holds. So one bad request - a view the module refuses,
+  // a spawn list over the ABI cap, a `startingLakePositions` override - failed
+  // every OTHER render in flight beside it, and did so with a constant string
+  // that named none of those causes.
+  //
+  // Catching it here settles exactly the request that failed, with the reason,
+  // and leaves the worker and its siblings alone.
+  try {
+    const result = runRenderRequest(req, engine);
+    self.postMessage(result, [result.buffer]);
+  } catch (err) {
+    const message: RenderErrorMessage = {
+      id: req.id,
+      error: err instanceof Error ? err.message : String(err),
+    };
+    self.postMessage(message);
+  }
 }
 
 self.onmessage = (e: MessageEvent<ElevationRenderRequest | EngineMessage>) => {

src/noise/preview/elevationRenderRequest.ts

@@ -55,7 +55,15 @@ export interface ElevationRenderRequest {
    * Vulcanus terrain colors rather than a Nauvis field composited on top.
    */
   planet?: Planet;
-  /** Omitted => the game's real lake positions are computed inside the render. */
+  /**
+   * **Refused.** Kept on the interface only because `eval/ctx.ts` and
+   * `expressions/elevationIsland.ts` still carry the field, so the type outlives
+   * every module that acted on it. Passing one - including `[]`, which used to
+   * mean "far-field only" - throws
+   * `STARTING_LAKE_POSITIONS_UNSUPPORTED`. The lake positions are derived inside
+   * the render from the seed and the starting positions, which is the game's own
+   * rule.
+   */
   startingLakePositions?: Point[];
   /**
    * Climate controls (Task 12b) - consumed only when `view: "terrain"`; the
@@ -402,13 +410,14 @@ function renderFulgoraThroughWasm(
  * **`"elevation"` is ported as of #227**, as three `view` codes rather than
  * one, because the common prefix has no `mapType` field. See the gate at the
  * tail of `runRenderRequest`, which still keeps two cases on the TypeScript
- * path: a caller-supplied `startingLakePositions`, and a non-Nauvis `planet`.
+ * path: a spawn list over the ABI cap, and a non-Nauvis `planet`.
  *
- * **A caller-supplied `startingLakePositions` also stays on the TypeScript
- * path**, for the same reason a moved spawn does. The module derives the lake
- * list from the seed and the origin spawn - the game's own rule, and what the
- * TypeScript does when the caller passes nothing - so an explicit list would be
- * a wrong answer rather than a slow one. The app never sets it; only tests do.
+ * **A caller-supplied `startingLakePositions` is refused outright** rather
+ * than routed anywhere - see `STARTING_LAKE_POSITIONS_UNSUPPORTED` and the
+ * guard at the top of `runRenderRequest`. The module derives the lake list from
+ * the seed and the origin spawn, which is the game's own rule, so an explicit
+ * list was always a wrong answer rather than a slow one; once the TypeScript
+ * arm goes there is nothing left that could honour it. The app never set it.
  *
  * **`waterLevel` is sent and deliberately ignored by the module** - issue #326.
  * `renderTerrain.ts` resolves every tile at `waterLevel = 0` however the slider
@@ -492,6 +501,16 @@ function renderNauvisThroughWasm(
   return { id: req.id, buffer: owned.buffer, width: req.width, height: req.height };
 }
 
+/**
+ * What a caller-supplied `startingLakePositions` is refused with.
+ *
+ * Exported so specs assert the exact string rather than a substring of whatever
+ * the `Error` happened to say.
+ */
+export const STARTING_LAKE_POSITIONS_UNSUPPORTED =
+  "startingLakePositions is not supported: the render derives the lake list from " +
+  "the seed and the starting positions, which is the game's own rule";
+
 /**
  * The view this request actually renders, which is not always the one it asks
  * for.
@@ -551,6 +570,33 @@ export function runRenderRequest(
   req: ElevationRenderRequest,
   engine?: EngineExports,
 ): ElevationRenderResult {
+  // FIRST, before the planet split, and deliberately so.
+  //
+  // The two checks this replaces sat inside leaves of the view/planet dispatch
+  // and between them missed three cases: the Vulcanus branch returns before the
+  // Nauvis gate is ever evaluated, the Fulgora branch likewise - and that one is
+  // reachable, since `findIslands` posts `planet: "fulgora", view: "landmask"` -
+  // and `"landmask"` on Nauvis is in the outer view test but absent from the
+  // Nauvis gate's allowlist. A guard that runs before any of that has no leaves
+  // to miss.
+  //
+  // `!== undefined` rather than a truthiness test because it states the type's
+  // own distinction, `Point[] | undefined`, instead of relying on a coincidence.
+  // The coincidence is real and was measured: `[]` is TRUTHY in JavaScript, so
+  // a truthiness test refuses an empty list too and the two forms agree on
+  // every value this field can legally hold. The form to avoid is a length
+  // test - `!== undefined && length > 0` - which would wave `[]` through, and
+  // `[]` is a meaningful value rather than an absent one: `elevationLakes.ts`
+  // documented "Pass `[]` for the old far-field-only behavior".
+  //
+  // An error rather than a silent no-op because the TYPE outlives every
+  // consumer: `eval/ctx.ts` and `expressions/elevationIsland.ts` survive #227
+  // while every module that acted on the override does not. Accepting and
+  // ignoring it would render a different planet than the caller asked for and
+  // say nothing.
+  if (req.startingLakePositions !== undefined) {
+    throw new Error(STARTING_LAKE_POSITIONS_UNSUPPORTED);
+  }
   const planet = req.planet ?? "nauvis";
   const view = servedView(planet, req.view);
   let image: ImageData;
@@ -727,10 +773,10 @@ export function runRenderRequest(
     // reaches the same distance terms. An over-long list is refused by the
     // writer rather than silently shortened, so it cannot arrive here wrong.
     //
-    // `startingLakePositions` still does force it: the module derives the lake
-    // list from the seed and the spawn, which is the game's own rule and what
-    // the TypeScript does when the caller passes nothing, so an explicit list
-    // would be a wrong answer rather than a slow one. The app never sets it.
+    // `startingLakePositions` no longer appears here: it is refused outright at
+    // the top of this function, so by the time the gate is evaluated there is
+    // nothing left to test. The spawn-list cap still forces the TypeScript path,
+    // and it is the last thing that does.
     if (
       engine !== undefined &&
       (view === "terrain" ||
@@ -740,7 +786,6 @@ export function runRenderRequest(
         view === "cliffs" ||
         view === "resources" ||
         view === "all") &&
-      req.startingLakePositions === undefined &&
       req.startingPositions.length <= NAUVIS_MAX_STARTING_POINTS
     ) {
       return renderNauvisThroughWasm(req, engine, view);
@@ -867,13 +912,13 @@ export function runRenderRequest(
     // no layout change. `mapType` picks the code because the common prefix has
     // no `mapType` field; see `VIEW` in `src/noise/wasm/request.ts`.
     //
-    // **`startingLakePositions` still forces the TypeScript path.** That is a
-    // CORRECTNESS carve-out, not a speed one: the module derives the lake list
-    // from the seed and the spawn, which is the game's own rule and what the
-    // TypeScript does when the caller passes nothing, so an explicit list would
-    // be a wrong answer rather than a slow one. It is an ABI carve-out too -
-    // the request is a fixed-size struct with no room for a variable-length
-    // array. The app never sets it; only a test does, and not on this path.
+    // **`startingLakePositions` is gone from this gate**, because it is refused
+    // at the top of the function now. It was a CORRECTNESS carve-out rather
+    // than a speed one - the module derives the lake list from the seed and the
+    // spawn, which is the game's own rule - and an ABI one besides, the request
+    // being a fixed-size struct with no room for a variable-length array. With
+    // the TypeScript arm going there is no path that could honour it, so the
+    // honest answer is to refuse rather than to ignore.
     //
     // **A non-Nauvis `planet` also stays on TypeScript.** `mapType` spans the
     // Nauvis family only, and the branch below ignores `planet` outright, so
@@ -882,7 +927,6 @@ export function runRenderRequest(
     if (
       engine !== undefined &&
       planet === "nauvis" &&
-      req.startingLakePositions === undefined &&
       req.startingPositions.length <= NAUVIS_MAX_STARTING_POINTS
     ) {
       return renderNauvisThroughWasm(

test/elevationPreviewPanel.spec.ts

@@ -394,18 +394,41 @@ describe("ElevationPreviewPanel", () => {
     expect(putImageData).toHaveBeenCalledTimes(4);
   });
 
-  it("shows an error when the render rejects", async () => {
+  it("shows the render's OWN message when it rejects, not a constant", async () => {
+    // Issue #341. This asserted only that the element existed, which stayed
+    // true while a bare `catch` replaced every cause with "Preview failed." -
+    // the same string `PreviewPanel.vue` produces from the unrelated Docker
+    // preview service, which is what made the two impossible to tell apart.
     stubCanvas();
     const renderer: ElevationRenderer = {
       render: vi.fn(async () => {
-        throw new Error("boom");
+        throw new Error("engine.wasm speaks ABI v3, this bundle writes v4");
+      }),
+      dispose: vi.fn(),
+    };
+    const w = setup("lakes", renderer);
+    await w.find('[data-test="generate"]').trigger("click");
+    await flushPromises();
+    const shown = w.find('[data-test="preview-error"]');
+    expect(shown.exists()).toBe(true);
+    expect(shown.text()).toContain("engine.wasm speaks ABI v3, this bundle writes v4");
+  });
+
+  it("falls back to a readable line when the render throws a non-Error", async () => {
+    // `e instanceof Error` is false for a thrown string or a rejected promise
+    // carrying one, and `String(e)` on some of those reads as "[object Object]".
+    // The constant is the floor rather than the default.
+    stubCanvas();
+    const renderer: ElevationRenderer = {
+      render: vi.fn(async () => {
+        throw "not an Error";
       }),
       dispose: vi.fn(),
     };
     const w = setup("lakes", renderer);
     await w.find('[data-test="generate"]').trigger("click");
     await flushPromises();
-    expect(w.find('[data-test="preview-error"]').exists()).toBe(true);
+    expect(w.find('[data-test="preview-error"]').text()).toContain("Preview failed.");
   });
 
   it("hides the view toggles when dev mode is off", () => {

test/renderWorkerEngine.spec.ts

@@ -2,7 +2,10 @@ import { readFileSync } from "node:fs";
 import { join } from "node:path";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test";
 
-import type { ElevationRenderRequest } from "../src/noise/preview/elevationRenderRequest";
+import {
+  STARTING_LAKE_POSITIONS_UNSUPPORTED,
+  type ElevationRenderRequest,
+} from "../src/noise/preview/elevationRenderRequest";
 import { surfaceSeedForPlanet } from "../src/model/planetSurfaceSeed";
 
 /**
@@ -190,4 +193,50 @@ describe("the render worker's engine handshake", () => {
     expect(message.id).toBe(7);
     expect(message.error).toContain("render engine failed to instantiate");
   }, 120000);
+
+  it("posts a render failure rather than throwing out of onmessage", async () => {
+    // The case that used to take the whole slot down. `serve()` had no `try`, so
+    // a throw escaped `onmessage`, fired the worker's `error` event, and the
+    // host terminated the worker and rejected EVERY tile it was holding - one
+    // bad request failing every good one beside it, under a constant string
+    // that named none of them.
+    //
+    // A `startingLakePositions` override is simply the cheapest way to make
+    // `runRenderRequest` throw. The claim here is about the worker, not about
+    // that particular refusal.
+    const w = await loadWorker();
+    const module = await WebAssembly.compile(readFileSync(wasmPath));
+    w.onmessage?.({ data: { kind: "engine", module } });
+
+    expect(() => {
+      w.onmessage?.({ data: { ...request(11), startingLakePositions: [{ x: 300, y: 300 }] } });
+    }, "a failing render must not escape onmessage").not.toThrow();
+
+    expect(posted).toHaveLength(1);
+    const message = replyAt(0);
+    expect(message.id, "the error must carry the id, or the host strands it").toBe(11);
+    expect(message.error, "the REAL cause, not a constant").toBe(
+      STARTING_LAKE_POSITIONS_UNSUPPORTED,
+    );
+    expect(posted[0]?.transfer, "no buffer, so nothing to transfer").toBeUndefined();
+  }, 120000);
+
+  it("keeps serving its other requests after one of them fails", async () => {
+    // The whole point of catching rather than crashing: the sibling tiles in
+    // the same slot are unaffected. Before the wrap the second request never
+    // got an answer at all, because the worker was gone by the time it arrived.
+    const w = await loadWorker();
+    const module = await WebAssembly.compile(readFileSync(wasmPath));
+    w.onmessage?.({ data: { kind: "engine", module } });
+
+    w.onmessage?.({ data: { ...request(12), startingLakePositions: [] } });
+    w.onmessage?.({ data: request(13) });
+
+    expect(posted).toHaveLength(2);
+    expect(replyAt(0).id).toBe(12);
+    expect(replyAt(0).error).toBe(STARTING_LAKE_POSITIONS_UNSUPPORTED);
+    expect(replyAt(1).id).toBe(13);
+    expect(replyAt(1).error, "the good request must be a result, not an error").toBeUndefined();
+    expect(pixelsOf(1).length).toBe(12 * 9 * 4);
+  }, 120000);
 });