fix(send): align own devices to LID namespace for LID-addressed DMs - #636
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds LID-aware device namespace normalization to the 1:1 DM fanout path. It detects whether the recipient is a LID from their bare encryption JID, then conditionally remaps the bot's own device entries in the fanout set from PN-form to LID-form device JIDs to keep device namespaces consistent and avoid server ACK failures. ChangesDM Fanout LID Device Normalization
Possibly related PRs
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Hey @jlucaso1 , would appreciate a review when you have a moment! This fixes ACK 400 on DM sends to LID-addressed recipients. Fix: after the sender-device retain filter, convert any remaining own PN devices to their LID equivalents when |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/device_registry.rs (1)
367-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop cross-joining aliases with namespaces.
This now builds a cartesian product of
{lid, pn} × {Lid, Pn}. For a known mapping that means you also delete sessions for impossible JIDs likepn@lidandlid@s.whatsapp.net, which can wipe unrelated sessions once the new ACK-400 path calls this helper.Suggested fix
pub(crate) async fn delete_sessions_for_devices(&self, user: &str, device_ids: &[u16]) { let lookup = self.resolve_lookup_keys(user).await; - let servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn]; - for server in servers { - for key in lookup.all_keys() { - for &device_id in device_ids { - let mut jid = Jid::new(key, server); - jid.device = device_id; - let addr = wacore::types::jid::JidExt::to_protocol_address(&jid); - self.signal_cache.delete_session(&addr).await; - } - } - } + let aliases = match &lookup { + UserLookupKeys::LidWithPn { lid, pn } | UserLookupKeys::PnWithLid { lid, pn } => { + vec![ + Jid::new(lid.as_str(), wacore_binary::Server::Lid), + Jid::new(pn.as_str(), wacore_binary::Server::Pn), + ] + } + UserLookupKeys::Unknown { user } => vec![ + Jid::new(user.as_str(), wacore_binary::Server::Lid), + Jid::new(user.as_str(), wacore_binary::Server::Pn), + ], + }; + for mut jid in aliases { + for &device_id in device_ids { + jid.device = device_id; + let addr = wacore::types::jid::JidExt::to_protocol_address(&jid); + self.signal_cache.delete_session(&addr).await; + } + } self.flush_signal_cache_logged("delete_sessions_for_devices", None) .await; }🤖 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 `@src/client/device_registry.rs` around lines 367 - 379, The current loop in resolve_lookup_keys -> delete_sessions_for_devices builds a cartesian product by combining servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn] with lookup.all_keys(), producing impossible JIDs (e.g., pn@lid); fix by only pairing each lookup key with its matching namespace/server instead of iterating all servers: use the namespace/server information from the lookup key (or a function on the key) when constructing the Jid (where Jid::new is called) and remove the outer servers array loop so delete_session is only called for valid JIDs generated for each key and device_id; update calls around resolve_lookup_keys, Jid::new, and signal_cache.delete_session accordingly.
🤖 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 `@src/send.rs`:
- Around line 1328-1340: The code may still produce a mixed PN/LID participant
list because when recipient_is_lid is true but own_lid (device_snapshot.lid) is
None, the loop that converts PN own devices in all_dm_jids is skipped; update
the logic so we never send mixed namespaces: either (a) resolve/obtain the
sender LID before building all_dm_jids (move or call the resolution that sets
device_snapshot.lid / own_lid earlier), or (b) fail fast when recipient_is_lid
is true and own_lid is None by returning an error (or propagating a clear
failure) instead of continuing; reference the recipient_is_lid check,
own_lid/device_snapshot.lid, own_cached/own_jid, and all_dm_jids so the fix
consistently converts PN own devices to Jid::lid_device or aborts before
sending.
---
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 367-379: The current loop in resolve_lookup_keys ->
delete_sessions_for_devices builds a cartesian product by combining servers =
[wacore_binary::Server::Lid, wacore_binary::Server::Pn] with lookup.all_keys(),
producing impossible JIDs (e.g., pn@lid); fix by only pairing each lookup key
with its matching namespace/server instead of iterating all servers: use the
namespace/server information from the lookup key (or a function on the key) when
constructing the Jid (where Jid::new is called) and remove the outer servers
array loop so delete_session is only called for valid JIDs generated for each
key and device_id; update calls around resolve_lookup_keys, Jid::new, and
signal_cache.delete_session accordingly.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71d3519d-f0a8-4c54-b0ec-32c6a716d7f6
📒 Files selected for processing (2)
src/client/device_registry.rssrc/send.rs
|
Thanks for the contribution.
This appears to be hacky, do you have any evidence/log that this is the correct approch? Some whatsmeow or whatsapp web source code? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/device_registry.rs (1)
252-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis public API is more destructive than its name says.
invalidate_device_cache()now sits on the publicClientsurface, but it also deletes persisted device rows. That’s not “cache invalidation” anymore; downstream callers can trigger a full registry reset by accident. Keep this internal, or rename/re-doc it so the destructive behavior is explicit before you publish it.🤖 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 `@src/client/device_registry.rs` around lines 252 - 264, The public method invalidate_device_cache is misleading because it also deletes persisted device rows (calls persistence_manager.backend().delete_devices) — either make it non-public/internal or rename and document the destructive behavior; specifically, change the visibility of invalidate_device_cache (and any public Client surface exposing it) to private/internal OR rename it to something explicit like clear_device_registry_persisted (and update its docstring) so callers understand it not only invalidates device_registry_cache.invalidate but also removes DB rows via persistence_manager.backend().delete_devices, and update any call sites or API docs that expose resolve_lookup_keys behavior accordingly.
🤖 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.
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 252-264: The public method invalidate_device_cache is misleading
because it also deletes persisted device rows (calls
persistence_manager.backend().delete_devices) — either make it
non-public/internal or rename and document the destructive behavior;
specifically, change the visibility of invalidate_device_cache (and any public
Client surface exposing it) to private/internal OR rename it to something
explicit like clear_device_registry_persisted (and update its docstring) so
callers understand it not only invalidates device_registry_cache.invalidate but
also removes DB rows via persistence_manager.backend().delete_devices, and
update any call sites or API docs that expose resolve_lookup_keys behavior
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b16ba9b1-3391-4d25-93ab-15dabe56c1cf
📒 Files selected for processing (2)
src/client/device_registry.rssrc/send.rs
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 `@src/send.rs`:
- Around line 765-780: The 400-path currently infers a DM from jid causing
status-reaction sends (rewritten into the author JID in send_message_impl) to
trigger DM cleanup; change spawn_phash_validation() to accept an explicit
"real_dm: bool" parameter, update all call sites so only genuine 1:1 DM sends
pass true (status sends/reactions must pass false), and replace the jid-based DM
check in the ACK handling (the block referencing ack_node, jid, message_id, and
client.* calls) to use the new real_dm flag instead of inferring from jid.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54310530-d4ae-4ee9-aa2d-4c9145de6af8
📒 Files selected for processing (2)
src/client/device_registry.rssrc/send.rs
Thank you for your time @jlucaso1 . Fair point, the spawn_phash_validation ACK 400 branch (session clear + cache invalidate) is not backed by whatsmeow or WA Web source code. Whatsmeow (send.go:445-446) simply wraps the error and returns it to the caller, no session cleanup at all on ACK 400. There is no captured WA Web JS available to cross-reference either. I'm happy to drop the ACK 400 session-clearing block if you'd prefer to keep the diff minimal and evidence-based. The phash mismatch handler below it (cache invalidation) is closer to what whatsmeow does (send.go:460-462) and could stand on its own. |
…nout exclusion Three related fixes for ACK 400 on DM sends: src/client/device_registry.rs — visibility widening (no logic change): - invalidate_device_cache: pub(crate) → pub Required by downstream crates that need to call this directly. - delete_sessions_for_devices: private → pub(crate) Required by the ACK 400 handler below. src/send.rs — three additions in send_message_impl: 1. Exclude own device 0 from all DM fanouts. Device 0 (primary phone) receives sent messages via the sync protocol, not direct E2E. Including own device 0 in the fanout causes ACK 400 from the server regardless of whether the recipient is self or another user. The previous filter only excluded it for self-DMs; this filter checks user identity so it applies to every outbound DM. 2. LID namespace conversion for own PN devices. own_cached is fetched via the bot's PN JID and always returns PN-format JIDs. When the recipient is LID-addressed, including a PN participant (e.g. 628xxx@s.whatsapp.net) in the stanza causes the server to reject the entire stanza with ACK error=400 — the server requires all participants to share the same JID namespace. Fix: after the sender-device and device-0 retain filters, convert any remaining own PN devices to their LID equivalents via Jid::lid_device so the stanza is uniformly LID-addressed. 3. ACK 400 defensive handler. When a DM ACK arrives with error="400", clears Signal sessions for own device 0 and invalidates the device cache. Does not retry — a blind retry with the same fanout produces an infinite loop. Fixes 1 and 2 are the primary prevention; this handler cleans up any residual stale state.
Two independent fixes in maybe_include_tc_token: 1. Remove the is_self guard. The guard used user-equality to detect own-account DMs and skipped token issuance entirely. This was too broad: when the bot's LID matched the admin's chat LID (e.g. after a companion re-pair), the guard fired for legitimate outbound messages and no TC token was attached, causing the server to return 463 indefinitely. Fix: remove the guard. Status broadcast and bot JIDs are already excluded by the check immediately below; own-account sends do not need a separate early return. 2. Default the AB prop to true. ab_props is an in-memory-only cache populated by the server's delta fetch on connection. On a fresh start the cache is empty, so is_enabled_or(PRIVACY_TOKEN_ON_ALL_1_ON_1_MESSAGES, false) returned false and no tokens were ever attached until the server pushed the prop — which may not happen on every connect cycle. Fix: default to true so tokens are attached immediately on startup. The AB prop can still disable sending if the server explicitly sets the flag to false.
|
I'm hitting LID <> PN problems with latest from |
The LID namespace conversion alone resolves ACK 400 on LID-addressed DMs: own devices arrive PN-addressed (registry keyed by the bot's PN) and the server rejects stanzas mixing PN and LID participants. This mirrors whatsmeow switching ownID to LID before fanout. Drop the rest as non-WA-Web-faithful: - own device-0 fanout exclusion: getFanOutList only excludes the current sender device, never the primary; excluding it breaks DeviceSentMessage sync to the primary phone. - ACK error=400 session-clear handler: no basis in whatsmeow/WA Web; the namespace fix prevents the 400 in the first place. - tc-token changes: the AB-prop default true diverges from WA Web (default is false); the perpetual 463 is a separate ab_props-population issue. - device_registry visibility widening: only the removed handler needed it.
|
Thanks for digging into this, your root cause is right. Own devices come back PN addressed and the server rejects a stanza that mixes PN and LID participants, so the LID conversion is the correct fix and it matches whatsmeow switching ownID to LID before fanout. I pushed a commit trimming the PR down to just that conversion. I dropped the rest after checking against WA Web and whatsmeow:
Build, clippy and the unit tests are green. Let me know if you disagree on any of these. |
jlucaso1
changed the title
Backport the focused namespace fixes from upstream oxidezap#636 and oxidezap#731. Resolve a mapped peer to LID for the entire DM stanza, align own companion devices to that namespace, correct reporting-token sender identity, and surface otherwise-hidden server nacks. Add regressions for PN behavior, missing own-LID state, companion alignment, and the complete outgoing LID stanza.
3 participants
Fixes ACK 400 on DMs to LID-addressed recipients.
Root cause: own devices are fetched via the bot's PN JID, so they come back PN addressed (628xxx@s.whatsapp.net). The server rejects a stanza that mixes PN and LID participants, so including a PN own device in a LID stanza gets the whole stanza rejected with ACK error 400.
Fix: when the recipient is LID, convert the remaining own PN devices to their LID equivalents before dedup, so the stanza is uniformly LID addressed. This matches whatsmeow, which switches ownID to LID before GetUserDevices so own devices come back in the right namespace.