fix(): key models by an internal uid to avoid race conditions + add load abort signal

[ShaMan123]

Description

Adds a signal option to load() (#173) and fixes the concurrency bugs found along the way.

The root cause: all model state, on both threads, was keyed by the consumer's modelId, and a modelId can be reused and therefore not unique. Late async work of a model that had been disposed or replaced landed on the next model loaded under the same modelId: worker replies, queued tiles, material transfers, progress events, an in-flight dispose.

How

• Internal uid. Each model gets a uid from FragmentsModels that is never reused. All internal routing uses it:

  • messages between threads;
  • worker assignment;
  • progress callbacks;
  • the tile queue;
  • material definitions;
  • the worker's mesh cache;
  • editor state.

  modelId stays the consumer-facing name: models.list, disposeModel(), abort() and the editor API are still keyed by it. The pattern and its rules are written up in CONTRIBUTING.md under "Design patterns".

• dispose() takes effect before it returns. The model leaves models.list and the scene, its worker slot is freed, an in-flight load is aborted, and later requests to it reject. The returned promise resolves once the worker has deleted it. The modelId can be loaded again right away. Anything the old model's worker still sends is dropped silently.

• load({ signal }). Aborting rejects load() with a LoadAbortedError right away, without waiting for the worker to reach its next yield point, and disposes the partial model. Disposing the model mid-load does the same.

• Typed messaging.

  • Requests from the workers are typed as WorkerRequest, and the workers' senders are checked against it.
  • FragmentsModel._invoke() only takes method names of the worker-side model, with matching arguments. Results are still cast at the call sites, because class instances in them arrive as plain objects.

Review feedback

• abort(modelId) is kept. It now shares the signal's code path: it aborts whichever load currently runs under that ID, with no bookkeeping of its own.
• load() no longer throws while a disposed model is still being deleted on the worker. With uid keying, that window is harmless, so the modelId is free as soon as dispose() is called.
• fetchMeshCompute:
  • Its sends are caught now, as are the workers' other fire-and-forget sends (material transfers and load progress).
  • The main thread awaits its handler and answers a request it fails to handle with an error, which it also logs. The workers ignore that answer.
  • Messages for a disposed model are dropped before anything can throw.
  • Answers go back through the port the request came in on and are never routed by model.

Bugs fixed (all reproducible on main)

• Loading a modelId that is already loaded or loading silently clobbered the first model. It now throws.
• Dispose-then-reload of a modelId:
  • the old model's late tiles, materials and progress events were applied to the new model;
  • the old dispose's threads.delete(modelId) could remove the new model's worker assignment.
• The worker's mesh cache is shared by all models on a worker and was keyed by a hash of the modelId. Two models with the same modelId on one worker read, and evicted, each other's meshes.
• Any request for a model without a worker (abort() of an ID that isn't loading, a call on a disposed model) spawned a new worker. Only CREATE_MODEL assigns one now; other requests reject.
• Answers to worker requests were routed by modelId instead of going back to the worker that asked.
• Disposing a model mid-load:
  • the load carried on and, with autoCoordinate: false, fired onModelLoaded for the dead model;
  • on the worker, the delete disposed the half-built model while it was still being generated. A delete that lands mid-load now aborts the load and waits for it to unwind.
• Requests pending on a worker that gets terminated never settled. They now reject.
• A FINISH from a disposed model was dropped before it could settle forceUpdateFinish().
• Editor state was keyed by modelId, so a reloaded or saved model inherited the old one's delta models and queued element requests:
  • delta models were never disposed with their parent;
  • concurrent edits orphaned a delta model.
• getItemsGeometry() and getGeometries() returned transform as the plain object the worker's copy arrives as, typed as a THREE.Matrix4, so calling a Matrix4 method on it threw.

Behavior changes

• load() of a modelId that is already loaded or loading throws.
• Aborted loads reject with a LoadAbortedError instance (it was the worker's error string). That includes an abort that lands after the worker finished, which main ignored.
• abort(modelId) for an ID that isn't loading does nothing (it spawned a worker).
• dispose(), disposeModel() and FragmentsModels.dispose() apply their effects synchronously and still return a promise.
• A call on a disposed model, including a stale Element or Item, or the old model after editor.save(), rejects with "model N is not loaded". Before, it went to whatever model owned the modelId.
• editor.save(): the new model gets its own materials instead of reusing the old one's. They leaked, since reused materials were never tracked for disposal. The old model's delta models stay in the scene until the new model replaces them.
• Queuing editor element requests (createMaterial() etc.) for a model that isn't loaded throws.
• Delta model IDs end in a counter (<modelId>-DELTA-MODEL-<n>) instead of performance.now(). EditUtils.getRootModelId() still parses them.
• model.modelId no longer reads model.object.name, so renaming the object doesn't change it.
• Worker protocol: messages address models by uid. The worker bundle must match the library version, which FragmentsModels.getWorker() already guarantees.

Internal API changes

These are documented as "don't use directly", but they are public on their classes:

• new FragmentsModel({ modelId, uid, meshManager, threads, editor, threadGroup }) takes a single object.
• new Editor(core) no longer takes the connection.
• model.threads.invoke(uid, …) takes the uid. Models use model._invoke(…), which is typed against VirtualFragmentsModel.
• MaterialManager.releaseModelSlot() is removed.
• RequestsManager.clean(uid) and FragmentsConnection.delete(uid) take the uid, and the latter returns a promise.
• Connection<TInput> and ThreadHandler<T> are generic, and the new WorkerRequest type is exported.
• VirtualFragmentsModel signatures now declare what the methods already handle:
  • the highlight and visibility methods accept no ids, meaning every item;
  • getItemsData() accepts GUIDs.

Code quality

Typed the message layer (MessageBase, WorkerRequest, _invoke()) in place of any, and collapsed some indirection in Connection. Routing is synchronous, so a request that can't be routed rejects instead of staying pending forever.

Additional context

The branch history is kept for reviewers of the first version. The rework after review:

• b48ef60 key models by an internal uid, dispose synchronously
• 2d759f1 keep abort(modelId) (reverting its removal in a2ca482)
• e0fc555 key editor state by the model's uid
• 99be14b type the requests workers send to the main thread, catch the workers' fire-and-forget sends
• 11e8e78 type _invoke() against the worker model's methods
• 6610477 return geometry transforms as THREE.Matrix4
• d1d3dc0 document the modelId vs uid pattern in CONTRIBUTING.md

6610477 stands on its own if you'd rather take it separately, and so does the editor commit.

Worth a close look:

• The synchronous dispose() contract in FragmentsModel.dispose / DataManager.dispose / FragmentsConnection.delete.
• The worker-side delete-during-load in ThreadModelDeleter.

Tests: 168 passed, plus 1 expected fail (the existing #300 repro). New coverage:

• load/abort/dispose races: load-abort.test.ts;
• connection deletion and worker termination: fragments-connection.test.ts;
• delete-during-load on the real worker controllers over school_arq.frag: thread-model-deleter.test.ts;
• editor state: edit-helper.test.ts;
• geometry transforms: edit-manager.test.ts;
• a disposed model's FINISH: mesh-manager-fence.test.ts.

I checked that the key tests fail without their fix. None of this has been exercised in a browser with real workers yet.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following:

• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Follow the Conventional Commits v1.0.0 standard for PR naming (e.g. feat(examples): add hello-world example).
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

🤖 Generated with Claude Code

[ShaMan123]

reviewed

[ShaMan123]

cleanup types and indirection

[ShaMan123]

this needs a decision - probably a rename

[agviegas]

On removing abort(modelId): once an API is published we cannot simply delete it, that would break every consumer already using it. Let's extend instead: keep abort(modelId) exactly as it is, fully supported, and add the signal option alongside it as an alternative way to do the same thing. Worth keeping in mind that the same rule then applies to the signal option itself: once it ships we are committed to it, so if you have any doubt about its shape, now is the moment to raise it rather than after release.

Two more things before this lands. load() now throws when a model is still being disposed worker-side, which is an async window a caller cannot observe, so a dispose-then-reload cycle that works today would start throwing. And Connection.fetchMeshCompute calls fetch without catching it, so a mesh compute racing a disposal becomes an unhandled rejection under the new throw. Both need a guard.

The rest of the analysis holds up well: we verified all three race claims against main, and the suite passes 129 of 129 with your changes applied.

[ShaMan123]

I understand that deleting the abort function is breaking. I addressed the why briefly in the description and stressed that the feature was intended to be an abort signal, that is what I requested to begin with.
The whole point of an abort signal is scoping aborting to a single closure without state handling.
abort as a public method requires holding state.
This is a design decision about async handling, which is important to the repo because of the threading arch - AbortSignal is the industry's standard, the repo should align to it.
That is why I deleted abort - IMO breaking changes are a must in order to keep the repo's code quality.
I can look into it a different approach in order to keep it - off the top of mind is keeping an AbortController map and filling it in load and perhaps marking it a deprecated.
Thoughts?

Regarding the throws. What do you suggest?

load() now throws when a model is still being disposed worker-side, which is an async window a caller cannot observe, so a dispose-then-reload cycle that works today would start throwing.

I can suggest that aborting returns a promise that resolves once completed. This will require the consumer to wait upon it - not good DX.
Perhaps this requires further design.
I don't have the full grasp of the code so I can only suggest abstract ideas.
Can aborting/deleting a model first detach all objects before starting the async work? Can the code guarantee the async message is processed in a way that respects the sync dropping (e.g. messages are queued and nothing can override the order, a return message for a dropped model is dropped as well)?
This will safeguard the model list from concurrency (a new model starts loading before an old one with the same id has completed the abort/deletion round trip).
Anything else doesn't fix the concurrency.

Connection.fetchMeshCompute calls fetch without catching it, so a mesh compute racing a disposal becomes an unhandled rejection under the new throw. Both need a guard.

I assume this is exactly the async round trip being able to drop the message of a dropped model.

[agviegas]

Agreed on the direction, and thanks for laying out the reasoning. AbortSignal is the right primitive and we are happy for it to become the recommended way to cancel a load. What we cannot do is remove abort(modelId) outright: a lot of people build on this library, and a published method disappearing breaks all of them at once. So let's go with the middle path you suggested: keep abort(modelId) working, backed by an AbortController map filled in load, and mark it deprecated with a pointer to the signal option. That gives users a migration path instead of a break.

On the dispose-then-reload window: rather than making load() throw while a model is still being disposed worker-side, could you drop that part from this PR? The underlying race, a new load starting before the old dispose round trip finishes, predates your change, and your idea of detaching synchronously and dropping replies for a disposed model is the right shape for it, so let's handle it as its own issue. The catch in fetchMeshCompute still needs to go in, since the new not-loaded guard in fetchConnection makes that rejection reachable.

[ShaMan123]

I will rework the PR.

On the dispose-then-reload window: rather than making load() throw while a model is still being disposed worker-side, could you drop that part from this PR? The underlying race, a new load starting before the old dispose round trip finishes, predates your change, and your idea of detaching synchronously and dropping replies for a disposed model is the right shape for it, so let's handle it as its own issue. The catch in fetchMeshCompute still needs to go in, since the new not-loaded guard in fetchConnection makes that rejection reachable.

I think the simplest solution would be to manage an internal id mapping. A model will get an internal id that is unique (growing counter), modelId will be treated as metadata, a name. Methods will map back the internal id to modelId making the entire issue go away.

[ShaMan123]

Looking into the fix - I prefer first fixing modelId not being unique issue and add aborting on top of that.
How do you want it? 2 separate PRs?
I will push everything here and cherry-pick if you want 2 PRs.

[ShaMan123]

More work spawned out of this branch so I want to plan how to move forward.

Recommended split

Stacked, in merge order. Each boundary was verified green on upstream/main, except where marked not replayed.

PR	Scope	Commits	Size
1	Route model calls through _invoke (P0, new)	new commit	14 files, +165/−309
2	Typed RPC, and rebuilding what the worker copies	11e8e78 6610477 e434dda 68fa665′ b9e85a1 7595c93′ 8813c97 4d7e496 a7606b7 d2603f0 4f19562, plus e090aa1 (not replayed)	can split into 2a (up to b9e85a1) and 2b
3	load({ signal }) and message-layer hardening	4b39c26 a2ca482 bf1237e 8a84f60 d3f0b16 3e9cb9d	5 source files
4	uid and synchronous dispose, with the fence fix	b48ef60′ 2d759f1 f2a6ab5 c4ce4c4	34 source files; can't be split further
5	Editor keyed by uid, save() and delta races, docs	e0fc555 2f7a1f2 65526d9 118e068 d1d3dc0	4 source files
6	Typed worker → main messages	99be14b a628456 ac18a75 4c6cdaa (not replayed)	after PR4; can go in parallel with PR5

′ marks a commit adapted by hand during the rebase.

[ShaMan123]

if you don't mind that everything is on this branch - then I can review it, tidy up and hand it off

[ShaMan123]

Regardless the design is clear - modelId is not a uid, using a uid behind scenes is the fix.
The rest are bugs that were discovered - bad any types were masking a few.