fix: combined fixes first sweep - #9784
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13116 => 13129 — remove at least 14 warnings to merge. Warnings/errors in files changed by this PR (86)All Unity tests passed ✅
|
…ile constructor Profile.Create was removed on dev; use new Profile(UserId.New(...).Unwrap(), ...) like the rest of the test suite.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — bugsweep: combined fixes first sweep
STEP 2 — Root-cause check
This PR bundles several independent bug fixes, each targeting a distinct root cause:
- Gift notification copy — Gift-received notifications implied the item was already in the backpack. The copy is updated to say the gift is "on its way." Fixes the cause (misleading text).
- GLTF container double-dereference — After releasing a GLTF asset, the promise still held the resolved result, allowing a repeated cleanup path to dereference and dispose the asset a second time. Nulling the promise after release fixes the cause (stale cached result).
- Irrecoverable failure cache poisoning — Any failed load (including transient exceptions) was cached as irrecoverable, causing session-permanent asset loading failures. The fix restricts caching to definitive 4xx HTTP errors. Fixes the cause (over-broad caching predicate).
- Realm recovery after failed change — A cancelled realm change could leave the client with no configured realm. The fix adds a recovery path to fall back to the previous realm. Fixes the cause (missing recovery after partial teardown).
- Terrain partial cleanup — A cancelled terrain generation left
IsTerrainGenerated = truedespite the terrain being destroyed, and swallowedOperationCanceledExceptioninstead of propagating it. The fix moves the flag after success and re-throws cancellation. Fixes the cause (incorrect state after partial generation). - TeleportController landOnParcel — When the target parcel equals the scene's base parcel,
landOnParcelis cleared so spawn-point resolution runs. Fixes the cause (flag staying set when there's no sub-parcel to target). - Profile name dropdown — An unclaimed display name matching an owned NFT name pre-selected the dropdown, making Save permanently disabled. The fix checks
HasClaimedNamebefore pre-selecting. Fixes the cause (incorrect pre-selection logic).
Verdict: PASS — All fixes target root causes, not symptoms.
STEP 3 — Design & integration
GLTF promise-nulling (CleanUpGltfContainerSystem, ResetGltfContainerSystem): The AssetPromise.NULL sentinel is already used elsewhere in the codebase (e.g. InvalidatePromise in ResetGltfContainerSystem already sets it). The fix adds the same pattern to the two remaining release paths that were missing it. The lifecycle owner (the GLTF container systems) is the correct place for this. No new unit introduced. PASS.
IsIrrecoverableFailure (LoadSystemBase): The new gate lives inside the existing RepeatLoopAsync method on the system that owns the irrecoverable-failure cache. No new unit, no design change — just a tighter predicate. PASS.
RecoverUnconfiguredRealmAsync (RealmNavigator): This is a private recovery method on the existing RealmNavigator, which already owns the realm-change lifecycle. It reuses DoChangeRealmAsync (the same operation factory used for normal realm changes) and runs it through loadingScreen.ShowWhileExecuteTaskAsync (which has its own built-in timeout). Uses CancellationToken.None intentionally — the recovery must not be externally cancelled (the original token is already cancelled), and the loading-screen timeout is the safety net. No new unit. PASS.
DestroyPartialTerrain (TerrainGenerator): This is a private helper on the class that owns the terrain lifecycle. Ocean and Cliffs are children of TerrainRoot and are destroyed when it is. Wind is unparented and correctly destroyed separately. Trees is a data object (not a GameObject) and doesn't need destruction here. PASS.
TeleportController landOnParcel fix: The fix is placed immediately after the existing TryPickSpawnPointNameInParcel check, before the !landOnParcel branch that snaps to the base parcel. This is the correct location — it extends the existing spawn-point resolution logic. PASS.
STEP 4 — Member audit
GiftingTextIds.GiftOnItsWayMessage (new public const): Used by GiftNotificationView.Configure, GiftToastView.Configure, GiftToastView.UpdateSenderName, and GiftReceivedTitleFormat (4 consumers). Appropriate as a shared constant. However, GiftReceivedNotification.NOTIFICATION_TITLE inlines the same sentence — see finding below.
TerrainGenerator.DestroyPartialTerrain (new private method): Called from two catch blocks within the same class. Correctly extracted to avoid duplication.
RealmNavigator.RecoverUnconfiguredRealmAsync (new private method): Single consumer (TryChangeRealmAsync). It encapsulates the recovery logic cleanly; merging it inline would harm readability. Acceptable.
LoadSystemBase.IsIrrecoverableFailure (new private static method): Single consumer (RepeatLoopAsync). Extracted for clarity of the predicate — acceptable given the logic complexity.
STEP 5 — Line-level review
See inline comments for specific findings.
STEP 6 — Complexity assessment
COMPLEX — touches ECS systems (GLTF container cleanup), asset loading pipeline (LoadSystemBase irrecoverable failure caching), realm navigation lifecycle (RealmNavigator, RealmController), landscape generation (TerrainGenerator), and teleport controller logic across 26 files with 900+ lines of changes.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes affect runtime behavior: gift notification UI text, GLTF asset lifecycle, asset loading retry behavior, realm navigation and recovery, terrain generation, teleport positioning, and profile name editor UI.
STEP 8 — Non-blocking warnings
No main scene file modified. No warnings.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS GLTF container systems, StreamableLoading pipeline, realm navigation lifecycle, landscape generation, and teleport controller
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
…st test - Centralize the received-gift title copy in GiftingTextIds (new GiftReceivedSenderTitleFormat) and reference GiftOnItsWayMessage from GiftReceivedNotification instead of duplicating the sentence - Replace the fragile backing-field reflection in GiftReceivedCopyShould with a settable GiftToastView.TitleText, matching GiftNotificationView.HeaderText - Document why RealmNavigator's recovery uses CancellationToken.None
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — bugsweep: combined fixes first sweep
STEP 2 — Root-cause check
All five fixes address genuine root causes rather than symptoms:
| Fix | Root cause | Verdict |
|---|---|---|
| #9550 — Name equip | FindIndex matched on display-name text alone, ignoring whether the name was actually claimed. The fix guards with profile.HasClaimedName. |
✅ Cause fixed |
| #9546 — Events jump-in | landOnParcel: true survived even when the target parcel equals the scene's base parcel, forcing the raw-parcel-center branch instead of spawn-point resolution. The fix clears the flag for base-parcel targets. |
✅ Cause fixed |
| #9511 — GLTF cache poisoning | RepeatLoopAsync cached any concluded failure as irrecoverable — including transient exceptions with no HTTP status. IsIrrecoverableFailure now restricts caching to genuine 4xx client errors (excluding 408/425/429) via WebRequestUtils.IsIrrecoverableError(). |
✅ Cause fixed |
| #9776 — Gift copy | The notification copy implied immediate availability; the server-side ownership cache can lag by minutes. Copy now says the item is on its way. | ✅ Cause fixed |
| #9517 — Teleport-timeout race | Multiple interacting bugs: (a) TerrainGenerator set IsTerrainGenerated = true in finally even on failure, (b) OperationCanceledException was swallowed in both TerrainGenerator and RealmController.SetRealmAsync, (c) LoadLandscapeTeleportOperation ignored the landscape result, (d) the in-chain fallback ran on a cancelled token producing teardown side-effects. Fixed at each layer. |
✅ Causes fixed |
STEP 3 — Design & integration
Owner search for new units:
No new long-lived systems, plugins, managers, services, or controllers are introduced. All changes modify existing lifecycle paths:
DestroyPartialTerrain()— private helper inTerrainGenerator, called from existing catch blocks.TerrainGeneratoralready owns the terrain lifecycle (create inGenerateGenesisTerrainAndShowAsync, destroy inDispose). The new method centralizes cleanup for interrupted generation. Home is correct.RecoverUnconfiguredRealmAsync()— private method inRealmNavigator, which already owns the realm-change orchestration (including the existing in-chain fallback inDoChangeRealmAsync). The recovery fires only when the in-chain fallback couldn't run (cancelled token). Placing it here avoids duplicating theShowWhileExecuteTaskAsynccall chain. Home is correct. CheckedRealmController— it owns realm configuration but not the loading-screen / fallback orchestration, so recovery belongs inRealmNavigator.IsIrrecoverableFailure()— static method inLoadSystemBase, which already owns theSetIrrecoverableFailurecall and theRepeatLoopAsyncreturn path. The decision to restrict caching is a loading-system concern (the cache itself is agnostic). Home is correct.- GLTF promise nulling (
component.Promise = ...NULL) — added at the two existing release paths inCleanUpGltfContainerSystem.DestroyGLTFContainerandResetGltfContainerSystem.HandleComponentRemoval. These are the lifecycle owners for GLTF component teardown. Home is correct.
Teardown / consumption trace:
No new subscriptions, event hookups, callbacks, connections, rooms, buffers, or measurements are introduced. The changes modify existing control flow and add defensive cleanup.
RecoverUnconfiguredRealmAsyncusesCancellationToken.None— the loading screen's own timeout bounds the operation. No unbounded work.RealmController.SetRealmAsyncnow rethrowsOperationCanceledExceptioninstead of swallowing it — this is critical because the previous realm is already unloaded at that point; swallowing would leave a half-configured realm appearing successful.
Design verdict: PASS — no new lifecycle units, no polling patterns, no persistent state outside ECS.
STEP 4 — Member audit
| Member | Consumers | Audit |
|---|---|---|
GiftingTextIds.GiftOnItsWayMessage (new const) |
3 — GiftReceivedTitleFormat, GiftReceivedSenderTitleFormat, GiftReceivedNotification.NOTIFICATION_TITLE |
Shared message string factored into a const. No single-use concern. ✓ |
GiftingTextIds.GiftReceivedSenderTitleFormat (new const) |
3 — GiftToastView.Configure, GiftToastView.UpdateSenderName, GiftNotificationView.Configure |
Format string for the address/plain-name variant ({0} only). Parallel to the existing GiftReceivedTitleFormat (color+name, {0}+{1}). ✓ |
GiftToastView.TitleText setter (widened from private set to set) |
Test (GiftReceivedCopyShould) |
Aligns with GiftNotificationView.HeaderText which already has a public setter. [field: SerializeField] backing field was already inspector-writable. ✓ |
TerrainGenerator.DestroyPartialTerrain() (new private) |
2 — cancellation catch and error catch | Centralizes cleanup. Not single-use (called from two paths). ✓ |
RealmNavigator.RecoverUnconfiguredRealmAsync() (new private) |
1 — TryChangeRealmAsync |
Single consumer, but the extracted method is non-trivial (7 lines of orchestration). Extraction is warranted for readability and the explanatory comment. ✓ |
LoadSystemBase.IsIrrecoverableFailure() (new private static) |
1 — RepeatLoopAsync |
Single consumer, but encapsulates a specific policy decision with a clear name. ✓ |
STEP 5 — Line-level review
Pass A — Blocking issues: None found.
Pass B — Design, encapsulation & resource smells: None found.
Security review: No secrets, credentials, injection vectors, auth/authz issues, unsafe input handling, or sensitive data exposure introduced. The unsafe blocks in TerrainGenerator are existing NativeArray operations unrelated to this PR.
Detailed per-fix notes:
-
IsIrrecoverableFailure logic — The inner-exception loop correctly handles wrapped exceptions (e.g.,
AggregateExceptionwrappingUnityWebRequestException). The conjunctionResponseCode is >= 400 and < 500 && IsIrrecoverableError()double-filters: the range check excludes 5xx and aborted (code 0) requests;IsIrrecoverableError()further excludes 408/425/429 and SSL errors. Sound. -
TerrainGenerator exception handling — Moving
IsTerrainGenerated = trueinto the try block after success, and addingthrow;in the cancellation catch, are both critical. The oldfinallyblock's memory log (afterCleaning - beforeCleaning) was measuring the cost of setting a boolean — effectively a no-op. Removing it is a cleanup. -
RealmNavigator recovery guard —
!realmController.RealmData.Configured && !ct.IsCancellationRequestedcorrectly distinguishes loading-screen timeout (internal token cancelled, externalctintact → recover) from user/caller cancellation (externalctcancelled → skip recovery). The recovery usesCancellationToken.Nonewith the loading screen providing its own timeout bound. -
DoChangeRealmAsync cancellation guard —
if (ct.IsCancellationRequested) return opResult;before the fallback prevents a second round of teardown side-effects when the operation's token is already cancelled. -
GLTF promise nulling — Setting
component.Promise = AssetPromise<...>.NULLafterForgetLoadingandcache.Dereferenceprevents double-release when multiple cleanup paths run on the same entity (e.g., per-frameDeleteEntityIntentionfollowed by scene teardown).TryGetResulton a NULL promise returnsfalse, so subsequent release paths safely no-op. -
LoadLandscapeTeleportOperation result check —
ct.ThrowIfCancellationRequested()beforethrow new Exception(...)ensures cancellation propagates asOperationCanceledExceptionrather than a generic exception. Used in an awaited async method with exception handling — acceptable per CLAUDE.md §9. -
GiftNotificationView.Configure — The change from
$"{shortAddr} sent you a something!"tostring.Format(GiftingTextIds.GiftReceivedSenderTitleFormat, shortAddr)also fixes the existing typo ("a something").
STEP 6 — Complexity assessment
COMPLEX — The teleport-timeout fix (#9517) modifies async/cancellation flows across the realm navigation subsystem (RealmNavigator, RealmController, LoadLandscapeTeleportOperation), touches the terrain generation lifecycle (TerrainGenerator), and interacts with the loading-screen timeout mechanism. The GLTF cache-poisoning fix (#9511) modifies the asset loading pipeline's failure-caching policy.
STEP 7 — QA assessment
QA_REQUIRED: YES — All five fixes affect runtime behavior: teleporting, scene asset loading, terrain/LODs, UI notifications, and the profile name editor.
STEP 8 — Non-blocking warnings
semantic / title-matches-convention is failing. bugsweep: is not a standard type. Consider fix: combined bugsweep fixes (first sweep) or similar.
ProfileNameEditorControllerShould uses reflection to invoke the compiler-generated method for the SetUpClaimed local function. This is fragile against compiler implementation changes (method name mangling). A comment documents the rationale. Acceptable for now, but worth noting for future maintainers.
Test coverage
All five fixes include regression tests that fail without the fix:
ProfileNameEditorControllerShould— verifies dropdown stays unselected for unclaimed namesTeleportControllerShould— verifieslandOnParcelis cleared for base-parcel targetsLoadSystemBaseIrrecoverableFailureShould— verifies transient exceptions aren't cached; verifies 404 IS cachedGiftReceivedCopyShould— verifies all gift surfaces include the "on its way" copyLoadLandscapeTeleportOperationShould— verifies cancelled terrain load fails the teleport
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async/cancellation flows across realm navigation, terrain generation lifecycle, and the asset loading pipeline's failure-caching policy.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by alejandro-jimenez-dcl via GitHub
GiftReceivedNotification (DCL.SharedAPI) referenced GiftingTextIds (DCL.Social), but DCL.Social already depends on DCL.SharedAPI, so the reference could never compile. Move the shared 'on its way' copy into GiftReceivedNotification and alias it from GiftingTextIds, keeping a single definition without the cycle.
alejandro-jimenez-dcl
changed the title
|
PR #9784, run #32195050170 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
3 participants
Pull Request Description
What does this PR change?
Combines five bugsweep fixes into a single PR, rebased onto updated
dev, one squashed commit per original PR:96fe15f83c8d83d30ff742575884ddaace0c918e283e1/reloadreported "not in a SDK7 scene". The fallback is now guarded on cancellation.Follow-up commits on this branch:
1723583cdadapts the fix: don't pre-select owned NAME dropdown entry for an unclaimed matching display name #9609 regression test to the newUserId-basedProfileconstructor (Profile.Createwas removed on dev).Conflict resolutions (only #9611 conflicted)
TeleportController.cs: dev gained a named-spawn-point-in-parcel block (fix: events JumpIn lands in the middle of the scene #9567) overlapping the PR's change. Both are kept: dev's named-spawn-point check runs first, then this PR's base-parcel fallback.RealmNavigation.Tests.asmrefduplicated the asmref dev already ships (same assembly GUID), so it was dropped in favor of dev's existing one.Each fix includes a regression test that fails without it.
Replaces #9609, #9611, #9774, #9776 and #9777.
Test Instructions
Steps (standard run):
Overall smoke test (required)
This PR touches teleporting, scene asset loading, terrain and LODs, notifications, and the profile name editor, so a general pass is required on top of the specific checks below: log in, walk around Genesis City, teleport between a few scenes using the map, chat and the Events page, open the backpack and the profile, and confirm nothing looks or behaves worse than before.
Specific checks
/reloadmust work in the scene you landed in.Quality Checklist