From 5384cea2d9b4487b9fb117dc02edfc28e614b36c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 17 Mar 2026 18:29:47 -0300 Subject: [PATCH 1/3] perf: granular cache patching instead of invalidate+refetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace cache invalidation with in-place patching for device and group notifications, matching WhatsApp Web's addParticipantInfo / removeParticipantInfo / bulkCreateOrReplace patterns. Device notifications (type="devices"): - Add: append device to cached Vec + DeviceListRecord - Remove: filter out device from both caches - Update: update key_index in DeviceListRecord Group notifications (type="w:gp2"): - Add: extend GroupInfo.participants + update LID-PN maps - Remove: filter GroupInfo.participants + clean LID-PN maps Client-initiated (Groups::add/remove_participants): - Patch cached GroupInfo inline instead of invalidating All patches are no-ops if the cache entry doesn't exist — the next read fetches fresh from the backend. Zero risk of stale data since the notification carries the exact diff. Eliminates a full IQ round-trip on every participant/device change. --- src/client/device_registry.rs | 286 ++++++++++++++++++++++++++++++++++ src/features/groups.rs | 16 +- src/handlers/notification.rs | 86 ++++++++-- wacore/src/client/context.rs | 123 +++++++++++++++ 4 files changed, 492 insertions(+), 19 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 9fa7a1d69..49537f2de 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -206,6 +206,103 @@ impl Client { debug!("Invalidated device cache for user: {} ({:?})", user, lookup); } + /// Granularly patch device caches after a device notification. + /// + /// Matches WA Web's approach: read current → apply diff → write back. + /// If the entry is not cached, the patch is a no-op — the next read + /// will fetch fresh from the backend. + pub(crate) async fn patch_device_add( + &self, + user: &str, + from_jid: &Jid, + device: &wacore::stanza::devices::DeviceElement, + ) { + let device_id = device.device_id(); + let device_jid = Jid { + user: from_jid.user.clone(), + server: from_jid.server.clone(), + device: device_id as u16, + ..Default::default() + }; + + // Patch device_cache (Vec) + let device_cache = self.get_device_cache().await; + let non_ad = from_jid.to_non_ad(); + if let Some(mut devices) = device_cache.get(&non_ad).await + && !devices.iter().any(|d| d.device == device_jid.device) + { + devices.push(device_jid); + device_cache.insert(non_ad, devices).await; + } + + // Patch device_registry_cache (DeviceListRecord) + let lookup = self.resolve_lookup_keys(user).await; + for key in lookup.all_keys() { + if let Some(mut record) = self.device_registry_cache.get(key).await { + if !record.devices.iter().any(|d| d.device_id == device_id) { + record.devices.push(wacore::store::traits::DeviceInfo { + device_id, + key_index: device.key_index, + }); + self.device_registry_cache + .insert(key.to_string(), record) + .await; + } + return; + } + } + } + + /// Remove a device from both caches after a device remove notification. + pub(crate) async fn patch_device_remove(&self, user: &str, from_jid: &Jid, device_id: u32) { + // Patch device_cache + let device_cache = self.get_device_cache().await; + let non_ad = from_jid.to_non_ad(); + if let Some(mut devices) = device_cache.get(&non_ad).await { + let before = devices.len(); + devices.retain(|d| d.device != device_id as u16); + if devices.len() != before { + device_cache.insert(non_ad, devices).await; + } + } + + // Patch device_registry_cache + let lookup = self.resolve_lookup_keys(user).await; + for key in lookup.all_keys() { + if let Some(mut record) = self.device_registry_cache.get(key).await { + let before = record.devices.len(); + record.devices.retain(|d| d.device_id != device_id); + if record.devices.len() != before { + self.device_registry_cache + .insert(key.to_string(), record) + .await; + } + return; + } + } + } + + /// Update key_index for a device in the registry cache. + pub(crate) async fn patch_device_update( + &self, + user: &str, + device: &wacore::stanza::devices::DeviceElement, + ) { + let device_id = device.device_id(); + let lookup = self.resolve_lookup_keys(user).await; + for key in lookup.all_keys() { + if let Some(mut record) = self.device_registry_cache.get(key).await { + if let Some(d) = record.devices.iter_mut().find(|d| d.device_id == device_id) { + d.key_index = device.key_index; + self.device_registry_cache + .insert(key.to_string(), record) + .await; + } + return; + } + } + } + /// Background loop placeholder for device registry cleanup. /// Note: Cleanup functionality was removed as part of trait simplification. /// Device registry entries are managed through normal update/get operations. @@ -582,4 +679,193 @@ mod tests { "Device cache should be invalidated for PN JID (unknown PN user)" ); } + + // ── Granular patch tests ────────────────────────────────────────────── + + fn make_device_element( + device_id: u16, + key_index: Option, + ) -> wacore::stanza::devices::DeviceElement { + wacore::stanza::devices::DeviceElement { + jid: Jid { + user: "15551234567".into(), + server: "s.whatsapp.net".into(), + device: device_id, + ..Default::default() + }, + key_index, + lid: None, + } + } + + #[tokio::test] + async fn test_patch_device_add_to_existing_cache() { + let client = create_test_client().await; + let from_jid = Jid::pn("15551234567"); + let non_ad = from_jid.to_non_ad(); + + // Pre-populate device_cache with device 0 + let device_cache = client.get_device_cache().await; + let dev0 = Jid { + user: "15551234567".into(), + server: "s.whatsapp.net".into(), + device: 0, + ..Default::default() + }; + device_cache.insert(non_ad.clone(), vec![dev0]).await; + + // Patch: add device 3 + let elem = make_device_element(3, Some(5)); + client + .patch_device_add("15551234567", &from_jid, &elem) + .await; + + let devices = device_cache.get(&non_ad).await.unwrap(); + assert_eq!(devices.len(), 2); + assert!(devices.iter().any(|d| d.device == 3)); + } + + #[tokio::test] + async fn test_patch_device_add_deduplicates() { + let client = create_test_client().await; + let from_jid = Jid::pn("15551234567"); + let non_ad = from_jid.to_non_ad(); + + let dev3 = Jid { + user: "15551234567".into(), + server: "s.whatsapp.net".into(), + device: 3, + ..Default::default() + }; + let device_cache = client.get_device_cache().await; + device_cache.insert(non_ad.clone(), vec![dev3]).await; + + // Patch: add device 3 again — should not duplicate + let elem = make_device_element(3, None); + client + .patch_device_add("15551234567", &from_jid, &elem) + .await; + + let devices = device_cache.get(&non_ad).await.unwrap(); + assert_eq!(devices.len(), 1); + } + + #[tokio::test] + async fn test_patch_device_add_noop_on_miss() { + let client = create_test_client().await; + let from_jid = Jid::pn("15551234567"); + + // No pre-populated cache — patch should be a no-op + let elem = make_device_element(3, None); + client + .patch_device_add("15551234567", &from_jid, &elem) + .await; + + let device_cache = client.get_device_cache().await; + assert!(device_cache.get(&from_jid.to_non_ad()).await.is_none()); + } + + #[tokio::test] + async fn test_patch_device_remove() { + let client = create_test_client().await; + let from_jid = Jid::pn("15551234567"); + let non_ad = from_jid.to_non_ad(); + + let dev0 = Jid { + user: "15551234567".into(), + server: "s.whatsapp.net".into(), + device: 0, + ..Default::default() + }; + let dev3 = Jid { + user: "15551234567".into(), + server: "s.whatsapp.net".into(), + device: 3, + ..Default::default() + }; + let device_cache = client.get_device_cache().await; + device_cache.insert(non_ad.clone(), vec![dev0, dev3]).await; + + client + .patch_device_remove("15551234567", &from_jid, 3) + .await; + + let devices = device_cache.get(&non_ad).await.unwrap(); + assert_eq!(devices.len(), 1); + assert_eq!(devices[0].device, 0); + } + + #[tokio::test] + async fn test_patch_device_update_key_index() { + let client = create_test_client().await; + + // Pre-populate registry cache + let record = wacore::store::traits::DeviceListRecord { + user: "15551234567".to_string(), + devices: vec![ + wacore::store::traits::DeviceInfo { + device_id: 0, + key_index: None, + }, + wacore::store::traits::DeviceInfo { + device_id: 3, + key_index: Some(1), + }, + ], + timestamp: 1000, + phash: None, + }; + client + .device_registry_cache + .insert("15551234567".to_string(), record) + .await; + + // Patch: update device 3 key_index to 5 + let elem = make_device_element(3, Some(5)); + client.patch_device_update("15551234567", &elem).await; + + let updated = client + .device_registry_cache + .get("15551234567") + .await + .unwrap(); + let dev3 = updated.devices.iter().find(|d| d.device_id == 3).unwrap(); + assert_eq!(dev3.key_index, Some(5)); + } + + #[tokio::test] + async fn test_patch_device_add_updates_registry() { + let client = create_test_client().await; + let from_jid = Jid::pn("15551234567"); + + // Pre-populate registry cache + let record = wacore::store::traits::DeviceListRecord { + user: "15551234567".to_string(), + devices: vec![wacore::store::traits::DeviceInfo { + device_id: 0, + key_index: None, + }], + timestamp: 1000, + phash: None, + }; + client + .device_registry_cache + .insert("15551234567".to_string(), record) + .await; + + // Patch: add device 3 + let elem = make_device_element(3, Some(2)); + client + .patch_device_add("15551234567", &from_jid, &elem) + .await; + + let updated = client + .device_registry_cache + .get("15551234567") + .await + .unwrap(); + assert_eq!(updated.devices.len(), 2); + let dev3 = updated.devices.iter().find(|d| d.device_id == 3).unwrap(); + assert_eq!(dev3.key_index, Some(2)); + } } diff --git a/src/features/groups.rs b/src/features/groups.rs index 133037453..505fccd36 100644 --- a/src/features/groups.rs +++ b/src/features/groups.rs @@ -231,7 +231,13 @@ impl<'a> Groups<'a> { .client .execute(AddParticipantsIq::new(jid, participants)) .await?; - self.client.get_group_cache().await.invalidate(jid).await; + // Patch cache with the participants we just added (no phone_number known here) + let group_cache = self.client.get_group_cache().await; + if let Some(mut info) = group_cache.get(jid).await { + let new: Vec<_> = participants.iter().map(|p| (p.clone(), None)).collect(); + info.add_participants(&new); + group_cache.insert(jid.clone(), info).await; + } Ok(result) } @@ -244,7 +250,13 @@ impl<'a> Groups<'a> { .client .execute(RemoveParticipantsIq::new(jid, participants)) .await?; - self.client.get_group_cache().await.invalidate(jid).await; + // Patch cache by filtering out removed participants + let group_cache = self.client.get_group_cache().await; + if let Some(mut info) = group_cache.get(jid).await { + let users: Vec<&str> = participants.iter().map(|p| p.user.as_str()).collect(); + info.remove_participants(&users); + group_cache.insert(jid.clone(), info).await; + } Ok(result) } diff --git a/src/handlers/notification.rs b/src/handlers/notification.rs index b265a01a5..d6348e4ca 100644 --- a/src/handlers/notification.rs +++ b/src/handlers/notification.rs @@ -312,7 +312,9 @@ async fn handle_devices_notification(client: &Arc, node: &Node) { warn!("Failed to add LID-PN mapping from device notification: {e}"); } - // Process the single operation (per WhatsApp Web: one operation per notification) + // Process the single operation (per WhatsApp Web: one operation per notification). + // Granularly patch caches instead of invalidating — matches WA Web's + // bulkCreateOrReplace pattern and avoids a usync IQ round-trip. let op = ¬ification.operation; debug!( "Device notification: user={}, type={:?}, devices={:?}", @@ -321,7 +323,33 @@ async fn handle_devices_notification(client: &Arc, node: &Node) { op.device_ids() ); - client.invalidate_device_cache(notification.user()).await; + match op.operation_type { + wacore::stanza::devices::DeviceNotificationType::Add => { + for device in &op.devices { + client + .patch_device_add(notification.user(), ¬ification.from, device) + .await; + } + } + wacore::stanza::devices::DeviceNotificationType::Remove => { + for device in &op.devices { + client + .patch_device_remove( + notification.user(), + ¬ification.from, + device.device_id(), + ) + .await; + } + } + wacore::stanza::devices::DeviceNotificationType::Update => { + for device in &op.devices { + client + .patch_device_update(notification.user(), device) + .await; + } + } + } // Dispatch event to notify application layer let event = Event::DeviceListUpdate(DeviceListUpdate { @@ -1008,21 +1036,45 @@ async fn handle_group_notification(client: &Arc, node: &Node) { .unwrap_or_else(chrono::Utc::now); for action in notification.actions { - // Cache invalidation for participant list changes - if matches!( - action, - GroupNotificationAction::Add { .. } | GroupNotificationAction::Remove { .. } - ) { - client - .get_group_cache() - .await - .invalidate(¬ification.group_jid) - .await; - debug!( - target: "Client/Group", - "Invalidated group cache for {} after participant change", - notification.group_jid - ); + // Granularly patch group cache instead of invalidating — matches WA Web's + // addParticipantInfo / removeParticipantInfo pattern and avoids a + // group metadata IQ round-trip. + match &action { + GroupNotificationAction::Add { participants, .. } => { + let group_cache = client.get_group_cache().await; + if let Some(mut info) = group_cache.get(¬ification.group_jid).await { + let new: Vec<_> = participants + .iter() + .map(|p| (p.jid.clone(), p.phone_number.clone())) + .collect(); + info.add_participants(&new); + group_cache + .insert(notification.group_jid.clone(), info) + .await; + debug!( + target: "Client/Group", + "Patched group cache for {}: added {} participants", + notification.group_jid, participants.len() + ); + } + } + GroupNotificationAction::Remove { participants, .. } => { + let group_cache = client.get_group_cache().await; + if let Some(mut info) = group_cache.get(¬ification.group_jid).await { + let users: Vec<&str> = + participants.iter().map(|p| p.jid.user.as_str()).collect(); + info.remove_participants(&users); + group_cache + .insert(notification.group_jid.clone(), info) + .await; + debug!( + target: "Client/Group", + "Patched group cache for {}: removed {} participants", + notification.group_jid, participants.len() + ); + } + } + _ => {} } debug!( diff --git a/wacore/src/client/context.rs b/wacore/src/client/context.rs index 5e9017f8d..8dc8f6ddc 100644 --- a/wacore/src/client/context.rs +++ b/wacore/src/client/context.rs @@ -79,6 +79,43 @@ impl GroupInfo { self.pn_to_lid_map.get(phone_user) } + /// Append participants that are not already present. + /// + /// For LID-addressed groups, also updates the LID-to-PN and PN-to-LID maps + /// using the `phone_number` field from each participant. + pub fn add_participants(&mut self, new: &[(Jid, Option)]) { + for (jid, phone_number) in new { + if self.participants.iter().any(|p| p.user == jid.user) { + continue; + } + self.participants.push(jid.clone()); + if self.addressing_mode == AddressingMode::Lid + && let Some(pn) = phone_number + { + self.pn_to_lid_map + .insert(pn.user.clone(), Jid::lid(&jid.user)); + self.lid_to_pn_map.insert(jid.user.clone(), pn.clone()); + } + } + } + + /// Remove participants whose user part is in `users_to_remove`. + /// + /// Also cleans up the LID-to-PN and PN-to-LID maps. + pub fn remove_participants(&mut self, users_to_remove: &[&str]) { + self.participants + .retain(|p| !users_to_remove.iter().any(|u| *u == p.user)); + for user in users_to_remove { + if let Some(pn_jid) = self.lid_to_pn_map.remove(*user) { + self.pn_to_lid_map.remove(&pn_jid.user); + } + // Also try reverse: user might be a PN + if let Some(lid_jid) = self.pn_to_lid_map.remove(*user) { + self.lid_to_pn_map.remove(&lid_jid.user); + } + } + } + /// Convert a phone-based device JID to a LID-based device JID using the mapping. /// If no mapping exists, returns the original JID unchanged. pub fn phone_device_jid_to_lid(&self, phone_device_jid: &Jid) -> Jid { @@ -118,3 +155,89 @@ pub trait SendContextResolver: Send + Sync { None } } + +#[cfg(test)] +mod tests { + use super::*; + + fn pn(user: &str) -> Jid { + Jid::pn(user) + } + fn lid(user: &str) -> Jid { + Jid::lid(user) + } + + #[test] + fn add_participants_pn_mode() { + let mut info = GroupInfo::new(vec![pn("alice")], AddressingMode::Pn); + info.add_participants(&[(pn("bob"), None), (pn("carol"), None)]); + assert_eq!(info.participants.len(), 3); + assert!(info.participants.iter().any(|p| p.user == "bob")); + } + + #[test] + fn add_participants_deduplicates() { + let mut info = GroupInfo::new(vec![pn("alice"), pn("bob")], AddressingMode::Pn); + info.add_participants(&[(pn("bob"), None), (pn("carol"), None)]); + assert_eq!(info.participants.len(), 3); // bob not duplicated + } + + #[test] + fn add_participants_lid_mode_updates_maps() { + let mut info = GroupInfo::new(vec![lid("lid_alice")], AddressingMode::Lid); + info.add_participants(&[(lid("lid_bob"), Some(pn("bob_pn")))]); + + assert_eq!(info.participants.len(), 2); + assert_eq!( + info.phone_jid_for_lid_user("lid_bob") + .map(|j| j.user.as_str()), + Some("bob_pn") + ); + assert_eq!( + info.lid_jid_for_phone_user("bob_pn") + .map(|j| j.user.as_str()), + Some("lid_bob") + ); + } + + #[test] + fn remove_participants_basic() { + let mut info = GroupInfo::new( + vec![pn("alice"), pn("bob"), pn("carol")], + AddressingMode::Pn, + ); + info.remove_participants(&["bob"]); + assert_eq!(info.participants.len(), 2); + assert!(!info.participants.iter().any(|p| p.user == "bob")); + } + + #[test] + fn remove_participants_cleans_lid_maps() { + let lid_to_pn = HashMap::from([ + ("lid_alice".to_string(), pn("alice_pn")), + ("lid_bob".to_string(), pn("bob_pn")), + ]); + let mut info = GroupInfo::with_lid_to_pn_map( + vec![lid("lid_alice"), lid("lid_bob")], + AddressingMode::Lid, + lid_to_pn, + ); + + assert!(info.phone_jid_for_lid_user("lid_bob").is_some()); + assert!(info.lid_jid_for_phone_user("bob_pn").is_some()); + + info.remove_participants(&["lid_bob"]); + + assert_eq!(info.participants.len(), 1); + assert!(info.phone_jid_for_lid_user("lid_bob").is_none()); + assert!(info.lid_jid_for_phone_user("bob_pn").is_none()); + assert!(info.phone_jid_for_lid_user("lid_alice").is_some()); + } + + #[test] + fn remove_nonexistent_is_noop() { + let mut info = GroupInfo::new(vec![pn("alice")], AddressingMode::Pn); + info.remove_participants(&["nobody"]); + assert_eq!(info.participants.len(), 1); + } +} From 564a17a465a6b42d9bbc0df8a7d71c8a9acf24bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 17 Mar 2026 18:59:19 -0300 Subject: [PATCH 2/3] fix: address review feedback from CodeRabbit and Claude MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Patch all PN/LID aliases in device_cache via jids_for_lookup helper, not just from_jid.to_non_ad() (CodeRabbit) - Persist patched DeviceListRecords to backend via update_device_list so changes survive cache eviction/restart (CodeRabbit) - Fall back to invalidate on hash-only device Update notifications where op.devices is empty (CodeRabbit) - Backfill LID-PN maps even for existing participants: move map update before the dedup check so None→Some(phone) fills correctly when server notification follows client-initiated add (CodeRabbit) - Document get→mutate→insert race in group cache patching as acceptable for a best-effort cache (CodeRabbit) - Add regression test for LID map backfill case (CodeRabbit) --- src/client/device_registry.rs | 95 +++++++++++++++++++++-------------- src/features/groups.rs | 9 +++- src/handlers/notification.rs | 14 ++++-- wacore/src/client/context.rs | 38 ++++++++++++-- 4 files changed, 108 insertions(+), 48 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 49537f2de..58cc7752c 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -209,8 +209,12 @@ impl Client { /// Granularly patch device caches after a device notification. /// /// Matches WA Web's approach: read current → apply diff → write back. - /// If the entry is not cached, the patch is a no-op — the next read - /// will fetch fresh from the backend. + /// Patches **all** cached PN/LID aliases so stale alternate-key lookups + /// are avoided. Also persists the patched `DeviceListRecord` to the + /// backend so the change survives cache eviction / restart. + /// + /// If no entry is cached, the patch is a no-op — the next read will + /// fetch fresh from the backend. pub(crate) async fn patch_device_add( &self, user: &str, @@ -218,25 +222,26 @@ impl Client { device: &wacore::stanza::devices::DeviceElement, ) { let device_id = device.device_id(); - let device_jid = Jid { - user: from_jid.user.clone(), - server: from_jid.server.clone(), - device: device_id as u16, - ..Default::default() - }; + let device_hw = device_id as u16; - // Patch device_cache (Vec) + // Patch device_cache — all PN/LID aliases let device_cache = self.get_device_cache().await; - let non_ad = from_jid.to_non_ad(); - if let Some(mut devices) = device_cache.get(&non_ad).await - && !devices.iter().any(|d| d.device == device_jid.device) - { - devices.push(device_jid); - device_cache.insert(non_ad, devices).await; + let lookup = self.resolve_lookup_keys(user).await; + for jid in self.jids_for_lookup(&lookup, from_jid) { + if let Some(mut devices) = device_cache.get(&jid).await + && !devices.iter().any(|d| d.device == device_hw) + { + devices.push(Jid { + user: jid.user.clone(), + server: jid.server.clone(), + device: device_hw, + ..Default::default() + }); + device_cache.insert(jid, devices).await; + } } - // Patch device_registry_cache (DeviceListRecord) - let lookup = self.resolve_lookup_keys(user).await; + // Patch device_registry_cache + persist for key in lookup.all_keys() { if let Some(mut record) = self.device_registry_cache.get(key).await { if !record.devices.iter().any(|d| d.device_id == device_id) { @@ -244,9 +249,9 @@ impl Client { device_id, key_index: device.key_index, }); - self.device_registry_cache - .insert(key.to_string(), record) - .await; + if let Err(e) = self.update_device_list(record).await { + warn!("patch_device_add: failed to persist: {e}"); + } } return; } @@ -255,34 +260,37 @@ impl Client { /// Remove a device from both caches after a device remove notification. pub(crate) async fn patch_device_remove(&self, user: &str, from_jid: &Jid, device_id: u32) { - // Patch device_cache + let device_hw = device_id as u16; + + // Patch device_cache — all PN/LID aliases let device_cache = self.get_device_cache().await; - let non_ad = from_jid.to_non_ad(); - if let Some(mut devices) = device_cache.get(&non_ad).await { - let before = devices.len(); - devices.retain(|d| d.device != device_id as u16); - if devices.len() != before { - device_cache.insert(non_ad, devices).await; + let lookup = self.resolve_lookup_keys(user).await; + for jid in self.jids_for_lookup(&lookup, from_jid) { + if let Some(mut devices) = device_cache.get(&jid).await { + let before = devices.len(); + devices.retain(|d| d.device != device_hw); + if devices.len() != before { + device_cache.insert(jid, devices).await; + } } } - // Patch device_registry_cache - let lookup = self.resolve_lookup_keys(user).await; + // Patch device_registry_cache + persist for key in lookup.all_keys() { if let Some(mut record) = self.device_registry_cache.get(key).await { let before = record.devices.len(); record.devices.retain(|d| d.device_id != device_id); - if record.devices.len() != before { - self.device_registry_cache - .insert(key.to_string(), record) - .await; + if record.devices.len() != before + && let Err(e) = self.update_device_list(record).await + { + warn!("patch_device_remove: failed to persist: {e}"); } return; } } } - /// Update key_index for a device in the registry cache. + /// Update key_index for a device in the registry cache + backend. pub(crate) async fn patch_device_update( &self, user: &str, @@ -294,15 +302,28 @@ impl Client { if let Some(mut record) = self.device_registry_cache.get(key).await { if let Some(d) = record.devices.iter_mut().find(|d| d.device_id == device_id) { d.key_index = device.key_index; - self.device_registry_cache - .insert(key.to_string(), record) - .await; + if let Err(e) = self.update_device_list(record).await { + warn!("patch_device_update: failed to persist: {e}"); + } } return; } } } + /// Resolve all JID forms (PN + LID) that might be cached in `device_cache`. + fn jids_for_lookup(&self, lookup: &UserLookupKeys, from_jid: &Jid) -> Vec { + match lookup { + UserLookupKeys::LidWithPn { lid, pn } | UserLookupKeys::PnWithLid { lid, pn } => { + vec![Jid::lid(lid), Jid::pn(pn)] + } + UserLookupKeys::Unknown { .. } => { + // Unknown — use from_jid's non-AD form only + vec![from_jid.to_non_ad()] + } + } + } + /// Background loop placeholder for device registry cleanup. /// Note: Cleanup functionality was removed as part of trait simplification. /// Device registry entries are managed through normal update/get operations. diff --git a/src/features/groups.rs b/src/features/groups.rs index 505fccd36..c79746b75 100644 --- a/src/features/groups.rs +++ b/src/features/groups.rs @@ -231,7 +231,11 @@ impl<'a> Groups<'a> { .client .execute(AddParticipantsIq::new(jid, participants)) .await?; - // Patch cache with the participants we just added (no phone_number known here) + // Patch cache with the participants we just added (no phone_number + // known here — the server notification will backfill the LID map). + // Note: the get→mutate→insert is not atomic; a concurrent notification + // for the same group could race. This is acceptable — the cache is + // best-effort and a full refetch on next query_info() corrects it. let group_cache = self.client.get_group_cache().await; if let Some(mut info) = group_cache.get(jid).await { let new: Vec<_> = participants.iter().map(|p| (p.clone(), None)).collect(); @@ -250,7 +254,8 @@ impl<'a> Groups<'a> { .client .execute(RemoveParticipantsIq::new(jid, participants)) .await?; - // Patch cache by filtering out removed participants + // Patch cache by filtering out removed participants (see add_participants + // for race-condition note — same applies here). let group_cache = self.client.get_group_cache().await; if let Some(mut info) = group_cache.get(jid).await { let users: Vec<&str> = participants.iter().map(|p| p.user.as_str()).collect(); diff --git a/src/handlers/notification.rs b/src/handlers/notification.rs index d6348e4ca..c5533d825 100644 --- a/src/handlers/notification.rs +++ b/src/handlers/notification.rs @@ -343,10 +343,16 @@ async fn handle_devices_notification(client: &Arc, node: &Node) { } } wacore::stanza::devices::DeviceNotificationType::Update => { - for device in &op.devices { - client - .patch_device_update(notification.user(), device) - .await; + if op.devices.is_empty() { + // Hash-only update without device list — fall back to + // invalidation so the next read rehydrates from the server. + client.invalidate_device_cache(notification.user()).await; + } else { + for device in &op.devices { + client + .patch_device_update(notification.user(), device) + .await; + } } } } diff --git a/wacore/src/client/context.rs b/wacore/src/client/context.rs index 8dc8f6ddc..a748c39ac 100644 --- a/wacore/src/client/context.rs +++ b/wacore/src/client/context.rs @@ -82,13 +82,14 @@ impl GroupInfo { /// Append participants that are not already present. /// /// For LID-addressed groups, also updates the LID-to-PN and PN-to-LID maps - /// using the `phone_number` field from each participant. + /// using the `phone_number` field from each participant. Maps are updated + /// even for already-present participants so that a later call with + /// `Some(phone_number)` backfills a previous `None` entry. pub fn add_participants(&mut self, new: &[(Jid, Option)]) { for (jid, phone_number) in new { - if self.participants.iter().any(|p| p.user == jid.user) { - continue; - } - self.participants.push(jid.clone()); + // Always backfill LID maps — a re-add with phone_number fills a + // previous None (e.g., client-initiated add followed by server + // notification that carries the phone number). if self.addressing_mode == AddressingMode::Lid && let Some(pn) = phone_number { @@ -96,6 +97,11 @@ impl GroupInfo { .insert(pn.user.clone(), Jid::lid(&jid.user)); self.lid_to_pn_map.insert(jid.user.clone(), pn.clone()); } + + if self.participants.iter().any(|p| p.user == jid.user) { + continue; + } + self.participants.push(jid.clone()); } } @@ -240,4 +246,26 @@ mod tests { info.remove_participants(&["nobody"]); assert_eq!(info.participants.len(), 1); } + + #[test] + fn add_participants_backfills_lid_map_for_existing() { + let mut info = GroupInfo::new(vec![lid("lid_bob")], AddressingMode::Lid); + // First add without phone_number (simulates client-initiated add) + info.add_participants(&[(lid("lid_bob"), None)]); + assert!(info.phone_jid_for_lid_user("lid_bob").is_none()); + + // Second add with phone_number (simulates server notification backfill) + info.add_participants(&[(lid("lid_bob"), Some(pn("bob_pn")))]); + assert_eq!(info.participants.len(), 1); // not duplicated + assert_eq!( + info.phone_jid_for_lid_user("lid_bob") + .map(|j| j.user.as_str()), + Some("bob_pn") + ); + assert_eq!( + info.lid_jid_for_phone_user("bob_pn") + .map(|j| j.user.as_str()), + Some("lid_bob") + ); + } } From b3a40ad569ff45e317e1dfb4ee5445bb249d4735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Tue, 17 Mar 2026 19:02:53 -0300 Subject: [PATCH 3/3] fix: only patch group cache with server-accepted participants Filter ParticipantChangeResponse by status="200" before patching GroupInfo. Partial failures (403, 409) no longer leave the cache ahead of the authoritative group state. (CodeRabbit outside-diff) --- src/features/groups.rs | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/src/features/groups.rs b/src/features/groups.rs index c79746b75..3d767f46e 100644 --- a/src/features/groups.rs +++ b/src/features/groups.rs @@ -231,16 +231,21 @@ impl<'a> Groups<'a> { .client .execute(AddParticipantsIq::new(jid, participants)) .await?; - // Patch cache with the participants we just added (no phone_number - // known here — the server notification will backfill the LID map). + // Patch cache with only the participants the server accepted (status 200). // Note: the get→mutate→insert is not atomic; a concurrent notification // for the same group could race. This is acceptable — the cache is // best-effort and a full refetch on next query_info() corrects it. - let group_cache = self.client.get_group_cache().await; - if let Some(mut info) = group_cache.get(jid).await { - let new: Vec<_> = participants.iter().map(|p| (p.clone(), None)).collect(); - info.add_participants(&new); - group_cache.insert(jid.clone(), info).await; + let accepted: Vec<_> = result + .iter() + .filter(|r| r.status.as_deref() == Some("200")) + .map(|r| (r.jid.clone(), None)) + .collect(); + if !accepted.is_empty() { + let group_cache = self.client.get_group_cache().await; + if let Some(mut info) = group_cache.get(jid).await { + info.add_participants(&accepted); + group_cache.insert(jid.clone(), info).await; + } } Ok(result) } @@ -254,13 +259,18 @@ impl<'a> Groups<'a> { .client .execute(RemoveParticipantsIq::new(jid, participants)) .await?; - // Patch cache by filtering out removed participants (see add_participants - // for race-condition note — same applies here). - let group_cache = self.client.get_group_cache().await; - if let Some(mut info) = group_cache.get(jid).await { - let users: Vec<&str> = participants.iter().map(|p| p.user.as_str()).collect(); - info.remove_participants(&users); - group_cache.insert(jid.clone(), info).await; + // Patch cache with only the participants the server accepted. + let accepted: Vec<&str> = result + .iter() + .filter(|r| r.status.as_deref() == Some("200")) + .map(|r| r.jid.user.as_str()) + .collect(); + if !accepted.is_empty() { + let group_cache = self.client.get_group_cache().await; + if let Some(mut info) = group_cache.get(jid).await { + info.remove_participants(&accepted); + group_cache.insert(jid.clone(), info).await; + } } Ok(result) }