-
Notifications
You must be signed in to change notification settings - Fork 5.4k
fix: rm the message emit in src api to avoid race condition #6137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 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 |
|
shouldn't we prefer the event since it's extensible? |
fix: rm the message emit in src api to avoid race condition elizaOS#6137
|
@standujar hey what's the status on this? We saw the release but this was missed? |
Relates to
Risks
Low - This change removes a duplicate message emit that was causing race conditions. The message is still properly emitted through
serverInstance.createMessage(), so there is no risk of losing message processing functionality. The change only affects the/central-channels/:channelId/messagesendpoint in the channels router.Background
What does this PR do?
This PR removes a duplicate
internalMessageBus.emit('new_message', messageForBus)call from the/central-channels/:channelId/messagesendpoint inpackages/server/src/api/messaging/channels.ts.The message is already emitted as a root message when
serverInstance.createMessage(newRootMessageData)is called (seepackages/server/src/index.tslines 1702-1723), which automatically publishes the message to the internal message bus for agent consumption.What kind of change is this?
Bug fixes (non-breaking change which fixes an issue)
Why are we doing this? Any context or related work?
When a message was created through the
/central-channels/:channelId/messagesendpoint, it was being emitted to the internal message bus twice:serverInstance.createMessage()(which is the correct place)This double emission caused a race condition where the message processing race check in
DefaultMessageService.processMessage()(seepackages/core/src/services/default-message-service.tslines 361-374) would fail, resulting in the error: "Response discarded - newer message being processed for agent".By removing the duplicate emit, messages are now only emitted once through the proper channel (
serverInstance.createMessage()), eliminating the race condition and ensuring messages are processed correctly.Documentation changes needed?
My changes do not require a change to the project documentation.
Testing
Where should a reviewer start?
Review the change in
packages/server/src/api/messaging/channels.tsaround line 245, where the duplicateinternalMessageBus.emit()call was removed. Verify thatserverInstance.createMessage()inpackages/server/src/index.tsstill properly emits messages to the internal message bus.Detailed testing steps
Verify message emission still works:
/central-channels/:channelId/messagesendpoint[AgentServer] Published message {id} to internal message bus)Verify race condition is fixed:
Verify no duplicate processing:
None: Automated tests are acceptable.