Replication: authority delegation (cell-based elected syncer)#227
Open
Segfaultd wants to merge 3 commits into
Open
Replication: authority delegation (cell-based elected syncer)#227Segfaultd wants to merge 3 commits into
Segfaultd wants to merge 3 commits into
Conversation
Server-side proximity election that hands a non-owned entity's authority to a nearby client -- the syncer -- reusing the existing ownerGUID model. Adds a Delegation trait (opt-in delegatable flag, OrphanMode, pinned owner), an OnOwnershipChanged client hook, and a CanDelegateTo veto. The decision is a pure, testable ElectOwner policy (acquire/drop hysteresis, least-loaded with nearest tiebreak, soft cap), driven each interest rebuild by ReplicationManager::RunDelegation and applied via SetOwner. Orphans fall back to the server (frozen) or destroy per OrphanMode.
Unit tests for ElectOwner: nearest pick, load balancing, acquire/drop hysteresis, re-election on drift, orphan fallback, dimension/veto exclusion, load soft cap, and Z-up ground-plane distance.
WalkthroughAdds a proximity-based ownership delegation system: a new ChangesProximity-based ownership delegation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkPeer
participant ReplicationManager
participant ElectOwner
participant NetworkEntity
NetworkPeer->>ReplicationManager: Update() -> RunDelegation(now)
ReplicationManager->>ReplicationManager: build load map of owned entities
loop each delegatable entity
ReplicationManager->>ReplicationManager: collect eligible viewers (CanDelegateTo)
ReplicationManager->>ElectOwner: ElectOwner(pos, currentOwner, candidates, params)
ElectOwner-->>ReplicationManager: next owner guid
alt owner changed
ReplicationManager->>NetworkEntity: SetOwner(next)
NetworkEntity->>NetworkEntity: SetOwnerFromServer(next) triggers OnOwnershipChanged
else unassigned + OrphanMode::Destroy
ReplicationManager->>NetworkEntity: DestroyEntity()
end
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cut the narration to terse invariants and drop the third-party mod reference from the source.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/framework/src/networking/replication/delegation_policy.h (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding against acquireRange > dropRange misconfiguration.
The hysteresis contract relies on
acquireRange <= dropRange(owner retained up to the wider drop threshold, new candidates only accepted within the tighter acquire threshold). If a caller ever misconfiguresacquireRange > dropRange, the hysteresis logic inElectOwnerwould behave inconsistently. Consider a lightweightassertor a clamp in the constructor/accessor to catch this early.🛡️ Proposed defensive check
struct DelegationParams { float acquireRange = 80.0f; float dropRange = 130.0f; uint32_t electionIntervalMs = 500; + // Note: acquireRange should always be <= dropRange for hysteresis to be meaningful. // Soft cap on how many delegated entities one client is newly given (0 = unlimited). The load // balancer already prefers the least-loaded candidate; this refuses to pile more onto a client // already at the cap when a lighter one exists, but never revokes what it holds. int loadSoftCap = 0; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/networking/replication/delegation_policy.h` around lines 24 - 32, DelegationParams currently allows an invalid hysteresis setup if acquireRange is greater than dropRange, which can break the ownership election behavior. Add a lightweight defensive check where DelegationParams is initialized or consumed so acquireRange is always less than or equal to dropRange, using a clear assert or equivalent clamp in the path used by ElectOwner. Keep the check close to DelegationParams so misconfiguration is caught early before replication delegation logic runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/framework/src/networking/replication/replication_manager.cpp`:
- Around line 246-251: The pinned-owner logic in RunDelegation() keeps forcing
ownership to delegation.pinnedOwner even after that peer disconnects, so update
the disconnect handling or the re-pin check to ensure only live peers are used.
In replication_manager.cpp, either clear entity->delegation.pinnedOwner when the
pinned peer disconnects, or gate the SetOwner path on a current
viewer/alive-peer check before reassigning. Keep the fix localized around
RunDelegation() and the disconnect path that manages delegation state.
---
Nitpick comments:
In `@code/framework/src/networking/replication/delegation_policy.h`:
- Around line 24-32: DelegationParams currently allows an invalid hysteresis
setup if acquireRange is greater than dropRange, which can break the ownership
election behavior. Add a lightweight defensive check where DelegationParams is
initialized or consumed so acquireRange is always less than or equal to
dropRange, using a clear assert or equivalent clamp in the path used by
ElectOwner. Keep the check close to DelegationParams so misconfiguration is
caught early before replication delegation logic runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b96536c-e895-440c-94d3-12f1bd4be9f6
📒 Files selected for processing (8)
code/framework/src/networking/network_peer.cppcode/framework/src/networking/replication/delegation_policy.hcode/framework/src/networking/replication/network_entity.cppcode/framework/src/networking/replication/network_entity.hcode/framework/src/networking/replication/replication_manager.cppcode/framework/src/networking/replication/replication_manager.hcode/tests/framework_ut.cppcode/tests/modules/delegation_ut.h
Comment on lines
+246
to
+251
| if (entity->delegation.pinnedOwner != MafiaNet::UNASSIGNED_PEER_GUID) { | ||
| if (entity->ownerGUID != entity->delegation.pinnedOwner) { | ||
| SetOwner(entity, entity->delegation.pinnedOwner); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and surrounding symbols first.
ast-grep outline code/framework/src/networking/replication/replication_manager.cpp --view expanded || true
# Find all references to pinnedOwner, ownership changes, and disconnect handling.
rg -n --hidden --no-ignore-vcs \
-e 'pinnedOwner|OnClosedConnection|SetOwner\(|ownerGUID|UNASSIGNED_PEER_GUID|GetViewer\(' \
code/framework/src code -g '!**/build/**' -g '!**/dist/**' || true
# Read the relevant slices from the replication manager.
sed -n '200,320p' code/framework/src/networking/replication/replication_manager.cpp
# Inspect any disconnect/connection cleanup paths that might clear pinning.
rg -n --hidden --no-ignore-vcs -e 'clear.*pinnedOwner|pinnedOwner\s*=|delegation\.pinnedOwner' code/framework/src code -g '!**/build/**' -g '!**/dist/**' || trueRepository: MafiaHub/Framework
Length of output: 30953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the pinning/delegation definitions and any public APIs around them.
sed -n '130,220p' code/framework/src/networking/replication/network_entity.h
sed -n '1,220p' code/framework/src/networking/replication/network_entity.cpp
sed -n '120,180p' code/framework/src/networking/replication/replication_manager.h
# Search for any code that explicitly clears or updates pinnedOwner.
rg -n --hidden --no-ignore-vcs \
-e 'pinnedOwner\s*=|SetPinned|ClearPinned|delegation\.pinnedOwner|pinned syncer|pinned' \
code/framework/src code/tests -g '!**/build/**' -g '!**/dist/**' || trueRepository: MafiaHub/Framework
Length of output: 18815
Clear or validate pinnedOwner when the pinned peer disconnects code/framework/src/networking/replication/replication_manager.cpp:241-243
pinnedOwner is never cleared on disconnect, so RunDelegation() keeps reassigning the entity to a disconnected peer and bypasses normal election/orphan handling. Gate the re-pin on a live viewer, or clear delegation.pinnedOwner in the disconnect path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/networking/replication/replication_manager.cpp` around
lines 246 - 251, The pinned-owner logic in RunDelegation() keeps forcing
ownership to delegation.pinnedOwner even after that peer disconnects, so update
the disconnect handling or the re-pin check to ensure only live peers are used.
In replication_manager.cpp, either clear entity->delegation.pinnedOwner when the
pinned peer disconnects, or gate the SetOwner path on a current
viewer/alive-peer check before reassigning. Keep the fix localized around
RunDelegation() and the disconnect path that manages delegation state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds server-side authority delegation to the replication layer: a non-owned, server-authoritative entity can have its per-frame simulation handed to a nearby client (the syncer), which streams state up while the server relays to everyone else. This is the classic server-elected syncer model, generalized onto the existing
ownerGUIDauthority — the same primitive vehicles already use to delegate to a driver, now driven by proximity election instead of a single hard-coded trigger.Reusable Framework primitive (any mod benefits); no MafiaNet changes.
How
NetworkEntity(server-only, not replicated): opt-indelegatableflag,OrphanMode(Freeze / Destroy), andpinnedOwnerfor a scripted/persistent override.OnOwnershipChanged(bool nowOwner)— client seam to spin up / tear down the local simulation. Fires after construction so the replicated pose is populated before a seeding handler reads it. All client owner changes now funnel throughSetOwnerFromServer, so the callback can't be bypassed.CanDelegateTo(guid)— game veto (role / line-of-sight) layered on top of proximity + dimension.ElectOwner()(delegation_policy.h, header-only, networking-free): acquire/drop-range hysteresis, least-loaded with nearest tiebreak, optional load soft cap, returnsUNASSIGNEDwhen nobody qualifies.ReplicationManager::RunDelegation()— cadence-gated glue; builds candidates from the viewer map, honors pinned owners, applies viaSetOwner, defers orphan-destroys past the iteration. Called fromNetworkPeer::UpdateafterRebuildInterest.Set/GetDelegationParamsexposed for the integration/config layer.Behaviour / compatibility
Purely additive and opt-in — entities leave
delegatablefalse and are untouched by election, so existing replication behaviour is unchanged. Orphan-on-disconnect already returns owned entities to the server viaOnClosedConnection; the election pass re-delegates on the next tick.Tests
code/tests/modules/delegation_ut.hcoversElectOwnerin isolation: nearest pick, load balancing, acquire/drop hysteresis, re-election on drift, orphan fallback, dimension/veto exclusion, load soft cap, and Z-up ground-plane distance. Run withcmake --build build --target RunFrameworkTests.Next steps (not in this PR)
Consuming side (per-game, e.g. M2O)
This PR ships only the mechanism; a game still has to opt in and implement the local-sim seam. On the consuming side,
RunDelegationalready runs automatically each tick — the game only needs to:delegatable = true).OnOwnershipChanged(true)spawns/adopts and drives the local game object;OnOwnershipChanged(false)tears it down.OnSerializeConstruction/transform so the handoff doesn't stutter, and set game-unitDelegationParamsvia the integration layer (ground plane already follows the interest config).Framework follow-ups (deferred as premature)
lastOwnergrace window — briefly accept the previous owner's in-flight updates after a handoff to smooth the seam (today the elected client seeds from the already-replicated pose).forceSyncer/onSyncerChangeso script layers can pin or observe delegation.