fix: bugsweep dataroom 3 — compounded fixes (24 PRs) - #9828
fix: bugsweep dataroom 3 — compounded fixes (24 PRs)#9828lorenzo-ranciaffi wants to merge 25 commits into
Conversation
🚦 CI StatusNew build in progress, come back later! Lint in progress, come back later!
Failed tests (1)
|
Squashed changes from PR #9792. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Squashed changes from PR #9795. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Squashed changes from PR #9798. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Squashed changes from PR #9803. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Squashed changes from PR #9804. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c7a27fc to
c751557
Compare
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #9828
STEP 1 · Context
Compound PR merging 24 individually-reviewed bug fixes from the Data Room 3 sweep into dev. Changes span ~110 files across auth flow, avatar rendering, chat analytics, CRDT/ECS bridge, asset bundles, scene lifecycle, scene runtime, EventBus, WebSocket, map renderer, multiplayer, notifications, diagnostics/Sentry, debug console, scene loading screens, Web3 signing, KTX texture support, and scene UI canvas. Each sub-PR targets a distinct production bug or performance regression.
STEP 2 · Root-cause check
PASS — every fix addresses the actual root cause:
| Sub-PR | Root cause addressed |
|---|---|
| #9626 | C# operator precedence (?? vs +) — the arithmetic was grouping 0 + dt inside the fallback |
| #9634 | Oversized scene messages thrown instead of dropped — now fails gracefully |
| #9795 | CRDT rent slot leaked on processing failure — AbortSyncCommandBuffer releases cleanly |
| #9799 | GLTF cache disposed clones still checked out by containers — ownership transferred |
| #9806 | Nethereum returns unsigned-trimmed private key bytes — left-pad to 32 |
| #9807 | No serialization of realm changes + duplicate scene facades — semaphore + parcel dedup |
| #9810 | DisposeThumbnail guarded by IsInitialized (true for failed results) instead of Succeeded |
| #9811 | WebSocket ConnectAsync not cancellable — linked CTS unparks the pending await |
| All others | Each similarly addresses the underlying defect, not a symptom |
STEP 3 · Design & integration
PASS — no new lifecycle owners were introduced beyond what's needed:
CorruptAbCacheEvictor— static utility, zero lifecycle stateKtxNativeSupport— lazy static probe, fail-closed, session-permanentCategoryExclusionMatrix— decorator for Sentry scope, scoped to report lifetimePooledContinuation<T>— allocation-free optimization of existing EventBus hopDCLSemaphoreSliminRealmController— serializes realm changes at the correct scope
No duplicated subsystems; all new types sit at the appropriate abstraction level.
STEP 4 · Member audit
PASS — new public/internal members are well-scoped:
ICRDTWorldSynchronizer.AbortSyncCommandBuffer— two call sites (both catch blocks inEngineAPIImplementation), appropriate contract additionKtxNativeSupport.IsSupported/MarkUnsupported— consumed byGetTextureWebRequest,RequestHub,NFTShapePlugin; internalReset/probeOverrideare test-onlyInterlockedFlagonSceneRuntimeImpl.isDisposing— private, guards concurrentSetIsDisposingentryGenericDownloadHandlerUtils.PopulateInto<T>— internal, consumed byOverwriteFromNewtonsoftJsonAsync
STEP 5 · Line-level review
See inline comments. One P2 finding (defensive guard in LeftPad).
Notable well-executed patterns:
- SignedFetchWrap: Capturing
disposeCts.Token(value type) before theSwitchToMainThreadhop correctly prevents the race where the CTS is disposed during the hop. Excellent root-cause fix. - PooledContinuation: The copy-to-locals-then-recycle pattern prevents reentrant publish corruption and avoids retaining class-typed payloads in the pool. The double-run guard with
typedDelegate is not { }is a clean safety net. - CRDT
delegatedflag: Exactly-one-of-Apply-or-Abort semantics prevent both slot leaks and double-releases. The flag placement (set immediately before theApplySyncCommandBuffercall) is correct. - CommsApiWrap copy-on-write: Volatile array reference with
Interlocked.Exchangeon mutation is the right lock-free pattern for high-frequency reads on the LiveKit thread. - DebugMenuConsoleLogHistory: The pending-queue/main-thread-drain pattern with bounded capacity is correct. The
drainBuffer.Clear()is placed beforeLogsUpdated?.Invoke(), preventing stale entries on subscriber exceptions.
STEP 6 · Complexity
HIGH — 24 distinct bug fixes touching concurrency (CTS races, thread-safe dispose, lock-free data structures), memory management (pool patterns, ownership transfer, rent-slot lifecycle), ECS integration (structural changes during queries, component lifecycle), and native interop (KTX probe, Rust FFI key padding). Each fix is individually well-scoped, but the aggregate surface area is substantial.
STEP 7 · QA assessment
QA_REQUIRED: YES — production bug fixes across core subsystems (auth, rendering, comms, scene lifecycle, Web3 signing). 30+ new test files provide strong unit coverage, but integration-level validation of the compound effect is warranted.
STEP 8 · Warnings
- CI: Most checks show
skipping— expected for compound PRs where individual sub-PRs carried their own CI runs. Build Gate, Merge Eligibility, rsp match, and semantic title checks all pass. - No security issues found: No secrets committed, input validation improved (oversized message dropping), private key handling is correctness-only (left-padding, not new exposure), thread-safety improvements reduce race-condition attack surface.
STEP 9 · Verdict
REVIEW_RESULT: PASS
COMPLEXITY: HIGH
COMPLEXITY_REASON: 24 cross-cutting bug fixes spanning concurrency, memory management, ECS lifecycle, native interop, and thread safety across 110+ files
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
|
||
| private static byte[] LeftPad(byte[] value, int size) | ||
| { | ||
| if (value.Length == size) |
There was a problem hiding this comment.
[P2] Defensive guard: LeftPad crashes on oversized input
If value.Length > size, the Buffer.BlockCopy destination offset (size - value.Length) goes negative, throwing ArgumentOutOfRangeException. While secp256k1 private keys are always ≤ 32 bytes in practice, a >= guard is strictly safer — it lets RustEthSignServer.Initialize reject the oversized key with a meaningful error instead of crashing in LeftPad.
| if (value.Length == size) | |
| if (value.Length >= size) |
|
🔍 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
There was a problem hiding this comment.
PR Review — fix: bugsweep dataroom 3 — compounded fixes (24 PRs)
STEP 2 — Root-cause check: PASS ✅
All 24 sub-PRs address actual root causes rather than symptoms:
- Operator-precedence bug (
CharacterEmoteIntent):??binding0 + dtas the fallback → fixed by parenthesizing(playTimeout?.ElapsedTime ?? 0) + dt. - Shared-mutable-state (
ChatMessagesBusAnalyticsDecorator): staticJArrayreplaced with per-event local;UserId.Valueextracts the string instead of boxing the struct. - Thread-safety (
DebugMenuConsoleLogHistory): direct list mutation from log callback replaced with a producer queue + main-thread drain. - Rent-slot leak (
EngineAPIImplementation): CRDT reconciliation wrapped in try/catch to callAbortSyncCommandBufferon failure, ensuring the single rent slot is always released. - Realm race (
RealmController): addedDCLSemaphoreSlimto serializeSetRealmAsyncso parallel realm switches can't corrupt the entity graph. - Thumbnail disposal (4 storage classes): changed from
IsInitialized: truetoSucceeded: true— failed/cancelled results carry defaultSpriteDatathat must not haveRemoveReferencecalled. - Private-key padding (
RustEthereumAccount):LeftPadensures 32-byte big-endian scalar for the ~1/256 of keys whose MSB is zero. - Connection parking (
DCLWebSocket):connectAbortCTS unparks pendingConnectAsyncon close/dispose. - Input-block refcount leak (
SceneLoadingScreenController): idempotentBlock/Unblockwith aninputsBlockedflag prevents refcount drift when the outer token is cancelled or the scene never finishes loading. - MVC cascade failure (
MVCManager): try/catch in dispose loop so one controller's failure doesn't abort disposal of the rest. - KTX2 native fallback (
KtxNativeSupport): probe + runtimeMarkUnsupported()degrades gracefully when the native plugin can't load. - Asset ownership (
AssetPreLoadCache): removedCopieslist — clones are owned by their containers, fixing a double-dispose hazard. - CRDT slot leak (
CrdtEcsSynchronizer): newAbortSyncCommandBuffer()releases the semaphore for buffers that never reachApplySyncCommandBuffer. - Corrupt AB cache (
CorruptAbCacheEvictor+LoadAssetBundleSystem): evicts poisoned Unity Caching entries and retries once. - Allocation-free event dispatch (
EventBus):PooledContinuation<T>replaces the closure-based thread hop. - Allocation-free topic matching (
CommsApiWrap): copy-on-writeTopicLookupEntry[]with span comparison on the LiveKit thread.
No symptom-only workarounds detected.
STEP 3 — Design & integration: PASS ✅
Owner search results:
ControlSceneUpdateLoopSystem.AnyParcelHasLiveScene(): Runs once per entity whenScenePromiseresolves — not a per-frame scan. Correct placement at the creation moment.CorruptAbCacheEvictor: Stateless static utility called fromLoadAssetBundleSystem; no lifecycle to manage. Correct.CategoryExclusionMatrix: Thin 19-line decorator overICategorySeverityMatrix; appropriate for the local-scene-dev exclusion. Correct.KtxNativeSupport: Machine-level static capability check; legitimate as a static class. Correct.PooledContinuation<T>: Implementation detail nested insideEventBus. Correct home.SceneLoadingScreenController.inputsBlocked: UI-layer state correctly outside ECS.MVCManagertry/catch: Exception routed throughReportHub.LogException, not swallowed. Correct.ISceneTipsProviderasync→sync: All 3 implementations and sole caller updated consistently; async was vestigial. Correct.
Teardown trace: Subscriptions, CTS, semaphores, and buffers in the diff all have matching teardown/release paths — one minor note on realmChangeSemaphore disposal below.
Security review: No critical or high findings. Medium: private key bytes in RustEthereumAccount not zeroed after Initialize() (pre-existing pattern, not introduced by this PR). All thread-safety, race-condition, and resource-leak patterns reviewed — no issues found.
STEP 5 — Line-level findings
All findings are P2 (minor). No P0 or P1 issues found.
See inline comments for specific suggestions.
Additional notes (body-only):
-
[P2 — Security]
RustEthereumAccount.cs: TheLeftPadcopy of the private key bytes (byte[32]) is abandoned to the GC afterRustEthSignServer.Initialize(bytes). Pre-existing pattern — considerArray.Clear(bytes, 0, bytes.Length)afterInitialize()in a follow-up. -
[P2 — Simplification]
KtxNativeSupport.csprobe: The eagerProbe()with garbage input duplicates the lazyMarkUnsupported()fallback already inGetTextureWebRequest.cs. The probe adds complexity and a harmless-but-expected error log in debug builds. Consider removing the probe in a follow-up, defaultingIsSupportedtotrue, and relying solely onMarkUnsupported(). -
[P2 — Design]
EngineAPIImplementation.csdelegatedflag: The ownership-transfer between the caller'sdelegatedboolean and the synchronizer's internalfinallyrelease is correct today but comment-linked, not compiler-enforced. A future change toCrdtEcsSynchronizer.ApplySyncCommandBuffercould silently reintroduce a slot leak. Low risk, but worth noting.
STEP 6 — Complexity: COMPLEX
Touches ECS systems, CRDT synchronization, scene lifecycle, networking, asset loading, async/cancellation, memory management, and diagnostics across 110+ files.
STEP 7 — QA: YES
Extensive runtime code changes affecting scenes, avatars, networking, input, loading screens, asset loading, and diagnostics.
STEP 8 — Warnings
No Main.unity changes detected.
STEP 9 — Verdict
Excellent compound PR. 24 root-cause fixes with comprehensive regression tests (~40 new test files). Each sub-PR addresses a real bug with thorough documentation. All findings are P2 minor — no blockers.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS systems, CRDT synchronization, scene lifecycle, networking, asset loading, async/cancellation, and diagnostics across 110+ files
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| ReportHub.LogWarning(GetReportData(), $"Duplicate scene definition for '{definitionComponent.Definition.GetLogSceneName()}': discarding its facade"); | ||
| scene.DisposeAsync().Forget(); |
There was a problem hiding this comment.
[P2] Detached async disposal (CLAUDE.md §9). scene.DisposeAsync().Forget() silently drops any exception from the rejected facade's disposal. While .Forget() for disposal has codebase precedent, adding SuppressToResultAsync ensures exceptions are logged.
| ReportHub.LogWarning(GetReportData(), $"Duplicate scene definition for '{definitionComponent.Definition.GetLogSceneName()}': discarding its facade"); | |
| scene.DisposeAsync().Forget(); | |
| ReportHub.LogWarning(GetReportData(), $"Duplicate scene definition for '{definitionComponent.Definition.GetLogSceneName()}': discarding its facade"); | |
| scene.DisposeAsync().SuppressToResultAsync(ReportCategory.SCENE_LOADING).Forget(); |
|
PR #9828, run #32468727655 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
Trim the comments this branch added down to their load-bearing facts: drop Arrange/Act/Assert scaffolding, restated-code narration, and multi-line essays; keep regression references (issues and Sentry IDs). Review fixes: - ControlSceneUpdateLoopSystem: log duplicate-facade disposal failures via SuppressToResultAsync instead of a bare Forget - RustEthereumAccount: LeftPad returns oversized input unchanged so RustEthSignServer.Initialize rejects it instead of crashing - EventBus: justify the default! on the pooled continuation payload - PrivateConversationUserStateService: drop the file-wide InconsistentNaming suppression
Pull Request Description
What does this PR change?
Compounded bugsweep branch (
bugsweep/dataroom-3→dev) that merges the changes from the following PRs, one commit per PR:Based on
dev:Based on
main(only the PR's own changes were taken):Test Instructions
Test Steps
All remaining changes (#9634, #9633, #9632, #9791, #9792, #9794, #9795, #9798, #9801, #9802, #9804, #9806, #9811) are internal fixes not directly experienceable — a general smoke test (log in, walk around, chat, teleport, quit) validates them.
Quality Checklist
🤖 Generated with Claude Code