[Communication] PR4: IAgent and AgentBase Supports Mailbox Delivery#215
[Communication] PR4: IAgent and AgentBase Supports Mailbox Delivery#215Joseph0120 wants to merge 4 commits into
Conversation
|
🧱 Stack PR · Part of stack (9 PRs total) Stack Structure:
|
![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 implements a messaging system for agents by integrating AgentBase with a Mailbox service. It adds registration logic, message handling, and a new NodeType property to the IAgent interface. Feedback was provided regarding the performance of the broadcast-based message delivery and the need for a null check to handle destroyed Unity objects in the message handler.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/AgentBase.cs`:
- Around line 358-362: HandleMailboxDelivery is currently invoked for every
subscriber causing O(n) broadcasts; change the Mailbox to dispatch directly to
the intended receiver instead of broadcasting: add a lookup (e.g.,
Dictionary<IAgent, Action<Message>> or Dictionary<IAgent, Queue<Message>>)
inside Mailbox and register agents when they subscribe, update the
Mailbox.Send/Deliver method to find the target IAgent via that dictionary and
call its registered handler (OnMessage) or enqueue the Message, and remove the
broadcast loop so HandleMailboxDelivery is no longer invoked for non-target
agents; ensure the dictionary uses thread-safe access (lock or
ConcurrentDictionary) and update subscription/unsubscription code to maintain
the map.
- Around line 222-225: The unsubscribe uses Mailbox.Instance but registration
used Mailbox.GetOrCreateInstance(), so store the mailbox reference when
registering (e.g., add a private field like _mailboxInstance and set it in
TryRegisterMailbox to Mailbox.GetOrCreateInstance()), use that stored
_mailboxInstance for adding/removing the HandleMailboxDelivery handler and for
null checks, set _mailboxRegistered = false and clear _mailboxInstance after
unsubscribing to avoid stale references and ensure consistent unsubscription.
🪄 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: 7a15b87a-74af-4a86-a02b-7e725059abe0
📒 Files selected for processing (2)
Assets/Scripts/Agents/AgentBase.csAssets/Scripts/Agents/IAgent.cs
Joseph0120
force-pushed
the
joseph/communication_delay_PR3
branch
2 times, most recently
from
5ed7bdf to
941d5d3
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR4
branch
from
f132eca to
0edbb50
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR3
branch
from
941d5d3 to
9b184c7
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR4
branch
from
51491dc to
a2a4c9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Tests/EditMode/AgentBaseTests.cs`:
- Around line 256-257: Add a brief inline comment above the test lines calling
SetMailboxInstance(null) and _agent.InvokeOnDestroyForTest() explaining that the
test nulls the public MailboxInstance to ensure the agent's OnDestroy logic uses
its cached private _mailboxInstance for unsubscription (rather than re-querying
Mailbox.Instance), i.e., this verifies that the implementation unsubscribes from
the cached instance; reference SetMailboxInstance, MailboxInstance,
_mailboxInstance, Mailbox.Instance and InvokeOnDestroyForTest in the comment for
clarity.
🪄 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: 446e9387-893e-4b87-b923-258a3857cea3
📒 Files selected for processing (2)
Assets/Scripts/Agents/AgentBase.csAssets/Tests/EditMode/AgentBaseTests.cs
| if (_mailboxRegistered) { | ||
| return; | ||
| } | ||
| _mailboxInstance = Mailbox.GetOrCreateInstance(); |
There was a problem hiding this comment.
There should always be an instance already, or am I missing something?
| public class AgentBase : MonoBehaviour, IAgent { | ||
| // Make sure the same agent does not subscribe to the mailbox event more than once (keeping | ||
| // track). | ||
| private bool _mailboxRegistered = false; |
There was a problem hiding this comment.
no need for this flag if you can check whether _mailbox is null?
| // Make sure the same agent does not subscribe to the mailbox event more than once (keeping | ||
| // track). | ||
| private bool _mailboxRegistered = false; | ||
| private Mailbox _mailboxInstance; |
There was a problem hiding this comment.
| private Mailbox _mailboxInstance; | |
| private Mailbox _mailbox; |
| // Start is called before the first frame update. | ||
| protected virtual void Start() {} | ||
| protected virtual void Start() { | ||
| TryRegisterMailbox(); |
There was a problem hiding this comment.
why in Awake and Start? I think it should just be in Awake
| _mailboxRegistered = false; | ||
| _mailboxInstance = null; |
| if (!ReferenceEquals(receiver, this)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Idt you need this. Mailbox should have done the check already
| }; | ||
| } | ||
|
|
||
| protected virtual void OnMessage(Message message) {} |
There was a problem hiding this comment.
I'd rename this to be OnReceiveMessage to be precise
| // Edit-mode tests can create a mailbox without entering play mode, where DontDestroyOnLoad is | ||
| // invalid. | ||
| if (Application.isPlaying) { | ||
| DontDestroyOnLoad(mailboxObject); |
There was a problem hiding this comment.
why is this necessary and not in the other singleton instances that use DontDestroyOnLoad(gameObject);?
| } | ||
|
|
||
| private static void SetMailboxInstance(Mailbox mailbox) { | ||
| FieldInfo instanceField = typeof(Mailbox).GetField( |
There was a problem hiding this comment.
No, this is not the right approach. I think you might need to use play mode tests for time.
Joseph0120
force-pushed
the
joseph/communication_delay_PR3
branch
3 times, most recently
from
d09ba92 to
1274296
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR4
branch
from
5ce599b to
403d41f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 137-149: DeliverMessages currently leaves _messageBuffer uncleared
if pending.Receiver.Receive or OnMessageDelivered throws, which can cause
already-dequeued messages to be delivered again on later Update calls. Update
the DeliverMessages method in Mailbox so the buffered messages are always
cleared even when delivery fails, by wrapping the delivery loop in a try/finally
or equivalent cleanup around _messageBuffer.Clear().
- Around line 123-125: The packet drop check in Mailbox’s delivery logic uses
the wrong comparison, so a zero PacketDeliveryRatio can still let a packet
through when UnityEngine.Random.value is exactly 0. Update the condition in the
packet delivery path so the drop happens on greater-than-or-equal comparison,
keeping the behavior deterministic for “drop all packets” cases. Focus on the
check inside Mailbox’s delivery flow where linkConfig.PacketDeliveryRatio is
compared against UnityEngine.Random.value.
In `@Assets/Tests/EditMode/AgentBaseTests.cs`:
- Around line 258-262: The test in AgentBaseTests is checking unsubscription
indirectly through Mailbox.Send()/Update, which can be influenced by direct
receiver delivery instead of the OnDestroy unsubscribe path. Update the
assertion to verify the mailbox’s OnMessageDelivered subscription list no longer
contains the agent handler after InvokeOnDestroyForTest(), or add a test seam
that explicitly broadcasts OnMessageDelivered and confirms the handler is
removed. Use the existing AgentBaseTests, Mailbox.DeliverMessages, and
InvokeOnDestroyForTest references to locate the test setup.
🪄 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: 2d41fbb6-a765-4ff7-880c-79effb278328
📒 Files selected for processing (3)
Assets/Scripts/Communication/Mailbox.csAssets/Tests/EditMode/AgentBaseTests.csAssets/Tests/EditMode/MailboxTests.cs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 137-149: DeliverMessages currently leaves _messageBuffer uncleared
if pending.Receiver.Receive or OnMessageDelivered throws, which can cause
already-dequeued messages to be delivered again on later Update calls. Update
the DeliverMessages method in Mailbox so the buffered messages are always
cleared even when delivery fails, by wrapping the delivery loop in a try/finally
or equivalent cleanup around _messageBuffer.Clear().
- Around line 123-125: The packet drop check in Mailbox’s delivery logic uses
the wrong comparison, so a zero PacketDeliveryRatio can still let a packet
through when UnityEngine.Random.value is exactly 0. Update the condition in the
packet delivery path so the drop happens on greater-than-or-equal comparison,
keeping the behavior deterministic for “drop all packets” cases. Focus on the
check inside Mailbox’s delivery flow where linkConfig.PacketDeliveryRatio is
compared against UnityEngine.Random.value.
In `@Assets/Tests/EditMode/AgentBaseTests.cs`:
- Around line 258-262: The test in AgentBaseTests is checking unsubscription
indirectly through Mailbox.Send()/Update, which can be influenced by direct
receiver delivery instead of the OnDestroy unsubscribe path. Update the
assertion to verify the mailbox’s OnMessageDelivered subscription list no longer
contains the agent handler after InvokeOnDestroyForTest(), or add a test seam
that explicitly broadcasts OnMessageDelivered and confirms the handler is
removed. Use the existing AgentBaseTests, Mailbox.DeliverMessages, and
InvokeOnDestroyForTest references to locate the test setup.
🪄 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: 2d41fbb6-a765-4ff7-880c-79effb278328
📒 Files selected for processing (3)
Assets/Scripts/Communication/Mailbox.csAssets/Tests/EditMode/AgentBaseTests.csAssets/Tests/EditMode/MailboxTests.cs
🛑 Comments failed to post (3)
Assets/Scripts/Communication/Mailbox.cs (2)
123-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use
>=for the drop check.With the current
>comparison, aPacketDeliveryRatioof0fstill delivers whenRandom.valueis exactly0. That makes “drop all packets” nondeterministically leak messages.Proposed fix
- if (UnityEngine.Random.value > linkConfig.PacketDeliveryRatio) { + if (UnityEngine.Random.value >= linkConfig.PacketDeliveryRatio) { return; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Applies packet delivery ratio (PDR). This is done before sending into PQ. if (UnityEngine.Random.value >= linkConfig.PacketDeliveryRatio) { return;🤖 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 `@Assets/Scripts/Communication/Mailbox.cs` around lines 123 - 125, The packet drop check in Mailbox’s delivery logic uses the wrong comparison, so a zero PacketDeliveryRatio can still let a packet through when UnityEngine.Random.value is exactly 0. Update the condition in the packet delivery path so the drop happens on greater-than-or-equal comparison, keeping the behavior deterministic for “drop all packets” cases. Focus on the check inside Mailbox’s delivery flow where linkConfig.PacketDeliveryRatio is compared against UnityEngine.Random.value.
137-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Always clear
_messageBuffer, even if delivery throws.If
pending.Receiver.Receive(...)or anOnMessageDeliveredsubscriber throws,_messageBuffer.Clear()is skipped. Those already-dequeued messages stay buffered and can be redelivered on subsequentUpdate()calls.Proposed fix
private void DeliverMessages() { float currentTime = SimManager.Instance.ElapsedTime; while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { _messageBuffer.Add(_messageQueue.Dequeue()); } - foreach (PendingMessage pending in _messageBuffer) { - if (!IsReceiverValid(pending.Receiver)) { - continue; - } - pending.Receiver.Receive(pending.Message); - OnMessageDelivered?.Invoke(pending.Receiver, pending.Message); + try { + foreach (PendingMessage pending in _messageBuffer) { + if (!IsReceiverValid(pending.Receiver)) { + continue; + } + pending.Receiver.Receive(pending.Message); + OnMessageDelivered?.Invoke(pending.Receiver, pending.Message); + } + } finally { + _messageBuffer.Clear(); } - _messageBuffer.Clear(); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private void DeliverMessages() { float currentTime = SimManager.Instance.ElapsedTime; while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { _messageBuffer.Add(_messageQueue.Dequeue()); } try { foreach (PendingMessage pending in _messageBuffer) { if (!IsReceiverValid(pending.Receiver)) { continue; } pending.Receiver.Receive(pending.Message); OnMessageDelivered?.Invoke(pending.Receiver, pending.Message); } } finally { _messageBuffer.Clear(); } }🤖 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 `@Assets/Scripts/Communication/Mailbox.cs` around lines 137 - 149, DeliverMessages currently leaves _messageBuffer uncleared if pending.Receiver.Receive or OnMessageDelivered throws, which can cause already-dequeued messages to be delivered again on later Update calls. Update the DeliverMessages method in Mailbox so the buffered messages are always cleared even when delivery fails, by wrapping the delivery loop in a try/finally or equivalent cleanup around _messageBuffer.Clear().Assets/Tests/EditMode/AgentBaseTests.cs (1)
258-262: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t verify event unsubscription through
Mailbox.Send()to the same agent.
Mailbox.DeliverMessages()directly callspending.Receiver.Receive(...)before broadcastingOnMessageDelivered, so this assertion can pass or fail because of direct receiver delivery/validity rather than becauseOnDestroy()removed the mailbox event subscription. Prefer asserting the mailbox event invocation list no longer contains the agent handler, or add a test seam that broadcastsOnMessageDelivereddirectly afterInvokeOnDestroyForTest().🤖 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 `@Assets/Tests/EditMode/AgentBaseTests.cs` around lines 258 - 262, The test in AgentBaseTests is checking unsubscription indirectly through Mailbox.Send()/Update, which can be influenced by direct receiver delivery instead of the OnDestroy unsubscribe path. Update the assertion to verify the mailbox’s OnMessageDelivered subscription list no longer contains the agent handler after InvokeOnDestroyForTest(), or add a test seam that explicitly broadcasts OnMessageDelivered and confirms the handler is removed. Use the existing AgentBaseTests, Mailbox.DeliverMessages, and InvokeOnDestroyForTest references to locate the test setup.
2 participants
Files Modified:
AgentBase -> TryRegisterMailbox() when Start() or Awake(). Unsubscribe to mailbox when OnDestroy().
TryRegisterMailbox() subscribes to the mailbox, and prevents redundant registration.
HandleMailboxDelivery(IAgent receiver, Message message) Check if self is the right receiver, if so then handle the message. Because a popped message is broadcasted to all subscribed agents and it is the agent's responsibility to check if it is the recipient. (Mailbox sends mail to everyone, and only the right recipient has the "key" to unlock that message)
IAgent -> Added NodeType, which tells the mailbox what type of Agent it is reading from.