Commit e712ef7
authored
fix(ecs): never generate, recycle or delete a renderer-reserved entity id (#1544)
* fix(ecs): never generate, recycle or delete a renderer-reserved entity id
`createEntityContainer` could hand a scene an entity id inside the
renderer-reserved range, where it collides exactly with a live remote
player's avatar entity.
Two defects combine:
1. The CRDT receive path calls `updateRemovedEntity` for every inbound
DELETE_ENTITY (systems/crdt/index.ts:153), including the renderer's own
tombstones for departed remote players. That records a reserved entity
NUMBER in `removedEntities`. Note the DELETE_ENTITY branch never
consults entity state at all, so `EntityState.Reserved` cannot skip it.
2. `generateEntity`'s recycling loop scans `removedEntities` with no
reserved-range filter, so it returns `toEntityId(reservedNumber,
version + 1)`.
The result is not merely an overlapping range but the SAME id: the
avatar-communication system reissues a vacated slot as
`toEntityId(number, storedVersion + 1)`, and this loop recycles it with
the same expression off the same stored version. The version bits that
were meant to separate the two allocators are what synchronise them.
`removeEntity` then compounds it by comparing the PACKED id against
`reservedStaticEntities`. The version occupies the high 16 bits, so that
only catches version 0 -- number 32 version 1 packs to 65568 and passes.
A scene could therefore delete a live remote player's entity, and each
such removal re-armed the slot at version+1, making every poisoned slot a
renewable source of collisions. `getEntityState` already decomposed
correctly; these now agree.
Fixes:
- `generateEntity`: skip free-list entries whose number is reserved.
- `removeEntity`: decompose before comparing, via a new
`isReservedEntityNumber` helper used by every site.
- `updateRemovedEntity` / `updateUsedEntity`: refuse reserved numbers, so
the free list only ever holds numbers the container owns. Reserved
numbers need no tombstone -- `getEntityState` reports them as `Reserved`
before consulting `removedEntities`, so `Removed` is unreachable.
- `Engine.removeEntity`: ask the container FIRST and purge components only
if it accepts. This is what actually stops the damage. Purging a refused
removal desynchronizes the scene from the renderer with no path back,
because the outgoing DELETE ops are dropped by the scene write guard, so
the renderer keeps the entity alive and never re-sends. A streamed
component (Transform) recovers on the next packet; a one-shot one
(PlayerIdentityData) does not, leaving a moving Transform with no
identity that is invisible to every
getEntitiesWith(PlayerIdentityData, ...) query the scene makes.
Field impact, from a 3h world-server log: of 223 player-left events, 16
had no peer disconnect within 3s and 15 of those shared a millisecond
with a blocked scene CRDT op, with none occurring without one. 13
identifiable players were silently removed from the authoritative scene's
world model while still connected and playing; none recovered, over
windows up to 24 minutes.
Adds test/ecs/reserved-entity-range.spec.ts (11 tests), which fails 5/11
without these changes. Imports avoid the package barrel deliberately: it
eagerly instantiates extended components and cannot be required from a
spec without a prior codegen build.
* test(ecs): cover the reserved-range guard in updateUsedEntity
The `isReservedEntityNumber` guard added to `updateUsedEntity` had no test: it
is unreachable from the CRDT receive path, because `getEntityState` returns
`Reserved` rather than `Unknown` for those numbers and only `Unknown` calls
`updateUsedEntity`. With this repo gating on 100% branch coverage, an
unreachable-but-present guard fails CI.
Asserted directly instead of dropping the guard, because `updateUsedEntity` is
public on `IEntityContainer` and its `v > 0` branch would otherwise seed
`removedEntities` with a reserved entity number — the same defect through a
different door.
Measured on packages/@dcl/ecs/src/engine/entity.ts: origin/main leaves 4
branches uncovered by the runnable specs; with this change it is 2, and every
branch the fix introduces is covered.
* fix(ecs): mask inline in the reserved check; refresh bundle snapshots
CI's `test` job failed on the bundle-size and VM-allocation snapshots. The
guard adds code to @dcl/ecs, so SCENE_COMPILED_JS_SIZE_PROD, MALLOC_COUNT,
ALIVE_OBJS_DELTA and MEMORY_USAGE_COUNT all shift and the committed
snapshots have to be regenerated. Only those metric lines changed across the
12 snapshot files — no CRDT output differs.
While regenerating, dropped an avoidable allocation the guard introduced:
isReservedEntityNumber called EntityUtils.fromEntityId, which builds a
[number, number] tuple, purely to read the entity number — on every
removeEntity / updateRemovedEntity / updateUsedEntity. Masks inline instead.
`entity & MAX_U16` already lands in [0, 65535], so fromEntityId's `>>> 0` is
a no-op for this comparison; verified equivalent across 104 (number, version)
pairs spanning the reserved boundary and both 16-bit extremes.
That saved ~0.1k of bundle size. It did NOT move MALLOC_COUNT, which stays at
17430: the boot path these snapshots measure never calls the guarded
functions, so the remaining delta is the closure itself, not per-call
allocation. Keeping the inline mask on its own merits.
Regenerated with `UPDATE_SNAPSHOTS=true` after `make build`. Note that
`make build` regenerates packages/@dcl/ecs/src/components/generated/, which
is untracked — a stale copy there makes 94 of 104 test/ecs suites fail to
load locally with an unrelated TouchScreenControls error.
* fix(ecs): type IEngine.removeEntity as boolean, matching what it returns
Review follow-up. The tests assert that removeEntity returns false for a
refused reserved-entity removal, but IEngine declared it `void`, so no caller
could act on that without a cast — `if (!engine.removeEntity(e))` does not
compile against a void return.
The mismatch predates this branch: main already ended removeEntity with
`return entityContainer.removeEntity(entity)`, and IEntityContainer has
always typed that as boolean. The API report shows both side by side, one
`void` and one `boolean`. What changed here is that the value became
meaningful — false now specifically means "refused, components untouched" —
so this is the right place to stop the type lying about it.
Non-breaking: widening a return type does not affect callers that ignore it,
and nothing implements IEngine. The only `Pick<IEngine, ... 'removeEntity'>`
consumers are removeEntityWithChildren / removeNetworkEntityChildrens in
tree.ts, both fed the internal removeEntity, which already returns boolean.
removeEntityWithChildren is deliberately left `void`: it delegates to
removeNetworkEntityChildrens, which is genuinely void, and iterates a tree
where a per-entity result has no single meaning.
Public API surface changes by exactly one line, regenerated via `make build`:
- removeEntity(entity: Entity): void;
+ removeEntity(entity: Entity): boolean;
Bundle snapshots are unchanged, as expected for a types-only edit. Full suite
green: 160 suites, 1216 tests.
* fix(ecs): keep purging the named static entities; make the regression test real
Two defects found in review of this branch.
1. Skipping the component purge for the WHOLE reserved range broke a working
API. `engine.removeEntity(engine.PlayerEntity)` previously purged the
scene's local components and emitted DELETE_COMPONENT on the wire; after
the first version of this change it did nothing at all — no purge, no
frame, no notification, and a `false` return every existing caller
discards. A scene clearing player state that way would leave the player
permanently locked by a stale InputModifier with nothing in the log.
The justification was wrong, not just the bound: the renderer denies scene
component ops only on the AVATAR range, so deletes on RootEntity/
PlayerEntity/CameraEntity do reach it and are applied — the frame is
byte-identical to InputModifier.deleteFrom(engine.PlayerEntity), which
demonstrably works. RESERVED_STATIC_ENTITIES was serving two different
ownership regimes.
Now split: the container still refuses to release ANY reserved id (that is
what caused the cross-wire), while the purge is skipped only for entities
the renderer streams — `isRendererStreamedNumber`, i.e. numbers 3 up to the
bound. Verified both directions: removeEntity(PlayerEntity) purges again,
removeEntity(#32 v1) still leaves a live avatar's components intact.
2. The headline regression test passed against the unfixed container, so it
pinned nothing. `removedEntities` is a Map iterated in insertion order and
the recycling loop returns the FIRST eligible entry — the old setup
released scene entity 512 before recording the avatar tombstone, so 512 was
handed out and the avatar key was never reached. The composite case was
worse: it only marked entities used, which leaves entityCounter at 512, so
`usedSize + 512 >= entityCounter` held and the recycling loop was never
entered at all.
Both rewritten to establish the two real preconditions — a standing
allocation deficit AND the avatar key leading the insertion order — with the
composite case now setting DCL_MAX_COMPOSITE_ENTITY for real rather than
simulating it. Added two tests that assert the preconditions themselves, so
the setup cannot silently stop reproducing. The spec now fails 8 of 16
against origin/main and passes 16/16 here.
Also hoists the reserved-range predicate to module scope so there is no
closure per container. Note this does NOT reduce bundle size — the second
predicate needed for (1) costs more than the closure saved, so
SCENE_COMPILED_JS_SIZE_PROD rises to 621.1k. Snapshots regenerated. Public API
surface unchanged (the new helper is @internal).
* fix(ecs): take the renderer-streamed bound from the container, not the default
Review follow-up (P2). Engine.removeEntity called isRendererStreamedNumber with
the module default of RESERVED_STATIC_ENTITIES, so the purge decision and the
id-release decision could disagree whenever a custom container is injected via
the public IEngineOptions.entityContainer seam. They are two halves of one
question and have to use the same bound.
Confirmed reachable, and it breaks in BOTH directions — the more damaging one
being the opposite of what you might expect:
createEntityContainer({ reservedStaticEntities: 64 }), entity #100 v0
container released the id? true
components purged? false
getEntitiesWith(C) after removal: 1 entry, still yields 100 = {"v":111}
The id goes back into the recycling pool while the entity stays visible to
component queries with its data intact — engine.removeEntity returns true and
the entity is not removed. A caller cannot detect that.
The other direction is the narrower one: with reservedStaticEntities: 1024,
entity #700 is renderer-owned per the container (release refused) yet the purge
ran anyway, reopening for 512..1024 exactly the corruption this branch fixes.
Fix: IEntityContainer exposes the bound it enforces, and Engine.removeEntity
uses it. The member is OPTIONAL, so a third-party IEntityContainer stays
source-compatible; when absent the engine falls back to the module default,
which is correct for any container that also uses it. Public API grows by one
line:
+ readonly reservedStaticEntities?: number;
Tests cover both directions and fail 3/21 without the change. Note the named
static entities stay at numbers 0-2 regardless of the bound — root, player and
camera are fixed by the protocol, not by the reserved range — and a bound below
3 degenerates safely to "always purge", matching a container that reserves
nothing.
Snapshots regenerated. Full suite: 160 suites, 1223 tests.
* refactor(ecs): classify via getEntityState instead of a new public member
Review sweep. The previous commit answered "is this entity renderer-owned?" by
adding `IEntityContainer.reservedStaticEntities` and an `@internal`
`isRendererStreamedNumber`. The container already answers it, on a non-optional
member every implementation must provide: `getEntityState` returns
`EntityState.Reserved` for exactly `(entity & MAX_U16) < reservedStaticEntities`.
Using that instead removes, in order of value:
- The new public API member. The api-report diff for this branch is now one
line — `removeEntity(entity: Entity): boolean` — which is what a bugfix
should cost.
- The silent-wrong-answer path. The optional member let a third-party container
that simply does not know about it inherit the 512 policy with no error; its
own doc comment admitted as much. `getEntityState` cannot be got wrong,
because implementing it is not optional.
- The `@internal` export and its never-exercised default parameter.
- ~25 lines of comment, and the re-indentation of the pre-existing purge loop.
Equivalence verified over 570 (bound, number, version) combinations spanning
bounds {3,64,512,1024,65535} and both 16-bit extremes: zero mismatches. Both
custom-bound regression blocks from the previous commit pass unchanged, so the
behaviour they pin is preserved.
`getEntityState` now routes through the same `isReservedEntity` helper as the
container's three guards, so the engine's classification and the container's
release decision are literally the same expression rather than two that agree
by convention. `NAMED_STATIC_ENTITIES` moves to engine/index.ts, next to the
RootEntity/PlayerEntity/CameraEntity fields it is derived from.
Tests: the named-static boundary had NO coverage — collapsing
NAMED_STATIC_ENTITIES to 0 left the entire suite green, so the distinction this
change spends its comments justifying was load-bearing prose. Added four cases
covering both sides of it, including number 3 as the first non-named reserved
number. Mutation-checked: `<` -> `<=`, 3 -> 0, 3 -> 4, inverting the purge
guard, dropping the lower bound, and always returning true are all killed now.
The one surviving mutant is the reserved-skip in generateEntity, which is
unreachable by construction — every writer into `removedEntities` refuses
reserved numbers — and the comment now says so instead of narrating history.
Comments cut roughly threefold against the package's actual conventions (its
longest existing prose block is 3 lines). The rationale is stated once, at the
decision site in engine/index.ts, rather than three times. Fixed a wrong claim
on the old NAMED_STATIC_ENTITIES docblock — "the three entity numbers a scene
may legitimately author components on"; a scene authors on all its own
entities, the point is that these three are reserved. Dropped the spec's
barrel-import justification, which was false: test/ecs/engine.spec.ts imports
the barrel and passes.
Full suite: 160 suites, 1227 tests. Snapshots regenerated.
* test(ecs): pass the required onChangeFunction, and pin it with an inbound message
Correctness review of this branch found the only certain defects on it were
inside its own regression test.
The two `Engine({ entityContainer })` calls omitted `onChangeFunction`, which
IEngineOptions declares as required. That is not cosmetic: engine/index.ts:310
is `options?.onChangeFunction(...)` — the optional chain guards `options`, not
the member — so with a partial options object the call is `undefined(...)`.
Those engines threw on their first inbound message. Every test in both blocks
passed because none of them ticked.
Verified directly:
WITH onChangeFunction -> update() resolved, component landed
WITHOUT onChangeFunction -> update() THREW: options?.onChangeFunction is not a function
no options at all -> update() resolved (the optional chain short-circuits)
Added a test per block that feeds a real inbound PUT through a transport,
because `onChangeFunction` is only invoked from the CRDT receive path — a bare
update() with no transport never reaches it, so my first two attempts at this
test were vacuous. Negative control: removing the option now fails 1 of 26 with
that TypeError.
Also rewrote the setup comment above the recycling block. It claimed the avatar
tombstone had to be recorded before any recyclable scene number "or the avatar
key is never reached" — true against an unfixed container, but on this branch
`updateRemovedEntity` refuses the tombstone outright, so `removedEntities` never
holds that key and the loop guard is never consulted. The tests were passing for
a different reason than the comment gave. Now says what the setup is actually
for: making them discriminate against unfixed source.
* fix(ecs): delegate engine.getEntityState instead of aliasing the container's
Two findings from the review of this branch that belong with it, because the
branch is what made them matter.
engine/index.ts exposed `getEntityState: partialEngine.entityContainer.getEntityState`
— a detached method reference, so calling `engine.getEntityState(e)` binds `this`
to the engine object rather than the container. Harmless for the built-in
container, whose implementation is a closure, but a custom IEntityContainer that
uses `this` silently reports the WRONG state through the public API while the
engine itself reports the right one, because removeEntity calls the container
directly. That divergence was cosmetic before; this branch made removeEntity
classify through getEntityState, so the public API would now contradict the
engine's own behaviour. Delegated instead of aliased.
Pinned with a class-based container that reads `this.bound`. Negative control:
restoring the aliased form fails 1 of 29.
Also documents on IEngine.removeEntityWithChildren that it can complete only
partially and does not report it. `removeEntity` refuses renderer-reserved
nodes, so a reserved node anywhere in a Transform tree survives while its
descendants are removed, leaving its `Transform.parent` pointing at a removed
entity. That is reachable only if a scene parents a reserved entity under a
scene entity, and fixing it properly means either skipping such subtrees or
returning a result — a public API change this PR should not carry. Documented
rather than silently left as a surprise.
Deliberately NOT fixed here: systems/crdt/index.ts purges every component for
any inbound DELETE_ENTITY with no reserved-range guard, which is reachable from
the network transport and lets a peer erase a live remote player's identity and
transform from a scene, which the scene then forwards onward. An unconditional
guard there would be wrong — the renderer's tombstone for a departed player MUST
purge — so the fix is transport-gating, a separate change to the CRDT receive
semantics that deserves its own review. Filing separately.
Public API surface unchanged: still one line for this branch.
Full suite: 160 suites, 1231 tests.1 parent 07a0182 commit e712ef7
17 files changed
Lines changed: 509 additions & 52 deletions
File tree
- packages/@dcl
- ecs/src/engine
- playground-assets/etc
- test
- ecs
- snapshots
- development-bundles
- production-bundles
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
101 | 101 | | |
102 | 102 | | |
103 | 103 | | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
104 | 115 | | |
105 | 116 | | |
106 | 117 | | |
| |||
143 | 154 | | |
144 | 155 | | |
145 | 156 | | |
146 | | - | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
147 | 162 | | |
148 | 163 | | |
149 | 164 | | |
| |||
158 | 173 | | |
159 | 174 | | |
160 | 175 | | |
161 | | - | |
| 176 | + | |
162 | 177 | | |
163 | 178 | | |
164 | 179 | | |
| |||
185 | 200 | | |
186 | 201 | | |
187 | 202 | | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
188 | 209 | | |
189 | 210 | | |
190 | 211 | | |
| |||
199 | 220 | | |
200 | 221 | | |
201 | 222 | | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
202 | 228 | | |
203 | 229 | | |
204 | 230 | | |
| |||
216 | 242 | | |
217 | 243 | | |
218 | 244 | | |
219 | | - | |
220 | | - | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
221 | 248 | | |
222 | 249 | | |
223 | 250 | | |
| 251 | + | |
| 252 | + | |
224 | 253 | | |
225 | 254 | | |
226 | 255 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
| 11 | + | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
32 | 35 | | |
33 | 36 | | |
34 | 37 | | |
| |||
49 | 52 | | |
50 | 53 | | |
51 | 54 | | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
52 | 69 | | |
53 | 70 | | |
54 | 71 | | |
| |||
57 | 74 | | |
58 | 75 | | |
59 | 76 | | |
60 | | - | |
| 77 | + | |
61 | 78 | | |
62 | 79 | | |
63 | 80 | | |
| |||
337 | 354 | | |
338 | 355 | | |
339 | 356 | | |
340 | | - | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
341 | 363 | | |
342 | 364 | | |
343 | 365 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
92 | 92 | | |
93 | 93 | | |
94 | 94 | | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
95 | 98 | | |
96 | | - | |
| 99 | + | |
97 | 100 | | |
98 | 101 | | |
99 | 102 | | |
100 | 103 | | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
101 | 108 | | |
102 | 109 | | |
103 | 110 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1461 | 1461 | | |
1462 | 1462 | | |
1463 | 1463 | | |
1464 | | - | |
| 1464 | + | |
1465 | 1465 | | |
1466 | 1466 | | |
1467 | 1467 | | |
| |||
0 commit comments