[Communication] PR3: Added Central Mailbox Component.#214
Open
Joseph0120 wants to merge 10 commits into
Open
Conversation
|
🧱 Stack PR · Part of stack (9 PRs total) Stack Structure:
|
This was referenced Apr 1, 2026
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized Mailbox component for handling delayed message delivery between agents, featuring configurable latency and Gaussian jitter. The review feedback identifies several areas for improvement, including optimizing the delivery loop to avoid per-frame allocations, addressing potential null reference exceptions due to initialization order, ensuring singleton persistence for manually placed components, and resolving a discrepancy regarding missing packet delivery ratio logic.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new Mailbox MonoBehaviour singleton that centralizes inter-agent message delivery with configurable latency, jitter, and packet delivery ratio. Messages are scheduled into a priority queue using deterministic simulation time and optional Gaussian jitter; DeliverDueMessages() dispatches due messages each Update after validating receivers. Mailbox.Configure(...) rebuilds defaults and per-link overrides from Configs.CommunicationConfig; SimManager.StartSimulation now calls Mailbox.GetOrCreateInstance().Configure(...) at run start. A new edit-mode test suite exercises delivery timing, queue reinitialization, PDR filtering, per-link overrides, batch semantics, exception handling during delivery, and config fallbacks. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/Agents/Messaging/Mailbox.cs`:
- Around line 39-47: The IndividualLatency/default latency arrays are missing
two directed CommsNode pairs (IADS -> Interceptor and Interceptor -> IADS),
causing those routes to default to 0f; update the DefaultLatencyOverrides array
(and the second similar array around the other occurrence) to include
LatencyOverride entries for From = CommsNode.IADS, To = CommsNode.Interceptor,
Seconds = 0.2f and From = CommsNode.Interceptor, To = CommsNode.IADS, Seconds =
0.2f so all 9 directed pairs between CommsNode.IADS, CommsNode.Carrier, and
CommsNode.Interceptor are explicitly specified.
- Around line 87-94: Send() dereferences _latencyTable and _messageQueue which
can be null if ApplyLatencyOverrides() hasn't run; update Send(Message) to check
for null (e.g., if _latencyTable == null || _messageQueue == null) and return
early (or log a warning) before accessing them, so PendingMessage creation and
_latencyTable.Get(...) are protected; reference the Send method and
ApplyLatencyOverrides, and ensure any unit tests that call Send before
initialization still pass by handling the uninitialized mailbox state
gracefully.
- Around line 122-135: DeliverDueMessages is dequeuing all due PendingMessage
items and immediately invoking OnMessageDelivered for each, but it never applies
the Packet Delivery Rate (PDR) probabilistic filter; update DeliverDueMessages
to apply PDR per message before delivery: when iterating dueMessages, for each
PendingMessage use the configured PDR value (e.g., a float field or getter) to
perform a random check (System.Random or UnityEngine.Random.value) and only call
OnMessageDelivered(pending.Receiver, pending.Message) if the random check passes
and IsReceiverValid(pending.Receiver) returns true; ensure dropped messages are
simply not delivered (no further action) and preserve current receiver validity
check and any logging if desired so the rest of the queue logic (_messageQueue,
Peek/Dequeue, PendingMessage.DeliverAt, GetCurrentTime) remains unchanged.
- Around line 128-132: The code in Mailbox.Update is allocating a new
List<PendingMessage> (dueMessages) every frame; replace that per-frame
allocation by either processing messages inline as you Dequeue from
_messageQueue (iterate while !_messageQueue.IsEmpty() and handle each
PendingMessage immediately) or reuse a single preallocated List<PendingMessage>
field (e.g., _reusableDueMessages) that you Clear() each frame before adding;
reference the existing _messageQueue, PendingMessage, DeliverAt and the Update()
delivery loop when applying the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4393003b-061f-4451-8a17-92ad61c4b9ac
📒 Files selected for processing (1)
Assets/Scripts/Agents/Messaging/Mailbox.cs
Joseph0120
changed the title
This was referenced Apr 1, 2026
Joseph0120
changed the title
This was referenced Apr 1, 2026
Joseph0120
force-pushed
the
joseph/communication_delay_PR2
branch
from
b203cf9 to
735c1ae
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR3
branch
from
66fd29c to
bc3780d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Assets/Scripts/Agents/Messaging/Mailbox.cs (2)
110-128:⚠️ Potential issue | 🟠 Major
Send()can throwNullReferenceExceptionif called before initialization.
_messageQueueis only initialized inClearPendingMessages()(called byConfigure()). IfSend()is called beforeConfigure()completes, line 127 will throw aNullReferenceException.🐛 Proposed fix: Add defensive initialization
public void Send(Message message) { if (message == null || message.Sender == null || message.Receiver == null) { return; } + if (_messageQueue == null) { + return; // Mailbox not yet configured + } LinkRuntimeConfig linkConfig = GetEffectiveLinkConfig(message.Sender, message.Receiver);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Scripts/Agents/Messaging/Mailbox.cs` around lines 110 - 128, Send() can NRE if _messageQueue is null because it's only set in ClearPendingMessages()/Configure(); update Send() to defensively handle uninitialized queue by either lazily initializing _messageQueue (create a new priority queue when null) or early-return with a safe log if Configure() hasn't run, ensuring you reference the _messageQueue field and avoid enqueuing into a null object; modify Send(), and if choosing lazy init, ensure behavior matches ClearPendingMessages() semantics so PendingMessage enqueuing uses the same queue implementation.
137-140: 🧹 Nitpick | 🔵 TrivialAvoid per-frame allocation in hot path.
Creating a new
List<PendingMessage>everyUpdate()generates unnecessary GC pressure. Messages can be processed inline during dequeue.♻️ Proposed refactor: process inline
private void DeliverDueMessages() { if (_messageQueue == null) { return; } float currentTime = GetCurrentTime(); - var dueMessages = new List<PendingMessage>(); while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { - dueMessages.Add(_messageQueue.Dequeue()); - } - foreach (PendingMessage pending in dueMessages) { + PendingMessage pending = _messageQueue.Dequeue(); if (!IsReceiverValid(pending.Receiver)) { continue; } OnMessageDelivered?.Invoke(pending.Receiver, pending.Message); } }Note: If same-frame message isolation is intentional (preventing handlers from processing messages they send immediately), document this behavior explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Scripts/Agents/Messaging/Mailbox.cs` around lines 137 - 140, The per-frame allocation of a new List<PendingMessage> inside Mailbox.Update() causes GC pressure; instead process messages inline as you dequeue from _messageQueue (use while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { var msg = _messageQueue.Dequeue(); HandleMessage(msg); }) or reuse a private List<PendingMessage> field if batching is required; if same-frame message isolation is intentional, add an explicit comment on Mailbox.Update() explaining that handlers should not see messages sent during the same frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Assets/Scripts/Agents/Messaging/Mailbox.cs`:
- Around line 110-128: Send() can NRE if _messageQueue is null because it's only
set in ClearPendingMessages()/Configure(); update Send() to defensively handle
uninitialized queue by either lazily initializing _messageQueue (create a new
priority queue when null) or early-return with a safe log if Configure() hasn't
run, ensuring you reference the _messageQueue field and avoid enqueuing into a
null object; modify Send(), and if choosing lazy init, ensure behavior matches
ClearPendingMessages() semantics so PendingMessage enqueuing uses the same queue
implementation.
- Around line 137-140: The per-frame allocation of a new List<PendingMessage>
inside Mailbox.Update() causes GC pressure; instead process messages inline as
you dequeue from _messageQueue (use while (!_messageQueue.IsEmpty() &&
currentTime >= _messageQueue.Peek().DeliverAt) { var msg =
_messageQueue.Dequeue(); HandleMessage(msg); }) or reuse a private
List<PendingMessage> field if batching is required; if same-frame message
isolation is intentional, add an explicit comment on Mailbox.Update() explaining
that handlers should not see messages sent during the same frame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29fa05e8-6a71-4600-a171-3a9a4e3588fb
📒 Files selected for processing (2)
Assets/Scripts/Agents/Messaging/Mailbox.csAssets/Scripts/Managers/SimManager.cs
Joseph0120
force-pushed
the
joseph/communication_delay_PR2
branch
from
b0b9526 to
a45b09b
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR3
branch
from
5ed7bdf to
941d5d3
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR2
branch
from
0f2c041 to
8ed1ce6
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR3
branch
from
941d5d3 to
9b184c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Assets/Scripts/Communication/Mailbox.cs`:
- Around line 147-153: The delivery loop can throw and prevent
_dueMessages.Clear() from running; wrap the delivery logic in a try/finally so
_dueMessages.Clear() is executed regardless of exceptions: locate the foreach
over _dueMessages in Mailbox.Deliver (the block that calls IsReceiverValid and
OnMessageDelivered with PendingMessage), move the clear into a finally block,
and keep the existing receiver validation and delivery calls inside the try so
any thrown exception still allows _dueMessages to be cleared; optionally
consider iterating a snapshot (e.g., a copy of _dueMessages) if you want to
avoid concurrent modification concerns while delivering.
- Around line 121-127: The PDR check is currently performed inside Send() (the
UnityEngine.Random.value >= linkConfig.PacketDeliveryRatio early return) which
drops packets at enqueue time and breaks delayed-loss semantics; remove that
Random-based drop from Send() and instead apply the packet delivery ratio when
actually delivering queued items in DeliverDueMessages(): in the delivery loop
inside DeliverDueMessages() consult linkConfig.PacketDeliveryRatio and use
UnityEngine.Random to skip (drop) the packet at delivery time, preserving
enqueue/timestamp behavior and any delay semantics; update or add tests that
expect loss to occur at delivery rather than enqueue for
Mailbox.Send/DeliverDueMessages.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bec77c47-46e8-4885-bd4f-30de84b28a12
📒 Files selected for processing (5)
Assets/Scripts/Communication/Mailbox.csAssets/Scripts/Communication/Mailbox.cs.metaAssets/Scripts/Managers/SimManager.csAssets/Tests/EditMode/MailboxTests.csAssets/Tests/EditMode/MailboxTests.cs.meta
tryuan99
reviewed
Jun 14, 2026
| // Compares two link keys for dictionary lookup. | ||
| public bool Equals(LinkKey other) => From == other.From && To == other.To; | ||
| // Produces a stable hash for use in the per-link override dictionary. | ||
| public override int GetHashCode() => ((int)From * 397) ^ (int)To; |
Member
There was a problem hiding this comment.
You should look into using HashCode.Combine(From, To)
| // Runtime communication settings for one link, taken directly from proto files. | ||
| private readonly struct LinkRuntimeConfig { | ||
| // Standard LinkRuntimeConfig that guarantees a 0 latency, 0 jitter, and with PDR = 1. | ||
| public static readonly LinkRuntimeConfig ZeroLatencyGuaranteedDelivery = |
| private PriorityQueue<PendingMessage> _messageQueue; | ||
| // _dueMessages is a reusable temporary buffer; due messages are added, processed, then cleared | ||
| // every delivery frame. | ||
| private readonly List<PendingMessage> _dueMessages = new(); |
Member
There was a problem hiding this comment.
maybe just call it _messageBuffer
|
|
||
| // Advances message delivery once per frame using simulation time. | ||
| private void Update() { | ||
| DeliverDueMessages(); |
Member
There was a problem hiding this comment.
DeliverMessages() is more succint
| } | ||
|
|
||
| // Converts a protobuf link config into runtime values. | ||
| private static LinkRuntimeConfig ToRuntimeConfig(Configs.LinkConfig linkConfig) { |
Member
There was a problem hiding this comment.
no, this should be a constructor option in LinkRuntimeConfig
Comment on lines
+205
to
+211
| if (receiver == null) { | ||
| return false; | ||
| } | ||
| if (receiver is UnityEngine.Object unityObject && unityObject == null) { | ||
| return false; | ||
| } | ||
| return !receiver.IsTerminated; |
Member
There was a problem hiding this comment.
overly safe. why not just if receiver != null && !receiver.IsTerminated and inline this?
|
|
||
| public void StartSimulation() { | ||
| // Each simulation run reconfigures the mailbox and clears old queued messages. | ||
| Mailbox.GetOrCreateInstance().Configure(SimulationConfig?.CommunicationConfig); |
Member
There was a problem hiding this comment.
Mailbox should listen for the OnSimulationStarted event and initialize there
| : base(sender, receiver, MessageType.AssignTargetRequest) {} | ||
| } | ||
|
|
||
| private sealed class TestAgent : IAgent { |
Member
There was a problem hiding this comment.
This seems like a more useful utility than something that lives in MailboxTests.cs
|
|
||
| SetElapsedTime(2f); | ||
| InvokePrivateMethod(_mailbox, "Update"); | ||
| Assert.AreEqual(1, deliveredCount); |
Member
There was a problem hiding this comment.
you should add a test for a message that was intended for some agent that then got deleted (i.e., agent is null or terminated). what happens during the delivery in this case?
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.
Files Created:
Mailbox.cs -> The centralized mailbox is what all agents use to send and receive delayed messages.
Descriptions:
There are 3 modes under "LatencyMode" for constructing the latency table. NoLatency sets mailbox to Ideal communication with zero latency (assume no latency); UniformLatency set all latency entries to one singular "_uniformLatency" value; IndividualLatency allows you to configure each "from->to" pair independently. "_latencyJitterStdSeconds" sets Latency jitter standard deviation in seconds.
Send(Message message) enqueue a message for delayed delivery. Message will be released when DeliverTime has reached.
DeliverDueMessages() repeatedly pop due messages off the queue. Apply PDR so only certain amount of messages pass through.