chore: combined QA build for the clean-* fix set (#9416–#9429)#9430
chore: combined QA build for the clean-* fix set (#9416–#9429)#9430eordano wants to merge 30 commits into
Conversation
…nnect Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-PFR: 1 events / 1 users, last seen 2026-07-10 - UNITY-EXPLORER-PF9: 1 events / 1 users, last seen 2026-07-06 - UNITY-EXPLORER-PE6: 1 events / 1 users, last seen 2026-07-01 - UNITY-TEST-ENVIRONMENT-1CK: 1 events / 1 users, last seen 2026-07-08 - UNITY-TEST-ENVIRONMENT-1F2: 1 events / 1 users, last seen 2026-06-30 - UNITY-TEST-ENVIRONMENT-1EE: 1 events / 1 users, last seen 2026-06-19
Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-NR2: 110 events / 20 users, last seen 2026-04-28 - UNITY-TEST-ENVIRONMENT-156: 1 events / 1 users, last seen 2026-04-20
… the identity itself
…, map, plugins, init Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-TEST-ENVIRONMENT-16E: 60 events / 1 users, last seen 2026-04-25 - UNITY-TEST-ENVIRONMENT-18V: 5 events / 4 users, last seen 2026-05-14
…guards, expected-error suppression
…onest failure Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-TEST-ENVIRONMENT-137: 61 events / 3 users, last seen 2026-05-21 - UNITY-EXPLORER-P5K: 35 events / 3 users, last seen 2026-05-30 - UNITY-EXPLORER-P3N: 10 events / 2 users, last seen 2026-05-16 - UNITY-TEST-ENVIRONMENT-TA: 8 events / 1 users, last seen 2026-02-10 - UNITY-EXPLORER-NMN: 6 events / 3 users, last seen 2026-04-24 - UNITY-EXPLORER-PCA: 3 events / 1 users, last seen 2026-07-12
…Ls (uses WebRequestUtils) Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-NNZ: 1079 events / 28 users, last seen 2026-07-16 - UNITY-EXPLORER-KVF: 52 events / 40 users, last seen 2026-05-18 - UNITY-TEST-ENVIRONMENT-13Z: 8 events / 2 users, last seen 2026-04-20 - UNITY-EXPLORER-P6J: 7 events / 6 users, last seen 2026-07-01 - UNITY-EXPLORER-KEF: 6 events / 3 users, last seen 2026-03-25 - UNITY-TEST-ENVIRONMENT-13Y: 2 events / 2 users, last seen 2026-04-20
…n scoping Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-TEST-ENVIRONMENT-17Z: 33 events / 24 users, last seen 2026-06-26 - UNITY-TEST-ENVIRONMENT-1CK: 22 events / 5 users, last seen 2026-07-08 - UNITY-TEST-ENVIRONMENT-16D: 13 events / 1 users, last seen 2026-05-05 - UNITY-TEST-ENVIRONMENT-187: 5 events / 3 users, last seen 2026-05-20 - UNITY-TEST-ENVIRONMENT-15W: 1 events / 1 users, last seen 2026-04-23 - UNITY-TEST-ENVIRONMENT-WN: 1 events / 1 users, last seen 2026-02-18
…ity swap fully unequips Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-PF3: 17 events / 1 users, last seen 2026-07-06 - UNITY-EXPLORER-KW0: 5 events / 2 users, last seen 2026-07-13 - UNITY-TEST-ENVIRONMENT-TT: 4 events / 2 users, last seen 2026-03-26 - UNITY-EXPLORER-NJ1: 3 events / 3 users, last seen 2026-04-19 - UNITY-EXPLORER-NJ0: 3 events / 3 users, last seen 2026-03-17 - UNITY-EXPLORER-MBC: 3 events / 1 users, last seen 2026-02-16
UNITY-EXPLORER-NQE (904 evts/15 users): ClearScript ScriptEngineException "WebSocket state is 3, cannot send data" bubbling out of SceneFacade.UpdateLoopAsync. The scene-facing JS WebSocket shim threw on any non-OPEN state; a scene calling send() after close (state 3 = CLOSED) threw once per tick and each was reported to Sentry. Per the WHATWG spec, send() on CLOSING/CLOSED silently discards; only CONNECTING is an error. Make CLOSING/CLOSED a no-op. NOTE: StreamingAssets JS ships with the build — takes effect next client build, not hot-patchable. Unverified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-NQE: 652 events / 14 users, last seen 2026-07-16
UNITY-EXPLORER-NT8 (824 evts/2 users): EcsSystemException [GliderPropControllerSystem]. The glider prop View is parented to the avatar transform; when the avatar GameObject is torn down by Unity, the child View is destroyed while the ECS GliderProp component still references it. CleanUpDestroyedAvatarsProp then dereferences the destroyed View (PrepareForNextActivation / .gameObject), throwing out of Update. Early-return on Unity-null View (overloaded ==), matching the codebase's SafeDestroy/null-guard idiom. Unverified (no Unity build). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-NT8: 39 events / 1 users, last seen 2026-07-06
UNITY-EXPLORER-P13 (637 evts/242 users — widest reach): "Segment error: Error
executing send loop (will retry): ClientError {...}". The Rust Segment SDK's
background send loop reports transient, self-recovering upload failures via
ErrorCallback, which called ReportHub.LogException → Sentry (error level). The
existing once-per-session guard still emitted one Sentry error per user. Route
"(will retry)" messages to LogWarning on ReportHandler.DebugLog (local log, never
Sentry); genuine non-transient Segment errors keep Sentry reporting. Removed the
now-unused ONCE_PATTERN_ALREADY_CAUGHT field. Report-noise reduction. Unverified.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Production evidence (decentraland Sentry, archive snapshot 2026-07-17):
- UNITY-EXPLORER-P13: 29 events / 15 users, last seen 2026-07-16
- UNITY-EXPLORER-JVR: 8 events / 1 users, last seen 2026-02-22
UNITY-EXPLORER-P4K (3010 evts/7 users): Utility.Networking.WebSocketException from scene JS connect(). WebSocketApiWrapper.ConnectAsync wrapped the call in .ReportAndRethrowException(exceptionsHandler), so every failed connection to a scene-supplied (dead/invalid) endpoint became a Sentry error — a scene retry loop produced thousands. The sibling SimpleFetchApiWrapper.Fetch deliberately does not report. Drop the report from connect only; ToDisconnectedPromise still rejects the scene's JS promise so its catch handler runs. Send/Receive/Close keep reporting. Report-noise reduction, not a functional bug. Unverified (no build). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-P4K: 94 events / 21 users, last seen 2026-07-15 - UNITY-EXPLORER-KG0: 11 events / 5 users, last seen 2026-03-15
UNITY-EXPLORER-P76 (4911 evts/68 users — #1 issue): EcsSystemException [CreateGltfAssetFromAssetBundleSystem]. The success branch of ConvertFromAssetBundle calls Utils.TryCreateGltfObject (Object.Instantiate + GetComponentsInChildren), which can throw when the asset bundle's underlying Unity object was destroyed/unloaded (ref-counted bundle evicted) — TryGetAsset only checks the C# list, not Unity-liveness, so it hands back a fake-null object. The throw escaped Update and surfaced as a wrapped EcsSystemException. Wrap in try/catch and emit a failed StreamableLoadingResult (mirroring the existing else-branch), which downstream finalize systems already handle. Uses only symbols already in scope (no new usings). Unverified (no Unity build). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-P76: 5999 events / 71 users, last seen 2026-07-17
decentraland-bot
added
the
ext-contribution
This comment has been minimized.
This comment has been minimized.
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR review — Step-by-step summary
STEP 1 — Context & scope
Loaded the repo review instructions, CLAUDE.md, docs/README.md, and relevant subsystem docs for systems, web requests, async, and avatar rendering. Checked out PR #9430 locally and compared chore/clean-combined-qa against origin/dev.
STEP 2 — Root-cause check
This PR is a compound robustness/noise-reduction set for several production crashes and teardown/cancellation edge cases. Several fixes appear to address the intended failure modes, but the current diff introduces a build-breaking API replacement (GetInstanceID() → GetEntityId()) in runtime/editor Unity-object code, so the review fails before runtime behavior can be trusted.
STEP 3 — Design & integration
No new long-lived system/manager/service/controller was introduced that required a separate lifecycle-owner relocation. Existing lifecycle-related edits were checked for disposal/cancellation direction; the blocking issue is local and build-time: GetEntityId() is only present on avatar attachment abstractions in this repo, not on UnityEngine.Object/GameObject/Mesh/AudioClipConfig.
STEP 4 — Member audit
Changed public/member surfaces inspected in the touched files. No public API consumer break was found; this is an internal Unity client change. The GetEntityId() calls have zero valid consumers on the affected Unity types and should be reverted to Unity's GetInstanceID().
STEP 5 — Line-level findings
Posted line-level suggestions for the compile blocker in each affected file. This is P0 — Build under the review severity table: the PR cannot compile as-is.
Security pass: no hardcoded secrets or sensitive-data exposure found in the diff. The media-converter URL guard is security-sensitive and should ideally be enforced server-side too, but it is not the merge blocker in this review.
STEP 6 — Complexity assessment
COMPLEX: large runtime diff touching ECS systems, avatar/rendering paths, scene runtime, networking/web requests, async cancellation, Web3 identity, UI, and editor/runtime Unity object handling.
STEP 7 — QA assessment
QA is required because runtime code under Explorer/ changes user-facing avatar loading, friends, loading screens, networking, media, authentication/Web3, and UI behavior.
STEP 8 — Non-blocking warnings
No Explorer/Assets/Scenes/Main.unity change detected.
CI status
Current checks are not green: Lint is failing; Windows/macOS builds and editmode tests were still in progress when checked; playmode passed.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Large runtime change set touching ECS, avatar/rendering, web requests, networking, async cancellation, Web3 identity, and UI systems.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Mikhail Agapov (<@U04A7TYN13L>) via Slack
| private void OnContinuousAudioStarted(AudioClipConfig audioClipConfig) | ||
| { | ||
| if (audioClipConfig.GetInstanceID() != backgroundMusic.GetInstanceID()) | ||
| if (audioClipConfig.GetEntityId() != backgroundMusic.GetEntityId()) |
There was a problem hiding this comment.
[P0] Build: AudioClipConfig derives from ScriptableObject; this repo has no GetEntityId() extension for it, so this replacement does not compile. Keep Unity object identity checks on GetInstanceID().
| if (audioClipConfig.GetEntityId() != backgroundMusic.GetEntityId()) | |
| if (audioClipConfig.GetInstanceID() != backgroundMusic.GetInstanceID()) |
|
|
||
| // TODO Jobify: can be invoked from a worker thread | ||
| Physics.BakeMesh(mesh.GetInstanceID(), false); | ||
| Physics.BakeMesh(mesh.GetEntityId(), false); |
There was a problem hiding this comment.
[P0] Build: Mesh does not expose GetEntityId(), and Physics.BakeMesh expects Unity's mesh instance ID. Revert this to GetInstanceID().
| Physics.BakeMesh(mesh.GetEntityId(), false); | |
| Physics.BakeMesh(mesh.GetInstanceID(), false); |
|
|
||
| for (int i = 0; i < dirtyParcels.Count; i++) | ||
| meshes[i] = dirtyParcels[i].Mesh.GetInstanceID(); | ||
| meshes[i] = dirtyParcels[i].Mesh.GetEntityId(); |
There was a problem hiding this comment.
[P0] Build: Mesh does not expose GetEntityId(). The bake job needs Unity instance IDs, so keep GetInstanceID() here.
| meshes[i] = dirtyParcels[i].Mesh.GetEntityId(); | |
| meshes[i] = dirtyParcels[i].Mesh.GetInstanceID(); |
| mapMarker = null; | ||
|
|
||
| if (gameObject.GetInstanceID() != homeMarker.MarkerObject.gameObject.GetInstanceID()) | ||
| if (gameObject.GetEntityId() != homeMarker.MarkerObject.gameObject.GetEntityId()) |
There was a problem hiding this comment.
[P0] Build: GameObject does not expose GetEntityId() in this repo. Revert the marker identity comparison to Unity's GetInstanceID().
| if (gameObject.GetEntityId() != homeMarker.MarkerObject.gameObject.GetEntityId()) | |
| if (gameObject.GetInstanceID() != homeMarker.MarkerObject.gameObject.GetInstanceID()) |
| public bool TryDeHighlightObject(GameObject gameObject) | ||
| { | ||
| if (gameObject.GetInstanceID() != homeMarker.MarkerObject.gameObject.GetInstanceID()) | ||
| if (gameObject.GetEntityId() != homeMarker.MarkerObject.gameObject.GetEntityId()) |
There was a problem hiding this comment.
[P0] Build: same GameObject.GetEntityId() compile break as above; use Unity's instance ID.
| if (gameObject.GetEntityId() != homeMarker.MarkerObject.gameObject.GetEntityId()) | |
| if (gameObject.GetInstanceID() != homeMarker.MarkerObject.gameObject.GetInstanceID()) |
| mapRenderMarker = null; | ||
|
|
||
| if (gameObject.GetInstanceID() != homeMarker.MarkerObject.gameObject.GetInstanceID()) | ||
| if (gameObject.GetEntityId() != homeMarker.MarkerObject.gameObject.GetEntityId()) |
There was a problem hiding this comment.
[P0] Build: same GameObject.GetEntityId() compile break as above; use Unity's instance ID.
| if (gameObject.GetEntityId() != homeMarker.MarkerObject.gameObject.GetEntityId()) | |
| if (gameObject.GetInstanceID() != homeMarker.MarkerObject.gameObject.GetInstanceID()) |
| if (child.name.Contains("_collider")) { | ||
| MeshFilter meshFilter = child.GetComponent<MeshFilter>(); | ||
| Physics.BakeMesh(meshFilter.sharedMesh.GetInstanceID(), false); | ||
| Physics.BakeMesh(meshFilter.sharedMesh.GetEntityId(), false); |
There was a problem hiding this comment.
[P0] Build: Mesh does not expose GetEntityId(), and Physics.BakeMesh expects Unity's mesh instance ID. Revert this call.
| Physics.BakeMesh(meshFilter.sharedMesh.GetEntityId(), false); | |
| Physics.BakeMesh(meshFilter.sharedMesh.GetInstanceID(), false); |
|
Warnings not reduced: 19087 => 19091 — remove at least one warning to merge. |
|
Claude finished @decentraland-bot's task in 3m 5s —— View job Security-focused dependency & workflow review (re-run on synchronize)
Re-ran the supply-chain pass against the current head ( Step 1 — Dependency-change inventory (re-verified)
Non- Step 2 — Security-relevant source (unchanged since last review, re-confirmed benign)
Nothing here adds network reach, dynamic assembly loading, external content rendering, or region/remote-config-gated behavior. Note on the other bot's P0 "build break" (out of my security scope, but worth flagging before anyone acts on it)The Jarvis review posted 7 P0 "does not compile" comments asking to revert
Confidence: [Inference], HIGH — I can't run the Unity compiler on this headless runner, so please treat the CI Windows/macOS build result (not the static-analysis bot) as the source of truth. If those builds go green, the P0 comments can be dismissed. Separately, the current merge blocker is the warning ratchet ( Summary
No HIGH/MEDIUM supply-chain findings. No
DEPENDENCY_REVIEW: PASS |
|
|
||
| (int vertCount, int totalBoneBufferCount) = SetupCounters(meshesData, boneCount); | ||
|
|
||
| if (vertCount == 0 || boneCount == 0) |
There was a problem hiding this comment.
Not clear under which conditions this path is reachable
|
|
||
| skinningComponent.SetVertOutRegion(vertOutBuffer.Rent(skinningComponent.VertCount)); | ||
| // an empty avatar (VertCount == 0) owns no region of the shared buffer (Rent rejects 0) | ||
| if (skinningComponent.VertCount > 0) |
There was a problem hiding this comment.
How is it possible to have an avatar with 0 bones? The path was added in this PR, probably it simply hides another error, so it's not a fix
|
|
||
| public void StartMaskedLegacyEmote(AnimationClip clip, AvatarMask avatarMask, bool loop) | ||
| { | ||
| if (AvatarAnimator == null) return; |
There was a problem hiding this comment.
It's a serialized feild which is always assigned
|
|
||
| public void StartMaskedLegacyEmote(AnimationClip clip, AvatarMask avatarMask, bool loop) | ||
| { | ||
| if (AvatarAnimator == null) return; |
There was a problem hiding this comment.
Also checking by null a non-nullable field contradicts the nullability rules
| public void SetPointAtLayerWeight(float weight) => | ||
| public void SetPointAtLayerWeight(float weight) | ||
| { | ||
| if (AvatarAnimator == null) return; |
There was a problem hiding this comment.
So all these checks are redundant
| // ClearTips runs; skip the already-destroyed entries. | ||
| foreach (TipView tip in tips) | ||
| Destroy(tip.gameObject); | ||
| if (tip != null) Destroy(tip.gameObject); |
There was a problem hiding this comment.
It's a fix of nothing - probably the call order from the upper layer is incorrect
| private static long EstimateTextureByteSize(Texture2D tex2d) | ||
| { | ||
| // Destroyed textures (Unity fake-null) account as zero instead of throwing. | ||
| if (tex2d == null) return 0L; |
There was a problem hiding this comment.
it's not clear under which conditions the underlying texture can be nullified. Most likely once again it's not a real fix, as it will simply move the bug to the later moment
| // A null profile must fail the operation: adding it to the world poisons every | ||
| // Profile-querying system with a per-frame NRE (observed when the profiles endpoint | ||
| // returns no avatars for the signed-in wallet). | ||
| if (profile == null) |
There was a problem hiding this comment.
Breaking on start-up should be a super verbose operation with a clear description, the provided exception doesn't indicate the criticality and the moment correctly
| using var textReader = new StreamReader(stream, Encoding.UTF8); | ||
| using var jsonReader = new JsonTextReader(textReader); | ||
|
|
||
| if (Target == null) |
There was a problem hiding this comment.
Nullability contradiction, if it's null the review of the upper call chain required
| public static bool IsNonPubliclyRoutable(string url) | ||
| { | ||
| if (IsLocalhost(url)) return true; | ||
| if (!Uri.TryCreate(url, UriKind.Absolute, out Uri? uri)) return false; |
There was a problem hiding this comment.
Creating an object on every URL / every call is a strong NO. This whole function should be done in an entirely zero-allocation way - it's a hot path!
|
This PR contains a lot of AI noise and some of the problems require deeper investigation of the real causes: I'd recommend to leave in this PR the unquestionable changes only, and investigate problems where nulls shouldn't be (but they are present) in individual ones with proper analysis of the source. |
3 participants
Compound PR for the
chore/clean-*fix set — this is the review and merge unit (team decision; the 14 individual PRs #9416–#9429 are closed and all further fixes for this set land here). Each fix is its own signed merge commit, so per-fix review works commit-by-commit.Constituents (each independently rebased on
dev, mutually conflict-free):Each merge commit's message carries the production-error evidence (Sentry issue ids + event/user counts) motivating that fix.
QA focus areas: avatar/wearable loading, friends panel across reconnects, loading screens & teleports, media scenes, profile edits. The rest are Sentry-noise/defensive changes with no expected behavior delta.
92 files, +602/−182 vs dev. Compile-verified green (216 assemblies, 0 errors) on Unity 6000.4.11f1.