feat(lid-pn): source-aware write policy matching WA Web createLidPnMappings - #1011
Merged
Merged
Conversation
…ppings
The LID-PN learn path previously overwrote the cache unconditionally on
every learn, ignoring the LearningSource. WhatsApp Web's
createLidPnMappings instead branches on learningSource: directed sources
(usync, peer messages, migration, blocklist-active) overwrite on any
change, while observational bulk sources ("other": history sync, group
seeds, device/contact notifications, status, voip) only seed genuinely
new LIDs and, on a conflict with an already-known LID, re-resolve the
phone via a live LID query rather than clobbering the live mapping.
Known-stale sources (migration-sync-old, blocklist-inactive) are stamped
created_at=0 so they never outrank fresh data.
- Add lid_pn_write_policy() / is_stale_source() mirroring the WA Web
switch, and route every learn (single + batch) through it via a
RecordOutcome { Skipped | Written | NeedsUsync }.
- Fire a single detached LidQuerySpec usync for conflicted phones
(WA Web syncContactListJob mode:query); loop-safe because the result
learns under LearningSource::Usync, which never re-queries.
- Preserve the durability/eviction re-warm for self-consistent pairs
(exact forward match, or reverse-only after a bounded-cache PN
eviction) so a half-evicted pair is not mistaken for a conflict.
- Switch the history-sync field-15 harvest from MigrationSyncLatest to
Other, matching WA Web's learningSource:"other" for this bulk seed.
Adds a pure switch-matrix unit test plus behavioral tests for
conservative/directed conflict resolution, stale-source ordering,
exact-match heal, and batch usync collection.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR changes LID↔phone mapping learning to use explicit write-policy outcomes, routes observational conflicts through async usync reconciliation, switches history-sync seeding to ChangesLID-PN mapping write-policy refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🤖 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/client/lid_pn.rs`:
- Around line 282-327: Repeated LID usync reconciles are being spawned for every
NeedsUsync without any deduplication or in-flight guard. Update
spawn_lid_usync_reconcile and reconcile_lid_mappings_via_usync in
src/client/lid_pn.rs to reuse the existing batched dirty-set path or add
per-phone in-flight/recent-query tracking so the same phone does not trigger
multiple concurrent network queries. Keep the best-effort behavior, but ensure
repeated conflicts coalesce before calling execute or spawning the background
task.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d79ff0a6-3907-4365-9953-8dbfdabea1d0
📒 Files selected for processing (3)
src/client/lid_pn.rssrc/history_sync.rswacore/src/types/lid_pn.rs
|
| Filename | Overview |
|---|---|
| src/client/lid_pn.rs | Core of the PR: introduces lid_pn_write_policy, is_stale_source, RecordOutcome, and BatchRecordOutcome; routes all single and batch learns through the source-aware write policy; adds spawn_lid_usync_reconcile + reconcile_lid_mappings_via_usync for best-effort live re-resolution; comprehensive test suite added. |
| src/history_sync.rs | Single-line source change from MigrationSyncLatest to Other for the field-15 LID harvest; now correctly treats history-sync as an observational seed that will not clobber live-learned mappings. |
| wacore/src/types/lid_pn.rs | Doc-comment update to LearningSource: clarifies that the source is load-bearing (drives the write policy), not merely provenance. No logic changes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[learn LID-PN pair
source + lid + phone] --> B{can_skip_relearn?}
B -- yes --> C[Skipped]
B -- no --> D[get current_lid
get reverse_pn]
D --> E{exact match
or forward-evicted?}
E -- yes --> F[re-warm cache
Written is_new=false]
E -- no --> G{lid_pn_write_policy
source, lid_unseen, exact}
G -- write=true --> H[LidPnEntry::with_timestamp
created_at=0 if stale source]
H --> I[cache.add
Written is_new=current_lid.is_none]
G -- needs_usync=true --> J[NeedsUsync]
G -- neither --> C
J --> K[spawn_lid_usync_reconcile]
K --> L[reconcile_lid_mappings_via_usync
LidQuerySpec IQ]
L --> M{IQ response}
M -- Ok --> N[add_lid_pn_mapping
LearningSource::Usync]
M -- Err --> O[debug log
cache unchanged]
N --> A
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[learn LID-PN pair
source + lid + phone] --> B{can_skip_relearn?}
B -- yes --> C[Skipped]
B -- no --> D[get current_lid
get reverse_pn]
D --> E{exact match
or forward-evicted?}
E -- yes --> F[re-warm cache
Written is_new=false]
E -- no --> G{lid_pn_write_policy
source, lid_unseen, exact}
G -- write=true --> H[LidPnEntry::with_timestamp
created_at=0 if stale source]
H --> I[cache.add
Written is_new=current_lid.is_none]
G -- needs_usync=true --> J[NeedsUsync]
G -- neither --> C
J --> K[spawn_lid_usync_reconcile]
K --> L[reconcile_lid_mappings_via_usync
LidQuerySpec IQ]
L --> M{IQ response}
M -- Ok --> N[add_lid_pn_mapping
LearningSource::Usync]
M -- Err --> O[debug log
cache unchanged]
N --> A
Reviews (4): Last reviewed commit: "refactor(lid-pn): heal self-consistent p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- In
src/client/lid_pn.rs,spawn_lid_usync_reconcilein thelearn_lid_pn_mapping_fire_and_forgethot path can launch repeated detached usync calls on eachNeedsUsyncresult without in-flight/per-phone dedup, which risks request storms, extra load, and user-visible latency under bursty traffic — add an in-flight or per-phone dedup/throttle guard before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/lid_pn.rs">
<violation number="1" location="src/client/lid_pn.rs:288">
P2: `spawn_lid_usync_reconcile` fires a detached usync network round-trip on every `NeedsUsync` outcome with no in-flight or per-phone dedup guard. In the per-message hot path (`learn_lid_pn_mapping_fire_and_forget`), a noisy observational conflict for the same phone could spawn many concurrent reconcile queries. Consider adding per-phone deduplication (e.g., a set of in-flight phones checked before spawning) or batching these into the existing dirty-set flush cadence to avoid redundant network calls.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Trim the re-warm comment to the reasoning (why, not what). - Cover PeerLidMessage/RecipientLatestLid in the is_stale_source test by iterating the full source set minus the stale ones, so a new variant is checked automatically. - Qualify the LearningSource doc: created_at=0 orders only the per-phone PN->LID direction; the LID->PN reverse map always takes the latest write.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The is_stale_source / switch-matrix tests iterate a hand-written ALL_SOURCES array. Add all_sources_is_exhaustive: a wildcard-free match that fails to compile when a LearningSource variant is added, with an arm-count assertion tied to ALL_SOURCES.len(), so a new variant must be appended to both. Drops the misleading "covered automatically" comment.
greptile-apps
Bot
dismissed
their stale review
July 8, 2026 22:35
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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/lid_pn.rs (1)
391-424: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep reverse-only self-heals out of the directed-write path.
Line 393 writes before Line 411’s
same_pair_forward_evictedcheck. For directed sources, a self-consistent reverse-only cache entry is treated asWritten { is_new: true }, which can re-run persistence/migration instead of preserving the intended eviction re-warm behavior.Proposed fix
let exact = current_lid.as_deref() == Some(lid); let lid_unseen = reverse_pn.is_none(); let (write, needs_usync) = lid_pn_write_policy(source, lid_unseen, exact); + // Re-warm/re-affirm durability for a pair that is already the cached + // mapping (exact, or reverse-only after a bounded-cache PN eviction). + // Must precede write/conflict handling so a self-consistent pair is + // not classified as a new directed overwrite. + let same_pair_forward_evicted = + current_lid.is_none() && reverse_pn.as_deref() == Some(phone_number); + if exact || same_pair_forward_evicted { + let existing = match self.lid_pn_cache.get_entry_by_phone(phone_number).await { + Some(entry) => Some(entry), + None => self.lid_pn_cache.get_entry_by_lid(lid).await, + }; + return match existing { + Some(entry) => { + self.lid_pn_cache.add(&entry).await; + RecordOutcome::Written { + entry, + is_new: false, + } + } + None => RecordOutcome::Skipped, + }; + } + if write { let created_at = if is_stale_source(source) { 0 } else { wacore::time::now_secs() @@ is_new: current_lid.is_none(), }; } - - // Re-warm/re-affirm durability for a pair that is already the cached - // mapping (exact, or reverse-only after a bounded-cache PN eviction). - // Must precede the conflict branch so a half-evicted but self-consistent - // pair is not mistaken for an observational conflict and re-queried. - let same_pair_forward_evicted = - current_lid.is_none() && reverse_pn.as_deref() == Some(phone_number); - if exact || same_pair_forward_evicted { - let existing = match self.lid_pn_cache.get_entry_by_phone(phone_number).await { - Some(entry) => Some(entry), - None => self.lid_pn_cache.get_entry_by_lid(lid).await, - }; - return match existing { - Some(entry) => { - self.lid_pn_cache.add(&entry).await; - RecordOutcome::Written { - entry, - is_new: false, - } - } - None => RecordOutcome::Skipped, - }; - }🤖 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/lid_pn.rs` around lines 391 - 424, The directed-write logic in `RecordOutcome` handling allows reverse-only self-heals to take the main write path before `same_pair_forward_evicted` is checked, which can incorrectly treat a self-consistent cached pair as a new write. Update the decision flow around `lid_pn_write_policy`, `same_pair_forward_evicted`, and the `exact` branch so reverse-only cache re-warms are handled only by the re-affirmation path and never by the directed-write `Written { is_new: true }` path.
🤖 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/client/lid_pn.rs`:
- Around line 1102-1128: The `all_sources_is_exhaustive` guard only compares
`ALL_SOURCES.len()` to the `LearningSource` arm count, so a duplicate entry can
still pass even if one variant is missing. Update this test to validate
`ALL_SOURCES` contains each `LearningSource` variant exactly once, using
`ALL_SOURCES` and `all_sources_is_exhaustive` as the main check points. Keep the
existing exhaustive `match` on `LearningSource` for compile-time coverage, and
add a uniqueness/deduplication assertion so duplicates are rejected as well.
---
Outside diff comments:
In `@src/client/lid_pn.rs`:
- Around line 391-424: The directed-write logic in `RecordOutcome` handling
allows reverse-only self-heals to take the main write path before
`same_pair_forward_evicted` is checked, which can incorrectly treat a
self-consistent cached pair as a new write. Update the decision flow around
`lid_pn_write_policy`, `same_pair_forward_evicted`, and the `exact` branch so
reverse-only cache re-warms are handled only by the re-affirmation path and
never by the directed-write `Written { is_new: true }` path.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2facb0de-9728-43b1-a650-2d45583b4cdd
📒 Files selected for processing (1)
src/client/lid_pn.rs
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Move the exact / reverse-only re-warm ahead of the write-policy branch so a forward-evicted but self-consistent pair (bounded caches only) is re-affirmed with is_new=false instead of re-written as a fresh directed learn that re-runs the idempotent PN->LID migration. Compute the write policy lazily, only when the pair is not already cached. - Reject duplicate ALL_SOURCES entries in all_sources_is_exhaustive so a repeated variant cannot mask a missing one behind the count check.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/lid_pn.rs">
<violation number="1" location="src/client/lid_pn.rs:416">
P2: Observational overwrite protection assumes a negative `get_phone_number` result means the incoming LID is genuinely brand-new, but the LID→PN reverse cache can be independently evicted under a capacity bound. When that happens, a known LID appears unseen, so observational sources (e.g. `Other`) get `(write=true, needs_usync=false)` from `lid_pn_write_policy` and immediately overwrite the phone's mapping instead of returning `NeedsUsync`. The codebase already handles forward-only eviction (`same_pair_forward_evicted`) and has tests proving reverse-cache misses are possible, but there is no corresponding guard here.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Follow-up to #1010. The LID-PN learn path overwrote the cache unconditionally on every learn, ignoring
LearningSource— the source string was pure provenance. WhatsApp Web'screateLidPnMappings(WAWebDBCreateLidPnMappings) does the opposite: it branches onlearningSourceto decide whether an incoming pair may overwrite what the cache already holds. This PR brings that source-differentiated write policy over so the cache behaves like WA Web instead of last-write-wins.The concrete gap this closes: a bulk/observational seed (history sync field 15, group participant lists, device/contact notifications) could clobber a freshly, authoritatively learned LID for a peer. WA Web deliberately refuses to trust those sources over live data and instead re-resolves via a live query.
What changed
Mirrors WA Web's
switch (learningSource)(thev/b/Slogic increateLidPnMappings):Usyncusyncc || C)PeerPnMessage,PeerLidMessage,RecipientLatestLid,MigrationSyncLatest,MigrationSyncOld,BlocklistActive,BlocklistInactive!y)Other,Pairing,DeviceNotificationother(default)v = c,b = C)MigrationSyncOld,BlocklistInactivecreated_at = 0so they never outrank fresh data (S)lid_pn_write_policy()/is_stale_source()encode the switch; every learn (single + batch) routes through it viaRecordOutcome { Skipped | Written | NeedsUsync }.LidQuerySpecusync (the analog of WA Web'ssyncContactListJob({mode:"query"})), learning the authoritative result asUsync— loop-safe, sinceUsyncnever re-queries.MigrationSyncLatest→Other, matching WA Web'slearningSource: "other"for that seed (verified inWAWebHistorySyncChunk).LidPnCache::addis unchanged; it never readlearning_sourceand still resolves the PN→LID direction by most-recentcreated_at.Evidence
Cross-referenced against captured WhatsApp Web JS:
createLidPnMappingsbranches onlearningSource, and every bulk/observational call site (HistorySyncChunk,DeviceNotification,ContactNotification, status, voip, contact-sync) passes"other";"migration-sync-latest"is used only by the live LID 1:1 migration flow, not the history seed.Scope boundaries
migrate_*_on_lid_discovery, gated onis_new); did not port WA Web's separatehandleNewIdentityidentity-key-change subsystem.created_at = 0; dropping it yields the same resolved PN→LID mapping.createLidPnMappingsvscreateLidPnMappingsInBatchessplit) — out of scope for this change.Behavioral note
For observational sources, a conflicting update is now applied eventually (via the async LID re-query) rather than immediately overwriting. This is the intended WA-Web-parity behavior: the client declines to trust an unverified observational claim over the live mapping, and self-heals through the authoritative usync. A genuinely new LID still writes immediately.
Tests
(lid_unseen, exact).created_at = 0ordering (does not outrank fresh; still seeds an empty cache); batch splits written entries vs. usync phones; end-to-end history-sync-style seed preserves a live mapping.cargo clippy --testsclean,cargo fmtclean.Review
Reviewed for correctness, WA-Web fidelity, and blast-radius, with each finding independently verified against the code. This surfaced one low-severity regression — an observational re-learn of a pair whose PN-cache side was evicted returned
NeedsUsyncinstead of re-warming from the surviving LID side — which is fixed here. Two further items were a doc clarification (thecreated_at = 0ordering guards only the PN→LID direction) and the intended async-heal behavior noted above.