fix: sdk reserved entities recycle version bump - #9847
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 12191 => 12201 — remove at least 11 warnings to merge. Warnings/errors in files changed by this PR (34)All Unity tests passed ✅
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix: sdk reserved entities recycle version bump
STEP 1 — Context & Scope
Files reviewed:
Explorer/Assets/DCL/Infrastructure/CRDT/CRDTEntity.cs— addedMAX_VERSIONconstantExplorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerCRDTEntitiesHandlerSystem.cs— version tracking for reserved entity numbersExplorer/Assets/DCL/Multiplayer/SDK/Tests/PlayerCRDTEntitiesHandlerSystemShould.cs— 4 new tests + updated assertions
Surrounding files read in full:
PlayerCRDTEntitycomponent (Components/PlayerCRDTEntity.cs)SpecialEntitiesID(CrdtEcsBridge/Components/SpecialEntitiesID.cs)MultiplayerPlugin(global plugin registration)- Downstream consumers:
PlayerSceneCRDTEntity, propagation systems,OutgoingCRDTMessagesProvider
Reference docs: CLAUDE.md, review-instructions.md
STEP 2 — Root-cause check: ✅ PASS
Problem: ADR-245 requires a new generation (version) every time a reserved entity number (32–255) is reassigned to a different remote player. Unity Explorer never bumped the version — the pool was a bare bool[224], so every player inheriting a slot got version 0. Scenes store deleted entities as number → version and discard messages whose version isn't strictly greater. A tombstoned slot made every subsequent player on that number invisible to that scene.
Fix: Adds per-slot version tracking. Each recycle bumps the generation. This addresses the root cause (missing version increment), not a symptom. The fix is correct and aligns with Hammurabi, Bevy, and Godot implementations.
STEP 3 — Design & integration: ✅ PASS
Owner search:
- The reserved entity number pool is owned exclusively by
PlayerCRDTEntitiesHandlerSystem(created in constructor viaClearReservedEntities(), managed throughTryReserveNextFreeEntity/FreeReservedEntity). - No other system creates, destroys, or manages this pool.
MultiplayerPlugin.cs:167— registered as a global plugin system, created once per session.
Assessment: The version tracking is added to the existing lifecycle owner. No new units are introduced — reservedEntityVersions[] is a parallel tracking structure alongside the existing reservedEntities[]. This is the natural and correct home for this state.
Pre-existing note: CLAUDE.md §1 states "Systems must not contain state — all state goes into ECS." The system already held persistent state (reservedEntities[], currentReservedEntitiesCount, reservedEntitiesExhaustionReported) before this PR. The entity number pool is a global resource shared across all entities (not per-entity state), which is a legitimate exception. This PR extends an existing pattern — it does not introduce the concern.
Teardown/consumption trace: No subscriptions, callbacks, or event hookups added. The int[] is a plain array requiring no disposal. ClearReservedEntities() resets both arrays. ✅
STEP 4 — Member audit: ✅ PASS
| Member | Visibility | Consumers | Assessment |
|---|---|---|---|
CRDTEntity.MAX_VERSION |
public const |
FreeReservedEntity (line 216), test RetireReservedNumberWhenItRunsOutOfVersions (line 442) |
Correctly public — other code may need to reference the version bound. Naming follows project convention (SCREAMING_SNAKE_CASE, matching SpecialEntitiesID constants). ✅ |
TryReserveNextFreeEntity(out CRDTEntity) |
private |
AddPlayerCRDTEntity (line 73) |
Single caller. Replaces sentinel-returning ReserveNextFreeEntity(). The Try pattern with out is idiomatic C# and eliminates the ambiguity of the old -1 sentinel when version bits live in Id. ✅ |
FreeReservedEntity(CRDTEntity) |
private |
RemoveComponent (line 160) |
Single caller. Changed from int entityId to CRDTEntity — indexes by EntityNumber (line 208), which correctly extracts the number from the packed id via Id & 0xffff. ✅ |
STEP 5 — Line-level review
Pass A — Blocking issues: None found.
Bit-packing correctness verified:
CRDTEntity.Create(32, 0xffff)→32 | (0xffff << 16)→0xffff0020(negative as signed int)EntityNumber:0xffff0020 & 0xffff=0x0020= 32 ✅EntityVersion:(0xffff0020 >> 16) & 0xffff— C# arithmetic right shift sign-extends, giving0xffffffff, then& 0xffff=0xffff= 65535 ✅crdtEntity.Id == SpecialEntitiesID.PLAYER_ENTITYchecks inResolvePlayerCRDTScene(line 117) andRemovePlayerFromScene(line 169) remain correct — local player (PLAYER_ENTITY=1) is never pooled, so its version is always 0 and its Id always equals 1 ✅
Version lifecycle verified:
- Init:
reservedEntityVersions[i] = -1(inClearReservedEntities, called from constructor) - First handout:
++(-1) = 0→ version 0 ✅ - Recycle:
++(prev)→ monotonically increasing ✅ - Retirement:
reservedEntityVersions[index] >= MAX_VERSION→ slot staystrue, count not decremented →TryReserveNextFreeEntitycorrectly skips retired slots ✅ - No window exists between field initializer (
new int[224], defaults to 0) andClearReservedEntities()because the object isn't accessible before the constructor completes ✅
Idempotency verified:
- Double-free guard (
if (!reservedEntities[index]) return;, line 212) prevents count corruption ✅ - Retired slot:
reservedEntities[index]staystrue, so a second free attempt would hit the retirement check and log again — but this is impossible in practice because a retired slot is never handed out again, so no entity will ever hold a CRDTEntity with that retired number+version ✅
currentReservedEntitiesCount accounting verified:
- Increment: line 193 (on reservation) ✅
- Decrement: line 226 (on non-retired free) ✅
- No decrement on retirement: correct — retired slots are permanently occupied ✅
- Full retirement scenario: if all 224 slots are retired,
currentReservedEntitiesCount == 224 == reservedEntities.Length→TryReserveNextFreeEntityreturnsfalseat the short-circuit (line 185) ✅
No allocation in hot path: TryReserveNextFreeEntity and FreeReservedEntity do no allocations. The string interpolation in the retirement warning (line 218) only fires once per slot lifetime (on the slot's final free), not per-frame. ✅ (CLAUDE.md §4)
Pass B — Design smells: None found. No new magic values, no naming issues, no encapsulation violations.
Test quality:
- 4 new tests cover: version propagation to scene entity, independent per-number versioning, slot retirement at MAX_VERSION, and correct fallback to the next slot after retirement.
- Existing tests updated to assert full
CRDTEntityequality (number + version) instead of rawId— strengthens the assertions. - AAA pattern followed. NUnit + NSubstitute used correctly. ✅ (CLAUDE.md §10)
RetireReservedNumberWhenItRunsOutOfVersionsloops 65,536 times to exhaust the version space — necessary to test the boundary condition and acceptable given the lightweight per-iteration cost (no I/O, minimal ECS operations on a single-entity world).
STEP 6 — Complexity
COMPLEX — modifies CRDT entity id assignment and version management in the multiplayer synchronization layer. Directly affects how remote players are identified across scene boundaries.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes runtime multiplayer code that determines whether remote players are visible in scenes. The fix addresses a player-facing visibility bug. The PR includes detailed QA test steps.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
Security Review
No security issues found.
- No secrets, tokens, or credentials in the diff
- No user input handling — the entity pool is managed internally by the system
- No auth/authz changes
- Entity numbers in log messages are not sensitive data
- Version exhaustion attack (rapid connect/disconnect to retire all slots) would require ~14.7M cycles on a single slot — practically infeasible and limited in impact (pool shrinks by one slot per exhausted number out of 224 available)
Consumer Impact
No public API surface is broken. The new CRDTEntity.MAX_VERSION constant is additive. All changes to PlayerCRDTEntitiesHandlerSystem are to private methods. The PlayerCRDTEntity component's public interface is unchanged (its constructor already accepted CRDTEntity). The versioned id flows unchanged into downstream systems (PlayerSceneCRDTEntity, OutgoingCRDTMessagesProvider, CRDTSerializer). No consumer impact.
Summary
Clean, well-scoped fix that addresses the root cause of ADR-245 non-compliance. The version tracking is added to the correct lifecycle owner with no architectural concerns. Bit-packing, version lifecycle, pool accounting, and edge cases (retirement at MAX_VERSION) are all correct. Tests are thorough — 4 new tests cover the key scenarios including boundary conditions. No blocking issues.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies CRDT entity id versioning in the multiplayer synchronization layer (reserved entity pool management, version packing, scene-level entity identity)
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: fix: sdk reserved entities recycle version bump
STEP 2 — Root-cause check ✅
Problem: ADR-245 requires a new version (generation) every time a reserved CRDT entity number is reassigned to a different player. The pool was a bare bool[224] that never bumped the version, so every player inheriting a number got version 0. Since the CRDT protocol stores deleted entities as number → version and discards any message whose version is not greater than the stored one (CRDTProtocol.cs:68-73), a tombstoned number at version 0 made every subsequent player on that slot invisible to the scene.
This diff fixes the cause, not a symptom. The version tracking is added at the reservation layer (TryReserveNextFreeEntity) so the versioned entity flows naturally through PlayerSceneCRDTEntity, OutgoingCRDTMessagesProvider, and CRDTSerializer without any downstream changes needed.
STEP 3 — Design & integration ✅
Lifecycle owner search:
- The entity being managed is the reserved CRDT entity number pool (slots 32–255) for remote players.
- The existing owner is
PlayerCRDTEntitiesHandlerSystem, which already maintainsbool[] reservedEntitiesandcurrentReservedEntitiesCountas persistent cross-scene state. This system creates slots inTryReserveNextFreeEntityand releases them inFreeReservedEntity. Files examined:PlayerCRDTEntitiesHandlerSystem.cs— sole owner of the reservation poolPlayerCRDTEntity.cs— component carrying theCRDTEntityin the global worldPlayerSceneCRDTEntity.cs— component carrying theCRDTEntityin scene worldsPlayerTransformPropagationSystem.cs,PlayerProfileDataPropagationSystem.cs— downstream consumers (read-only, useCRDTEntity.Idfor local player check only)
- The version tracking array is added alongside the existing
reservedEntitiesarray in the same owner — there is no alternative owner. The pool is a cross-scene protocol resource; it cannot live in ECS (no single entity or scene owns it).
Design verdict: correct. The new int[] reservedEntityVersions extends the existing persistent pool state in its natural owner. No duplicate lifecycle, no frame-based reconciliation, no polling.
Teardown trace:
reservedEntities[i]set totrueinTryReserveNextFreeEntity→ set tofalseinFreeReservedEntity(or permanently retired at version exhaustion) ✅ClearReservedEntities()resets both arrays — called in the constructor ✅- Retired slots (version ≥
MAX_VERSION) intentionally stay reserved to avoid repeating a generation, which is correct protocol behavior ✅
STEP 4 — Member audit ✅
CRDTEntity.MAX_VERSION (new public const): Used in FreeReservedEntity (line 216) and RetireReservedNumberWhenItRunsOutOfVersions test (line 442). Two consumers. As a const int, it is a value boundary — appropriate as a public constant on the type it describes.
TryReserveNextFreeEntity(out CRDTEntity) (changed from ReserveNextFreeEntity()): Private method, sole caller is AddPlayerCRDTEntity. The Try pattern correctly replaces the ambiguous -1 sentinel with a bool return + out parameter. Single-use is fine — it is the pool allocation primitive, not a derived predicate.
FreeReservedEntity(CRDTEntity) (parameter changed from int): Private method, sole caller is RemoveComponent. Takes CRDTEntity instead of raw int because the raw Id no longer equals the entity number when version > 0. The method extracts EntityNumber to index into the pool — correct.
STEP 5 — Line-level review ✅
Pass A — Blocking issues: None found.
- Array bounds:
FreeReservedEntityindexes bycrdtEntity.EntityNumber - OTHER_PLAYER_ENTITIES_FROM. Since only entities produced byTryReserveNextFreeEntityreach this method, and that method constrainsito[0, reservedEntities.Length), the index is always in bounds. The range guard at lines 209 also handles unexpected values defensively. - Integer overflow:
++reservedEntityVersions[i]increments from -1 to 0 on first use, then monotonically. The retirement check (>= MAX_VERSION) fires when version reaches 0xffff, preventingCRDTEntity.Createfrom ever receiving version 0x10000 (which would overflow the 16-bit field in the packed Id). crdtEntity.Id == PLAYER_ENTITYchecks: All downstream systems (RemovePlayerFromScene:169,PlayerTransformPropagationSystem:41,WritePlayerTransformSystem:43/59,PropagateAvatarLocomotionOverridesSystem:55) compare the fullIdtoPLAYER_ENTITY(=1). The local player is never pooled (getsPLAYER_ENTITYdirectly at line 72), so its version is always 0 andIdremains 1. Remote players have numbers ≥ 32, so their Ids are always ≥ 32 even at version 0. These checks remain correct. ✅- Version bump timing: Version is incremented at reservation time (
TryReserveNextFreeEntity:196), not at free time. This is correct — bumping on free would waste versions for slots that sit idle. currentReservedEntitiesCountconsistency: Retired slots staytrueinreservedEntitiesand are counted as occupied, which correctly prevents reuse.currentReservedEntitiesCountis only decremented when a slot is actually freed (line 226). ✅- String interpolation in warnings: Only triggered on version exhaustion (line 218) and pool exhaustion (line 81) — not hot paths. No allocation concern. ✅
- Allocations in
Update(): No new allocations added to any query method. The version tracking uses pre-allocated arrays. ✅
Pass B — Design smells: None found. No new types, no new dependencies, no magic values. The pool logic is self-contained in the existing system.
Security review ✅
No security issues found. The changes are purely internal CRDT entity management. No user input processing, no network data handling, no authentication changes. The version tracking is a protocol-compliance fix that affects only how entity IDs are packed locally.
Test coverage ✅
Three new tests added:
PropagateRecycledEntityVersionToTheSceneEntity— verifies the versioned entity flows through toPlayerSceneCRDTEntityin the scene world after a number is recycled.BumpEntityVersionsIndependentlyPerReservedNumber— verifies each reserved number tracks its version independently (number 32 at v1, number 33 still at v0).RetireReservedNumberWhenItRunsOutOfVersions— exhausts all 65,536 versions on a single number and verifies the slot is retired, forcing the next player onto the next available number.
Existing tests updated to expect CRDTEntity.Create(number, version) instead of raw int IDs. Comments updated to reflect the version bump semantics.
STEP 6 — Complexity
COMPLEX — touches CRDT entity management, multiplayer sync, and the reserved entity pool lifecycle.
STEP 7 — QA assessment
QA required — the change affects how remote players are exposed to scenes at runtime. The PR includes detailed QA test steps for verifying entity ID reuse with version bumps in a multiplayer scenario.
STEP 8 — Non-blocking warnings
None. Main scene is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies CRDT entity number pool lifecycle and multiplayer entity version tracking in PlayerCRDTEntitiesHandlerSystem
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
This comment has been minimized.
This comment has been minimized.
pravusjif
added
no-warning-ratchet
This comment has been minimized.
This comment has been minimized.
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
Evidence
fixpravus.mp4
|
PR #9847, run #33542931736 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Apple M1
|
4 participants
Problem
ADR-245 reserves entity numbers 32–255 for remote players and requires a new generation
(version) every time a number is reassigned. Unity Explorer never bumped it: the pool was a
bare
bool[224], so every player on a given number got version 0.Entity ids pack
versionin the upper 16 bits andnumberin the lower 16 (ADR-117), and ascene's CRDT state stores deleted entities as
number -> version, discarding any message whoseversion is not greater than the stored one. So once a number was tombstoned in a scene, every
player who later inherited that slot stayed invisible to that scene for the rest of its lifetime —
no recovery short of a scene reload.
Hammurabi, Bevy and Godot all bump; Unity was the only host that didn't.
Fix
PlayerCRDTEntitiesHandlerSystemnow tracks a generation per reserved number:reservedEntityVersions[]runs alongsidereservedEntities[], initialised to-1, so thefirst hand-out of a number is version 0 and every recycle advances it.
ReserveNextFreeEntity()becameTryReserveNextFreeEntity(out CRDTEntity)and returnsCRDTEntity.Create(number, version). The old-1exhaustion sentinel is gone — once versionbits live in
Id, a sentinel int id is ambiguous.FreeReservedEntitytakes aCRDTEntityand indexes byEntityNumber; the rawIdno longerequals the number.
generation, and logs once.
CRDTEntity.MAX_VERSIONwas added for that bound.Nothing downstream needed changing: the versioned id flows unchanged into
PlayerSceneCRDTEntity,OutgoingCRDTMessagesProvider(keyed by the fullCRDTEntity) andCRDTSerializer(writes all4 bytes). The
Id == PLAYER_ENTITYchecks in the propagation systems stay correct — the localplayer is never pooled, so its version is always 0.
QA TEST STEPS
This test requires opening more than 1 Explorer in parallel to test in a multiplayer test scene.
Use the build from this PR to enter the ZONE/SEPOLIA world
sdk7testscenes.dcl.ethat position93,-8:DO NOT interact with the red sphere in the scene, it's used for other tests.
--multi-instance trueparam, logged with a 2nd account, "account B"--multi-instance trueparam, logged with a 3rd account, "account C")#32 v1)