fix(meta): resolve race condition in Business API sender identification#2493
Open
LeonardPeixoto wants to merge 1 commit intoEvolutionAPI:mainfrom
Open
fix(meta): resolve race condition in Business API sender identification#2493LeonardPeixoto wants to merge 1 commit intoEvolutionAPI:mainfrom
LeonardPeixoto wants to merge 1 commit intoEvolutionAPI:mainfrom
Conversation
The BusinessStartupService shared a mutable this.phoneNumber property across concurrent webhook requests. When two webhooks arrived near- simultaneously for the same phone_number_id, the second request could overwrite phoneNumber before the first finished processing, causing messages to be attributed to the wrong sender (wrong remoteJid). Changes: - Compute senderJid as a local variable before calling eventHandler - Pass senderJid as parameter through eventHandler -> messageHandle - Await eventHandler to prevent concurrent mutation - Replace all this.phoneNumber reads inside messageHandle with the local senderJid parameter Made-with: Cursor
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the Meta WhatsApp Business webhook flow to compute a per-request sender JID and thread it through the event handling pipeline, removing reliance on a mutable instance field and awaiting the handler to avoid race conditions between concurrent webhook requests. Sequence diagram for updated Meta WhatsApp webhook sender identificationsequenceDiagram
participant MetaWebhook as MetaWebhook
participant BusinessStartupService as BusinessStartupService
participant EventHandler as eventHandler
participant MessageHandle as messageHandle
MetaWebhook->>BusinessStartupService: inboundWebhook(content)
BusinessStartupService->>BusinessStartupService: loadChatwoot()
BusinessStartupService->>BusinessStartupService: senderJid = createJid(from or recipient_id)
BusinessStartupService->>BusinessStartupService: phoneNumber = senderJid
BusinessStartupService->>EventHandler: eventHandler(content, senderJid)
activate EventHandler
EventHandler->>EventHandler: database = get DATABASE
EventHandler->>EventHandler: settings = findSettings()
alt content has messages
EventHandler->>MessageHandle: messageHandle(content, database, settings, senderJid)
else content has statuses
EventHandler->>MessageHandle: messageHandle(content, database, settings, senderJid)
end
deactivate EventHandler
MessageHandle->>MessageHandle: key.remoteJid = senderJid
MessageHandle->>MessageHandle: key.fromMe = (senderJid === metadata.phone_number_id)
Updated class diagram for BusinessStartupService event handlingclassDiagram
class ChannelStartupService
class Database
class BusinessStartupService {
- phoneNumber string
+ eventHandler(content any, senderJid string) void
+ messageHandle(received any, database Database, settings any, senderJid string) void
}
BusinessStartupService --|> ChannelStartupService
BusinessStartupService ..> Database
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- If the goal is to eliminate shared mutable state causing race conditions, consider removing
this.phoneNumberentirely and consistently passingsenderJidthrough all call sites and usages rather than still assigning to the instance property. - Now that
eventHandlerdepends on asenderJidargument, it might be safer to encapsulate the JID derivation insideeventHandleritself (or validate the argument) so that future callers don’t accidentally pass an inconsistent value and reintroduce attribution issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If the goal is to eliminate shared mutable state causing race conditions, consider removing `this.phoneNumber` entirely and consistently passing `senderJid` through all call sites and usages rather than still assigning to the instance property.
- Now that `eventHandler` depends on a `senderJid` argument, it might be safer to encapsulate the JID derivation inside `eventHandler` itself (or validate the argument) so that future callers don’t accidentally pass an inconsistent value and reintroduce attribution issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 task
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
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.
The BusinessStartupService shared a mutable this.phoneNumber property across concurrent webhook requests. When two webhooks arrived near- simultaneously for the same phone_number_id, the second request could overwrite phoneNumber before the first finished processing, causing messages to be attributed to the wrong sender (wrong remoteJid).
Changes:
Made-with: Cursor
📋 Description
🔗 Related Issue
Closes #(issue_number)
🧪 Type of Change
🧪 Testing
📸 Screenshots (if applicable)
✅ Checklist
📝 Additional Notes
Summary by Sourcery
Bug Fixes: