From f931925a8608ea3bf482e32fc5efc1cf9c17c80f Mon Sep 17 00:00:00 2001 From: giswqs Date: Tue, 18 Aug 2026 20:42:00 -0400 Subject: [PATCH 1/4] fix(processing): let Whitebox Dissolve take a grouping field The Dissolve dialog rendered its `dissolve_field` parameter as a second layer picker with a file-path box, so the attribute to dissolve by could not be entered at all. The parameter kind came from geolibre-wasm's manifest, which inferred a parameter's type from its description -- and a column parameter's description describes the data it *indexes*, not the value the user types. "Optional attribute field used to dissolve polygons within groups" read as a polygon layer to open. Bump geolibre-wasm to 1.5.2, which carries three upstream fixes: - opengeos/whitebox-wasm#19 types a `*_field`/`*_attribute` parameter as the column name it is. 54 params across 28 tools stop asking for a file or a checkbox, so the dialog's attribute picker (GeoLibre#1459) now reaches them: Dissolve, join_tables, merge_table_with_csv, the route event family, and every network tool's `one_way_field`. - opengeos/whitebox-wasm#20 makes `dissolve` emit one feature per group. Parts of a group that shared a value but no boundary were separate features, so 290 polygons over 12 values dissolved to 48, not 12. - opengeos/whitebox-wasm#21 decodes GeoJSON/TopoJSON strings as UTF-8. Both parsers read each byte as a Latin-1 code point and re-encoded it, so a non-ASCII attribute gained a layer of mojibake on every pass through a tool. Also correct two comments that cited `join_tables.primary_key_field` as a field-named parameter that is legitimately a dataset input. It was one of the 40 the manifest mistyped, and it is a string now; the sidecar catalog's `classify_objects_svm.class_field` is a live example, so the scalar-string guard those comments explain still earns its place. Fixes #1977 --- .../src/components/processing/ProcessingDialog.tsx | 5 +++-- apps/geolibre-desktop/src/lib/whitebox-field-params.ts | 9 ++++++--- package-lock.json | 8 ++++---- packages/processing/package.json | 2 +- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx b/apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx index d306d5f05..14d7ba683 100644 --- a/apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx +++ b/apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx @@ -190,8 +190,9 @@ function isSubsetUrlParameter(tool: WhiteboxTool, param: WhiteboxToolParameter): // vector inputs (points_to_line's `line_field`/`sort_field`, and ~170 other // tools), so the dialog can offer the selected layer's attribute names instead // of asking the user to recall a column name (GeoLibre#1459). The kind check is -// what keeps a same-named *dataset* param out (join_tables' `primary_key_field` -// is a vector input): only a scalar string names a column. +// what keeps a same-named *dataset* param out (the catalog types +// classify_objects_svm's `class_field` as a LiDAR input): only a scalar string +// names a column. function isFieldParameter(param: WhiteboxToolParameter): boolean { return parameterKind(param) === "string" && isFieldParameterName(param.name); } diff --git a/apps/geolibre-desktop/src/lib/whitebox-field-params.ts b/apps/geolibre-desktop/src/lib/whitebox-field-params.ts index 914116d3a..6c06daad0 100644 --- a/apps/geolibre-desktop/src/lib/whitebox-field-params.ts +++ b/apps/geolibre-desktop/src/lib/whitebox-field-params.ts @@ -17,9 +17,12 @@ export const FIELD_PARAM_SUFFIX = /(^|_)(fields?|attributes?)$/i; /** * Whether a parameter name reads as an attribute-column name. * - * Callers must also check the parameter is a scalar string: `join_tables` - * exposes `primary_key_field` as a *dataset* input, and a dataset parameter - * names a file, not a column. + * Callers must also check the parameter is a scalar string: the sidecar's + * catalog exposes `classify_objects_svm`'s `class_field` as a *dataset* input, + * and a dataset parameter names a file, not a column. (The WASM manifests used + * to mistype ~40 of these the same way, `dissolve_field` among them, until + * opengeos/whitebox-wasm#19 taught the manifest inference that a `*_field` name + * is a column; the sidecar catalog still carries a few.) * * @param name - The tool parameter's name. * @returns `true` when the name ends in a field/attribute suffix. diff --git a/package-lock.json b/package-lock.json index 13fcc5780..4111411e4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15291,9 +15291,9 @@ "link": true }, "node_modules/geolibre-wasm": { - "version": "1.5.1", - "resolved": "https://registry.npmjs.org/geolibre-wasm/-/geolibre-wasm-1.5.1.tgz", - "integrity": "sha512-axGG7xfuFsFiyk1Y1SkrmjXwzHu0/LfO1WX/xw4rrVbFbdTenqAs7VdDlpnIjBn4+Enm9hhINqq3p40f+87bRQ==", + "version": "1.5.2", + "resolved": "https://registry.npmjs.org/geolibre-wasm/-/geolibre-wasm-1.5.2.tgz", + "integrity": "sha512-YVG2DsWMfqx1UcdImpX3CKHhAwsCfQG8lO1u5k9RqxYwT+hm3q1GJo0YSYx1DM9sGJXiWlXyIjHGdyIqjpG0tQ==", "license": "MIT", "dependencies": { "@bjorn3/browser_wasi_shim": "^0.4.2" @@ -23911,7 +23911,7 @@ "@turf/voronoi": "^7.4.0", "dggal": "^0.0.6", "fflate": "^0.8.3", - "geolibre-wasm": "^1.5.1", + "geolibre-wasm": "^1.5.2", "geotiff": "^3.0.5", "onnxruntime-web": "1.27.0", "s2js": "^1.44.0" diff --git a/packages/processing/package.json b/packages/processing/package.json index 1058654b8..c138f499b 100644 --- a/packages/processing/package.json +++ b/packages/processing/package.json @@ -34,7 +34,7 @@ "@turf/voronoi": "^7.4.0", "dggal": "^0.0.6", "fflate": "^0.8.3", - "geolibre-wasm": "^1.5.1", + "geolibre-wasm": "^1.5.2", "geotiff": "^3.0.5", "onnxruntime-web": "1.27.0", "s2js": "^1.44.0" From ca59dda55f14c0b9cb0d6c4083aa6a43d416bab9 Mon Sep 17 00:00:00 2001 From: giswqs Date: Tue, 18 Aug 2026 21:42:40 -0400 Subject: [PATCH 2/4] fix(processing): run Whitebox WASM tools off the main thread The WASI runner is a single synchronous `wasi.start()` with no yield points, so running it on the main thread freezes the whole UI -- no repaint, no input -- for as long as the tool takes. Dissolving the 290-polygon layer from #1977 takes ~60s, and a stall probe measured 58,573 ms without a single animation frame: the app looks hung. wasm-convert.ts already routed its tiling calls to a one-shot Worker for exactly this reason, and its worker script is generic. Lift that machinery into `wasm-tool-runner.ts`, rename the script to `wasm-tool.worker.ts` now that it serves both callers, and route `runWhiteboxToolWasm` through it. One implementation, so the two paths cannot drift. This freeze predates the dissolve fix -- every WASM tool blocked the main thread -- but a long-running Dissolve was unreachable until the grouping field became typeable, so it surfaces there first. Measured on the same run: max main-thread stall drops from 58,573 ms to 756 ms, and the output is unchanged at 12 features. --- packages/processing/src/wasm-client.ts | 11 ++- packages/processing/src/wasm-convert.ts | 61 +-------------- packages/processing/src/wasm-tool-runner.ts | 76 +++++++++++++++++++ ...-convert.worker.ts => wasm-tool.worker.ts} | 0 tests/wasm-convert.test.ts | 4 +- 5 files changed, 91 insertions(+), 61 deletions(-) create mode 100644 packages/processing/src/wasm-tool-runner.ts rename packages/processing/src/{wasm-convert.worker.ts => wasm-tool.worker.ts} (100%) diff --git a/packages/processing/src/wasm-client.ts b/packages/processing/src/wasm-client.ts index eac56746a..682483ab2 100644 --- a/packages/processing/src/wasm-client.ts +++ b/packages/processing/src/wasm-client.ts @@ -8,6 +8,7 @@ import type { FeatureCollection } from "geojson"; import { convertGeoTiffToCog } from "./cog-convert"; import { normalizeVectorOutputFormat } from "./sidecar-client"; +import { runWasmToolInBackground } from "./wasm-tool-runner"; import type { RunWhiteboxToolRequest, VectorOutputFormat, @@ -651,7 +652,6 @@ export async function ensureWhiteboxRasterCog(bytes: Uint8Array): Promise { - const { runTool } = await loadToolsModule(); const encoder = new TextEncoder(); const input: Record = {}; const args: string[] = []; @@ -770,7 +770,14 @@ export async function runWhiteboxToolWasm(request: RunWhiteboxToolRequest): Prom } } - const { exitCode, stdout, files } = await runTool(request.tool_id, { args, input }); + // Off the main thread: the WASI runner is one synchronous call with no yield + // points, so running it here would freeze the UI for the tool's whole + // duration (~60s for the 290-polygon dissolve in GeoLibre#1977). + const { exitCode, stdout, files } = await runWasmToolInBackground({ + tool: request.tool_id, + args, + input, + }); if (exitCode !== 0) { return job( request.tool_id, diff --git a/packages/processing/src/wasm-convert.ts b/packages/processing/src/wasm-convert.ts index 248ab2a6c..98e48fb83 100644 --- a/packages/processing/src/wasm-convert.ts +++ b/packages/processing/src/wasm-convert.ts @@ -14,7 +14,7 @@ // All three run entirely client-side, so the web build needs no Python sidecar // for them. import type { RunToolOptions, ToolResult } from "geolibre-wasm/tools"; -import type { WasmToolRequest, WasmToolResponse } from "./wasm-convert.worker"; +import { runWasmToolInBackground } from "./wasm-tool-runner"; /** The subset of `geolibre-wasm/tools` these converters use. */ interface ConvertToolsModule { @@ -59,61 +59,6 @@ export async function initConvertTools( await initTools(source); } -/** - * Run a tool on a one-shot Web Worker and resolve with its result. - * - * No timeout: how long a tool runs is bounded by the data, not the clock (a - * country-scale tile pyramid is minutes), and cutting off work that would have - * finished is worse than waiting. `error`/`messageerror` still reject, so the - * promise settles on every failure the worker can report. - */ -function runToolOnWorker(request: WasmToolRequest): Promise { - return new Promise((resolve, reject) => { - const worker = new Worker(new URL("./wasm-convert.worker.ts", import.meta.url), { - type: "module", - }); - worker.addEventListener("message", (event: MessageEvent) => { - worker.terminate(); - if (event.data.ok) resolve(event.data.result); - else reject(new Error(event.data.error || `${request.tool} failed.`)); - }); - worker.addEventListener("error", (event) => { - worker.terminate(); - reject(new Error(event.message || `The ${request.tool} worker failed.`)); - }); - // `error` does not fire when a posted message cannot be deserialized, which - // would otherwise leave this promise pending forever. - worker.addEventListener("messageerror", () => { - worker.terminate(); - reject(new Error(`The ${request.tool} worker posted an undeserializable message.`)); - }); - // The input files are structured-cloned rather than transferred: these - // wrappers do not otherwise take ownership of the caller's bytes, and a - // neutered input array would be a trap the sibling converters don't set. - try { - worker.postMessage(request); - } catch (error) { - // A throw here (e.g. DataCloneError) rejects the promise on its own, but - // the worker is already spawned and would leak without this. - worker.terminate(); - reject(error instanceof Error ? error : new Error(String(error))); - } - }); -} - -/** - * Run a tool off the main thread where Workers exist, inline where they do not - * (node, tests). The inline path is why {@link initConvertTools} still takes an - * explicit wasm source: a worker resolves its own bundled copy instead. - */ -async function runToolInBackground(request: WasmToolRequest): Promise { - if (typeof Worker === "undefined") { - const { runTool } = await loadToolsModule(); - return runTool(request.tool, { args: request.args, input: request.input }); - } - return runToolOnWorker(request); -} - /** An input file for a WASM conversion: its name (the extension drives format * detection) and its raw bytes. */ export interface WasmConvertFile { @@ -295,7 +240,7 @@ export interface VectorToPmtilesOptions { * `siblings`, exactly as in {@link convertVectorWithWasm}. * * Unlike its siblings here this runs on a Web Worker (see - * {@link runToolInBackground}). Tiling is by far the heaviest of these tools — + * {@link runWasmToolInBackground}). Tiling is by far the heaviest of these tools — * a US-wide layer to the default zoom 14 is millions of tiles and minutes of * uninterrupted WASM — so running it on the main thread would freeze the UI for * the whole conversion. The others finish quickly enough not to warrant the @@ -322,7 +267,7 @@ export async function tileVectorToPmtiles( ]); const files: Record = { [input.name]: input.data }; for (const sibling of siblings) files[sibling.name] = sibling.data; - const result = await runToolInBackground({ + const result = await runWasmToolInBackground({ tool: "vector_to_pmtiles", args, input: files, diff --git a/packages/processing/src/wasm-tool-runner.ts b/packages/processing/src/wasm-tool-runner.ts new file mode 100644 index 000000000..87f837131 --- /dev/null +++ b/packages/processing/src/wasm-tool-runner.ts @@ -0,0 +1,76 @@ +// Shared plumbing for running a `geolibre-wasm/tools` WASI tool off the main +// thread. +// +// The runner has no yield points once a tool starts: `wasi.start()` is one +// synchronous call that returns only when the tool is done. On the main thread +// that freezes the whole UI — no repaint, no input — for the tool's entire +// duration, which is bounded by the data rather than the clock. Dissolving the +// 290-polygon layer from GeoLibre#1977 blocks it for ~60s. +// +// wasm-convert.ts routed its tiling calls through a worker for exactly this +// reason. This module is that machinery, lifted out so the Whitebox toolbox +// (wasm-client.ts) shares one implementation with it instead of growing a +// second copy that could drift. +import type { ToolResult } from "geolibre-wasm/tools"; +import type { WasmToolRequest, WasmToolResponse } from "./wasm-tool.worker"; + +export type { WasmToolRequest, WasmToolResponse }; + +/** + * Run a tool on a one-shot Web Worker and resolve with its result. + * + * No timeout: how long a tool runs is bounded by the data, not the clock (a + * country-scale tile pyramid is minutes), and cutting off work that would have + * finished is worse than waiting. `error`/`messageerror` still reject, so the + * promise settles on every failure the worker can report. + */ +function runToolOnWorker(request: WasmToolRequest): Promise { + return new Promise((resolve, reject) => { + const worker = new Worker(new URL("./wasm-tool.worker.ts", import.meta.url), { + type: "module", + }); + worker.addEventListener("message", (event: MessageEvent) => { + worker.terminate(); + if (event.data.ok) resolve(event.data.result); + else reject(new Error(event.data.error || `${request.tool} failed.`)); + }); + worker.addEventListener("error", (event) => { + worker.terminate(); + reject(new Error(event.message || `The ${request.tool} worker failed.`)); + }); + // `error` does not fire when a posted message cannot be deserialized, which + // would otherwise leave this promise pending forever. + worker.addEventListener("messageerror", () => { + worker.terminate(); + reject(new Error(`The ${request.tool} worker posted an undeserializable message.`)); + }); + // The input files are structured-cloned rather than transferred: these + // wrappers do not otherwise take ownership of the caller's bytes, and a + // neutered input array would be a trap the callers don't set. + try { + worker.postMessage(request); + } catch (error) { + // A throw here (e.g. DataCloneError) rejects the promise on its own, but + // the worker is already spawned and would leak without this. + worker.terminate(); + reject(error instanceof Error ? error : new Error(String(error))); + } + }); +} + +/** + * Run a tool off the main thread where Workers exist, inline where they do not + * (node, tests). The inline path is why the callers still expose an explicit + * wasm-source init (`initConvertTools`): a worker resolves its own bundled copy + * instead, in its own module scope. + * + * @param request - The tool id, CLI args, and files to place under `/work`. + * @returns The tool's exit code, captured output, and the files it wrote. + */ +export async function runWasmToolInBackground(request: WasmToolRequest): Promise { + if (typeof Worker === "undefined") { + const { runTool } = await import("geolibre-wasm/tools"); + return runTool(request.tool, { args: request.args, input: request.input }); + } + return runToolOnWorker(request); +} diff --git a/packages/processing/src/wasm-convert.worker.ts b/packages/processing/src/wasm-tool.worker.ts similarity index 100% rename from packages/processing/src/wasm-convert.worker.ts rename to packages/processing/src/wasm-tool.worker.ts diff --git a/tests/wasm-convert.test.ts b/tests/wasm-convert.test.ts index 44efa7a88..75853f65b 100644 --- a/tests/wasm-convert.test.ts +++ b/tests/wasm-convert.test.ts @@ -456,9 +456,11 @@ describe("wasm-convert", () => { }); // Vite only bundles the worker when it can statically see this exact shape. + // The script is shared with the Whitebox toolbox runner (wasm-client.ts), + // which is why it is named for tools rather than for conversion. it("loads the worker as an ES module", () => { const { worker } = startRun(); - assert.match(worker.url.href, /wasm-convert\.worker\.ts$/); + assert.match(worker.url.href, /wasm-tool\.worker\.ts$/); assert.deepEqual(worker.options, { type: "module" }); }); From 72c4122d8ed06d4e7ee0cf8dadd39833710a2e42 Mon Sep 17 00:00:00 2001 From: giswqs Date: Tue, 18 Aug 2026 22:07:27 -0400 Subject: [PATCH 3/4] Address Claude review feedback - reuse WASM tool workers instead of discarding each after one run. A worker compiles the ~23 MB geolibre-cli.wasm in its own module scope, and the main thread's copy is not shared with it, so one worker per run made every run pay that again. Invisible next to a minutes-long tiling job, the only caller before this PR, but not next to the many Whitebox tools that finish in well under a second. An idle worker is taken when there is one and a new one spawned otherwise, so concurrent runs still overlap rather than serializing behind a single shared worker; a worker that fails at the worker level is terminated rather than parked, and listeners are removed so a reused worker does not accumulate them. Adds `releaseIdleWasmToolWorkers()` to free the warm workers, which test teardown needs so a parked worker is not handed to the next case. - fix the stale `wasm-convert.worker.ts` reference in the afterEach comment left by this PR's rename. --- packages/processing/src/wasm-tool-runner.ts | 80 ++++++++++++++++++--- packages/processing/src/wasm-tool.worker.ts | 16 +++-- tests/wasm-convert.test.ts | 40 +++++++++-- 3 files changed, 114 insertions(+), 22 deletions(-) diff --git a/packages/processing/src/wasm-tool-runner.ts b/packages/processing/src/wasm-tool-runner.ts index 87f837131..af67e3f7c 100644 --- a/packages/processing/src/wasm-tool-runner.ts +++ b/packages/processing/src/wasm-tool-runner.ts @@ -16,34 +16,91 @@ import type { WasmToolRequest, WasmToolResponse } from "./wasm-tool.worker"; export type { WasmToolRequest, WasmToolResponse }; +// Idle workers, kept alive to be reused. A worker compiles the ~23 MB +// `geolibre-cli.wasm` in its *own* module scope, and the main thread's +// already-compiled copy is not shared with it, so a worker discarded after +// every run makes each run pay that fetch and compile again. That is invisible +// next to a minutes-long tiling job — the only thing that used this path +// before — but not next to the many Whitebox tools that finish in well under a +// second, where it would dominate the run. +// +// Reuse rather than a single shared worker: a WASI run is synchronous inside +// its worker, so one worker would serialize concurrent runs that used to +// overlap. Taking an idle worker when there is one and spawning otherwise keeps +// that parallelism and still pays the compile once per worker. +const idleWorkers: Worker[] = []; + +// How many idle workers to keep warm. Each holds its compiled module (tens of +// MB) for the rest of the session, and real usage is one tool at a time, so +// one warm worker captures nearly all of the benefit; extras are terminated +// rather than parked. +const MAX_IDLE_WORKERS = 1; + /** - * Run a tool on a one-shot Web Worker and resolve with its result. + * Terminate every parked worker and forget them, freeing the compiled module + * each one holds. Runs in flight are unaffected — they own their worker until + * it answers. Call it to reclaim that memory, and in tests, so a worker parked + * by one case is not handed to the next. + */ +export function releaseIdleWasmToolWorkers(): void { + for (const worker of idleWorkers.splice(0)) worker.terminate(); +} + +function acquireWorker(): Worker { + return ( + idleWorkers.pop() ?? + new Worker(new URL("./wasm-tool.worker.ts", import.meta.url), { type: "module" }) + ); +} + +/** Park a still-healthy worker for reuse, or terminate it if enough are warm. */ +function releaseWorker(worker: Worker): void { + if (idleWorkers.length < MAX_IDLE_WORKERS) idleWorkers.push(worker); + else worker.terminate(); +} + +/** + * Run a tool on a Web Worker and resolve with its result. * * No timeout: how long a tool runs is bounded by the data, not the clock (a * country-scale tile pyramid is minutes), and cutting off work that would have * finished is worse than waiting. `error`/`messageerror` still reject, so the * promise settles on every failure the worker can report. + * + * A worker that answers is parked for reuse; one that fails at the worker level + * is terminated, since its state after that is not something to hand the next + * caller. Listeners are removed on the way out so a reused worker does not + * accumulate them. */ function runToolOnWorker(request: WasmToolRequest): Promise { return new Promise((resolve, reject) => { - const worker = new Worker(new URL("./wasm-tool.worker.ts", import.meta.url), { - type: "module", - }); - worker.addEventListener("message", (event: MessageEvent) => { - worker.terminate(); + const worker = acquireWorker(); + const onMessage = (event: MessageEvent) => { + cleanup(); + releaseWorker(worker); if (event.data.ok) resolve(event.data.result); else reject(new Error(event.data.error || `${request.tool} failed.`)); - }); - worker.addEventListener("error", (event) => { + }; + const onError = (event: ErrorEvent) => { + cleanup(); worker.terminate(); reject(new Error(event.message || `The ${request.tool} worker failed.`)); - }); + }; // `error` does not fire when a posted message cannot be deserialized, which // would otherwise leave this promise pending forever. - worker.addEventListener("messageerror", () => { + const onMessageError = () => { + cleanup(); worker.terminate(); reject(new Error(`The ${request.tool} worker posted an undeserializable message.`)); - }); + }; + const cleanup = () => { + worker.removeEventListener("message", onMessage); + worker.removeEventListener("error", onError); + worker.removeEventListener("messageerror", onMessageError); + }; + worker.addEventListener("message", onMessage); + worker.addEventListener("error", onError); + worker.addEventListener("messageerror", onMessageError); // The input files are structured-cloned rather than transferred: these // wrappers do not otherwise take ownership of the caller's bytes, and a // neutered input array would be a trap the callers don't set. @@ -52,6 +109,7 @@ function runToolOnWorker(request: WasmToolRequest): Promise { } catch (error) { // A throw here (e.g. DataCloneError) rejects the promise on its own, but // the worker is already spawned and would leak without this. + cleanup(); worker.terminate(); reject(error instanceof Error ? error : new Error(String(error))); } diff --git a/packages/processing/src/wasm-tool.worker.ts b/packages/processing/src/wasm-tool.worker.ts index 11f7bcf8f..9a66e9db3 100644 --- a/packages/processing/src/wasm-tool.worker.ts +++ b/packages/processing/src/wasm-tool.worker.ts @@ -2,13 +2,17 @@ import { runTool } from "geolibre-wasm/tools"; import type { ToolResult } from "geolibre-wasm/tools"; -// Runs one `geolibre-wasm/tools` WASI tool off the main thread. The runner has -// no yield points once a tool starts, so on the main thread a long job freezes -// the whole UI for its duration — tiling a country-scale vector layer to zoom 14 -// is minutes of that, which is why wasm-convert.ts routes it here. +// Runs `geolibre-wasm/tools` WASI tools off the main thread. The runner has no +// yield points once a tool starts, so on the main thread a long job freezes the +// whole UI for its duration — tiling a country-scale vector layer to zoom 14 is +// minutes of that, and dissolving 290 polygons by an attribute is ~60s, which is +// why wasm-convert.ts and wasm-client.ts both route through wasm-tool-runner.ts. // -// One tool per worker: the caller terminates this worker as soon as the terminal -// message arrives, so nothing here has to be reusable across runs. +// One run at a time, but reusable across runs: the caller parks this worker for +// its next call instead of terminating it, so the ~23 MB `geolibre-cli.wasm` +// compile in this worker's module scope is paid once rather than per run. Each +// run still gets a fresh WASI instance and a fresh /work from `runTool`, so +// nothing carries over between them. const worker = self as unknown as DedicatedWorkerGlobalScope; /** A tool to run: its id, CLI args, and the files to place under /work. */ diff --git a/tests/wasm-convert.test.ts b/tests/wasm-convert.test.ts index 75853f65b..4a427c662 100644 --- a/tests/wasm-convert.test.ts +++ b/tests/wasm-convert.test.ts @@ -9,6 +9,7 @@ import { renderRasterToPmtiles, tileVectorToPmtiles, } from "../packages/processing/src/wasm-convert"; +import { releaseIdleWasmToolWorkers } from "../packages/processing/src/wasm-tool-runner"; const fixture = (name: string) => new Uint8Array(readFileSync(fileURLToPath(new URL(`./fixtures/${name}`, import.meta.url)))); @@ -385,6 +386,13 @@ describe("wasm-convert", () => { this.listeners.set(type, [...(this.listeners.get(type) ?? []), fn]); } + removeEventListener(type: string, fn: (event: unknown) => void): void { + this.listeners.set( + type, + (this.listeners.get(type) ?? []).filter((it) => it !== fn), + ); + } + postMessage(message: unknown): void { if (FakeWorker.postMessageError) throw FakeWorker.postMessageError; this.posted.push(message); @@ -408,8 +416,11 @@ describe("wasm-convert", () => { }); afterEach(() => { + // A worker parked for reuse would otherwise be handed to the next case, + // which then spawns none and sees an empty `instances`. + releaseIdleWasmToolWorkers(); // Leaving the stub installed would push the inline tests above onto the - // worker path, where `new URL("./wasm-convert.worker.ts")` cannot load. + // worker path, where `new URL("./wasm-tool.worker.ts")` cannot load. if (!hadWorker) delete (globalThis as { Worker?: unknown }).Worker; }); @@ -464,20 +475,39 @@ describe("wasm-convert", () => { assert.deepEqual(worker.options, { type: "module" }); }); - it("resolves with the worker's result and terminates it", async () => { + it("resolves with the worker's result and parks it for reuse", async () => { const { promise, worker } = startRun(); worker.emit("message", { data: { ok: true, result: okResult } }); const result = await promise; assert.deepEqual(result.data, Uint8Array.from(PMTILES_MAGIC)); assert.deepEqual(result.messages, ["packing PMTiles archive"]); - assert.equal(worker.terminated, true, "the worker should not leak"); + // Parked, not terminated: a worker compiles the ~23 MB wasm in its own + // module scope, so discarding it makes the next run pay that again. + assert.equal(worker.terminated, false, "a healthy worker should be reused"); }); - it("rejects with the error the worker reports", async () => { + // The compile is per worker, so the second run must not spawn a second one. + it("reuses the parked worker for the next run", async () => { + const first = startRun(); + first.worker.emit("message", { data: { ok: true, result: okResult } }); + await first.promise; + const second = startRun(); + assert.equal(FakeWorker.instances.length, 1, "the second run should reuse the first worker"); + assert.equal(second.worker, first.worker); + // Its listeners are removed on the way out, so the reused worker answers + // the new run once rather than resolving the previous promise again. + assert.equal(second.worker.listeners.get("message")?.length, 1); + second.worker.emit("message", { data: { ok: true, result: okResult } }); + await second.promise; + }); + + it("rejects with the error the worker reports and still parks it", async () => { const { promise, worker } = startRun(); worker.emit("message", { data: { ok: false, error: "runner exploded" } }); await assert.rejects(promise, /runner exploded/); - assert.equal(worker.terminated, true); + // A tool that failed inside a healthy worker says nothing about the + // worker: the runner caught it and answered, so it is still reusable. + assert.equal(worker.terminated, false); }); it("rejects when the worker itself fails", async () => { From 4d91e9cf38a4ffc0d5caf7d854b8fa91f99762cd Mon Sep 17 00:00:00 2001 From: giswqs Date: Tue, 18 Aug 2026 22:24:35 -0400 Subject: [PATCH 4/4] Address Claude review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - guard reuse of a parked worker with an acknowledgement. A worker killed out of band fires no `error` event and silently swallows `postMessage`, and this module deliberately puts no timeout on the run itself, so reusing one that died while idle would have left the run pending forever with nothing shown to the user. The worker now acks on receipt, before starting the run that blocks its thread; a reused worker that does not ack within 10s is terminated, replaced, and the request re-sent. Only reused workers are watched — a freshly spawned one has not had time to die, reports itself through `error`, and its module-graph startup can outlast any sensible ack window in a dev server. A false positive costs a respawn, never a failed run. --- packages/processing/src/wasm-tool-runner.ts | 86 ++++++++++++++++----- packages/processing/src/wasm-tool.worker.ts | 18 ++++- tests/wasm-convert.test.ts | 44 +++++++++++ 3 files changed, 128 insertions(+), 20 deletions(-) diff --git a/packages/processing/src/wasm-tool-runner.ts b/packages/processing/src/wasm-tool-runner.ts index af67e3f7c..e6336ec1b 100644 --- a/packages/processing/src/wasm-tool-runner.ts +++ b/packages/processing/src/wasm-tool-runner.ts @@ -46,13 +46,26 @@ export function releaseIdleWasmToolWorkers(): void { for (const worker of idleWorkers.splice(0)) worker.terminate(); } -function acquireWorker(): Worker { - return ( - idleWorkers.pop() ?? - new Worker(new URL("./wasm-tool.worker.ts", import.meta.url), { type: "module" }) - ); +function spawnWorker(): Worker { + return new Worker(new URL("./wasm-tool.worker.ts", import.meta.url), { type: "module" }); } +/** A worker to run on, and whether it came from the pool rather than being new. */ +function acquireWorker(): { worker: Worker; reused: boolean } { + const parked = idleWorkers.pop(); + return parked ? { worker: parked, reused: true } : { worker: spawnWorker(), reused: false }; +} + +// How long a *reused* worker has to acknowledge a request before it is treated +// as dead. A parked worker is idle by construction — it is only parked once it +// has answered — so an ack is a message hop away and this window is enormous by +// comparison. It exists because a worker killed out of band (an OOM kill, a +// discarded tab) fires no `error` event and silently swallows `postMessage`, and +// this module deliberately puts no timeout on the run itself, so without the ack +// a dead worker would leave the run pending forever. A false positive costs a +// respawn, never a failed run. +const REUSED_WORKER_ACK_MS = 10_000; + /** Park a still-healthy worker for reuse, or terminate it if enough are warm. */ function releaseWorker(worker: Worker): void { if (idleWorkers.length < MAX_IDLE_WORKERS) idleWorkers.push(worker); @@ -74,14 +87,42 @@ function releaseWorker(worker: Worker): void { */ function runToolOnWorker(request: WasmToolRequest): Promise { return new Promise((resolve, reject) => { - const worker = acquireWorker(); + let { worker, reused } = acquireWorker(); + // Only a reused worker is watched: a freshly spawned one has not had time to + // die, and a broken one reports itself through `error`. Its startup also + // includes loading this worker's module graph, which in a dev server can be + // slower than any ack window worth setting. + let ackTimer: ReturnType | undefined; + const armAck = () => { + if (!reused) return; + ackTimer = setTimeout(() => { + // Never acknowledged, so it died while parked. Replace it and re-post; + // the request has not started, so nothing is lost by retrying once. + cleanup(); + worker.terminate(); + worker = spawnWorker(); + reused = false; + attach(); + post(); + }, REUSED_WORKER_ACK_MS); + }; + const disarmAck = () => { + if (ackTimer !== undefined) clearTimeout(ackTimer); + ackTimer = undefined; + }; const onMessage = (event: MessageEvent) => { + if (event.data.ok === "ack") { + disarmAck(); + return; + } + disarmAck(); cleanup(); releaseWorker(worker); if (event.data.ok) resolve(event.data.result); else reject(new Error(event.data.error || `${request.tool} failed.`)); }; const onError = (event: ErrorEvent) => { + disarmAck(); cleanup(); worker.terminate(); reject(new Error(event.message || `The ${request.tool} worker failed.`)); @@ -89,6 +130,7 @@ function runToolOnWorker(request: WasmToolRequest): Promise { // `error` does not fire when a posted message cannot be deserialized, which // would otherwise leave this promise pending forever. const onMessageError = () => { + disarmAck(); cleanup(); worker.terminate(); reject(new Error(`The ${request.tool} worker posted an undeserializable message.`)); @@ -98,21 +140,29 @@ function runToolOnWorker(request: WasmToolRequest): Promise { worker.removeEventListener("error", onError); worker.removeEventListener("messageerror", onMessageError); }; - worker.addEventListener("message", onMessage); - worker.addEventListener("error", onError); - worker.addEventListener("messageerror", onMessageError); + const attach = () => { + worker.addEventListener("message", onMessage); + worker.addEventListener("error", onError); + worker.addEventListener("messageerror", onMessageError); + }; // The input files are structured-cloned rather than transferred: these // wrappers do not otherwise take ownership of the caller's bytes, and a // neutered input array would be a trap the callers don't set. - try { - worker.postMessage(request); - } catch (error) { - // A throw here (e.g. DataCloneError) rejects the promise on its own, but - // the worker is already spawned and would leak without this. - cleanup(); - worker.terminate(); - reject(error instanceof Error ? error : new Error(String(error))); - } + const post = () => { + try { + worker.postMessage(request); + armAck(); + } catch (error) { + // A throw here (e.g. DataCloneError) rejects the promise on its own, but + // the worker is already spawned and would leak without this. + disarmAck(); + cleanup(); + worker.terminate(); + reject(error instanceof Error ? error : new Error(String(error))); + } + }; + attach(); + post(); }); } diff --git a/packages/processing/src/wasm-tool.worker.ts b/packages/processing/src/wasm-tool.worker.ts index 9a66e9db3..54b0455cc 100644 --- a/packages/processing/src/wasm-tool.worker.ts +++ b/packages/processing/src/wasm-tool.worker.ts @@ -22,11 +22,25 @@ export interface WasmToolRequest { input: Record; } -/** The single message this worker posts back. */ -export type WasmToolResponse = { ok: true; result: ToolResult } | { ok: false; error: string }; +/** + * What this worker posts back: an immediate `ack` on receipt, then one terminal + * message when the tool finishes. + * + * The ack is what lets the caller tell a live parked worker from one that died + * while idle. Nothing here can report that: a worker killed out of band fires + * no `error` event, and posting to it silently does nothing, so without an ack + * a reused-but-dead worker would leave the run pending forever. + */ +export type WasmToolResponse = + | { ok: "ack" } + | { ok: true; result: ToolResult } + | { ok: false; error: string }; worker.addEventListener("message", async (event: MessageEvent) => { const { tool, args, input } = event.data; + // Before the run, so it lands even though the WASI call that follows blocks + // this worker's thread until the tool is done. + worker.postMessage({ ok: "ack" } satisfies WasmToolResponse); try { // runTool compiles the bundled geolibre-cli.wasm on first use, in this // worker's own module scope — a copy already compiled on the main thread diff --git a/tests/wasm-convert.test.ts b/tests/wasm-convert.test.ts index 4a427c662..aa24124b2 100644 --- a/tests/wasm-convert.test.ts +++ b/tests/wasm-convert.test.ts @@ -501,6 +501,50 @@ describe("wasm-convert", () => { await second.promise; }); + // A worker killed while parked fires no `error` and swallows postMessage, so + // without the ack the run would stay pending forever. It is replaced instead. + it("replaces a parked worker that never acknowledges the request", async (t) => { + t.mock.timers.enable({ apis: ["setTimeout"] }); + const first = startRun(); + first.worker.emit("message", { data: { ok: true, result: okResult } }); + await first.promise; + + const second = startRun(); + assert.equal(second.worker, first.worker, "it should try the parked worker first"); + t.mock.timers.tick(10_000); + + assert.equal(first.worker.terminated, true, "the silent worker should be terminated"); + assert.equal(FakeWorker.instances.length, 2, "a replacement should be spawned"); + const replacement = FakeWorker.instances[1]; + assert.deepEqual( + replacement.posted, + [first.worker.posted[0]], + "the request should be re-sent", + ); + + // The replacement answers, and the run resolves as if nothing happened. + replacement.emit("message", { data: { ok: true, result: okResult } }); + const result = await second.promise; + assert.deepEqual(result.data, Uint8Array.from(PMTILES_MAGIC)); + }); + + // A live parked worker acks, so it is kept rather than replaced. + it("keeps a parked worker that acknowledges the request", async (t) => { + t.mock.timers.enable({ apis: ["setTimeout"] }); + const first = startRun(); + first.worker.emit("message", { data: { ok: true, result: okResult } }); + await first.promise; + + const second = startRun(); + second.worker.emit("message", { data: { ok: "ack" } }); + t.mock.timers.tick(10_000); + assert.equal(FakeWorker.instances.length, 1, "no replacement should be spawned"); + assert.equal(second.worker.terminated, false); + + second.worker.emit("message", { data: { ok: true, result: okResult } }); + await second.promise; + }); + it("rejects with the error the worker reports and still parks it", async () => { const { promise, worker } = startRun(); worker.emit("message", { data: { ok: false, error: "runner exploded" } });