fix: validate phash from server ack to detect stale device lists - #494
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a crate-visible async Changes
Sequence DiagramsequenceDiagram
participant App as App
participant Client as Client
participant Sender as Send Function
participant Network as Server/Network
participant AckTask as Ack Validation Task
participant Cache as Cache System
App->>Client: register_ack_waiter(message_id)
Client->>Client: create oneshot (tx, rx) and insert tx into response_waiters
Client-->>App: return rx
App->>Sender: send_message_impl/status(stanza)
Sender->>Sender: extract phash (if present)
alt phash present
Sender->>Client: register_ack_waiter(message_id)
Client-->>Sender: return rx
end
Sender->>Network: send_node(stanza)
alt phash present
Sender->>AckTask: spawn background task (10s) waiting on rx
end
Network-->>Client: ack_response received
Client->>Client: handle_ack_response -> resolve oneshot sender
Client->>AckTask: oneshot delivers ack Node
AckTask->>AckTask: compare server phash vs local phash
alt phash mismatch
AckTask->>Cache: invalidate sender_key_device_cache (jid)
AckTask->>Cache: invalidate group cache (if group send)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 21s —— View job Code Review: phash mismatch handling
Overall this is a clean, well-scoped change. The approach of fire-and-forget phash validation in a background task is sound and matches the whatsmeow reference. A few items worth addressing: Bug: waiter leak when
|
Bencher Report
1669f31 to
d1d5fc7
Compare
Register an ack waiter before sending group/status messages, then validate the server's phash response in a background task. On mismatch (participant list changed during send), invalidate sender key device cache and group info cache so the next send uses fresh device lists. Non-blocking: send returns immediately after writing to socket. The phash validation runs in a detached background task with 10s timeout. Matches whatsmeow's approach (send.go:448-464) of cache invalidation on phash mismatch. WA Web does a full resend to missing devices, but cache invalidation is sufficient — the next message reaches everyone.
d1d5fc7 to
344ba72
Compare
1 participant
Summary
When sending a group message, the server ack includes a
phash(participant list hash). If it differs from ours, the participant/device list changed during the send — some devices may not have received the message. Previously we never read the server ack, so these mismatches were silently lost.How it works
Zero latency impact —
send_message_implandsend_status_messagereturn immediately after writing to the socket, same as before. The phash validation happens in a detached background task.What changes on mismatch
sender_key_device_cacheinvalidated → SKDM targets recomputed on next sendThis matches whatsmeow's approach (
send.go:448-464). WA Web does a full resend to missing devices (Resend/GroupMsg.js), but cache invalidation is sufficient — the next message reaches everyone.Files changed
src/client.rs—register_ack_waiter(): registers oneshot waiter for server ack by message IDsrc/send.rs—send_message_impl()andsend_status_message(): register waiter before send, spawn background phash validationTest plan
cargo test -p whatsapp-rust— 345 tests passSummary by CodeRabbit