Skip to content

Give exports their own worker#60

Open
kmatzen wants to merge 2 commits into
mainfrom
split-export-worker
Open

Give exports their own worker#60
kmatzen wants to merge 2 commits into
mainfrom
split-export-worker

Conversation

@kmatzen

@kmatzen kmatzen commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Part of #51 — the head-of-line blocking half. This is not cancellation.

Stacked on #50 (base is worker-bridge-correlation-ids), because it builds directly on the correlation-id map. Same consequence as #59: ci.yml only triggers on PRs targeting main, so this gets no CI until #50 merges and GitHub retargets it. Flagging that up front this time.

The problem

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³ 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.

#50 fixed the settlement bug — before it, an export displaced by an edit never settled at all. But head-of-line blocking was untouched: the hang became a stall.

The change

Two WorkerChannels, each a worker plus its own ready handshake, from one spawn() helper. Evaluates go to one, both export formats to the other.

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 worker crash rejected everything outstanding; an eval-worker crash now leaves a running export alone.

What's still open in #51

A superseded evaluate runs to completion rather than being cancelled. evalSeq discards results, never work, so rapid edits during a gizmo drag still burn CPU on evaluations whose results are thrown away. They just no longer wait behind exports. Cooperative cancellation — a cancel set checked inside the subdivide recursion at sdfWorker.ts:93 — is the follow-up if you want it.

Testing limitation

Stating this plainly because the test names oversell it: FakeWorker does not block. It records postMessage calls and replays responses on demand, so the test asserting an evaluate settles while an export is outstanding would also pass against the single-worker bridge. It does not prove the viewport stays responsive.

What the tests do verify:

  • Routing — evaluates and exports reach separate worker instances. This is the mechanism.
  • Crash isolation — an eval-worker failure no longer rejects a running export. New behaviour.
  • Every correlation-id guarantee from Route worker responses by correlation id #50 still holds across the refactor.

That the viewport stays responsive during an export follows from the mechanism given the worker is single-threaded and runs each message to completion. That is a sound argument from the code, but it is an argument, not a test. Confirming it needs an e2e that starts an export and checks the viewport still evaluates.

No meaningful discrimination check. Unlike #56#59, I cannot show these tests failing on the base for the right reason: beforeEach asserts two workers exist, so on the single-worker base all 9 fail with the same harness assertion (expected length 2 but got 1). That tells you the harness changed, not that behaviour did.

Note on CI

If stacked PRs are going to recur here, the blocker is ci.yml's pull_request: branches: [main] filter. Dropping the filter would run CI on every PR regardless of base. One line, but it is a repo policy change so I have not made it.

🤖 Generated with Claude Code

@kmatzen

kmatzen commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

Added an e2e test (a874fe8) that measures the property the unit tests can't.

It starts a real 256³ export, waits for the progress bar to confirm it is genuinely in flight, mutates a parameter, waits for the resulting evaluation to land, 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 finishes and the bar is gone, so this fails against the pre-split bridge.

It 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 have fired and the test would have passed for the wrong reason.

Also adds data-testid="export-progress", which the progress bar lacked.

I could not run it locally. Playwright's bundled Chromium download failed repeatedly on my machine (killed mid-download, leaving corrupt bundles), and system Chrome launches but never completes its debugging handshake. So the honest status of this PR is:

claim evidence
requests route to separate workers unit tests ✅
a worker crash rejects only its own channel's requests unit tests ✅
correlation-id guarantees survive the refactor unit tests ✅
exports no longer block viewport evaluation e2e test written, not yet executed anywhere

CI will run the e2e suite on this PR once #50 merges and GitHub retargets it to main, which will confirm the passing direction. It will not by itself demonstrate that the test fails on the old design — if you want that, I can push a throwaway branch carrying this test without the worker split and open a draft PR so CI runs it red, then close it. Say the word; I did not want to create a deliberately-failing PR unasked.

@kmatzen
kmatzen force-pushed the worker-bridge-correlation-ids branch from 103c0e4 to 1ac903f Compare July 25, 2026 16:38
@kmatzen
kmatzen force-pushed the split-export-worker branch from a874fe8 to 2f3a0b3 Compare July 25, 2026 16:38
@kmatzen
kmatzen force-pushed the worker-bridge-correlation-ids branch from 1ac903f to 9d7a12b Compare July 25, 2026 17:00
@kmatzen
kmatzen changed the base branch from worker-bridge-correlation-ids to main July 25, 2026 17:08
kmatzen and others added 2 commits July 25, 2026 10:08
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@kmatzen
kmatzen force-pushed the split-export-worker branch from 2f3a0b3 to 9f70db8 Compare July 25, 2026 17:09
@github-actions

Copy link
Copy Markdown

🚀 Preview deployed: https://split-export-worker.sinter.pages.dev

(updates on every push to this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant