feat: poc enable comms for PEXs interacting with the authoritative-server by cyaiox · Pull Request #9059 · decentraland/unity-explorer

cyaiox · 2026-06-23T17:24:35Z

Pull Request Description

What does this PR change?

Makes an authoritative-multiplayer Portable Experience run its server-side scene (Hammurabi) end-to-end.

Problem. A scene whose metadata sets authoritativeMultiplayer: true routes its outgoing comms to a hardcoded authoritative-server LiveKit participant, assuming that participant is already in the scene's comms room. That assumption holds when you teleport into the world (the client dials the world's comms adapter, which triggers the world-content-server's USER_JOIN → spawn). But when the same world is loaded as a Portable Experience, the client never joins a comms room for it, so:

the world-content-server never spawns the authoritative server (no join carrying the sceneId), and
even if it were spawned, the scene's CRDT is dropped — the scene pipe is gated on IsSceneConnected(sceneId), which is false for a PX.

Root cause: a PX realm is configured with an empty WorldManifest, so RealmData.SingleScene == true, which routes comms to the world-level room (worlds/{realm}/comms) that carries no sceneId. The single-realm RoomHub (Island/Scene/Chat/Voice) has no slot for a second world's room.

Solution. Give each running authoritative PX scene its own scene-level comms connection and route its CRDT through it, without touching the single-realm RoomHub:

PortableExperienceSceneRoom — a ConnectiveRoom that always connects to worlds/{realm}/scenes/{sceneId}/comms, carrying the sceneId the world-content-server needs to spawn the authoritative server.
PortableExperienceWorldComms — owns one room + message pipe per running authoritative PX scene, keyed by sceneId, reconciled on load/unload (RetainOnly). Created and disposed by CommsContainer.
SceneCommunicationPipe (extended) — routes a registered PX scene's CRDT to/from its own room instead of the host's current scene room. Hoisted out of SceneSharedContainer into CommsContainer so the new global system can share the one instance every scene already uses.
PortableExperienceWorldCommsSystem + PortableExperienceCommsPlugin — each frame, connects and registers authoritative PX scenes and reconciles unloaded ones away. (Lives in the DCL.Multiplayer.Connections.PortableExperiences namespace, not DCL.PluginSystem.*, because the Arch query generator emits unqualified World, which the DCL.PluginSystem.World namespace would shadow.)
WriteRealmInfoSystem — reports RealmInfo.isConnectedSceneRoom for PX scenes (OR-ing in the PX room's connection state), which is the gate the SDK uses to initiate the REQ_CRDT_STATE / RES_CRDT_STATE handshake.

Only authoritativeMultiplayer PEX are affected; ordinary PEX open no extra connection.

Docs: docs/authoritative-portable-experiences.md (full flow, architecture, the three root causes, message types, diagnostics, and verification).

_{21 files changed, 773(+) / 19(−). No new .asmdef; comms types stay in DCL.Multiplayer, the system/plugin under DCL.Plugins.}

Test Instructions

Requires the world-content-server + comms-gatekeeper + sdk-multiplayer-server (Hammurabi) infra — this is the client half of a cross-service flow.

Steps (standard run):

metaforge explorer run 9059

Expected result: after /loadpx <name> for a PX deployed to a world with authoritativeMultiplayer: true, the COMMS_SCENE_HANDLER logs show the client connecting to worlds/<realm>/scenes/<sceneId>/comms and the room reaching Running; the authoritative server spawns and joins as authoritative-server; the scene's CRDT REQ/RES state handshake completes and the scene syncs.

Prerequisites

A Portable Experience deployed to a world with authoritativeMultiplayer: true in its scene metadata (e.g. voxelpets.dcl.eth).
comms-gatekeeper and sdk-multiplayer-server reachable for the target environment.

Test Steps

/loadpx <name> to load the authoritative PX.
Filter logs on COMMS_SCENE_HANDLER: confirm a POST to worlds/<realm>/scenes/<sceneId>/comms and the room reaching Running.
Confirm authoritative-server appears as a participant and the scene's entities populate (REQ_CRDT_STATE → RES_CRDT_STATE applied).
/killpx <name>: confirm the PX room + pipe are torn down and unregistered.
Load an ordinary (non-authoritative) PEX: confirm no extra comms connection is opened.

Additional Testing Notes

Two simultaneous LiveKit connections exist for the host (current realm + the PX world) — verify identity/token handling doesn't trip duplicate-identity logic.
Verified end-to-end manually: server spawns on join, the CRDT handshake completes, and the scene syncs.

Quality Checklist

Changes have been tested locally (verified end-to-end against live infra: spawn → handshake → sync).
Documentation has been updated (docs/authoritative-portable-experiences.md + README index entry).
Performance impact has been considered (system Update() is allocation-free; per-frame scratch dictionaries are cleared and reused; temporary hot-path diagnostics removed before commit).
For SDK features: Test scene is included — N/A (client-side comms wiring, not an SDK component).

Note

Unity EditMode/PlayMode tests were not run in this session (the Editor held the project lock). Please run the RealmInfo tests via the Test Runner; WriteRealmInfoSystemShould was updated for the new constructor arg.

Code Review Reference

Please review our Branch & PR Standards before submitting.

An authoritativeMultiplayer Portable Experience never joined a scene comms room, so the world-content-server never spawned its authoritative server (Hammurabi) and no CRDT was exchanged. PX realms run with an empty WorldManifest (RealmData.SingleScene == true), which routed comms to the world-level room that carries no sceneId. - PortableExperienceSceneRoom: scene-level LiveKit connection to worlds/{realm}/scenes/{sceneId}/comms, carrying the sceneId the server needs to spawn the authoritative server. - PortableExperienceWorldComms: owns one room + message pipe per running authoritative PX scene, keyed by sceneId, reconciled on load/unload. - SceneCommunicationPipe: routes a registered PX scene's CRDT to/from its own room instead of the host's current scene room (hoisted out of SceneSharedContainer into CommsContainer so global systems can reach it). - PortableExperienceWorldCommsSystem/Plugin: connects and registers authoritative PX scenes each frame. - WriteRealmInfoSystem: reports isConnectedSceneRoom for PX scenes so the SDK initiates the REQ/RES CRDT-state handshake. Docs: authoritative-portable-experiences.md.

github-actions · 2026-06-23T17:25:05Z

New build in progress, come back later!

cyaiox · 2026-06-24T10:03:53Z

@claude review

claude · 2026-06-24T10:04:42Z

Claude encountered an error after 8m 50s —— View job

Code Review

Todo

Load tools
Read CLAUDE.md and docs/README.md
Read changed files and understand the diff
Analyze for bugs, performance issues, and code standards violations
Post review findings

claude · 2026-06-24T10:13:40Z

+                extraRoomsBySceneId[sceneId] = new RoomChannel(room, roomPipe);
+            }
+
+            roomPipe.Subscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber, IMessagePipe.ThreadStrict.ORIGIN_THREAD);


Threading concern: subscribe happens outside the lock.

The entry is added to extraRoomsBySceneId under lock(extraRoomsBySceneId), but roomPipe.Subscribe is called after the lock is released. Between those two points, SendMessage (called from a scene thread) can already find the entry and route outbound messages to the pipe, while the pipe's inbound subscription (InvokeSubscriber) is not yet registered.

In practice this window is harmless — the room isn't connected yet when RegisterSceneRoom is first called, so no server-to-scene messages can arrive — but the gap between "entry in dict" and "subscription active" breaks the obvious invariant. Either move Subscribe inside the lock, or add a comment explaining why the window is safe.

Suggested change

roomPipe.Subscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber, IMessagePipe.ThreadStrict.ORIGIN_THREAD);

public void RegisterSceneRoom(string sceneId, IConnectiveRoom room, IMessagePipe roomPipe)

{

// Subscribe before adding to the dict so the inbound subscription is active

// the moment SendMessage can route to this pipe.

roomPipe.Subscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber, IMessagePipe.ThreadStrict.ORIGIN_THREAD);

lock (extraRoomsBySceneId)

{

if (extraRoomsBySceneId.ContainsKey(sceneId))

{

// Already registered — undo the subscribe we just added.

roomPipe.Unsubscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber);

return;

}

extraRoomsBySceneId[sceneId] = new RoomChannel(room, roomPipe);

}

}

(Or simply document the safe-because-not-yet-connected invariant if the reorder is undesirable.)

claude · 2026-06-24T10:13:49Z

+                        staleSceneIdsBuffer.Add(sceneId);
+
+                foreach (string sceneId in staleSceneIdsBuffer)
+                    extraRoomsBySceneId.Remove(sceneId);


No unsubscribe on stale entry removal.

RetainOnlyRooms removes the entry from the routing dictionary but never calls roomPipe.Unsubscribe. The subscription lives until PortableExperienceWorldComms.DisposeEntry disposes the pipe (which presumably cleans up its own subscriptions). During the brief window between removal here and disposal in worldComms.RetainOnly, incoming server messages still fire InvokeSubscriber, which then falls through to sceneRoom.IsSceneConnected(sceneId) (returning false) and drops the message — so no data corruption.

The assumption is that MessagePipe.Dispose() correctly removes subscriptions. That should hold, but it's an implicit contract worth documenting, or explicitly unsubscribing here before removal to make teardown self-contained:

Suggested change

extraRoomsBySceneId.Remove(sceneId);

foreach (string sceneId in staleSceneIdsBuffer)

{

RoomChannel channel = extraRoomsBySceneId[sceneId];

channel.Pipe.Unsubscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber);

extraRoomsBySceneId.Remove(sceneId);

}

mikhail-dcl

The design of the feature is weak: AI didn't scan enough surface in order to understand where the pieces should be integrated. It requires significant refactoring

mikhail-dcl · 2026-06-24T10:21:04Z

+
+        [Query]
+        [None(typeof(PortableExperienceRealmComponent), typeof(DeleteEntityIntention))]
+        private void ActivateAuthoritativeComms(in PortableExperienceComponent portableExperience, in SceneDefinitionComponent sceneDefinition)


It doesn't make sense to run this query every frame: the lifecycle cycle of the scene/PX is known and controlled by LoadSceneSystemLogicBase.

Instead of worldComms.EnsureConnected(sceneId, realmData); the logic to connect to the LiveKit room should be async and should be a necessary part of the scene loading flow without which the PX can't be loaded.

This flow, though, is sort of ad-hoc and doesn't fit the scene loading cycle.

mikhail-dcl · 2026-06-24T10:26:39Z

+            ActivateAuthoritativeCommsQuery(World);
+
+            // Stop routing for unloaded scenes before disposing their rooms.
+            sceneCommunicationPipe.RetainOnlyRooms(liveSceneIds);


The design is wrong. There are immediate smells:

The system contains the state (it shouldn't), it can be acceptable only in the minority of cases. It's not that case

PX ids are collected every frame, however destruction is already handled by UnloadPortableExperiencesSystem and UnloadSceneSystem. There is no needed to re-query it. Unloading of the related LiveKit room should be nicely incorporated into the disposal of the corresponding PX.

mikhail-dcl · 2026-06-24T10:26:59Z

+    ///     unregistered and disconnected.
+    /// </summary>
+    [UpdateInGroup(typeof(PresentationSystemGroup))]
+    public partial class PortableExperienceWorldCommsSystem : BaseUnityLoopSystem


Based on my concerns this system shouldn't exist at all

mikhail-dcl · 2026-06-24T10:27:18Z

+    ///     server) and exchange CRDT with it over that room. The underlying connections are owned and disposed by
+    ///     <c>CommsContainer</c> via <see cref="PortableExperienceWorldComms" />.
+    /// </summary>
+    public class PortableExperienceCommsPlugin : IDCLGlobalPluginWithoutSettings


Therefore, shouldn't exist this plugin either

mikhail-dcl · 2026-06-24T10:29:25Z

+        ///     Stops routing for any registered scene room whose sceneId is no longer present in
+        ///     <paramref name="liveSceneIds" />. Disposing the underlying room/pipe is the caller's responsibility.
+        /// </summary>
+        public void RetainOnlyRooms(ICollection<string> liveSceneIds)


RetainOnly should be changed to Remove(id) as the disposal moment is explicitly defined so it doesn't make sense to query through collections

mikhail-dcl · 2026-06-24T10:43:04Z

+            entriesBySceneId[sceneId] = new Entry(room, pipe);
+
+            ReportHub.Log(ReportCategory.COMMS_SCENE_HANDLER, $"Portable Experience comms: connecting scene '{sceneId}' of world '{portableExperienceRealm.RealmName}'");
+            ConnectAsync(sceneId, room).Forget();


As I mentioned earlier it should be integrated into the scene loading cycle and not hanging in the detached state

mikhail-dcl · 2026-06-24T10:45:28Z

+                // A Portable Experience scene is connected through its own scene room (not the host's current scene room),
+                // so the SDK only initiates the CRDT handshake when this reflects the PX room's connection.
+                bool isConnectedToSceneRoom = sceneRoom.IsSceneConnected(sceneData.SceneEntityDefinition.id)
+                                              || portableExperienceWorldComms.IsConnected(sceneData.SceneEntityDefinition.id);


Nullability violated

mikhail-dcl · 2026-06-24T10:46:53Z

+                // A Portable Experience scene is connected through its own scene room (not the host's current scene room),
+                // so the SDK only initiates the CRDT handshake when this reflects the PX room's connection.
+                bool isConnectedToSceneRoom = sceneRoom.IsSceneConnected(sceneData.SceneEntityDefinition.id)
+                                              || portableExperienceWorldComms.IsConnected(sceneData.SceneEntityDefinition.id);


Checking the PX state should be driven from the Scene Facade of the current scene (taken from ISceneCache) and not via a intermediary which requires to lookup for the appropriate data every time

mikhail-dcl · 2026-06-24T10:47:30Z

+
+                // A Portable Experience scene is connected through its own scene room (not the host's current scene room),
+                // so the SDK only initiates the CRDT handshake when this reflects the PX room's connection.
+                bool isConnectedToSceneRoom = sceneRoom.IsSceneConnected(sceneData.SceneEntityDefinition.id)


The first check should be entirely omitted for PX as now it creates a false intent

mikhail-dcl · 2026-06-24T10:50:08Z

+    ///     <see cref="TryGetRoom" /> so the scene comms multiplexer can route that scene's CRDT to/from the server;
+    ///     this class never references the scene-runtime pipe (assembly direction is SceneRuntime → DCL.Multiplayer).
+    /// </summary>
+    public class PortableExperienceWorldComms : IDisposable


Very questionable design of this class overall: its functionality should be integrated into LoadSceneSystemLogicBase (or its inheritor) and PortableExperienceSceneFacade instead

cyaiox · 2026-06-24T11:08:47Z

Hey @mikhail-dcl, thanks for the review! will take a deep review of your comments. This is a PoC, but your insight is very appreciated.

Addresses Mikhail's review: the authoritative Portable Experience scene room was driven by a global, stateful, per-frame reconciler that re-derived state the explicit load/unload systems already own. Move ownership into the scene's lifecycle instead. - PortableExperienceSceneFacade now owns the scene room + its message pipe: connected as an awaited step of LoadSceneSystemLogicBase.FlowAsync (the join triggers the server spawn, so the PX is not loaded until connected) and torn down (routing removed, room stopped/disposed) in DisposeInternal on unload. - PortableExperienceRoomFactory: stateless builder of the room + pipe (replaces the stateful PortableExperienceWorldComms keyed dictionary). - Delete PortableExperienceWorldCommsSystem + PortableExperienceCommsPlugin and their wiring; the per-frame query and reconciliation are gone. - SceneCommunicationPipe: RetainOnlyRooms(collection) -> explicit RemoveSceneRoom(id); subscribe inside the lock on register and unsubscribe on remove. Add IMessagePipe.Unsubscribe to make teardown self-contained. - WriteRealmInfoSystem: read isConnectedSceneRoom from the scene's own facade via IScenesCache (not an injected intermediary); branch PX vs non-PX so the host scene-room check is omitted for a PX; fix the nullability violation. - Drop the unused WithThroughputMeasure buffers. - The PX RealmData reaches the load flow on PortableExperienceComponent, read into GetSceneFacadeIntention by the radius system.

Mikhail's review: the connect must be a load precondition 'without which the PX can't be loaded'. Awaiting StartAsync but discarding its result let a soft connection failure proceed; now a failed join throws and fails the load.

@claude

…tions) (#9063) * chore: consolidate PR review feedback into AI review instructions Fold the design-level review of #9059 into .github/prompts/review-instructions.md so the automated reviewer catches architectural defects, not just line-level bugs: - Add a DESIGN & INTEGRATION CHECK, run first, with a mandatory owner search (name the *Load*/*Unload*/facade/disposal owners that already own the lifecycle), anti-rationalization guards (author framing, "it's hard", and assembly boundaries are not justifications), and a teardown/consumption trace for every subscription/buffer. - Require judging whether a new long-lived unit should exist at all vs. integrating into the existing lifecycle owner. - Add blocking categories: subscription/resource leaks, allocated-but-unconsumed infrastructure, detached async for essential work, nullability-contract violations, dead/false-intent conditions. Verified clean-room against #9059: reproduces the human review's architectural findings reliably on Opus and the resource/lifecycle findings on Sonnet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: add construction/encapsulation/resource checks to AI review instructions Consolidates a manual PR review of the media-stream camera-off placeholder work into .github/prompts/review-instructions.md, complementing the existing DESIGN & INTEGRATION CHECK. - Add a CONSTRUCTION, ENCAPSULATION & RESOURCE CHECKLIST: DI vs self-new, invariant-at-construction, naming-vs-responsibility, AI-comment smell, encapsulation/responsibility leaks, side-effect-free funcs, magic values + MordorConstants reuse, thread-safety docs, and resource lifecycle (use-after-destroy ownership, cache cost/benefit, all-or-nothing eviction, per-frame GPU expense). - Reframe the reporting guardrail so project-standard violations are blocking, not "nice-to-have" nitpicks (this was suppressing exactly these findings). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: make @claude review model-selectable and report run cost On-demand `@claude review` now accepts a model, defaulting to Sonnet: - "@claude review opus" runs claude-opus-4-6; otherwise claude-sonnet-4-6. - Model-aware --max-turns ceiling (Sonnet 50, Opus 30) to bound spend. A capped run fails as error_max_turns, so caps are sized to let a normal review finish rather than pay-and-truncate. - The selected model is shown in the pending status. Both the on-demand and auto-review jobs append the run's USD cost to the end of the review comment (after the QA_REQUIRED block), e.g. "Review cost: $0.42 - claude-opus-4-6 - 28 turns". Runs on always() so failed/aborted reviews still report cost (annotated "did not finish"), is idempotent across re-runs, falls back to a standalone comment if the bot comment can't be edited, and is continue-on-error so it never fails the job. The push-triggered auto-review job stays on claude-sonnet-4-6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: sharpen AI review predicate/accessor checks with a mandatory member audit Consolidates NickKhalow's second review round on the media-stream PR (#9043) into the AI review instructions and sharpens the weakest checks. - Naming: flag names that hide a precondition the member depends on (e.g. IsCurrentVideoMuted -> IsCurrentCameraVideoMuted). - Comments: extend the smell list to over-explanation that restates a well-named member and assumptions about code outside the scope. - Add a Predicates, accessors & single-use members section, framed as a MANDATORY MEMBER AUDIT (enumerate each touched member's consumers, then test it against single-use-merge, absent-not-false/null, and don't-re-derive), mirroring the existing OWNER SEARCH pattern to force systematic coverage instead of opportunistic line-by-line flagging. - Carve thin forwarding accessors (e.g. fromIdentity => key.identity) out of the single-use rule so it does not recommend deleting intentional encapsulation — a false positive caught during local validation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: extract Claude review cost reporting into a shared script The cost-reporting github-script block was duplicated verbatim in the auto-review and on-demand-review jobs. Move it into .github/scripts/report-review-cost.js, called from both via require(), so there is a single copy and it can be run locally with a stubbed github/context. Addresses NickKhalow review feedback on #9063. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: restructure AI review instructions into an ordered step protocol Keep the single-agent flow and one parsed verdict, but split the work into an explicit STEP 1..9 protocol (load context, root-cause, design/owner search, member audit, line-level review, complexity, QA, warnings, verdict). Ordering is now structural with a required written output per step, rather than the soft "do this before any line-level review" wording the reviewer distrusted. The verdict trailer is unchanged, so the auto-approve/QA automation is unaffected. Also folds in the rest of NickKhalow's review feedback on #9063: - enumerate concrete leak openers and their mirrors in the leak check - add RAII-for-native-resources to the resource-lifecycle checks - split the dead/false-intent check: keep the semantic "misleading intent" part, leave unreachable / always-true-false to ReSharper Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: let the AI reviewer search the repo with Read/Glob/Grep Add Read, Glob, and Grep to --allowedTools in both the auto-review and on-demand review jobs so the reviewer can run ripgrep-style searches. This is additive to the action's default read-only file access; Bash stays limited to the gh allowlist and the auto-approve path is unchanged (the new tools are read-only, and approval is a separate parsed step). Wire the instructions to use it: Step 1 states the working tree is checked out with Read/Glob/Grep available; Step 5's leak check (#7) now greps each changed *.cs for the openers and their mirrors. Addresses NickKhalow's grep-driven-leak-detection feedback on #9063. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

mikhail-dcl · 2026-06-26T14:00:58Z

@claude review opus

claude · 2026-06-26T14:01:44Z

Claude encountered an error after 2m 42s —— View job

Code Review

Load project docs (CLAUDE.md, docs/README.md)
Read full PR diff and all changed files
Analyze architecture against project standards
Post inline comments on blocking issues
Post summary comment with review result

decentraland-bot

PR Review: feat: poc enable comms for PEXs interacting with the authoritative-server

Step 2 — Root-cause check

PASS. The PR correctly identifies three distinct root causes preventing authoritative-multiplayer Portable Experiences from functioning: (1) no scene-comms room join → no server spawn, (2) world-level room carries no sceneId → server ignores the join, (3) WriteRealmInfoSystem reads the host scene room → isConnectedSceneRoom is always false for PX → SDK never sends REQ_CRDT_STATE. The diff addresses all three at their source rather than working around symptoms.

Step 3 — Design & integration

Room lifecycle ownership (PASS). The PX comms room is owned by PortableExperienceSceneFacade — connected as an awaited step of LoadSceneSystemLogicBase.FlowAsync, disposed in DisposeInternal. The scene facade is the natural lifecycle owner: it is created by the scene load flow and disposed by UnloadSceneSystem. No polling system was introduced. This is the correct design.

SceneCommunicationPipe hoisting (PASS). Previously created per-scene in SceneSharedContainer, now created once in CommsContainer and shared. The pipe already partitions state by sceneId internally. This is valid because the pipe is stateless per-scene except for the new extraRoomsBySceneId dictionary, which is keyed by sceneId. The single-instance model avoids needing an ObjectProxy.

Step 4 — Member audit / Step 5 — Line-level review

P0 — Blockers

1. [Build] Assembly boundary violation — ISceneCommunicationPipe.cs references DCL.Multiplayer types without assembly reference

File: Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/ISceneCommunicationPipe.cs:1-2, 41
Problem: The interface adds using DCL.Multiplayer.Connections.Messaging.Pipe and using DCL.Multiplayer.Connections.Rooms.Connective, and the new RegisterSceneRoom method signature takes IConnectiveRoom and IMessagePipe. The CrdtEcsBridge assembly (CRDT.asmdef) does not reference the DCL.Multiplayer assembly. This causes error CS0234 across all CI builds, tests, and lint.
Fix: Do not leak IConnectiveRoom/IMessagePipe into the ISceneCommunicationPipe interface. Options: (a) define a thin registration interface (e.g. ISceneRoomRegistration) in a shared lower-level assembly, (b) move RegisterSceneRoom/RemoveSceneRoom out of ISceneCommunicationPipe and onto the concrete SceneCommunicationPipe class (which lives in a higher assembly that already references DCL.Multiplayer), or (c) add the assembly reference to CRDT.asmdef (least clean — inverts the dependency direction).
CI evidence: error CS0234: The type or namespace name 'Messaging' does not exist in the namespace 'DCL.Multiplayer.Connections' (repeated across all build/test/lint jobs).

2. [Build] Type mismatch in SceneFactory.cs — messagePipesHub passed where ISceneCommunicationPipe expected

File: Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFactory.cs:284
Problem: The diff passes messagePipesHub as the 4th argument to PortableExperienceSceneFacade(sceneData, runtimeDeps, portableExperienceRoomFactory, messagePipesHub), but the constructor expects ISceneCommunicationPipe sceneCommunicationPipe. IMessagePipesHub does not implement ISceneCommunicationPipe. This is a compile error (masked by the P0 above).
Fix: SceneFactory needs to receive and store the shared SceneCommunicationPipe instance (from CommsContainer) and pass it here instead of messagePipesHub.

P1 — Significant concerns

3. [Encapsulation] WriteRealmInfoSystem downcasts ISceneFacade to concrete PortableExperienceSceneFacade

File: Explorer/Assets/DCL/SDKComponents/RealmInfo/WriteRealmInfoSystem.cs:113-119
Problem: facade is PortableExperienceSceneFacade { IsConnectedSceneRoom: true } breaks the ISceneFacade abstraction. The system depends on a concrete type from a different assembly. If other systems ever need the connection state, they'll all need the same cast.
Fix: Add bool IsConnectedSceneRoom { get; } to ISceneFacade (default true for non-PX scenes, since the host scene room already handles them). Remove the type cast.

4. [Concurrency] Use-after-dispose race in SceneCommunicationPipe.SendMessage

File: Explorer/Assets/DCL/Infrastructure/CrdtEcsBridge/JsModulesImplementation/Communications/SceneCommunicationPipe.cs:149-173
Problem: The pipe reference is obtained inside lock (extraRoomsBySceneId) but used after the lock is released. If RemoveSceneRoom runs concurrently (from the main thread during scene unload) and the caller subsequently disposes the pipe, SendMessage would write to a disposed pipe. While current usage may make this race unlikely (scene thread vs main thread timing), it's structurally unsafe.
Fix: Either extend the lock to cover the send, or make the pipe tolerant of post-dispose sends (return silently), or use a ref-counting checkout pattern.

5. [Resource leak] Undisposed pools in PortableExperienceRoomFactory

File: Explorer/Assets/DCL/Multiplayer/Connections/PortableExperiences/PortableExperienceRoomFactory.cs:28-29
Problem: multiPool = new DCLMultiPool() and memoryPool = new ArrayMemoryPool(ArrayPool<byte>.Shared!) are created in the constructor but the factory has no IDisposable implementation — these pools are never cleaned up. The factory itself lives for the entire session (created once in SceneFactory), so this is a lifetime leak.
Fix: Make PortableExperienceRoomFactory implement IDisposable and dispose its pools, or inject the pools from the composition root so their lifetime is managed externally.

P2 — Minor

6. [DRY] Duplicated room-connection check

Files: PortableExperienceSceneFacade.cs:IsConnectedSceneRoom and SceneCommunicationPipe.cs:IsRoomConnected
Problem: Both encode the same two-condition check (State.Running + ConnConnected). Divergence risk if the "connected" definition changes.
Fix: Extract a shared static helper or extension method on IConnectiveRoom.

7. [Style/CLAUDE.md] Comments narrate server-side behavior

Files: LoadSceneSystemLogicBase.cs:63-64, PortableExperienceSceneFacade.cs:45-47, PortableExperienceSceneFacade.cs:57-58
Problem: Comments like "joining the room is what makes the world-content-server spawn the authoritative server" describe server-side behavior, not what this code guarantees. Per CLAUDE.md §11, comments must not narrate caller/external behavior.
Fix: Rewrite to state the local invariant only. The server-side rationale already lives in docs/authoritative-portable-experiences.md.

8. [Hardening] StopAndDisposeAsync missing inner try-catch in finally

File: PortableExperienceSceneFacade.cs:79-84
Problem: If room.Dispose() throws inside the finally block, the exception is swallowed silently by .Forget(). Minor gap.
Fix: Wrap room.Dispose() in a try-catch with ReportHub.LogException.

Step 6 — Complexity

COMPLEX. The PR modifies LiveKit comms rooms, scene CRDT routing, multiplayer connections, plugin/container wiring, and the scene loading pipeline across 30 files with 700+ additions.

Step 7 — QA assessment

QA_REQUIRED: YES. Changes affect runtime comms connections, scene loading flow, and multiplayer sync — all user-facing.

Step 8 — Non-blocking warnings

No Main.unity modification detected. ✅

CI Status

❌ All builds, tests, and lint are failing due to the assembly reference error in ISceneCommunicationPipe.cs.

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies LiveKit comms rooms, CRDT scene routing, multiplayer connections, container wiring, and the scene loading pipeline
QA_REQUIRED: YES

Reviewed by Jarvis 🤖 · Requested by Mikhail Agapov via Slack

cyaiox requested review from a team as code owners June 23, 2026 17:24

github-actions Bot requested review from Ludmilafantaniella, dalkia and popuz June 23, 2026 17:24

github-actions Bot assigned cyaiox Jun 23, 2026

cyaiox added the force-build Used to trigger a build on draft PR label Jun 23, 2026

cyaiox changed the title ~~feat: authoritative-server (Hammurabi) comms for Portable Experiences~~ feat: Enable comms for PEXs interacting with the authorative-server Jun 24, 2026

claude Bot reviewed Jun 24, 2026

View reviewed changes

cyaiox changed the title ~~feat: Enable comms for PEXs interacting with the authorative-server~~ feat: enable comms for PEXs interacting with the authoritative-server Jun 24, 2026

mikhail-dcl requested changes Jun 24, 2026

View reviewed changes

cyaiox changed the title ~~feat: enable comms for PEXs interacting with the authoritative-server~~ feat: poc enable comms for PEXs interacting with the authoritative-server Jun 24, 2026

mikhail-dcl mentioned this pull request Jun 24, 2026

chore: improve AI PR-review (model selection, cost reporting, instructions) #9063

Merged

6 tasks

cyaiox added 2 commits June 25, 2026 12:13

decentraland-bot requested changes Jun 26, 2026

View reviewed changes

popuz removed their request for review July 6, 2026 09:13

-            roomPipe.Subscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber, IMessagePipe.ThreadStrict.ORIGIN_THREAD);
+        public void RegisterSceneRoom(string sceneId, IConnectiveRoom room, IMessagePipe roomPipe)
+        {
+            // Subscribe before adding to the dict so the inbound subscription is active
+            // the moment SendMessage can route to this pipe.
+            roomPipe.Subscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber, IMessagePipe.ThreadStrict.ORIGIN_THREAD);
+            lock (extraRoomsBySceneId)
+            {
+                if (extraRoomsBySceneId.ContainsKey(sceneId))
+                {
+                    // Already registered — undo the subscribe we just added.
+                    roomPipe.Unsubscribe<Scene>(Packet.MessageOneofCase.Scene, InvokeSubscriber);
+                    return;
+                }
+                extraRoomsBySceneId[sceneId] = new RoomChannel(room, roomPipe);
+            }
+        }

Uh oh!

Conversation

cyaiox commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

What does this PR change?

Test Instructions

Prerequisites

Test Steps

Additional Testing Notes

Quality Checklist

Code Review Reference

Uh oh!

github-actions Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyaiox commented Jun 24, 2026

Uh oh!

claude Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Uh oh!

claude Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mikhail-dcl Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

cyaiox commented Jun 24, 2026

Uh oh!

mikhail-dcl commented Jun 26, 2026

Uh oh!

claude Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Uh oh!

decentraland-bot left a comment

Choose a reason for hiding this comment

PR Review: feat: poc enable comms for PEXs interacting with the authoritative-server

Step 2 — Root-cause check

Step 3 — Design & integration

Step 4 — Member audit / Step 5 — Line-level review

P0 — Blockers

cyaiox commented Jun 23, 2026 •

edited

Loading

github-actions Bot commented Jun 23, 2026 •

edited

Loading

claude Bot commented Jun 24, 2026 •

edited

Loading

mikhail-dcl left a comment •

edited

Loading

mikhail-dcl Jun 24, 2026 •

edited

Loading

mikhail-dcl Jun 24, 2026 •

edited

Loading

mikhail-dcl Jun 24, 2026 •

edited

Loading

claude Bot commented Jun 26, 2026 •

edited

Loading