diff --git a/e2e/app.spec.ts b/e2e/app.spec.ts index e8fb2ff..f817b2e 100644 --- a/e2e/app.spec.ts +++ b/e2e/app.spec.ts @@ -287,3 +287,67 @@ 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 }) => { + // 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. + 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. + // + // 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, { width: 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 && ( -
+
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 }, );