From f07142437f85a20d4053336d5dcdf2dec93ce333 Mon Sep 17 00:00:00 2001 From: Kevin Blackburn-Matzen Date: Sat, 18 Jul 2026 16:51:21 -0700 Subject: [PATCH 1/3] Give exports their own worker Part of #51 -- the head-of-line blocking half. Not cancellation. The worker runs each message to completion with no yield point (sdfWorker.ts:165), so with a single worker every viewport evaluation queued behind a 256-cubed export. useEvaluator fires on any store change, so the viewport froze for the duration of an export and then processed a backlog of superseded evaluations. Splitting the channels removes the contention rather than trying to interrupt the work. Two WorkerChannels, each a worker plus its own ready handshake. The pending map stays shared -- correlation ids are globally unique so routing is unchanged -- but each entry records its channel, so onerror rejects only that worker's requests. Previously a crash rejected everything; an eval worker crash now leaves a running export alone. Still outstanding from #51: a superseded evaluate runs to completion rather than being cancelled, so rapid edits during a drag still burn CPU on results that are discarded. Cooperative cancellation is the follow-up. Testing limitation, stated plainly: FakeWorker does not block, so the test asserting an evaluate settles during an export would also pass against the single-worker bridge. What the tests actually verify is routing (requests reach separate workers) and per-channel crash isolation. That the viewport stays responsive follows from the mechanism given the worker is single-threaded, but it is an argument, not a test -- confirming it needs an e2e that exports while checking the viewport still evaluates. Co-Authored-By: Claude Opus 4.8 --- src/engine/workerBridge.test.ts | 134 +++++++++++++++++++-------- src/engine/workerBridge.ts | 158 ++++++++++++++++++++------------ 2 files changed, 192 insertions(+), 100 deletions(-) diff --git a/src/engine/workerBridge.test.ts b/src/engine/workerBridge.test.ts index 2b1594b..49dd25c 100644 --- a/src/engine/workerBridge.test.ts +++ b/src/engine/workerBridge.test.ts @@ -2,20 +2,23 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import type { WorkerRequest } from '../types/geometry'; /** - * Regression tests for the WorkerBridge correlation-id protocol. + * Regression tests for the WorkerBridge protocol. * - * The first two cases are the counterexamples TLC produced against the old - * single-handler design (specs/WorkerBridge.tla), replayed against the real - * bridge. Both fail on that design and pass on the current one. + * The correlation-id cases replay counterexamples TLC produced against the + * original single-handler design (specs/WorkerBridge.tla). The channel cases + * cover the export/evaluate split: exports get their own worker so viewport + * evaluations never queue behind a 256³ export. */ class FakeWorker { - static latest: FakeWorker; + /** Every worker constructed, in order: [0] evaluates, [1] exports. */ + static instances: FakeWorker[] = []; + onmessage: ((e: MessageEvent) => void) | null = null; onerror: ((e: any) => void) | null = null; sent: WorkerRequest[] = []; - constructor() { FakeWorker.latest = this; } + constructor() { FakeWorker.instances.push(this); } postMessage(msg: WorkerRequest) { this.sent.push(msg); } terminate() {} @@ -44,29 +47,98 @@ function sdfResponse(rid: number, glsl: string) { }; } +const exportResult = (rid: number) => ({ + type: 'exportResult', rid, format: 'stl', data: new ArrayBuffer(8), +}); + let bridge: typeof import('./workerBridge').workerBridge; -let worker: FakeWorker; +let evalWorker: FakeWorker; +let exportWorker: FakeWorker; beforeEach(async () => { vi.resetModules(); + FakeWorker.instances = []; vi.stubGlobal('Worker', FakeWorker); bridge = (await import('./workerBridge')).workerBridge; - worker = FakeWorker.latest; - worker.emit({ type: 'ready' }); + + expect(FakeWorker.instances).toHaveLength(2); + [evalWorker, exportWorker] = FakeWorker.instances; + evalWorker.emit({ type: 'ready' }); + exportWorker.emit({ type: 'ready' }); await flush(); }); +describe('WorkerBridge channels', () => { + it('sends evaluates and exports to different workers', async () => { + void bridge.evaluate(null); + void bridge.exportSTL(null); + await flush(); + + expect(evalWorker.sent.map((r) => r.type)).toEqual(['evaluate']); + expect(exportWorker.sent.map((r) => r.type)).toEqual(['exportSTL']); + }); + + it('settles an evaluate while an export is still running', async () => { + // The point of the split. With one worker the export occupied it and the + // evaluate could not be answered until the export finished, freezing the + // viewport for the duration. + const exported = track(bridge.exportSTL(null)); + await flush(); + const evaluated = track(bridge.evaluate(null)); + await flush(); + + evalWorker.emit(sdfResponse(evalWorker.sent[0].rid, 'GLSL')); + await flush(); + + expect(evaluated.settled).toBe(true); + expect(evaluated.value?.glsl).toBe('GLSL'); + expect(exported.settled).toBe(false); + + // And the export still completes afterwards. + exportWorker.emit(exportResult(exportWorker.sent[0].rid)); + await flush(); + expect(exported.value).toBeInstanceOf(Blob); + }); + + it('routes both export formats to the export worker', async () => { + void bridge.exportSTL(null); + await flush(); + void bridge.export3MF(null); + await flush(); + + expect(exportWorker.sent.map((r) => r.type)).toEqual(['exportSTL', 'export3MF']); + expect(evalWorker.sent).toHaveLength(0); + }); + + it('rejects only the crashed worker\'s requests', async () => { + const exported = track(bridge.exportSTL(null)); + const evaluated = track(bridge.evaluate(null)); + await flush(); + + evalWorker.fail('eval worker died'); + await flush(); + + expect(evaluated.error?.message).toBe('eval worker died'); + // The export lives on a different worker and must be untouched. + expect(exported.settled).toBe(false); + + exportWorker.emit(exportResult(exportWorker.sent[0].rid)); + await flush(); + expect(exported.value).toBeInstanceOf(Blob); + }); +}); + describe('WorkerBridge correlation ids', () => { it('does not settle one evaluate with another evaluate\'s response', async () => { // TLC counterexample for NoCrossTalk (specs/WorkerBridge.tla). const first = track(bridge.evaluate(null)); const second = track(bridge.evaluate(null)); await flush(); - expect(worker.sent).toHaveLength(2); + expect(evalWorker.sent).toHaveLength(2); // The worker answers the FIRST request while the second is still in flight. - worker.emit(sdfResponse(worker.sent[0].rid, 'FIRST')); - worker.emit(sdfResponse(worker.sent[1].rid, 'SECOND')); + evalWorker.emit(sdfResponse(evalWorker.sent[0].rid, 'FIRST')); + evalWorker.emit(sdfResponse(evalWorker.sent[1].rid, 'SECOND')); await flush(); // The old design resolved `second` with FIRST's geometry here. @@ -77,25 +149,21 @@ describe('WorkerBridge correlation ids', () => { }); it('settles an export whose handler would have been displaced by an evaluate', async () => { - // TLC counterexample for NoOrphan: the export's handler is overwritten by - // the evaluate, and its exportResult then matches neither branch of the - // evaluate handler. - const data = new ArrayBuffer(8); + // TLC counterexample for NoOrphan. The single handler slot is gone, and so + // is the shared worker, but the settlement guarantee is what matters. const exported = track(bridge.exportSTL(null)); await flush(); - const exportRid = worker.sent[0].rid; - const evaluated = track(bridge.evaluate(null)); await flush(); - worker.emit({ type: 'exportResult', rid: exportRid, format: 'stl', data }); + exportWorker.emit(exportResult(exportWorker.sent[0].rid)); await flush(); // The old design left this pending forever — Toolbar's await never returned. expect(exported.settled).toBe(true); expect(exported.value).toBeInstanceOf(Blob); - worker.emit(sdfResponse(worker.sent[1].rid, 'GLSL')); + evalWorker.emit(sdfResponse(evalWorker.sent[0].rid, 'GLSL')); await flush(); expect(evaluated.settled).toBe(true); }); @@ -108,8 +176,8 @@ describe('WorkerBridge correlation ids', () => { void bridge.export3MF(null, (stage) => mf.push(stage)); await flush(); - worker.emit({ type: 'progress', rid: worker.sent[0].rid, stage: 'stl-stage', percent: 10 }); - worker.emit({ type: 'progress', rid: worker.sent[1].rid, stage: '3mf-stage', percent: 20 }); + exportWorker.emit({ type: 'progress', rid: exportWorker.sent[0].rid, stage: 'stl-stage', percent: 10 }); + exportWorker.emit({ type: 'progress', rid: exportWorker.sent[1].rid, stage: '3mf-stage', percent: 20 }); await flush(); expect(stl).toEqual(['stl-stage']); @@ -122,40 +190,28 @@ describe('WorkerBridge correlation ids', () => { const evaluated = track(bridge.evaluate(null)); await flush(); - worker.emit({ type: 'error', rid: worker.sent[0].rid, message: 'No geometry to export' }); + exportWorker.emit({ type: 'error', rid: exportWorker.sent[0].rid, message: 'No geometry to export' }); await flush(); expect(exported.error?.message).toBe('No geometry to export'); expect(evaluated.settled).toBe(false); - worker.emit(sdfResponse(worker.sent[1].rid, 'GLSL')); + evalWorker.emit(sdfResponse(evalWorker.sent[0].rid, 'GLSL')); await flush(); expect(evaluated.value?.glsl).toBe('GLSL'); }); - it('rejects every outstanding request when the worker itself fails', async () => { - const exported = track(bridge.exportSTL(null)); - const evaluated = track(bridge.evaluate(null)); - await flush(); - - worker.fail('boom'); - await flush(); - - expect(exported.error?.message).toBe('boom'); - expect(evaluated.error?.message).toBe('boom'); - }); - it('ignores a duplicate response for an already-settled request', async () => { const evaluated = track(bridge.evaluate(null)); await flush(); - const rid = worker.sent[0].rid; + const rid = evalWorker.sent[0].rid; - worker.emit(sdfResponse(rid, 'GLSL')); + evalWorker.emit(sdfResponse(rid, 'GLSL')); await flush(); expect(evaluated.value?.glsl).toBe('GLSL'); // Must not throw, and must not disturb any later request's entry. - expect(() => worker.emit(sdfResponse(rid, 'AGAIN'))).not.toThrow(); + expect(() => evalWorker.emit(sdfResponse(rid, 'AGAIN'))).not.toThrow(); await flush(); expect(evaluated.value?.glsl).toBe('GLSL'); }); diff --git a/src/engine/workerBridge.ts b/src/engine/workerBridge.ts index 7b1ffb7..c6664c6 100644 --- a/src/engine/workerBridge.ts +++ b/src/engine/workerBridge.ts @@ -4,6 +4,12 @@ import type { SDFDisplayData } from '../store/modelerStore'; type ProgressHandler = (stage: string, percent: number) => void; +/** A worker plus its `ready` handshake. */ +interface WorkerChannel { + worker: Worker; + ready: Promise; +} + /** * One in-flight request. Registered under its correlation id when the request * is posted, removed when — and only when — its promise settles. @@ -18,102 +24,130 @@ interface PendingRequest { seq: number; /** Blob MIME type for exports. */ mime: string; + /** Which channel is serving this request, so a crash rejects only its own. */ + channel: WorkerChannel; resolve: (value: any) => void; reject: (error: Error) => void; onProgress?: ProgressHandler; } class WorkerBridge { - private worker: Worker; - private readyPromise: Promise; - private resolveReady!: () => void; - /** Correlation id -> in-flight request. Replaces the old single handler slot. */ + /** + * Two workers, because the worker runs each message to completion with no + * yield point (sdfWorker.ts:165). A 256³ export takes seconds, and with a + * single worker every viewport evaluation queued behind it — useEvaluator + * fires on any store change, so the viewport froze for the duration of an + * export. Giving exports their own channel removes the contention instead + * of trying to interrupt the work. + * + * This is not cancellation: a superseded evaluate still runs to completion, + * it just no longer waits on an export. See #51. + */ + private evalChannel: WorkerChannel; + private exportChannel: WorkerChannel; + + /** Correlation id -> in-flight request, shared across both channels. */ private pending = new Map(); private nextRid = 1; private evalSeq = 0; constructor() { - this.readyPromise = new Promise((resolve) => { this.resolveReady = resolve; }); + this.evalChannel = this.spawn(); + this.exportChannel = this.spawn(); + } - this.worker = new Worker(new URL('../worker/sdfWorker.ts', import.meta.url), { type: 'module' }); + private spawn(): WorkerChannel { + let resolveReady!: () => void; + const ready = new Promise((resolve) => { resolveReady = resolve; }); - this.worker.onmessage = (event: MessageEvent) => { - const msg = event.data; - if (msg.type === 'ready') { this.resolveReady(); return; } + const worker = new Worker(new URL('../worker/sdfWorker.ts', import.meta.url), { type: 'module' }); + const channel: WorkerChannel = { worker, ready }; - // Route by correlation id alone — never by message type. A response for - // an unknown id is one we have already settled; drop it. - const req = this.pending.get(msg.rid); - if (!req) return; + worker.onmessage = (event: MessageEvent) => { + const msg = event.data; + if (msg.type === 'ready') { resolveReady(); return; } + this.dispatch(msg); + }; - if (msg.type === 'progress') { - req.onProgress?.(msg.stage, msg.percent); - return; + // A worker-level failure settles everything outstanding on THAT channel; + // otherwise those promises hang. Requests on the other worker are + // unaffected and must be left alone. + worker.onerror = (err) => { + console.error('Worker error:', err); + for (const [rid, req] of [...this.pending]) { + if (req.channel !== channel) continue; + this.pending.delete(rid); + req.reject(new Error(err.message || 'Worker error')); } + }; - this.pending.delete(msg.rid); + return channel; + } - if (msg.type === 'error') { - req.reject(new Error(msg.message)); - return; - } + /** Route a response by correlation id alone — never by message type. */ + private dispatch(msg: Extract) { + // A response for an unknown id is one we have already settled; drop it. + const req = this.pending.get(msg.rid); + if (!req) return; - if (req.kind === 'evaluate') { - if (msg.type !== 'sdf') { - req.reject(new Error(`Unexpected '${msg.type}' response for evaluate request`)); - return; - } - // A superseded evaluate still settles. Staleness is the caller's - // concern — useEvaluator has its own evalSeq guard — but the promise - // must never be stranded. - if (req.seq !== this.evalSeq) { req.resolve(null); return; } - if (!msg.glsl) { req.resolve(null); return; } - req.resolve({ - glsl: msg.glsl, - paramCount: msg.paramCount, - paramValues: msg.paramValues, - textures: msg.textures || [], - bbMin: msg.bbMin, - bbMax: msg.bbMax, - hasWarn: !!msg.hasWarn, - }); - return; - } + if (msg.type === 'progress') { + req.onProgress?.(msg.stage, msg.percent); + return; + } - if (msg.type !== 'exportResult') { - req.reject(new Error(`Unexpected '${msg.type}' response for export request`)); - return; - } - req.resolve(new Blob([msg.data], { type: req.mime })); - }; + this.pending.delete(msg.rid); - // A worker-level failure settles everything outstanding; otherwise every - // pending promise hangs forever. - this.worker.onerror = (err) => { - console.error('Worker error:', err); - const outstanding = [...this.pending.values()]; - this.pending.clear(); - for (const req of outstanding) { - req.reject(new Error(err.message || 'Worker error')); + if (msg.type === 'error') { + req.reject(new Error(msg.message)); + return; + } + + if (req.kind === 'evaluate') { + if (msg.type !== 'sdf') { + req.reject(new Error(`Unexpected '${msg.type}' response for evaluate request`)); + return; } - }; + // A superseded evaluate still settles. Staleness is the caller's + // concern — useEvaluator has its own evalSeq guard — but the promise + // must never be stranded. + if (req.seq !== this.evalSeq) { req.resolve(null); return; } + if (!msg.glsl) { req.resolve(null); return; } + req.resolve({ + glsl: msg.glsl, + paramCount: msg.paramCount, + paramValues: msg.paramValues, + textures: msg.textures || [], + bbMin: msg.bbMin, + bbMax: msg.bbMax, + hasWarn: !!msg.hasWarn, + }); + return; + } + + if (msg.type !== 'exportResult') { + req.reject(new Error(`Unexpected '${msg.type}' response for export request`)); + return; + } + req.resolve(new Blob([msg.data], { type: req.mime })); } private async issue( + channel: WorkerChannel, build: (rid: number) => WorkerRequest, - entry: Omit, + entry: Omit, ): Promise { - await this.readyPromise; + await channel.ready; const rid = this.nextRid++; return new Promise((resolve, reject) => { - this.pending.set(rid, { ...entry, resolve, reject }); - this.worker.postMessage(build(rid)); + this.pending.set(rid, { ...entry, channel, resolve, reject }); + channel.worker.postMessage(build(rid)); }); } async evaluate(tree: SDFNodeUI | null, _resolution?: number, clip?: ClipPlane): Promise { const seq = ++this.evalSeq; return this.issue( + this.evalChannel, (rid) => ({ type: 'evaluate', rid, tree, clip }), { kind: 'evaluate', seq, mime: '' }, ); @@ -121,6 +155,7 @@ class WorkerBridge { async exportSTL(tree: SDFNodeUI | null, onProgress?: ProgressHandler): Promise { return this.issue( + this.exportChannel, (rid) => ({ type: 'exportSTL', rid, tree }), { kind: 'export', seq: 0, mime: 'application/octet-stream', onProgress }, ); @@ -128,6 +163,7 @@ class WorkerBridge { async export3MF(tree: SDFNodeUI | null, onProgress?: ProgressHandler): Promise { return this.issue( + this.exportChannel, (rid) => ({ type: 'export3MF', rid, tree }), { kind: 'export', seq: 0, mime: 'application/vnd.ms-package.3dmanufacturing-3dmodel+xml', onProgress }, ); From 9f70db8150985a54131a93c2ff47df26a5bd7091 Mon Sep 17 00:00:00 2001 From: Kevin Blackburn-Matzen Date: Sat, 18 Jul 2026 18:29:08 -0700 Subject: [PATCH 2/3] Add e2e test that an export does not block viewport evaluation The unit tests for the export/evaluate worker split verify routing and per-channel crash isolation, but they cannot verify the property #51 is actually about: FakeWorker never blocks, so "evaluate settles during an export" passes against the single-worker bridge too. This test uses real workers. It starts a 256-cubed export, waits for the progress bar to confirm it is genuinely in flight, mutates a parameter, waits for the resulting evaluation to land, and then asserts the export progress bar is STILL visible. That ordering is the point: on a single worker the evaluation cannot arrive until the export has finished and the bar is gone. Compares paramValues rather than glsl as the change signal. Parameters compile to u_p[] uniforms, so editing a radius changes the values without changing the GLSL string -- watching glsl would never fire and the test would pass for the wrong reason. Adds data-testid="export-progress" to the progress bar, which had no test hook. Co-Authored-By: Claude Opus 4.8 --- e2e/app.spec.ts | 51 ++++++++++++++++++++++++++++++ src/components/toolbar/Toolbar.tsx | 2 +- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/e2e/app.spec.ts b/e2e/app.spec.ts index e8fb2ff..19fe3ed 100644 --- a/e2e/app.spec.ts +++ b/e2e/app.spec.ts @@ -287,3 +287,54 @@ test.describe('Modeler: Export', () => { await expect(stl).toBeEnabled(); }); }); + +test.describe('Modeler: Worker concurrency', () => { + test.beforeEach(async ({ page }) => { + await enterModeler(page); + }); + + // The property #51 is actually about: an export must not block viewport + // evaluation. The worker runs each message to completion with no yield + // point, so with a single shared worker every evaluation queued behind a + // 256³ export and the viewport froze for its duration. + // + // The unit tests cannot show this — their fake worker never blocks. This + // one exercises real workers doing real work, and fails against a + // single-worker bridge. + test('viewport keeps evaluating while an export runs', async ({ page }) => { + await addShape(page, 'Box'); + + // Wait for the first evaluation to settle so we have a baseline. + await page.waitForFunction(() => { + const s = (window as any).__MODELER_STORE__; + return s?.sdfDisplay && !s.evaluating; + }, null, { timeout: 15000 }); + + const baseline = await page.evaluate(() => + JSON.stringify((window as any).__MODELER_STORE__.sdfDisplay.paramValues)); + + // Kick off an export and wait until it is genuinely in flight. + await page.locator('[title="Export STL"]').click(); + const progress = page.locator('[data-testid="export-progress"]'); + await expect(progress).toBeVisible({ timeout: 15000 }); + + // Mutate the model while the export is running. This changes paramValues, + // so a completed evaluation is observable without a structural edit. + await page.evaluate(() => { + const store = (window as any).__MODELER_STORE__; + store.updateNodeParams(store.tree.id, { radius: 7 }); + }); + + // The evaluation must complete... + await page.waitForFunction((prev) => { + const s = (window as any).__MODELER_STORE__; + return s?.sdfDisplay && !s.evaluating + && JSON.stringify(s.sdfDisplay.paramValues) !== prev; + }, baseline, { timeout: 15000 }); + + // ...while the export is still running. This ordering is the whole point: + // on a single worker the evaluation could not land until the export + // finished and the progress bar had already gone. + await expect(progress).toBeVisible(); + }); +}); diff --git a/src/components/toolbar/Toolbar.tsx b/src/components/toolbar/Toolbar.tsx index c92550c..d6d95dc 100644 --- a/src/components/toolbar/Toolbar.tsx +++ b/src/components/toolbar/Toolbar.tsx @@ -243,7 +243,7 @@ export function Toolbar({ onMobileTree, onMobileProps }: { onMobileTree?: () => {exportProgress && ( -
+
Date: Sat, 25 Jul 2026 14:53:27 -0700 Subject: [PATCH 3/3] Fix the export-concurrency e2e: it mutated a param the Box does not have MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test added a Box and then set `{ radius: 7 }` on it to make a completed evaluation observable. A box's params are width/height/depth (NODE_DEFAULTS.box), and convert.ts reads only those, so the extra key left the emitted uniforms byte-identical. The wait for `paramValues !== baseline` could therefore never succeed, whatever the workers were doing — the test failed against its own feature. Setting `width` instead changes size[0]/2, which codegen emits as a uniform, so the evaluation is observable without a structural edit as intended. Also marked the test slow. It waits on a real 256-cubed export and runs 25-27s locally against the 30s default; on the CI run that exposed the bug, the 30s budget expired before the inner 15s wait had even begun. The suite uses 6.2m of its 10m global budget, so tripling one test's ceiling is affordable and beats leaving a real pass dependent on runner speed. Verified: 3/3 passes on this branch, and 3/3 with main merged in (24.7s, 26.6s, 27.3s), so the slower interval-based mesher from #75 does not push it over — the export dominates. Co-Authored-By: Claude Opus 5 (1M context) --- e2e/app.spec.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/e2e/app.spec.ts b/e2e/app.spec.ts index 19fe3ed..f817b2e 100644 --- a/e2e/app.spec.ts +++ b/e2e/app.spec.ts @@ -302,6 +302,13 @@ test.describe('Modeler: Worker concurrency', () => { // one exercises real workers doing real work, and fails against a // single-worker bridge. test('viewport keeps evaluating while an export runs', async ({ page }) => { + // Unlike the rest of the suite this waits on a real 256-cubed export, so it + // runs 25-27s on a fast machine against the 30s default. CI is slower — + // on the run that first exposed the bug below, the 30s test budget expired + // before the inner 15s wait had even started. Triple it rather than leave + // a genuine pass hostage to the runner's speed. + test.slow(); + await addShape(page, 'Box'); // Wait for the first evaluation to settle so we have a baseline. @@ -320,9 +327,15 @@ test.describe('Modeler: Worker concurrency', () => { // Mutate the model while the export is running. This changes paramValues, // so a completed evaluation is observable without a structural edit. + // + // It has to be a parameter this shape actually has: the node here is a Box, + // whose params are width/height/depth (NODE_DEFAULTS.box). Setting an + // unrelated key leaves the emitted uniforms byte-identical — convert.ts + // reads only width/height/depth for a box — so the wait below could never + // observe a change, whatever the workers were doing. await page.evaluate(() => { const store = (window as any).__MODELER_STORE__; - store.updateNodeParams(store.tree.id, { radius: 7 }); + store.updateNodeParams(store.tree.id, { width: 7 }); }); // The evaluation must complete...