fix: cache-aside fallback in get_lid_pn_entry - #565
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughClient::get_lid_pn_entry was changed from cache-only to cache-aside: it checks the in-memory Lid↔PN cache, queries the persistence backend on miss (direction based on Jid type), converts backend mappings into Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cache as In-Memory Cache
participant Backend as Persistence Backend
Client->>Cache: lookup LidPnEntry (Jid)
alt Cache hit
Cache-->>Client: return LidPnEntry
else Cache miss
Client->>Backend: query mapping (get_lid_mapping / get_pn_mapping)
Backend-->>Client: mapping or NotFound or Error
alt Mapping found
Client->>Client: mapping_to_entry (parse learning_source, with_timestamp)
Client->>Cache: add(&LidPnEntry)
Cache-->>Client: ack
Client-->>Client: return Ok(Some(LidPnEntry))
else NotFound
Client-->>Client: return Ok(None)
else Error
Backend-->>Client: error
Client-->>Client: return Err(error)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@claude review nitpicky also check opportunities of allocation-less reduce clone etc |
|
Claude finished @jlucaso1's task in 2m 40s —— View job Nitpicky Review:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/lid_pn.rs`:
- Around line 305-310: Extract the duplicated mapping→entry construction into a
single helper (e.g., a function like mapping_to_lid_pn_entry) and use it
wherever entries are created (the current site using LidPnEntry::with_timestamp
and the warm_up_lid_pn_cache function). The helper should accept the DB mapping
(with fields mapping.lid, mapping.phone_number, mapping.created_at,
mapping.learning_source) and return LidPnEntry by calling
LidPnEntry::with_timestamp(...,
LearningSource::parse(&mapping.learning_source)). Replace both manual
constructions with calls to this helper to centralize timestamp and source
parsing logic.
- Around line 296-303: The match currently swallows backend errors and returns
None; change the function that produces mapping to return a
Result<Option<LidPnEntry>, E> (or the crate's common error type) instead of
Option<LidPnEntry>, so backend Err(e) is propagated as Err(e) rather than being
converted to Ok(None). Locate the match over result (variables named result,
mapping, and the LidPnEntry type) and replace the Err(e) branch with return
Err(e.into()) (or appropriate error conversion), keep Ok(Some(m)) => Ok(Some(m))
and Ok(None) => Ok(None); then update callers (e.g., the code in features
handling groups that treats None as “missing mapping”) to handle
Result<Option<...>> and distinguish real misses from backend errors.
🪄 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: 9c6d1e8e-775d-4bf4-978c-8bd2e60d1038
📒 Files selected for processing (1)
src/client/lid_pn.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8422c8ce0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mapping.created_at, | ||
| LearningSource::parse(&mapping.learning_source), | ||
| ); | ||
| self.lid_pn_cache.add(entry.clone()).await; |
There was a problem hiding this comment.
When this path misses cache for a LID JID, get_lid_mapping can return a historical row for a phone number that now maps to a newer LID (the SQLite backend keeps multiple LIDs per phone and uses get_pn_mapping ordering to pick the current one). The unconditional add() call writes both directions, so a cold PN side gets repopulated with that stale LID and subsequent PN-based resolution (e.g. encryption/session routing) can use the wrong identity. This regression is specific to cache-miss LID lookups introduced in this commit; only the LID side should be hydrated here, or the PN side should be validated via get_pn_mapping first.
Useful? React with 👍 / 👎.
Benchmark Results59 unchanged benchmark(s)
|
- 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<Option<LidPnEntry>>`. 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)`.
`send_status_message` used to force every recipient to PN addressing
via an in-memory-only lookup, then abort the whole send if any LID had
no PN mapping in the cache. That made status reactions to LID-only
contacts fail indefinitely — the bot can decrypt the status (Signal
state is under the LID), but the status sender has no way to learn the
poster's phone number without a DM or usync round-trip.
This mismatch also contradicts WA Web. `WAWebEncryptAndSendStatusMsg`
(`docs/captured-js/WAWeb/Encrypt/AndSendStatusMsg.js:46`) does:
compactMap(x.list, WAWebLidMigrationUtils.toUserLid)
i.e. every recipient is mapped to its LID, and `compactMap` silently
drops entries that can't be resolved. Status is LID-addressed, not PN,
since the LID migration.
Changes:
- `send_status_message` now resolves each recipient to its LID form
(LID passes through; PN goes through the cache-aside
`Client::get_lid_pn_entry`, which falls back to the backend on a
cold cache — added in PR #565). Unresolvable entries are skipped
silently, matching WA Web. `AddressingMode::Lid` and `own_lid` drive
the downstream encryption path.
- New pure helper `wacore::send::assemble_status_participants` owns
the dedup + own-LID anchoring + empty-list guard. Linear Vec scan
(no HashSet allocation — status lists are small) and it consumes
the resolved JIDs by value, so CompactString lives exactly once per
kept recipient. 6 unit tests cover dedup, compactMap-style skipping,
the empty-after-skip error path, and the ad-vs-bare own-LID case.
- New helper `Client::resolve_recipient_to_lid` mirrors
`WAWebLidMigrationUtils.toUserLid` and is the single call site for
PN→LID resolution in the status path (DRY).
- The sender-key-name derivation and `resolve_skdm_targets` now key
off `own_lid` to match the stanza's addressing mode.
Error only when every resolved recipient is unresolvable — a single
LID-only contact no longer breaks the whole send.
1 participant
Summary
Client::get_lid_pn_entryonly queried the in-memory cache. Any mapping that lived exclusively on the backend (because the cache had been evicted, or warm-up wasn't run for that client instance) would returnNoneeven though the persistent data had the answer.Now it's cache-aside: on cache miss, falls back to
Backend::get_lid_mapping/get_pn_mapping, converts toLidPnEntry, populates the in-memory cache, and returns the entry. Any backend implementation gains this fallback without additional wiring.Change
src/client/lid_pn.rs:get_lid_pn_entrytries the in-memory cache first, then the backend on missLidPnMappingEntry→LidPnEntrymirrors the existingwarm_up_lid_pn_cachepath (LidPnEntry::with_timestamp+LearningSource::parse)Test plan
cargo test -p whatsapp-rust --lib client::lid_pn::tests— 6 cases pass, including newtest_get_lid_pn_entry_falls_back_to_backendregressioncargo clippy -p whatsapp-rust --tests