From f8422c8ce0b3fe2d48bb0cd4eab249eb65199616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 18 Apr 2026 01:02:14 -0300 Subject: [PATCH 1/2] fix: cache-aside fallback in get_lid_pn_entry Previously only queried the in-memory cache; mappings present only in the backend (after cache eviction, or when warm_up wasn't called) would return None even though the persistent data had the answer. Now falls back to `Backend::get_lid_mapping` / `get_pn_mapping` on cache miss and re-populates the cache. Any backend impl gains the fallback without additional wiring. --- src/client/lid_pn.rs | 78 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 70 insertions(+), 8 deletions(-) diff --git a/src/client/lid_pn.rs b/src/client/lid_pn.rs index ef35d6738..9a3479289 100644 --- a/src/client/lid_pn.rs +++ b/src/client/lid_pn.rs @@ -270,18 +270,46 @@ impl Client { } } - /// Look up the LID↔phone mapping for a JID. - /// - /// Routes automatically: LID JIDs search by LID, PN JIDs search by phone. - /// Returns `None` for non-user JIDs (groups, newsletters, etc.). + /// Look up the LID↔phone mapping for a JID. Cache-aside: falls back to + /// the backend on cache miss so mappings survive cache eviction and any + /// backend implementation gets the fallback without warm-up. pub async fn get_lid_pn_entry(&self, jid: &Jid) -> Option { - if jid.is_lid() { - self.lid_pn_cache.get_entry_by_lid(&jid.user).await + let (hit, is_lid) = if jid.is_lid() { + (self.lid_pn_cache.get_entry_by_lid(&jid.user).await, true) } else if jid.is_pn() { - self.lid_pn_cache.get_entry_by_phone(&jid.user).await + (self.lid_pn_cache.get_entry_by_phone(&jid.user).await, false) } else { - None + return None; + }; + + if let Some(entry) = hit { + return Some(entry); } + + let backend = self.persistence_manager.backend(); + let result = if is_lid { + backend.get_lid_mapping(&jid.user).await + } else { + backend.get_pn_mapping(&jid.user).await + }; + + let mapping = match result { + Ok(Some(m)) => m, + Ok(None) => return None, + Err(e) => { + debug!("LID-PN backend lookup failed for {jid}: {e:?}"); + return None; + } + }; + + let entry = LidPnEntry::with_timestamp( + mapping.lid, + mapping.phone_number, + mapping.created_at, + LearningSource::parse(&mapping.learning_source), + ); + self.lid_pn_cache.add(entry.clone()).await; + Some(entry) } } @@ -369,4 +397,38 @@ mod tests { assert_eq!(entry.lid, lid); assert_eq!(entry.phone_number, pn); } + + /// Cache-aside fallback: if the in-memory cache is missing an entry the + /// backend has, the lookup should still succeed and re-populate the cache. + #[tokio::test] + async fn test_get_lid_pn_entry_falls_back_to_backend() { + use wacore::store::traits::LidPnMappingEntry; + + let client: Arc = create_test_client().await; + let pn = "15555550123"; + let lid = "100000000000123"; + + let backend = client.persistence_manager.backend(); + backend + .put_lid_mapping(&LidPnMappingEntry { + lid: lid.into(), + phone_number: pn.into(), + created_at: 1, + updated_at: 1, + learning_source: "usync".into(), + }) + .await + .unwrap(); + + // Cache was never warmed from this backend write → cache miss path. + let entry = client.get_lid_pn_entry(&Jid::lid(lid)).await.unwrap(); + assert_eq!(entry.lid, lid); + assert_eq!(entry.phone_number, pn); + + // Subsequent lookup should now be served from cache (no backend call + // needed, but we can't assert that without a mock backend — the + // functional round-trip is sufficient). + let entry = client.get_lid_pn_entry(&Jid::pn(pn)).await.unwrap(); + assert_eq!(entry.lid, lid); + } } From 395b825c60d4a8aa4ad5769b4ae280a09950fc94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 18 Apr 2026 01:19:38 -0300 Subject: [PATCH 2/2] =?UTF-8?q?refactor:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?helper,=20error=20propagation,=20fewer=20clones?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract `mapping_to_entry` helper used by both warm_up and cache-aside fallback paths; centralizes `LearningSource::parse` boilerplate. - `get_lid_pn_entry` now returns `Result>`. Backend errors propagate instead of being silently swallowed as `None`, letting callers distinguish "missing mapping" from "lookup failed". - `LidPnCache::add` takes `&LidPnEntry` instead of `LidPnEntry`. Saves one clone per call site (`get_lid_pn_entry`, `add_lid_pn_mapping`, `warm_up`, and several retry/message paths). - Update `Groups::create_group` to propagate backend errors and keep the existing "Missing phone number mapping" error for `Ok(None)`. --- src/client/device_registry.rs | 2 +- src/client/lid_pn.rs | 108 ++++++++++++++++++++-------------- src/features/groups.rs | 2 +- src/lid_pn_cache.rs | 18 +++--- src/message.rs | 4 +- src/retry.rs | 8 +-- 6 files changed, 81 insertions(+), 61 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 5cf786f4f..e63de6163 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -553,7 +553,7 @@ mod tests { async fn setup_lid_pn(client: &Arc, lid: &str, pn: &str) { use crate::lid_pn_cache::LidPnEntry; let entry = LidPnEntry::new(lid.to_string(), pn.to_string(), LearningSource::Usync); - client.lid_pn_cache.add(entry).await; + client.lid_pn_cache.add(&entry).await; } async fn setup_device_record(client: &Arc, user: &str, device_ids: &[u32]) { diff --git a/src/client/lid_pn.rs b/src/client/lid_pn.rs index 9a3479289..1bbccf268 100644 --- a/src/client/lid_pn.rs +++ b/src/client/lid_pn.rs @@ -11,11 +11,22 @@ use anyhow::Result; use log::debug; +use wacore::store::traits::LidPnMappingEntry; use wacore_binary::Jid; use super::Client; use crate::lid_pn_cache::{LearningSource, LidPnEntry}; +/// Backend `LidPnMappingEntry` → in-memory `LidPnEntry`. +fn mapping_to_entry(m: LidPnMappingEntry) -> LidPnEntry { + LidPnEntry::with_timestamp( + m.lid, + m.phone_number, + m.created_at, + LearningSource::parse(&m.learning_source), + ) +} + impl Client { /// Warm up the LID-PN cache from persistent storage. /// This is called during client initialization to populate the in-memory cache @@ -29,19 +40,9 @@ impl Client { return Ok(()); } - let cache_entries: Vec = entries - .into_iter() - .map(|e| { - LidPnEntry::with_timestamp( - e.lid, - e.phone_number, - e.created_at, - LearningSource::parse(&e.learning_source), - ) - }) - .collect(); - - self.lid_pn_cache.warm_up(cache_entries).await; + self.lid_pn_cache + .warm_up(entries.into_iter().map(mapping_to_entry)) + .await; Ok(()) } @@ -66,7 +67,7 @@ impl Client { // Add to in-memory cache let entry = LidPnEntry::new(lid.to_string(), phone_number.to_string(), source); - self.lid_pn_cache.add(entry.clone()).await; + self.lid_pn_cache.add(&entry).await; // Persist to storage let backend = self.persistence_manager.backend(); @@ -273,43 +274,36 @@ impl Client { /// Look up the LID↔phone mapping for a JID. Cache-aside: falls back to /// the backend on cache miss so mappings survive cache eviction and any /// backend implementation gets the fallback without warm-up. - pub async fn get_lid_pn_entry(&self, jid: &Jid) -> Option { + /// + /// Backend errors are propagated — callers can distinguish "no mapping" + /// (`Ok(None)`) from "lookup failed" (`Err(_)`). + pub async fn get_lid_pn_entry(&self, jid: &Jid) -> Result> { let (hit, is_lid) = if jid.is_lid() { (self.lid_pn_cache.get_entry_by_lid(&jid.user).await, true) } else if jid.is_pn() { (self.lid_pn_cache.get_entry_by_phone(&jid.user).await, false) } else { - return None; + return Ok(None); }; if let Some(entry) = hit { - return Some(entry); + return Ok(Some(entry)); } let backend = self.persistence_manager.backend(); - let result = if is_lid { - backend.get_lid_mapping(&jid.user).await + let mapping = if is_lid { + backend.get_lid_mapping(&jid.user).await? } else { - backend.get_pn_mapping(&jid.user).await + backend.get_pn_mapping(&jid.user).await? }; - let mapping = match result { - Ok(Some(m)) => m, - Ok(None) => return None, - Err(e) => { - debug!("LID-PN backend lookup failed for {jid}: {e:?}"); - return None; - } + let Some(mapping) = mapping else { + return Ok(None); }; - let entry = LidPnEntry::with_timestamp( - mapping.lid, - mapping.phone_number, - mapping.created_at, - LearningSource::parse(&mapping.learning_source), - ); - self.lid_pn_cache.add(entry.clone()).await; - Some(entry) + let entry = mapping_to_entry(mapping); + self.lid_pn_cache.add(&entry).await; + Ok(Some(entry)) } } @@ -368,14 +362,24 @@ mod tests { let pn = "55999999999"; let lid = "100000012345678"; - assert!(client.get_lid_pn_entry(&Jid::pn(pn)).await.is_none()); + assert!( + client + .get_lid_pn_entry(&Jid::pn(pn)) + .await + .unwrap() + .is_none() + ); client .add_lid_pn_mapping(lid, pn, LearningSource::Usync) .await .unwrap(); - let entry = client.get_lid_pn_entry(&Jid::pn(pn)).await.unwrap(); + let entry = client + .get_lid_pn_entry(&Jid::pn(pn)) + .await + .unwrap() + .unwrap(); assert_eq!(entry.lid, lid); assert_eq!(entry.phone_number, pn); } @@ -386,14 +390,24 @@ mod tests { let pn = "55999999999"; let lid = "100000012345678"; - assert!(client.get_lid_pn_entry(&Jid::lid(lid)).await.is_none()); + assert!( + client + .get_lid_pn_entry(&Jid::lid(lid)) + .await + .unwrap() + .is_none() + ); client .add_lid_pn_mapping(lid, pn, LearningSource::Usync) .await .unwrap(); - let entry = client.get_lid_pn_entry(&Jid::lid(lid)).await.unwrap(); + let entry = client + .get_lid_pn_entry(&Jid::lid(lid)) + .await + .unwrap() + .unwrap(); assert_eq!(entry.lid, lid); assert_eq!(entry.phone_number, pn); } @@ -421,14 +435,20 @@ mod tests { .unwrap(); // Cache was never warmed from this backend write → cache miss path. - let entry = client.get_lid_pn_entry(&Jid::lid(lid)).await.unwrap(); + let entry = client + .get_lid_pn_entry(&Jid::lid(lid)) + .await + .unwrap() + .unwrap(); assert_eq!(entry.lid, lid); assert_eq!(entry.phone_number, pn); - // Subsequent lookup should now be served from cache (no backend call - // needed, but we can't assert that without a mock backend — the - // functional round-trip is sufficient). - let entry = client.get_lid_pn_entry(&Jid::pn(pn)).await.unwrap(); + // Subsequent lookup served from cache. + let entry = client + .get_lid_pn_entry(&Jid::pn(pn)) + .await + .unwrap() + .unwrap(); assert_eq!(entry.lid, lid); } } diff --git a/src/features/groups.rs b/src/features/groups.rs index 22463a1e1..d492c53f2 100644 --- a/src/features/groups.rs +++ b/src/features/groups.rs @@ -257,7 +257,7 @@ impl<'a> Groups<'a> { let entry = self .client .get_lid_pn_entry(&participant.jid) - .await + .await? .ok_or_else(|| { anyhow::anyhow!("Missing phone number mapping for LID {}", participant.jid) })?; diff --git a/src/lid_pn_cache.rs b/src/lid_pn_cache.rs index 93b09b3e2..7af0bba7f 100644 --- a/src/lid_pn_cache.rs +++ b/src/lid_pn_cache.rs @@ -116,7 +116,7 @@ impl LidPnCache { /// backends (e.g., Redis), concurrent `add()` calls for the same phone /// number can race. This is acceptable because the cache is best-effort /// and backed by persistent storage for correctness. - pub async fn add(&self, entry: LidPnEntry) { + pub async fn add(&self, entry: &LidPnEntry) { // Check if PN map needs update first let should_update_pn = match self.pn_to_entry.get(entry.phone_number.as_str()).await { Some(existing) => existing.created_at <= entry.created_at, @@ -131,7 +131,7 @@ impl LidPnCache { // Update PN -> Entry map (only if newer or equal timestamp) if should_update_pn { self.pn_to_entry - .insert(entry.phone_number.clone(), entry) + .insert(entry.phone_number.clone(), entry.clone()) .await; } } @@ -145,7 +145,7 @@ impl LidPnCache { let mut count = 0; for entry in entries { - self.add(entry).await; + self.add(&entry).await; count += 1; } @@ -196,7 +196,7 @@ mod tests { "559980000001".to_string(), LearningSource::Usync, ); - cache.add(entry).await; + cache.add(&entry).await; // Should be retrievable both ways assert_eq!( @@ -220,7 +220,7 @@ mod tests { 1000, LearningSource::Other, ); - cache.add(old_entry).await; + cache.add(&old_entry).await; assert_eq!( cache.get_current_lid("559980000001").await, @@ -234,7 +234,7 @@ mod tests { 2000, LearningSource::Usync, ); - cache.add(new_entry).await; + cache.add(&new_entry).await; // Should return the newer LID for PN lookup assert_eq!( @@ -264,7 +264,7 @@ mod tests { 2000, LearningSource::Usync, ); - cache.add(new_entry).await; + cache.add(&new_entry).await; // Try to add older mapping let old_entry = LidPnEntry::with_timestamp( @@ -273,7 +273,7 @@ mod tests { 1000, LearningSource::Other, ); - cache.add(old_entry).await; + cache.add(&old_entry).await; // PN -> LID should still return the newer one assert_eq!( @@ -326,7 +326,7 @@ mod tests { "559980000001".to_string(), LearningSource::Usync, ); - cache.add(entry).await; + cache.add(&entry).await; assert_eq!(cache.lid_count().await, 1); assert_eq!(cache.pn_count().await, 1); diff --git a/src/message.rs b/src/message.rs index 4e4657a6f..0cc6d9c1b 100644 --- a/src/message.rs +++ b/src/message.rs @@ -3614,7 +3614,7 @@ mod tests { phone.to_string(), crate::lid_pn_cache::LearningSource::PeerLidMessage, ); - client.lid_pn_cache.add(entry).await; + client.lid_pn_cache.add(&entry).await; // Verify the cache has the mapping let cached_lid = client.lid_pn_cache.get_current_lid(phone).await; @@ -3753,7 +3753,7 @@ mod tests { phone.to_string(), crate::lid_pn_cache::LearningSource::PeerLidMessage, ); - client.lid_pn_cache.add(entry).await; + client.lid_pn_cache.add(&entry).await; // Parse a PN-addressed DM message WITHOUT sender_lid attribute let dm_node_without_sender_lid = wacore_binary::builder::NodeBuilder::new("message") diff --git a/src/retry.rs b/src/retry.rs index d2deb6459..7f74692e5 100644 --- a/src/retry.rs +++ b/src/retry.rs @@ -2487,7 +2487,7 @@ mod tests { // Now add a LID mapping (simulates mapping arriving between send and retry) client .lid_pn_cache - .add(wacore::types::lid_pn::LidPnEntry { + .add(&wacore::types::lid_pn::LidPnEntry { lid: lid_jid.user.to_string(), phone_number: pn_jid.user.to_string(), created_at: 0, @@ -2536,7 +2536,7 @@ mod tests { client .lid_pn_cache - .add(wacore::types::lid_pn::LidPnEntry { + .add(&wacore::types::lid_pn::LidPnEntry { lid: lid_jid.user.to_string(), phone_number: pn_jid.user.to_string(), created_at: 0, @@ -2604,7 +2604,7 @@ mod tests { // Add LID mapping client .lid_pn_cache - .add(wacore::types::lid_pn::LidPnEntry { + .add(&wacore::types::lid_pn::LidPnEntry { lid: lid_jid.user.to_string(), phone_number: pn_jid.user.to_string(), created_at: 0, @@ -2702,7 +2702,7 @@ mod tests { // Add mapping but don't store any message client .lid_pn_cache - .add(wacore::types::lid_pn::LidPnEntry { + .add(&wacore::types::lid_pn::LidPnEntry { lid: lid_jid.user.to_string(), phone_number: pn_jid.user.to_string(), created_at: 0,