diff --git a/src/components/ElevationPreviewPanel.vue b/src/components/ElevationPreviewPanel.vue index 64ddec1..a8e9efd 100644 --- a/src/components/ElevationPreviewPanel.vue +++ b/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; } diff --git a/src/components/useElevationPreview.ts b/src/components/useElevationPreview.ts index 7153fff..18b5b59 100644 --- a/src/components/useElevationPreview.ts +++ b/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; diff --git a/src/noise/preview/elevationRender.worker.ts b/src/noise/preview/elevationRender.worker.ts index 8df0be6..4bfd88e 100644 --- a/src/noise/preview/elevationRender.worker.ts +++ b/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) => { diff --git a/src/noise/preview/elevationRenderRequest.ts b/src/noise/preview/elevationRenderRequest.ts index f48978e..bba076e 100644 --- a/src/noise/preview/elevationRenderRequest.ts +++ b/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( diff --git a/test/elevationPreviewPanel.spec.ts b/test/elevationPreviewPanel.spec.ts index 7d293bb..48e2a8d 100644 --- a/test/elevationPreviewPanel.spec.ts +++ b/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", () => { diff --git a/test/renderWorkerEngine.spec.ts b/test/renderWorkerEngine.spec.ts index 031e8b1..99a6fd0 100644 --- a/test/renderWorkerEngine.spec.ts +++ b/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); }); diff --git a/test/wasmElevationRenderParity.spec.ts b/test/wasmElevationRenderParity.spec.ts index edd024f..0307bf1 100644 --- a/test/wasmElevationRenderParity.spec.ts +++ b/test/wasmElevationRenderParity.spec.ts @@ -15,6 +15,7 @@ import { import { compileEngine, instantiateEngine, renderThroughWasm } from "../src/noise/wasm/engine"; import { runRenderRequest, + STARTING_LAKE_POSITIONS_UNSUPPORTED, type ElevationRenderRequest, } from "../src/noise/preview/elevationRenderRequest"; import { LAND_RGBA, WATER_RGBA } from "../src/noise/preview/palette"; @@ -356,43 +357,82 @@ describe("the elevation levers move both paths together", () => { }, 300000); }); -describe("a caller-supplied startingLakePositions stays on the TypeScript path", () => { +describe("a caller-supplied startingLakePositions is refused", () => { /** - * The one carve-out in the gate, and it is a CORRECTNESS one rather than a + * This block asserted the opposite until #227: that an explicit lake list + * forced the TypeScript path, which was a CORRECTNESS carve-out rather than a * speed one. The module derives the lake list from the seed and the spawn - - * the game's own rule, and what the TypeScript does when the caller passes - * nothing - so an explicit list is a different answer, not a slower one. The - * request is also a fixed-size struct with no room for a variable-length - * array, so there is nowhere to put one without an ABI change. + * the game's own rule - so an explicit list was a different answer, not a + * slower one, and the request is a fixed-size struct with nowhere to put a + * variable-length array besides. * - * Planted rather than read: the gate is asserted by giving the engine a - * request it would answer DIFFERENTLY, and requiring the TypeScript answer. + * With the TypeScript arm going there is no path left that could honour it, + * so the choice is between refusing and silently ignoring. Ignoring would + * render a different map than the caller asked for and say nothing, so it + * refuses. + * + * **Both arms are asserted.** With an engine present the request would + * otherwise route through WASM ignoring the list, which is precisely the + * wrong-answer case the carve-out existed to prevent; without one it would + * have taken the TypeScript path. Neither may quietly succeed. */ const w = WINDOWS[0]; const explicit = [{ x: 300, y: 300 }]; - // **Deliberately NOT frozen**, for the reason the Nauvis spec's ABI-cap test - // is not: both arms here are the TypeScript renderer - that is the whole - // claim - so a frozen row would capture an image the engine can never - // reproduce, and would fail the moment the carve-out goes. - // - // Unlike the spawn cap, this carve-out is documented as a CORRECTNESS one - // rather than a speed one, so #227 has to decide what a caller-supplied - // `startingLakePositions` means once there is no TypeScript path to fall back - // to. The app never sets it; only a test does. - - it("renders the TypeScript answer even with a live engine", async () => { + it("throws with the engine present and with it absent", async () => { const e = await engine(); const req = request(w, "lakes", { startingLakePositions: explicit }); - expect(Array.from(pixels(req, e)), "engine path taken").toEqual(Array.from(pixels(req))); + expect(() => pixels(req, e), "with an engine").toThrow(STARTING_LAKE_POSITIONS_UNSUPPORTED); + expect(() => pixels(req), "without an engine").toThrow(STARTING_LAKE_POSITIONS_UNSUPPORTED); }, 300000); - it("and that list actually changes the render, so the check above is not vacuous", async () => { - const withList = request(w, "lakes", { startingLakePositions: explicit }); - const derived = request(w, "lakes"); - expect(Array.from(pixels(withList)), "explicit lakes changed nothing").not.toEqual( - Array.from(pixels(derived)), - ); + it("refuses an EMPTY list too, which used to mean far-field only", () => { + // `elevationLakes.ts` documented "Pass `[]` for the old far-field-only + // behavior", so `[]` is a meaningful value rather than an absent one - + // rendering the derived lakes for a caller who asked for none is a wrong + // answer, not a default. + // + // Planted: this does NOT discriminate `!== undefined` from a truthiness + // test, because `[]` is truthy and both forms refuse it. What it catches is + // a LENGTH test, `!== undefined && length > 0`, which is the plausible + // mistake - it reads like a tidy-up and silently restores the old + // behaviour for the one value that used to select it. + const req = request(w, "lakes", { startingLakePositions: [] }); + expect(() => pixels(req)).toThrow(STARTING_LAKE_POSITIONS_UNSUPPORTED); + }); + + it("refuses on every planet and view, not only the one the old checks reached", async () => { + // The two checks this guard replaced 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 live, 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. + // + // Every other assertion in this describe is Nauvis `lakes`, so a guard put + // back into a dispatch leaf would satisfy all of them and still let these + // three through. This is the block that grades the guard's PLACEMENT. + const e = await engine(); + const holes = [ + { label: "vulcanus terrain", planet: "vulcanus", view: "terrain" }, + { label: "fulgora landmask", planet: "fulgora", view: "landmask" }, + { label: "nauvis landmask", planet: "nauvis", view: "landmask" }, + ] as const; + for (const h of holes) { + const req = request(w, "lakes", { + planet: h.planet, + view: h.view, + startingLakePositions: explicit, + }); + expect(() => pixels(req, e), h.label).toThrow(STARTING_LAKE_POSITIONS_UNSUPPORTED); + } + }, 300000); + + it("still renders when the field is simply absent", async () => { + // The guard runs before everything, so a bug in it would take out every + // render rather than only the overriding ones. + const e = await engine(); + expect(pixels(request(w, "lakes"), e).length).toBe(w.width * w.height * 4); }, 300000); }); diff --git a/test/workerHost.spec.ts b/test/workerHost.spec.ts index 7cdcdf6..ec42c2c 100644 --- a/test/workerHost.spec.ts +++ b/test/workerHost.spec.ts @@ -89,4 +89,32 @@ describe("createWorkerHost", () => { host.dispose(); expect(made[0]!.terminate).toHaveBeenCalled(); }); + + it("carries the crash's own message into every tile the slot held", async () => { + // `onerror` was declared with NO argument and substituted a constant, so a + // worker that died of a bad import and one that died of anything else + // reported the same six words. Failing every tile in the slot was already + // right - a crashed worker cannot answer any of them - but each one now + // says what happened. + const w = fakeWorker(); + const host = createWorkerHost(() => w, 1); + const first = host.execute(req(1), 0); + const second = host.execute(req(2), 0); + // Synchronously, before the fake's queued replies run. + w.onerror?.({ message: "Cannot find module './engine.wasm'" }); + await expect(first).rejects.toThrow("Cannot find module './engine.wasm'"); + await expect(second).rejects.toThrow("Cannot find module './engine.wasm'"); + host.dispose(); + }); + + it("keeps the bare label when the crash carries no message", async () => { + // A worker whose SCRIPT failed to load reports an empty `message`. An empty + // string would read as no error at all, so the constant stays as the floor. + const w = fakeWorker(); + const host = createWorkerHost(() => w, 1); + const only = host.execute(req(1), 0); + w.onerror?.({ message: "" }); + await expect(only).rejects.toThrow("Elevation render worker error"); + host.dispose(); + }); });