Repository navigation
fix: bugsweep week 2026-W34 - #9908
Conversation
…ch (#9839) Issue: #9839 Sentry: https://decentraland.sentry.io/issues/7501163616/ Exception: System.Exception: sceneMetadata of <wearable> is null (e.g. "Flight by Knight") Repro / trigger: equipping a smart wearable whose first scene-metadata fetch is cancelled (fast equip/unequip, realm or scene change) or fails transiently (network/JSON), then loading that wearable's scene again in the same session. Root cause: SmartWearableCache.CacheWearableInternalAsync added the CacheItem to the dictionary BEFORE populating it. If the awaited scene.json metadata fetch was cancelled (returned null without removing the entry) or threw (exception propagated, entry left behind), the dictionary permanently retained an item in a poisoned state: IsSmart == true, SceneContent != null, SceneMetadata == null. Because LoadSmartWearableSceneSystem is wired NoCache, this poisoned entry persisted for the whole session; every later GetCachedSceneInfoAsync returned (SceneContent, null), and LoadSmartWearableSceneSystem threw "sceneMetadata of <name> is null". A malformed wearable missing scene.json hit the same poisoned state via the scene.json-not-found branch. Fix (at the publisher, not the throw site): wrap the population following cache.Add in try/catch so any exception during the metadata fetch evicts the half-built entry and rethrows; explicitly evict + return null on the graceful cancellation path (callers already null-guard that path via ct.IsCancellationRequested, so no null is returned when ct is not cancelled); and, on the scene.json-not-found branch, downgrade the item to non-smart instead of caching a smart entry with no metadata. A later request then retries from a clean cache instead of returning a poisoned, metadata-less item. cache.Remove is O(1) and only on failure paths. Testing: no new unit test added — the sweep environment cannot execute the Unity test runner to validate a new NUnit test, and there is no existing SmartWearableCache test to extend; correctness deferred to CI and human review.
#9832) Issue: #9832 Build: V.171. Reproduces on both platforms, always. Repro: launch a local scene dev session (--realm http://127.0.0.1:8000/ --position 6,2 --local-scene true --debug), have another user send a direct message to the local-scene user. The local-scene user receives the DM but appears offline to others and has no UI to reply. Expected: a user on a local scene must not receive direct messages from others. Root cause: in local-scene development mode the whole Friends subsystem is disabled (FeaturesRegistry sets FeatureId.Friends = ... && !localSceneDevelopment), which removes the DM reply UI and keeps the user out of shared presence, but the inbound direct-message path was not gated by that flag. Private messages arrive over the LiveKit chat room via LiveKitChatMessagesBus, whose HandleChatPipeMessage already gates COMMUNITY messages behind isCommunitiesIncluded but delivered the USER (direct-message) branch unconditionally. The user therefore received DMs it could neither answer nor be seen replying to. Fix: gate the USER branch of HandleChatPipeMessage on FeatureId.Friends, mirroring the existing isCommunitiesIncluded gate. FeatureId.Friends is the correct root-cause gate: it is the same flag that removes the reply UI, is already "&& !localSceneDevelopment", and also covers any other Friends-disabled scenario (kill switch), rather than gating on a raw local-scene bool. The flag is read once in the constructor (where FeaturesRegistry.Instance is already used for other flags); no new subscription, no allocations, no CancellationToken surface. Testing: no new unit test added — the sweep environment cannot execute the Unity test runner to validate a new test; correctness deferred to CI and human review.
…rrors (#9840) Issue: #9840 Sentry: https://decentraland.sentry.io/issues/7313180903/ Exception: System.Exception: Segment operation <id> Flush failed with: Error Labels: bug, 3-low, analytics. Root cause: RustSegmentAnalyticsService.Callback (the native completion callback) raised any non-Success native result as an Exception via ReportHub.LogException, which Sentry captures as an error-level issue. The native segment-server library only reports Success/Error (no detail), persists operations in a local SQLite queue and retries them, so a non-Success result is a transient failure delivering an already-queued analytics event to Segment's external ingestion API - not a client logic bug and not data loss. Reporting it at error level floods the Sentry error budget. The sibling ErrorCallback in the same file already recognises this class of noise ("temportal sentry budget fix" dedup), but that mitigation was not applied to the Callback path. Fix: downgrade the external delivery failure from ReportHub.LogException to ReportHub.LogWarning(ReportCategory.ANALYTICS, ...). This is severity calibration, not symptom-hiding: the failure is still reported (a warning-level breadcrumb kept for crash correlation), consistent with the handler's existing breadcrumb-only treatment of other external noise (e.g. ASSET_BUNDLES). Genuine client faults keep their existing handling untouched - invalid operationId (operationId == 0) still LogError via AlertIfInvalid, and init/dispose failures still throw. Testing: no new unit test added - this is a logging-severity change on a native completion callback with no runnable Unity-test seam in the sweep environment; deferred to CI and human review.
…of hanging (#9781) Issue: #9781 Labels: bug, 1-high, sdk, Mac Only, qa-team. Repro: launch a built player pointed at a local scene dev server on a non-loopback LAN IP over http, e.g. open ./Decentraland.app --args --realm http://<LAN_IP>:8000 --position 0,0 --local-scene true --debug Accept the "custom catalyst" prompt. Expected: the app fails fast with an actionable error ("insecure connection not allowed" or similar). Actual: the loading screen hangs indefinitely with no error; sessions had to be force-quit. Reproduces on both the current production build and PR #9765. Root cause: RealmController.SetRealmExclusiveAsync issues the realm /about GET with CommonArguments whose default timeout is 0 (infinite in Unity). In built players the platform's transport security blocks non-loopback cleartext (http) connections; on macOS the blocked request stalls without ever completing or faulting, so the awaited /about request never returns. Because this bootstrap request runs before the loading screen's own 2-minute timeout guard (which only wraps the user-init flow), the await never completes, SetRealmAsync never throws, MainSceneLoader's catch(RealmChangeException) never runs, and no load-error popup is shown -> indefinite silent hang. "Mac only" reflects the divergent native behavior: other platforms fault promptly and already surface the popup. Fix: add a pre-flight guard in SetRealmExclusiveAsync (built players only, via #if !UNITY_EDITOR so editor LAN-http dev testing is unaffected) that, before issuing the /about request, detects an http (cleartext) realm URL that is not a loopback address (reusing the existing WebRequestUtils.IsLocalhost predicate) and fails fast: it logs via ReportHub/ReportCategory.REALM and throws RealmChangeException, which the existing MainSceneLoader.ShowLoadErrorPopupAsync (bootstrap) and RealmNavigator error mapping (in-app) already route into a user-facing error popup. https realms and loopback dev realms (localhost/127.0.0.1/[::1]) are untouched, so no working flow changes - only realms that currently hang now fail fast with a clear message. Adds a message-only RealmChangeException constructor for the no-inner-exception case. Scope note: only the fast-fail guard is included. A finite timeout on the /about request was considered as defense-in-depth but not applied, to avoid changing the timeout behavior of legitimately slow realm loads. Testing: no new unit test added - the behavior is platform-transport-specific (built-player macOS ATS) and gated behind #if !UNITY_EDITOR, so it is not exercisable by the editor test runner; correctness deferred to CI and human/QA review.
…d of an engine exception (#9833) Issue: #9833 Sentry: https://decentraland.sentry.io/issues/7630091621/ Exception: DCL.Web3.Web3Exception: The method is not allowed: wallet_switchEthereumChain Root cause: when an SDK scene calls a non-whitelisted Ethereum RPC method (e.g. wallet_switchEthereumChain, which is intentionally not in Web3WhitelistMethods), the provider chain throws Web3Exception "The method is not allowed". EthereumApiWrapper's SendAndFormatAsync caught it in its generic catch(Exception) and routed it through sceneExceptionsHandler.OnEngineException, which (a) reports it to Sentry as an engine-level error (the Sentry flood) and (b) counts it toward ENGINE_EXCEPTIONS_PER_MINUTE_TOLERANCE, so a scene/dapp calling a disallowed method 4+ times in 60s is forced into SceneState.EngineError. It also returned a result:null response with no JSON-RPC error member, so the scene could not tell "not allowed" from a null result. The method rejection itself is correct and intended; only the surfacing was wrong. (This differs from an expected on-chain error that must surface: it is a purely local allow-list decision driven by scene input.) Fix: introduce Web3MethodNotAllowedException : Web3Exception and throw it at the deterministic allow-list/permission rejection sites (DappWeb3EthereumApi, ThirdWebEthereumApi non-whitelisted method; RestrictedEthereumApi disabled Web3 API). EthereumApiWrapper.SendAndFormatAsync now catches that specific type BEFORE the generic handler and returns a spec-compliant JSON-RPC 2.0 error (code -32601 "Method not found", message = the rejection text) to the scene using the existing EthApiResponse.error / EthApiError primitive, WITHOUT calling OnEngineException. The allow-list is unchanged (wallet_switchEthereumChain stays unsupported); genuine faults still flow through OnEngineException unchanged. Because the new type derives from Web3Exception, any existing Web3Exception handler still catches it. The parallel ThirdWeb LogError for this deterministic case is also downgraded to a warning so it no longer spams Sentry. Testing: no new unit test added - the sweep environment cannot execute the Unity test runner to validate a new NUnit test; correctness deferred to CI and human review.
#9783) Issue: #9783 Labels: bug, 1-high, ui, qa-team. Reproduction: Always (100%). Repro: open the desktop Explorer Places tab while a scene (e.g. Clyde's yoga scene) is hosting a live event with concurrent users. Expected: scenes with live concurrent users are surfaced/sorted toward the top. Actual: the live scene does not appear at the top and is only discoverable via manual search, while decentraland.org/whats-on does show it. Root cause: ordering of the Places Browse list is driven entirely by the order_by query param the client sends to /api/destinations; there is no client-side re-sort of the Browse list. The Places tab defaulted its sort to Best Rated (SortBy.LIKE_SCORE -> order_by=like_score) on every tab activation, via PlacesView.ResetCurrentFilters and the matching PlacesFilterSelectorView.ResetFilters toggle default. So the client asked the backend to rank by likes, and a scene full of live users was not surfaced - even though the same backend queried with order_by=most_active (what /whats-on uses, and the IPlacesAPIService default) returns it first. withConnectedUsers/withLiveEvents were already requested; live data was fetched but not used as the sort key. Fix: default the Places tab sort to SortBy.MOST_ACTIVE in PlacesView.ResetCurrentFilters, and switch the matching default toggle in PlacesFilterSelectorView.ResetFilters from Best Rated to Most Active (both the invokeEvents and SetIsOnWithoutNotify branches) so the dropdown reflects the applied ordering. This aligns the client default with the service default and /whats-on. Users can still switch to Best Rated manually. Note: this is a user-visible default-behavior change for the Places tab and may warrant product/design sign-off; it directly matches the QA-filed expectation and the existing IPlacesAPIService default. Server-side highlighted/featured overrides and the web 5+ concurrent-user threshold live in the Places backend (out of this repo) and are unchanged. Testing: no new unit test added - the sweep environment cannot execute the Unity test runner; the change is a default-value alignment across two view classes. Deferred to CI and human review.
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings count reduced: 12178 => 12129 Warnings/errors in files changed by this PR (40)Lint run · full InspectCode report · took 31m 34s All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #33662695777. Full reportPR #9908, run #33662695777 Overall: 🔴 CPU average regressed on Apple M1; CPU 1% worst regressed on Apple M1; GPU 1% worst regressed on Apple M1 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Exception breakdown
Intel Core i5
|
This comment has been minimized.
This comment has been minimized.
alejandro-jimenez-dcl
left a comment
alejandro-jimenez-dcl
left a comment
There was a problem hiding this comment.
#9783: Valid
#9833: Move magic number -32601 to a const
#9781: Drop it, we'll fix it manually, it's complex. The solution provided doesnt address the root cause
#9840: Drop it, "Cannot flush: sqlite error: database or disk is full" should be handled in https://github.com/decentraland/segment
#9832: Drop it, drop connection to either global room or the livekit chat room as a whole
#9839: keep item.IsSmart as true but log the issue.
…Sentry errors (#9840)" This reverts commit 2c1ac8ee4bfe66aa53f0876368d75b1b70808043. Dropped per PR #9908 review: flush failures such as "Cannot flush: sqlite error: database or disk is full" should be handled in the https://github.com/decentraland/segment library, not silenced client-side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nt (#9832) Issue: #9832 Per PR #9908 review: instead of gating the inbound USER branch of LiveKitChatMessagesBus (reverted), do not connect the LiveKit chat room at all in local-scene development mode. Fix: in CommsContainer, pass IConnectiveRoom.Null.INSTANCE as the RoomHub chat room when localSceneDevelopment is on, mirroring the existing island room gate one line above. The Null room reports StartAsync success and exposes NullRoom/NullDataPipe, so RoomHub.StartAsync, the StartLiveKitRooms health check, MessagePipesHub, LiveKitChatMessagesBus and MultiplayerReactionMessageBus keep working - chat-room traffic is simply never sent or received. PrivateConversationUserStateService.InitializeAsync now skips its wait-for-connection in local scene development, since the room will never connect and the wait would only burn the 2-minute timeout and log a spurious error. Known accepted consequence: community chat messages and DM/community chat reactions also ride the chat room pipe, so they stop flowing in local-scene development sessions as well ("drop connection ... as a whole"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ad (#9839) Issue: #9839 Per PR #9908 review: on the missing-scene.json branch, keep item.IsSmart as true (the DTO advertises a smart wearable) and log the issue, instead of silently downgrading the wearable to non-smart. The half-built entry is still evicted from the cache so a metadata-less item is never served to a later request; each access to such a malformed wearable re-runs the (local, cheap) check and re-logs. The eviction paths for cancelled and throwing metadata fetches are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per PR #9908 review: replace the inline -32601 magic number in EthereumApiWrapper with JSON_RPC_METHOD_NOT_FOUND, following the McpJsonRpcDispatcher precedent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review point by point (follow-up commits, no history rewrite):
|
Strip narration of caller behavior and restatements of the code from the comments added in this PR; each surviving comment states only the constraint or rationale the code cannot show. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the disk is full the Segment persistent queue fails with SQLITE_FULL, but the native plugin collapsed every failure into a generic Response::Error, so Unity reported an opaque Sentry exception (#9840) and the user never learned their disk ran out of space. - native plugin: bump segment rev (typed DiskFull error) and forward Response::ErrorDiskFull = 2 from the flush/enqueue paths; rebuilt the universal mac dylib (Windows dll still needs a rebuild on Windows) - RustSegmentAnalyticsService: on ErrorDiskFull log a warning instead of a Sentry exception and publish AnalyticsDiskFullDetected once per session on the analytics event bus - AnalyticsDiskFullPopupPlugin: subscribes and shows a Storage Full popup via the already-registered ErrorPopupController Fixes #9840 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Explorer/Assets/DCL/Places/PlacesView.cs
This comment has been minimized.
This comment has been minimized.
Issues found while testing the disk-full flow in the editor: - the once-per-session latch lived on the publisher, so a disk-full flush during boot (before the popup plugin subscribed) consumed the only event and the popup never appeared; publish on every occurrence and deduplicate in AnalyticsDiskFullPopupPlugin instead - the native error-string channel still raised every disk-full as a Sentry exception (the #8715 spam); skip it for ErrorDiskFull since the typed code plus the C# warning carry the information - use SafeCancelAndDispose in the popup plugin so teardown cannot throw and leave the native segment server initialized (which blocks every subsequent editor play session) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decentraland-bot
added
the
ext-contribution
|
Slack notification sent to #explorer-ext-contributions for external review. |
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9908 fix: bugsweep week 2026-W34
STEP 2 — Root-cause check: PASS ✅
All five fixes address root causes, not symptoms:
| Issue | Root cause | Fix |
|---|---|---|
| #9839 SmartWearableCache null metadata | Half-built CacheItem committed before population; failure left poisoned entry |
try/catch/evict pattern — evict on cancellation, exception, or missing scene.json |
| #9832 Local scene DMs | Chat room connected even though Friends is disabled in local dev → unanswerable DMs | CommsContainer passes IConnectiveRoom.Null.INSTANCE for chat room, mirroring the island-room gate; PrivateConversationUserStateService skips the connection wait |
#9833 wallet_switchEthereumChain exception |
Allow-list rejections routed through OnEngineException (Sentry spam + scene suspension) |
New Web3MethodNotAllowedException caught before the generic handler; returns spec-compliant JSON-RPC error |
| #9783 Places tab sort | Default sort was like_score instead of most_active |
Default changed in PlacesView.ResetCurrentFilters and synced in PlacesFilterSelectorView |
| #9840 Segment disk full | SQLITE_FULL collapsed into generic Response::Error → Sentry exception, no user feedback |
Typed ErrorDiskFull response code from Rust, ReportHub.LogWarning instead of exception, AnalyticsDiskFullPopupPlugin shows a one-shot popup via IEventBus |
STEP 3 — Design & integration: PASS ✅
AnalyticsDiskFullPopupPlugin — new plugin registered in DynamicWorldContainer.GlobalPlugins. Owner search:
- The disk-full event originates in
RustSegmentAnalyticsService(native FFI callback) → lives in the analytics assembly. - The popup requires
IMVCManager→ lives inUIShellContainer. - These are separate containers with no direct dependency. A plugin in the global plugins list is the standard bridge pattern for cross-container concerns (same pattern as
ConnectionStatusPanelPlugin,VoiceChatPlugin). AnalyticsDiskFullDetectedplaced inUtilitynamespace avoids a UI→analytics assembly dependency — correct boundary decision.- Subscription/teardown trace:
SubscribeinInitializeAsync→subscription.Dispose()inDispose().CancellationTokenSource→cts.SafeCancelAndDispose()inDispose(). ✅
SmartWearableCache eviction — the cache is the lifecycle owner for cached items. Adding try/catch/evict at the population site is the correct placement. The eviction on the "missing scene.json" path (remove from cache but still return the item to the caller) is consistent: the returned item has IsSmart=true but RequiresAuthorization=false, so downstream flows that check authorization before showing the popup are protected.
CommsContainer chat room null — mirrors the existing localSceneDevelopment ? IConnectiveRoom.Null.INSTANCE : archipelagoIslandRoom pattern. Consistent.
EthereumApiWrapper error hierarchy — Web3MethodNotAllowedException : Web3Exception is the correct hierarchy: the specific catch comes before the generic catch in SendAndFormatAsync, so allow-list rejections return a JSON-RPC error while genuine faults still flow through OnEngineException.
EventBus thread safety — AnalyticsContainer.EventBus is created with invokeSubscribersOnMainThread: true. The native Segment callback publishes from a non-main thread; EventBus.Publish reads handlers via TryGetValue then schedules via PooledContinuation. Since subscriptions happen during plugin InitializeAsync (startup) and unsubscriptions during Dispose (shutdown), and the native callback only fires during normal operation, there is no concurrent read/write on the handlers Dictionary in practice.
STEP 4 — Member audit
| New/changed member | Consumer count | Assessment |
|---|---|---|
AnalyticsContainer.EventBus (public IEventBus) |
2: DynamicWorldContainer (injects into plugin), RustSegmentAnalyticsService (publishes) |
Correct — decouples analytics from UI |
Web3MethodNotAllowedException |
4: DappWeb3EthereumApi, ThirdWebEthereumApi, RestrictedEthereumApi, EthereumApiWrapper (catches) |
Correct — exception hierarchy serves its purpose |
AnalyticsDiskFullDetected |
2: RustSegmentAnalyticsService (publishes), AnalyticsDiskFullPopupPlugin (subscribes) |
Correct — event struct serves as the cross-assembly contract |
NativeMethods.Response.ErrorDiskFull |
2: NativeMethods (enum), RustSegmentAnalyticsService.Callback (matches) |
Correct |
STEP 5 — Line-level findings
Two P2 findings below (inline). No P0 or P1 issues.
Security review: No security issues found ✅
- No hardcoded secrets or credentials
- Web3 allow-list rejections do not leak internal state; JSON-RPC error response is spec-compliant (
-32601) - Native binary changes (segment-server.dylib/dll) add typed error codes; no sensitive data exposed
rusqlite0.37 (new Cargo dependency) is a well-established crate;bundledfeature compiles SQLite from source on Windows/Linux- EventBus cross-thread publishing properly marshals to main thread
- Scene RPC input validated against whitelist before execution
- Removed redundant wallet null check in
ThirdWebEthereumApiis safe: the early null guard at the top ofSendAsyncthrows before any downstream method is reached
STEP 6 — Complexity: COMPLEX
Touches async flow (SmartWearableCache, PrivateConversationUserStateService), plugin/container wiring (AnalyticsDiskFullPopupPlugin, AnalyticsContainer.EventBus), networking (CommsContainer LiveKit rooms), native FFI (Rust Segment plugin with cross-thread EventBus), Web3/auth paths, and multiple subsystems across 27 files.
STEP 7 — QA: YES
All changes affect runtime code (smart wearables, chat, Web3, analytics, Places UI).
STEP 8 — Non-blocking warnings
No Main.unity changes detected. ✅
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches async/cancellation flows, plugin/container wiring, native Rust FFI with cross-thread event publishing, LiveKit networking, Web3 exception hierarchy, and multiple UI subsystems.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown (<@unknown>) via Slack
This comment has been minimized.
This comment has been minimized.
|
Claude finished @lorenzo-ranciaffi's task in 5m 27s —— View job Security-focused dependency review + code pass (re-run after
|
| Prior finding | Status |
|---|---|
F1 concurrent caller sees half-built CacheItem |
Fixed — SmartWearableCache now dedups in-flight fetches through a UniTaskCompletionSource<CacheItem> keyed by id (SmartWearableCache.cs:134-157), and an entry only enters cache once fully built (:163-172). RequiresAuthorization can no longer be observed as false mid-fetch. |
F2 missing-scene.json evicted → error repeats every attempt |
Fixed — the branch now caches the item and logs once (:206-211). |
F6 bare ShowAsync(...).Forget(), popupShown set too early |
Fixed — ShowPopupAsync wraps the show in SuppressToResultAsync(ReportCategory.ANALYTICS) and resets popupShown on a non-cancellation error (AnalyticsDiskFullPopupPlugin.cs:49-62). The EnumResult<TaskError> pattern match is correct against Result.cs:72. |
D4 Linux .so not rebuilt |
Resolved as documented — README.md:91 now states Linux still completes with Error. |
| C1 (fixed last pass) | Still fixed. |
The concurrency design holds up under the races I walked: Clear() during a fetch unregisters the completion so RemoveInFlight refuses the write, a second caller starting a fresh fetch wins the slot, and TrySetResult is deliberately called outside the lock so awaiter continuations can't re-enter it. IsSmartAsync/RequiresAuthorizationAsync/RequiresWeb3APIAsync returning false on a null (cancelled) item preserves the old !ct.IsCancellationRequested && … semantics.
1. Dependency changes — unchanged from my previous pass
ea239d6 touched no dependency file. Cargo.toml, lib.rs, server.rs and both native binaries are byte-identical to what I reviewed at e095044. No Packages/manifest.json, packages-lock.json, .asmdef, NuGet or UPM changes. No .github/workflows/** or .github/prompts/** changes → the workflow/prompt section (W.1–W.10) does not apply.
| Dependency / file | Version | Source | Type | Scope | Risk | Confidence |
|---|---|---|---|---|---|---|
segment (.native/Cargo.toml) |
2d46f10 → 51d89ce |
git github.com/decentraland/segment |
source (compiled into the shipped native lib) | runtime, ships to users | MEDIUM | HIGH (pin), LOW (contents) |
rusqlite (new direct dep) |
"0.37" (caret), bundled on win/linux, unbundled on mac |
crates.io | source + C (sqlite amalgamation on win/linux) | runtime, ships to users | MEDIUM | HIGH |
Libraries/Mac/segment-server.dylib |
6,452,448 → 6,270,256 B | hand-built locally (LFS) | native binary | runtime, macOS retail | MEDIUM | MEDIUM |
Libraries/Windows/segment-server.dll |
4,560,896 → 4,324,352 B | hand-built locally (LFS) | native binary | runtime, Windows retail | MEDIUM | MEDIUM |
Libraries/Linux/segment-server.so |
unchanged | — | native binary | Any: enabled 1 in the importer |
LOW (now documented) | HIGH |
Still not verifiable from this job: no outbound network, so every [Registry metadata] lookup (crates.io for rusqlite, the decentraland/segment repo, the 2d46f10…51d89ce compare, advisories) failed again. The changed binaries are LFS pointers in this checkout — I did not inspect any binary internals and make no claims about them.
D1 — segment fork rev bump ships unreviewed native code (MEDIUM, unchanged). Pinned to an immutable 40-char commit on the org's own fork; provenance is fine. But the behaviour this PR depends on (a typed EnqueError::Sqlite rather than a collapsed error) lives entirely in that commit range, is invisible here, and compiles into the shipped .dylib/.dll. Someone should read that compare as part of this review — this PR is where it enters the product.
D2 — no Cargo.lock, floating rusqlite = "0.37" → builds aren't reproducible (MEDIUM, unchanged). .native/.gitignore:11 ignores Cargo.lock and the new dep uses a caret range. Nobody can reproduce the committed .dylib/.dll from this tree. Pre-existing for the crate, but this PR promotes a C-backed library to a direct dep, so it's worth fixing now. Fix this →
D3 — binaries are hand-built, nothing in CI builds or tests RustSegment (MEDIUM, unchanged). The only cargo/rustc references under .github/ are uuav-native.yml / uuav-verify.yml, which build a different crate. Commit order still checks out (d03ca1c rebuilt mac, 14fef20 rebuilt win, and 1670ba9/e095044/ea239d6 don't touch Rust), so the artifacts match this branch if they were built from it. Both binaries shrank despite added code, which points at a different toolchain than the previous artifact — harmless in itself, a concrete illustration of D2. The new Rust unit tests (server.rs:437-476) are good, but nothing in CI runs them.
D5 — bundled on win/linux, not on mac (LOW–MEDIUM, unchanged). Self-verifying (the code wouldn't compile if cargo didn't unify onto one rusqlite), but the consequence is worth stating: macOS links the OS libsqlite3 while Windows/Linux statically bundle the amalgamation, so the platforms can run different sqlite versions and in principle differ on which conditions surface as SQLITE_FULL.
Capability assessment. Unchanged: no new network surface, no UI/webview, no reflection or dynamic assembly loading, no platform/region-conditional behaviour, no filesystem access beyond the sqlite queue segment already owned. rusqlite is well-established and proportionate — sqlite was already transitive; this only promotes it so the bridge can read the error code.
2. Code findings
F10 (new) — the metadata fetch is now uncancellable, and one comment is stale because of it. BuildCacheItemAsync passes CancellationToken.None to the web request (SmartWearableCache.cs:216). The intent is sound — the fetch is cache-owned, so one caller cancelling must not poison it — but two consequences follow:
SmartWearableSystem.cs:483says "Stop every in-flight flow before the cache is cleared, otherwise it would repopulate it for the previous identity." CancellingsessionCtsno longer stops the fetch; the invariant is now held solely byinFlight.Clear()+ theRemoveInFlightidentity check. The guarantee survives, but the comment now describes a mechanism that isn't the one doing the work — and ifinFlight.Clear()is ever dropped fromClear(), the identity leak comes back silently.SmartWearableCachehas noDispose, so on logout/realm change the outstanding GETs run to completion (bounded by the web-request timeout) and their results are discarded, then re-issued. Minor duplicated work, not a leak. A cache-ownedCancellationTokenSourcecancelled inClear()would give the same "no caller can poison the fetch" property without either wrinkle. Fix this →
F11 (new) — no test coverage for what is now a genuinely concurrent component. ea239d6 turned SmartWearableCache into a lock-protected, in-flight-deduplicating async cache, and nothing in the repo references it from a test. The three invariants the commit introduces are all cheap to pin down in EditMode with a stubbed IWebRequestController: two concurrent callers issue exactly one request and both see the finished item; a throwing fetch caches nothing and faults every awaiter; a Clear() mid-flight does not repopulate. Worth adding while the reasoning is fresh — the failure modes here are the kind that only show up under load in retail. Fix this →
F2b — worth knowing for QA, not a defect. With the F2 fix, a wearable whose content has no resolvable scene.json is now cached as smart-with-null-metadata, so LoadSmartWearableSceneSystem.cs:64-69 still produces the sceneMetadata of <name> is null exception the issue was filed on. That is the honest outcome for a genuinely broken wearable, and LoadSystemBase's streamable cache keeps it from repeating — but it does mean the Sentry signature from #9839 can still appear after this PR, just once per wearable instead of on every load. Set expectations accordingly when closing the issue.
F3 (unchanged) — candidate root cause still open. IsSmart classifies on content.file.EndsWith("scene.json") (SmartWearableCache.cs:242), so female/scene.json counts, while SmartWearableSceneContent is always created with a hardcoded BodyShape.MALE (:203) and only probes male/scene.json then scene.json. A wearable shipping only the female-prefixed scene.json is classified smart and can never resolve its metadata — landing permanently in the F2b branch. Pre-existing, not introduced here, but if that's the shape of the reported crash then this PR makes it quieter rather than fixing it. Worth checking against the Sentry payload before closing #9839.
F5 (unchanged) — EventBus is not thread-safe, and this PR adds the first always-off-main-thread publisher. EventBus.handlers is a plain Dictionary<Type, Delegate> with no synchronization. RustSegmentAnalyticsService publishes from the native Segment callback thread while AnalyticsDiskFullPopupPlugin subscribes and disposes on the main thread. The main-thread hop is handled (invokeSubscribersOnMainThread: true), but the TryGetValue itself races with Subscribe / Unsubscriber.Dispose. Narrow window (subscribe at boot, dispose at shutdown vs. a flush callback), and the class design predates this PR.
F7 (unchanged) — latent hole in report_error_with_code. With code == ErrorDiskFull and id == None the error is dropped on both channels (server.rs:440-500). Not reachable today (both call sites pass Some(id)).
F8 (unchanged) — EthereumApiWrapper nits. SendAndFormatAsync used to take id/method as parameters specifically so the local function wouldn't capture; it now captures id from the enclosing scope. Also worth confirming out-of-repo that the SDK's sendAsync consumer treats a payload carrying error as a rejection — if it just resolves, the scene sees result: null and may still misbehave, only without the Sentry noise.
F9 (unchanged) — FYI on #9783. After the dev merge, PlacesView.cs is no longer in this PR's diff — origin/dev already sets SortBy.MOST_ACTIVE. The only remaining Places change here is the comment in PlacesFilterSelectorView.cs:48. The behaviour is real, it just no longer originates on this branch.
Nit — IsCached (SmartWearableCache.cs:104-107) has no callers anywhere in the repo; the commit added a lock to dead public API. Pre-existing, but a candidate for deletion while you're in the file.
Verified-good
- The in-flight dedup is correct under
Clear():RemoveInFlightcompares identity, so a fetch that outlived aClear()refuses to write and a replacement fetch owns the slot.TrySetResultoutside the lock avoids continuation re-entrancy. result.Error is { State: not TaskError.Cancelled }matchesEnumResult<TErrorEnum>.Error's nullable-tuple shape (Result.cs:72); a user-dismissed popup is aSuccess, sopopupShowncorrectly staystrueand the once-per-session contract holds.SmartWearableAuthorizationPopupController.UpdatePermissionsAsyncearly-returns on null metadata;LoadSmartWearableSceneSystemguards both tuple elements. The nullableGetCachedSceneInfoAsynccontract has no unguarded consumer.- BOM removal on
SmartWearableCache.cs:1is consistent withExplorer/.editorconfig:9(charset = utf-8, notutf-8-bom). PrivateConversationUserStateService's early return skips only the connection wait and the initial participant seed;SubscribeToEvents()has already run, andFeatureId.LocalSceneDevelopmentresolves from the same launch-mode booleanCommsContaineruses, so the two gates cannot diverge.IConnectiveRoom.Null.INSTANCEis a proper null-object returningNullRoom.INSTANCE, so the chat-room gate can't NRE its consumers.Web3MethodNotAllowedException : Web3Exceptionwith the specific catch ordered before the generic one is the right shape.- FFI enum mirrored in
NativeMethods.cs, documented in the README, Rust-side mapping unit-tested. - Removing the second
wallet == nullcheck inThirdWebEthereumApi.SendAsyncis safe: the guard at lines 71-76 dominates both paths.
CI
Unity Test, Unity Cloud Build and Dependency Security Review are still in progress for this push; Enforce QA and DEV Approvals fails as expected (approvals-gated). The previous run reported no results for both EditMode and PlayMode, so confirm Unity Test is actually green rather than reading a
Verdict. ea239d6 closes the two findings I'd flagged as blocking-ish (F1, F2) and the async-hygiene one (F6), and the concurrency rework is well-reasoned — the lock discipline, the identity check on RemoveInFlight, and completing outside the lock are all the right calls. What keeps this from a clean PASS is unchanged and entirely on the dependency side: the substance of the change (the segment fork commit range and two hand-built, CI-unverified binaries) is not reviewable from this PR, and the build isn't reproducible without a lock file. On the code side only F10 (uncancellable fetch + stale comment) and F11 (no tests for the new concurrent path) are new, and neither blocks.
DEPENDENCY_REVIEW: NEEDS_ATTENTION
· branch fix/bugsweep-week-2026-w34
NickKhalow
left a comment
NickKhalow
left a comment
There was a problem hiding this comment.
All's good! But we have to implement the binary-lock and CI/CD build of the native binaries for security reasons (as an example take a look at UUAV)
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
✅ QA Approved
Platforms tested: Windows & macOS
Verified:
- Places tab (#9783): sort defaults to "Most Active" in FILTER & SORT
- Local scene development (#9832): confirmed DMs don't reach the client while in local-scene dev mode
- Web3 unsupported method (#9833): no scene available with an unsupported wallet method to trigger directly — smoke tested standard Web3 flow instead (login/logout with old and new Metamask accounts, Google, and Discord), all working correctly
- Smart wearables (#9839): equipped multiple smart wearables, played normally, no issues
Not covered:
- Storage Full popup (#9840): requires a genuinely full disk to trigger; not tested this pass
Result: No blockers found in the scope tested. Approving from QA side.
04.09.2026_10.21.39_REC.mp4
✅Smoke test performed:
- ✔️ Log In/Log Out
- ✔️ Backpack and wearables in world
- ✔️ Emotes in world and in backpack
- ✔️ Teleport with map/coordinates/Jump In
- ✔️ Chat and multiplayer
summary comment of actionables after the triage
|
🧠 Bugsweep continuous-learning pass — distilled the review of this sweep into two documentation PRs (open for review):
Considered and dropped: the -32601 → const request (#9833, already covered by the quality bar) and the "postfix with Event" suggestion (one-off, not applied). Pipeline note carried to the team: the native-binary lock / CI-built binaries request. |
5 participants
Pull Request Description
What does this PR change?
Automated weekly bug sweep for ISO week 2026-W34 (issues created Mon 2026-08-17 → Sun 2026-08-23). The sweep collected 13 open issues created that week (all unassigned; 0 out of scope by assignee), fixed 5, and could not work on 7 (each explained below). One commit per issue; each fix commit body carries the issue link, Sentry link, repro, root cause and fix shape.
Review round (2026-08-31): addressed alejandro-jimenez-dcl's review with follow-up commits (no history rewrite): dropped the #9781 fix via revert, reverted the original #9840 fix (silencing the error client-side) and re-addressed it with a proper disk-full handling path (see Fixed), replaced the #9832 fix with a chat-room disconnect in local scene development, kept
IsSmarttrue (log + evict) for #9839, and moved the-32601magic number to a const for #9833.Fixed
sceneMetadata of <wearable> is null(Sentry).SmartWearableCachecommitted aCacheItemto the dictionary before populating it, so a cancelled/failed/throwing scene-metadata fetch (or a missingscene.json) left a poisoned entry (IsSmart=true,SceneContentset,SceneMetadata=null) that crashed every later load for the session. Fix: evict the half-built entry on cancellation/exception so a later request retries from a clean cache; a wearable whose content lacksscene.jsonstays smart (per review) — the issue is logged with the wearable id and the entry is evicted so a metadata-less item is never served from the cache.[QA] Local scene | User can receive messages from friends when being on local scene. In local-scene dev mode the Friends subsystem is disabled (no reply UI, user not in shared presence) but the client still connected the LiveKit chat room, so DMs arrived unanswerable. Fix (per review): keep the chat room disconnected in local scene development —CommsContainerpassesIConnectiveRoom.Null.INSTANCEtoRoomHub, mirroring the existing island-room gate, andPrivateConversationUserStateServiceskips its wait-for-connection so no spurious timeout error is logged. Note: community chat messages/reactions ride the same room and stop flowing in local-scene dev too ("drop connection as a whole").Web3Exception: The method is not allowed: wallet_switchEthereumChain(Sentry). A non-whitelisted scene RPC method was routed throughOnEngineException— spamming Sentry and, on repeated calls, suspending the scene — and returned no JSON-RPC error. Fix: introduceWeb3MethodNotAllowedExceptionfor deterministic allow-list/permission rejections;EthereumApiWrappernow catches it before the generic handler and returns a spec-compliant JSON-RPC error (JSON_RPC_METHOD_NOT_FOUND = -32601) to the scene without treating it as an engine fault. The allow-list is unchanged (chain-switching stays unsupported).[QA] Places | Places tab not surfacing scenes with live concurrent users. The Places tab defaulted its sort to Best Rated (order_by=like_score), so live-user scenes were not surfaced (whereas/whats-onand the service default usemost_active). Fix: default the Places tab sort toMOST_ACTIVEinPlacesView.ResetCurrentFiltersand sync the dropdown default inPlacesFilterSelectorView.ResetFilters. Confirmed valid by review.Segment operation <id> Flush failed with: Error. The native Segment plugin collapsed every failure (includingSQLITE_FULL) into a genericResponse::Error, so Unity raised an opaque Sentry exception and the user was never told their disk had run out of space. Fix: bump the segment rev to get a typed DiskFull error and forward it asResponse::ErrorDiskFull = 2from the flush/enqueue paths (Mac dylib and Windows dll rebuilt);RustSegmentAnalyticsServicelogs a warning instead of a Sentry exception and publishesAnalyticsDiskFullDetectedon the analytics event bus; the newAnalyticsDiskFullPopupPluginshows a "Storage Full" popup once per session through the existingErrorPopupController. Response codes are documented inPlugins/RustSegment/README.md.Dropped after review (reverted on the branch)
Not worked on
[Explorer] Desktop - Spawn Location Selection on Login: feature request (labeledfeature/suggestion), not a bug.InvalidOperationException(Sentry): only generic il2cpp frames (Generics__30.cpp), no source location or repro — insufficient info to locate the root cause.SoundChannel.cpp ... invalid seek position(Sentry): native FMOD audio engine error with no C# source; platform-native, complexity too high for the sweep.Video fails to play on scene entry in Worlds when admin tools are active (Mac): Mac-only, labeledneed QA validation; the sweep cannot reproduce or verify on macOS.scene-banssigned fetch returns 401 (Sentry): the client signs a lowercasedx-identity-metadatabut sends camelCase; comms-gatekeeper's crypto-middleware v6 stopped lowercasing on verify → 401. The root cause is the app-wideWebRequestSignInfosigning primitive (changing it risks regressing every backend still on old middleware); the safe fix is server-side (canonicalMetadataKeysin comms-gatekeeper), out of this repo.Admin tools button overlapped/hidden behind debug tools buttons: the debug-toolsUIDocument(sortingOrder 1000) draws over the scene-side Admin Toolkit smart item, which lives out-of-repo. A code-onlyInteractableAreareservation only helps if that smart item honors the contract (unverifiable here; the "fixed screen position" symptom suggests it does not). The only guaranteed explorer-side fixes are a UX relocation of the debug panel (product decision) or a prefab sort-order change (wrong layering). Needs a maintainer/cross-repo decision.[QA] Friends | Pending friend request notification not showing on Windows: Windows-only, could not reproduce/verify; root cause unconfirmed. The badge/bell UI logic correctly handles repeat requests. The strongest candidate is a blockingawait selfProfile.ProfileAsync(ct)inside theRPCFriendsServicefriendship-update stream loop (freezing all updates after the first), but a safe fix requires widening theISelfProfileinterface, and the Windows repro could instead be a WebSocket/transport or server-stream issue this would not address. Needs live Windows confirmation with RPC stream logging before landing.Test Instructions
Places tab ([QA] Places | Places tab not surfacing scenes with live concurrent users #9783)
Local scene development ([QA] Local scene | User can recieve messeges from friends when being on local scene #9832)
--realm http://127.0.0.1:8000 --position 0,0 --local-scene true --debug— NOTE: modifypositionto your needs).Unsupported wallet methods (DCL.Web3.Web3Exception: The method is not allowed: wallet_switchEthereumChain #9833)
Smart wearables (System.Exception: sceneMetadata of Flight by Knight is null #9839)
Storage Full popup (System.Exception: Segment operation 3984 Flush failed with: Error #9840)
Quality Checklist
Requested by Alejandro Jimenez via Slack