fix: discard duplicate scene facade for already-cached parcels and serialize realm changes - #9807
fix: discard duplicate scene facade for already-cached parcels and serialize realm changes#9807alejandro-jimenez-dcl wants to merge 1 commit into
Conversation
…rialize realm changes ## Problem `ArgumentException: An item with the same key has already been added. Key: (2, -4)` from `ScenesCache.Add` (Sentry UNITY-EXPLORER-PA9, 75 events / 8 users, ongoing since v0.150). The crash is the visible tip: session breadcrumbs show every scene loading twice from session start, zombie facades ripping the live scene's cache mappings on unload, and a ~1.5 s dispose/re-create churn loop under the player. ## Root cause Two composing defects. (1) `RealmController.SetRealmAsync` has no re-entrancy guard: two overlapping calls both pass the unload phase and both create a realm entity → two independent scene-pointer pipelines mint every scene-definition entity twice for the whole session. (2) The consume path assumes the one-entity-per-parcel invariant instead of enforcing it: the duplicate consume's `Dictionary.Add` throw is swallowed, but `World.Add(entity, scene)` still runs — the duplicate keeps a zombie facade that never started, and its eventual unload removes whatever facade owns the parcel, i.e. the live scene's cache entries. ## Fix (~30 LOC, two loci) - A (invariant at the single cache writer): `ControlSceneUpdateLoopSystem.HandleNotCreatedScenes` detects a parcel collision after a successful consume (plain for-loop, non-PX only), logs a warning, disposes the duplicate facade, and returns without attaching it — the duplicate entity stays inert and its unload takes the promise-only path, which cannot touch the live scene's mappings. Keep-first is correct (in-session duplicates are always the same scene id). - B (close the mint): `SetRealmAsync` serialized behind a `DCLSemaphoreSlim(1,1)` (the repo's WebGL-safe wrapper; raw SemaphoreSlim is lint-forbidden) — overlapping realm changes now execute sequentially, restoring the at-most-one-realm-entity invariant all pointer dedup relies on. ## Test Two new EditMode tests in the existing `ControlSceneUpdateLoopSystemShould` harness: `DiscardDuplicateSceneForSameParcelsKeepingLiveFacade` (duplicate disposed exactly once, never started, exactly one facade attached, cache holds the kept facade, no error log) and `KeepLiveSceneCacheMappingWhenDuplicateEntityUnloads` (cascade guard through a real `UnloadSceneSystem`). Fix B validated by review (mocking the /about + WorldManifest stack is out of proportion). ## Validation Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 2/2 as intended (both failed via the unhandled same-key ArgumentException surfacing through LogAssert) / GREEN PASS 2/2. Fixes #8911 Related: #8883, #8720, #8492/#8441/#8183 (closed same-key family in sibling subsystems, context for the defect class) Includes inspection-warning cleanup in all touched files.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: discard duplicate scene facade for already-cached parcels and serialize realm changes
STEP 2 — Root-cause check: PASS ✅
Problem: ArgumentException from ScenesCache.Add when duplicate scene-definition entities mint two facades for the same parcel. Zombie facades rip live-scene cache mappings on unload, causing a churn loop.
Root cause: Two composing defects: (1) SetRealmAsync has no re-entrancy guard — overlapping calls each create a realm entity and an independent scene-pointer pipeline, minting every scene-definition entity twice. (2) The consume path assumes but does not enforce the one-entity-per-parcel invariant — the duplicate Dictionary.Add throws, but World.Add(entity, scene) still runs, leaving a zombie facade whose eventual unload removes the live scene's cache entries.
Does the diff fix the cause? Yes, on two levels:
- Fix B (semaphore) eliminates the root cause: serializing realm changes behind
DCLSemaphoreSlim(1,1)restores the at-most-one-realm-entity invariant. - Fix A (parcel collision guard) provides defense-in-depth at the consume path: if a duplicate entity somehow arrives, it is detected and discarded before attachment.
Neither fix is a symptom workaround — Fix B prevents the over-mint, Fix A enforces the invariant the cache writer relies on.
STEP 3 — Design & integration: PASS ✅
No new long-lived units are introduced. The semaphore is a field on the existing RealmController; the guard is a conditional inside the existing HandleNotCreatedScenes query method.
Owner search for Fix A (parcel collision guard):
- Lifecycle managed: scene facade creation (consume) and destruction (unload)
- Creation owner:
ControlSceneUpdateLoopSystem.HandleNotCreatedScenes— this is the single writer that consumes anAssetPromise<ISceneFacade>and attaches the facade viaWorld.Add(entity, scene). The guard is placed directly in this method ✓ - Destruction owner:
UnloadSceneSystem.UnloadLoadedScene— this query requiresISceneFacadein its signature. A facade-less discarded entity will NOT match this query, so its unload follows theAbortLoadingScenespath (which only callsForgetLoadingon the consumed promise — no cache mutation). ✓ - Conclusion: The guard is placed at the correct lifecycle moment (creation) and the unload path correctly bypasses cache mutations for facade-less entities.
Owner search for Fix B (realm serialization):
- Lifecycle managed: realm entity creation/destruction
- Owner:
RealmController.SetRealmAsync— the public entry point that unloads the current realm and creates a new one. The semaphore guards this entry point directly. ✓ - Callers:
ChangeRealmTeleportOperation,Bootstraper,RealUserInAppInitializationFlow— overlapping calls from teleport + initialization flows are now correctly serialized.
The guard is NOT per-frame reconciliation. AnyParcelHasLiveScene runs only when an AssetPromise is successfully consumed (an explicit lifecycle moment), not on every Update tick. The query filter [None(typeof(ISceneFacade))] ensures it only runs for entities that haven't yet been processed.
STEP 4 — Member audit: PASS ✅
| Member | Visibility | Consumers | Verdict |
|---|---|---|---|
AnyParcelHasLiveScene(IReadOnlyList<Vector2Int>) |
private | 1 (HandleNotCreatedScenes) |
Loop extraction into named method — acceptable for readability. Not a derived predicate; it's a simple existence check against the cache. |
SetRealmExclusiveAsync(URLDomain, CancellationToken) |
private | 1 (SetRealmAsync) |
Standard semaphore-wrapper pattern: public method handles acquisition/release, private method holds the logic. |
realmChangeSemaphore |
private readonly field | SetRealmAsync only |
Application-lifetime scoped; no IDisposable on RealmController is consistent with existing design. |
STEP 5 — Line-level review: PASS ✅
Pass A — Blocking-issue categories: No P0 or P1 issues found.
-
Semaphore pattern (RealmController.cs:130-133):
WaitAsync(ct)is correctly placed OUTSIDE the try block. If cancellation throws during wait,Release()is not called — this is correct because the semaphore was never acquired. Release infinallyguarantees release after successful acquisition, even on exception. -
Parcel collision guard (ControlSceneUpdateLoopSystem.cs:84-89): After
TryConsumesucceeds, the guard checksAnyParcelHasLiveScene. If true, it disposes the facade and returns BEFOREWorld.Add(entity, scene)— the entity remains facade-less. This is verified correct:UnloadLoadedScenerequiresISceneFacadein its query signature, so the facade-less entity takes the safeAbortLoadingScenespath. -
scene.DisposeAsync().Forget()(line 87): Fire-and-forget disposal of a never-started facade. This follows the established codebase pattern (cf.UnloadSceneSystem.UnloadLoadedPortableExperienceSceneline 118,AbortSucceededScenesPromisesline 137). Per CLAUDE.md §9, detached async is acceptable for non-essential cleanup, and the duplicate facade is never started. -
WaitForFixedScenePromisesAsyncrefactor (RealmController.cs:211-217):FixedScenePointersis confirmed as astruct. The old code captured it viaoutparameter in a closure (potentially stale copy). The new code uses inlineout varin the lambda and re-reads withGet<>afterWaitUntil— this is a correctness improvement. No practical TOCTOU risk: the semaphore serializes realm changes, and within Unity's single-threaded execution model, no system can mutate the component betweenWaitUntilreturning and theGet<>call. -
AnyParcelHasLiveScene(lines 96-103): Allocation-free for-loop with dictionary lookup — compliant with CLAUDE.md §4 (no LINQ, allocation-free in Update path). -
ChangeSceneFPS→ChangeSceneFpsrename: Follows .NET naming guidelines for 3+ character acronyms. ✓ -
Parameter rename
realmEntity→targetRealmEntity(lines 324, 329): Avoids shadowing the instance fieldrealmEntity. ✓
Pass B — Design, encapsulation & resource smells: No issues found. No new persistent state, no construction smells, no naming issues, no magic values, no encapsulation leaks.
Teardown trace: The DCLSemaphoreSlim is application-lifetime scoped (same as RealmController). The disposed facade's DisposeAsync() handles its own internal cleanup. No new subscriptions, event hookups, or resource acquisitions are introduced.
Security Review: PASS ✅
- No secrets or credentials in the diff
- No injection vulnerabilities
- No auth/authz changes
- Semaphore cannot deadlock:
CancellationTokenpropagation removes cancelled waiters;finallyguarantees release - No external input can cause semaphore starvation
Tests: Well-structured ✅
Two new tests in ControlSceneUpdateLoopSystemShould:
-
DiscardDuplicateSceneForSameParcelsKeepingLiveFacade— Verifies: duplicate facade disposed exactly once, never started, exactly one facade attached to entity, cache holds kept facade, no error log. Correctly handles non-deterministic entity consumption order viafirstEntityKeptcheck. -
KeepLiveSceneCacheMappingWhenDuplicateEntityUnloads— Verifies the cascade guard: after discarding a duplicate, addingDeleteEntityIntentionand runningUnloadSceneSystemdoes NOT remove the live scene's cache mappings.
Both tests use real ScenesCache (not mock) for integration coverage, NSubstitute for facades, and follow AAA pattern. ✓
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS scene lifecycle systems, async/UniTask patterns (DCLSemaphoreSlim, .Forget()), entity structural changes (World.Add), and the scene cache — core scene-loading infrastructure.
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. |
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 12724 => 13120 — remove at least 397 warnings to merge. No warnings in files changed by this PR — showing general ones you can remove to unblock (50 of 13120)All Unity tests passed ✅
|
|
PR #9807, run #32283579001 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
|
Superseded by #9828, which compounds this fix together with the rest of the bugsweep batch. Closing in favor of that combined PR. |
4 participants
Problem
ArgumentException: An item with the same key has already been added. Key: (2, -4)fromScenesCache.Add(Sentry UNITY-EXPLORER-PA9, 75 events / 8 users, ongoing since v0.150).The crash is the visible tip: session breadcrumbs show every scene loading twice from
session start, zombie facades ripping the live scene's cache mappings on unload, and a
~1.5 s dispose/re-create churn loop under the player.
Root cause
Two composing defects. (1)
RealmController.SetRealmAsynchas no re-entrancy guard: twooverlapping calls both pass the unload phase and both create a realm entity → two
independent scene-pointer pipelines mint every scene-definition entity twice for the whole
session. (2) The consume path assumes the one-entity-per-parcel invariant instead of
enforcing it: the duplicate consume's
Dictionary.Addthrow is swallowed, butWorld.Add(entity, scene)still runs - the duplicate keeps a zombie facade that neverstarted, and its eventual unload removes whatever facade owns the parcel, i.e. the live
scene's cache entries.
Fix (~30 LOC, two loci)
ControlSceneUpdateLoopSystem.HandleNotCreatedScenesdetects a parcel collision after a successful consume (plain for-loop, non-PX only), logs
a warning, disposes the duplicate facade, and returns without attaching it - the duplicate
entity stays inert and its unload takes the promise-only path, which cannot touch the live
scene's mappings. Keep-first is correct (in-session duplicates are always the same scene id).
SetRealmAsyncserialized behind aDCLSemaphoreSlim(1,1)(therepo's WebGL-safe wrapper; raw SemaphoreSlim is lint-forbidden) - overlapping realm
changes now execute sequentially, restoring the at-most-one-realm-entity invariant all
pointer dedup relies on.
Test
Two new EditMode tests in the existing
ControlSceneUpdateLoopSystemShouldharness:DiscardDuplicateSceneForSameParcelsKeepingLiveFacade(duplicate disposed exactly once,never started, exactly one facade attached, cache holds the kept facade, no error log) and
KeepLiveSceneCacheMappingWhenDuplicateEntityUnloads(cascade guard through a realUnloadSceneSystem). Fix B validated by review (mocking the /about + WorldManifest stackis out of proportion).
Validation
Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 2/2 as intended (both failed via
the unhandled same-key ArgumentException surfacing through LogAssert) / GREEN PASS 2/2.
Fixes #8911
Related: #8883, #8720, #8492/#8441/#8183 (closed same-key family in sibling subsystems,
context for the defect class)
Includes inspection-warning cleanup in all touched files.
Fixes #8720