fix: scene message handler duplicate key

[lorux0]

What does this PR change?

Fixes #8183

Fixes any possible key duplication when registering the scene message handler.
The root of the issue might be caused due disposal issues, but it should have been fixed here: #8702

Test Instructions

Go through different scenes. Check that they work normally.

Quality Checklist

• Changes have been tested locally
• Documentation has been updated (if required)
• Performance impact has been considered
• For SDK features: Test scene is included

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

[github-actions]

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name	Link
Commit	4df3266
Logs	https://github.com/decentraland/unity-explorer/actions/runs/26107746212
Download Windows	https://github.com/decentraland/unity-explorer/suites/69732968102/artifacts/7089215887
Download Windows S3	https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/duplicate-key-scene-message-handler/pr-22408-4df3266/Decentraland_windows64.zip
Download Mac	https://github.com/decentraland/unity-explorer/suites/69732968102/artifacts/7089289248
Download Mac S3	https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/duplicate-key-scene-message-handler/pr-22408-4df3266/Decentraland_macos.zip
Built on	2026-05-19T16:27:34Z

[claude]

Claude finished @lorux0's task in 1m 58s —— View job

---

Review

• Gather context
• Read changed file and diff
• Analyze root cause vs symptom
• Check for blocking issues
• Post final review

---

Root-cause assessment

Problem being solved: Dictionary.Add() throws ArgumentException when AddSceneMessageHandler is called twice with the same (sceneId, msgType) key. This happens when a scene re-initializes before the old CommunicationsControllerAPIImplementationBase instance has been disposed (and its RemoveSceneMessageHandler call run).

Root cause or symptom? This is a symptom fix. The underlying cause is an improper disposal ordering — the old scene module instance isn't fully torn down before the new one registers. PR #8702 attempted to address that lifecycle issue. This PR defensively prevents the crash when it still occurs.

That said, silently replacing the handler is the correct semantic (the newest registration should win), so this is a reasonable defensive measure — provided the removal side is updated to match (see blocking issue below).

---

Blocking issue

RemoveSceneMessageHandler ignores its onSceneMessage parameter — it always removes the key unconditionally. With the new replacement strategy this creates a correctness regression:

1. Instance A registers handler for key K
2. Instance B registers for key K — A's value is replaced by B's (this PR's fix)
3. Instance A's Dispose() later runs → sceneMessageHandlers.Remove(key) → removes B's handler
4. Scene B silently stops receiving any comms messages

Before this PR, step 2 would throw, making the ordering problem visible. Now the duplicate add is absorbed but the destructive removal still happens.

See inline comment on RemoveSceneMessageHandler for the exact fix.

---

Non-blocking

The comment // Dont add but replace describes what the code does (already obvious from the indexer), violating the CLAUDE.md no-what-comments rule. See inline suggestion.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-file change inside the scene comms pipe, touching dictionary insertion/removal logic only.
QA_REQUIRED: YES

[claude]

Claude finished @lorux0's task in 1m 10s —— View job

---

Review

• Gather context
• Read current file state and diff
• Check if previous blocking issue was addressed
• Post final review

---

Root-cause assessment

Problem being solved: Dictionary.Add() throws ArgumentException when AddSceneMessageHandler is called twice with the same (sceneId, msgType) key — occurring when a scene re-initializes before the previous CommunicationsControllerAPIImplementationBase instance is disposed.

Root cause or symptom? Symptom fix. The underlying cause is improper disposal ordering; PR #8702 attempted to address that. This PR defensively prevents the crash. Swapping to the indexer is the correct semantic (newest registration wins), so the fix is reasonable — but only if the removal side is also updated to match.

---

Blocking issue — still unaddressed

The previous review flagged that RemoveSceneMessageHandler (line 97) removes the key unconditionally, ignoring the onSceneMessage parameter. This was not fixed in the latest push.

With the new replacement strategy the sequence becomes:

1. Instance A registers handler for key K
2. Instance B registers for key K → A's handler is silently replaced (this PR's fix)
3. Instance A's Dispose() runs → sceneMessageHandlers.Remove(key) → B's handler is deleted
4. Scene B stops receiving all comms messages with no error or log

The fix needed (see inline comment on line 97):

lock (sceneMessageHandlers)
{
    if (sceneMessageHandlers.TryGetValue(key, out var current) && current == onSceneMessage)
        sceneMessageHandlers.Remove(key);
}

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-file change inside the scene comms pipe, touching dictionary insertion/removal logic only.
QA_REQUIRED: YES

[claude]

Claude finished @lorux0's task in 1m 2s —— View job

---

Review

• Gather context
• Read current file state and diff
• Check if previous blocking issues were addressed
• Post final review

---

Root-cause assessment

Problem being solved: Dictionary.Add() throws ArgumentException when AddSceneMessageHandler is called twice with the same (sceneId, msgType) key — occurring when a scene re-initializes before the previous CommunicationsControllerAPIImplementationBase instance is disposed.

Root cause or symptom? Symptom fix. The underlying cause remains improper disposal ordering (PR #8702 attempted that fix). This PR provides the correct defensive guard, and the fix is semantically sound (newest registration wins), now paired with a corrected removal path.

---

Previous blocking issue — now fixed ✅

RemoveSceneMessageHandler (line 97–107) now includes the equality guard:

if (sceneMessageHandlers.TryGetValue(key, out var current) && current == onSceneMessage)
    sceneMessageHandlers.Remove(key);

This correctly prevents Instance A's Dispose() from removing Instance B's handler after a replacement, resolving the use-after-replacement bug flagged in the two previous reviews.

Previous non-blocking — now fixed ✅

The what-comment // Dont add but replace was replaced with an issue reference // See: https://github.com/decentraland/unity-explorer/issues/8183, consistent with CLAUDE.md comment standards.

---

No blocking issues remain.

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-file change in the scene comms pipe, touching only dictionary insertion/removal logic with no ECS, async, or cross-world involvement.
QA_REQUIRED: YES

[github-actions]

Auto-approved by Claude — simple fix/chore with no blocking issues. QA approval is still required.

[balducciv]

✅ PR reviewed and approved by QA on both platforms following the PR test instructions.
✅ Smoke test performed on Windows and Mac to verify the normal flow is working as expected.

Build: v0.148.0-alpha-fix/duplicate-key-scene-message-handler-4df3266 (PR #8821)
OS: macOS 15.7.4 — Apple M3 Pro | Windows 11 (10.0.26200)

Test results:

• No ArgumentException / duplicate key errors in Player.log on Mac (across 2 sessions)
• No ArgumentException / duplicate key errors in Player.log on Windows
• Scene loading completed successfully on both platforms — multiple Loading stage: Completed entries confirmed
• Teleported between Genesis Plaza and Worlds — scenes loaded and unloaded without duplicate key registration errors
• Smoke test passed — general flow, scene interaction, and multiplayer presence working as expected on both platforms

Unrelated errors noted (do not affect verdict):

• GPUInstancerPro NullReferenceException (occlusion culling) — known recurring noise, present on both Mac and Windows
• MVC.ViewBase / MinimapController NullReferenceException — dispose-time exception, unrelated to scene message handler
• Curl error 42: Callback aborted — network cleanup on scene unload, unrelated
• Multiple log categories suppressed in alpha build (SCENE_LOADING, SCENE_FACTORY, ECS, COMMS_SCENE_HANDLER, etc.) — absence of additional logging does not confirm or deny behavior for those systems

Verdict: PASS ✅

Additional note (unrelated to this PR): On Mac, during the first session, teleporting from Genesis Plaza to a World caused all GP users to appear as ghost avatars in the destination World. This did not reproduce after restarting the build (second session). Windows was not affected. Will file separately if reproducible.

Logs:
Player 8821 windows.log
Player-prev.log
Player.log

Issue reproduced only once and no longer reproduced while testing this PR

19.05.2026_17.40.15_REC.PR-8821.first.teleport.mp4

19.05.2026_17.45.14_REC.PR-8821.mp4