Skip to content

fix: Ensure all loaded scenes are registered on session owner promotion - #4214

Open
EmandM wants to merge 6 commits into
develop-3.x.xfrom
fix/scene-validation-after-host-migration
Open

EmandM wants to merge 6 commits into
develop-3.x.xfrom
fix/scene-validation-after-host-migration

Conversation

@EmandM

@EmandM EmandM commented Oct 23, 2026

Copy link
Copy Markdown
Member

Purpose of this PR

Currently, when a client starts the session as a DA session owner their NetworkSceneManger initializes all the existing loaded scenes so they can correctly synchronize incoming clients.

This PR ensures that clients promoted to session owner mid-session also initialize their existing loading scenes.

This fixes an error where an active scene that was loaded before the game started would break synchronization on late-joining clients after a new session owner was promoted.

Jira ticket

UUM-154485

Changelog

  • Fixed: Issue where a client promoted to session owner could throw an exception when synchronizing a newly joining client.

Documentation

  • No documentation changes or additions were necessary.

Testing & QA (How your changes can be verified during release Playtest)

Functional Testing

Manual testing :

  • Manual testing done

Automated tests:

  • Covered by existing automated tests
  • Covered by new automated tests

Does the change require QA team to:

  • Review automated tests?
  • Execute manual tests?
  • Provide feedback about the PR?

If any boxes above are checked the QA team will be automatically added as a PR reviewer.

Up-port

n/a

Backports

Was reported on 2.0.0 so probably needs a backport.

@EmandM
EmandM requested a review from a team as a code owner October 23, 2026 20:51

@u-pr u-pr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Harness Review

Needs changes

The change extends scene initialization to session-owner promotion, but late-join correctness still needs stronger handling and verification.

Reviewed commit 00c2276

🤖 Helpful? 👍/👎

// Should be incredibly rare
// If a promoted session owner's scene handle conflicts with an already registered server scene handle,
// ensure the already registered (and synchronized with other clients) scene handle wins.
if (ClientSceneHandleToServerSceneHandle.ContainsKey(loadedScene.handle))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1

After promotion, CMB late joining still throws with server-to-client mapping 7 -> 3 and an unregistered, non-active local scene 7. Scene handles are only locally unique. ClientSceneHandleToServerSceneHandle lacks 7, so this check passes. UpdateServerClientSceneHandle(7, 7, ...) returns false because ServerSceneHandleToClientSceneHandle already contains 7. Initialization ignores that result. If validation accepts the scene, AddSceneToClientSynchronization indexes the missing reverse-map entry and throws. Preserve existing mappings. Assign the new scene a collision-free canonical handle. Check that registration succeeds.

🤖 Helpful? 👍/👎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this, it's working fine. I think this is a theoretical case rather than a real thing

AssertOnTimeout("Session owner promotion failed!");

// Connect a late-joining client to verify the new session owner handles the previously unregistered scene.
yield return CreateAndStartNewClient();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

This test can pass if a future change removes promotion-time scene registration and the late joiner never synchronizes EmptyScene. AddActiveSceneToClientSynchronization now warns and returns when the active scene has no mapping. EmptyScene contains no NetworkObject, and CreateAndStartNewClient checks connection and player clones rather than scene synchronization. Assert that the promoted owner registers the scene after promotion. Assert that the late joiner has a synchronized EmptyScene mapped to the promoted owner's canonical handle.

🤖 Helpful? 👍/👎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression test for a bug where subsequent joins fail. I don't really care if the scene fails to synchronize, only that subsequent joiners can successfully join.

@codecov-github-com

codecov-github-com Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...cts/Runtime/SceneManagement/NetworkSceneManager.cs 86.36% 3 Missing ⚠️
@@              Coverage Diff               @@
##           develop-3.x.x    #4214   +/-   ##
==============================================
  Coverage          76.05%   76.05%           
==============================================
  Files                159      159           
  Lines              27344    27341    -3     
==============================================
- Hits               20797    20795    -2     
+ Misses              6547     6546    -1     
Flag Coverage Δ
NGOv2_project_testproject_ubuntu 75.10% <88.46%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...cts/Runtime/Connection/NetworkConnectionManager.cs 82.19% <ø> (-0.02%) ⬇️
...netcode.gameobjects/Runtime/Core/NetworkManager.cs 72.48% <100.00%> (+0.11%) ⬆️
...me/Messaging/Messages/ConnectionApprovedMessage.cs 67.35% <ø> (+1.09%) ⬆️
...cts/Runtime/SceneManagement/NetworkSceneManager.cs 80.01% <86.36%> (+0.16%) ⬆️

... and 1 file with indirect coverage changes

Components Coverage Δ
com.unity.netcode.gameobjects 76.05% <88.46%> (+<0.01%) ⬆️

ℹ️ Need help interpreting these results?

@NoelStephensUnity NoelStephensUnity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐱

@EmandM
EmandM enabled auto-merge (squash) October 23, 2026 23:13
@EmandM
EmandM disabled auto-merge October 23, 2026 23:13
@EmandM
EmandM marked this pull request as draft October 23, 2026 23:16
@EmandM
EmandM marked this pull request as ready for review October 23, 2026 23:18

@u-pr u-pr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Harness Review

Ship it

The change centralizes loaded-scene registration on session owner promotion and adjusts active-scene synchronization for CMB. I examined startup and promotion ordering, scene tracking, service synchronization, and the regression test; no additional actionable issues emerged beyond those previously reported.

Reviewed commit 84d252d

⚠️ Earlier blocking findings with unresolved threads: 1. The rating above does not cover them; resolve each thread after addressing it.

🤖 Helpful? 👍/👎

@EmandM
EmandM enabled auto-merge (squash) October 23, 2026 23:20

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants