feat(groups): backfill participant phone_number from LID-PN mapping - #909
Conversation
The server often omits the phone_number attribute on <participant> nodes of LID-addressed groups, leaving GroupMetadata LID-only. Consumers that cross-reference data keyed by PN then treat current members as absent. get_participating/get_metadata now backfill each LID participant's phone_number from the persisted lid_pn_mapping the client already learned (single backend load + in-memory join, only when a LID-addressed group has a participant missing its PN).
|
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 (1)
📝 WalkthroughSummary by CodeRabbitBug Fixes
Tests
WalkthroughAdds phone number backfill for LID-addressed group participants. A new ChangesLID→PN Phone Number Backfill for Group Participants
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Merging this PR will not alter performance
Comparing 
Footnotes
|
jlucaso1
left a comment
jlucaso1
left a comment
There was a problem hiding this comment.
Nice catch on the problem. In LID groups the server really does drop phone_number a lot, and anything keyed by PN ends up thinking present members are gone. The needs_pn_fill gate and the graceful fallback are good.
One thing on the approach though. Instead of reloading the whole lid_pn_mapping table with get_all_lid_mappings on every call, you can use the warm in-memory cache the client already keeps (lid_pn_cache, warmed from that same table at startup). create_group right above already does exactly this kind of lookup for the same "LID with no phone_number" case.
Two reasons it matters:
- Cost.
get_metadataon one small group will spawn_blocking, scan the full table and build a HashMap of every mapping in the account just to fill a couple of fields. The cache lookup is in-memory and only touches the participants that actually need it. - Freshness. Mappings learned offline only live in the cache and aren't persisted (
learn_lid_pn_mapping_fast/_batchwithis_offline), and the detached persist can lag or fail. So a DB-only load can miss something the cache already has, and the participant stays PN-less even though we could resolve it.
What I'd suggest, dropping load_lid_pn_map entirely:
for p in meta.participants.iter_mut() {
if p.phone_number.is_none()
&& p.jid.is_lid()
&& let Some(pn) = self.client.lid_pn_cache.get_phone_number(&p.jid.user).await
{
p.phone_number = Some(Jid::pn(pn));
}
}get_phone_number degrades the same way you already do (unknown returns None). If you want it to survive a bounded/evicted cache, use get_lid_pn_entry instead, same as create_group. Keep needs_pn_fill if you like, it just doesn't need to gate any I/O anymore.
Couple of small things:
- Keep the
addressing_mode != Lidcheck insidefill_participant_pns. Inget_participatingthe fill runs over every group including PN ones, so it's actually doing work there, not redundant. - The tests only cover the pure helper. A quick one through the client (warmed mapping in, filled PN out) would cover the part that changed.
- The red CodSpeed integration check isn't your fault, it's a workflow template error (
codspeed.ymlline 82), happens on fork PRs.
Happy to merge once the lookup goes through the cache.
…able load Per review: instead of reloading the whole lid_pn_mapping table on every call, look each LID participant up through the client's warm in-memory cache via get_lid_pn_entry (same path create_group uses). Cheaper (only touches participants that need it) and fresher (picks up offline-learned mappings the detached DB persist may lag). Drops load_lid_pn_map/needs_pn_fill; the addressing_mode guard stays inside fill_participant_pns. Tests now go through a client with a warmed mapping.
|
Thanks for the review — all applied in 6a70fad.
Good call on both the cost and freshness points. |
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/features/groups.rs`:
- Around line 361-365: The sequential await calls in the loop over
meta.participants for get_lid_pn_entry are causing performance bottlenecks on
large groups. Instead of awaiting each lookup one at a time in the for loop,
collect all the concurrent lookup futures for participants that need LID to PN
resolution (those with no phone number and is_lid), execute them concurrently
with a bounded fan-out using a stream buffer to limit parallelism, and then
apply the resolved results back to the participants array by their original
indices. This will allow multiple lookups to proceed in parallel while still
maintaining control over resource usage.
🪄 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: e81e4d59-e841-4d3a-8b30-92009e027e7e
📒 Files selected for processing (1)
src/features/groups.rs
Per CodeRabbit nitpick: the per-participant get_lid_pn_entry awaits ran serially, which on a large group with a cold cache serializes the DB fallbacks. Collect the PN-less LID participants by index and resolve them with buffer_unordered(16), then apply results back. Cache hits stay cheap; cold-cache large groups no longer serialize.
|
@Salientekill e esse lint ai (clippy) |
Vagabunda kskskskkskskkskskksks |
clippy::filter_map_bool_then under -D warnings. Same result, no behavior change.
2 participants
Problem
In LID-addressed groups the server frequently omits the
phone_numberattribute on<participant>nodes, so theGroupMetadatareturned byget_participating/get_metadataends up LID-only (phone_number: Nonefor every participant).Any consumer that cross-references data keyed by PN (legacy rows, anything that canonicalizes to phone number) then treats current members as absent — even though the client already knows their PN via the
lid_pn_mappingit learned from messages/usync. Downstream this shows up as e.g. orphan-cleanup deleting data for members who are still in the group, or mentions failing to resolve.Change
get_participatingandget_metadatanow backfill each LID participant'sphone_numberfrom the persistedlid_pn_mapping:get_all_lid_mappings) + in-memory join — not N per-participant lookups.needs_pn_fill: only runs when a LID-addressed group actually has a participant missing its PN, so PN-addressed groups and already-complete metadata pay nothing.The fill happens only on the read paths that hand
GroupMetadatato callers; the persisted blob / phash used for not-modified detection is untouched.Tests
fill_participant_pns_backfills_lid_from_mapping— LID participant gets its PN from the map.fill_participant_pns_noop_in_pn_group— PN-addressed groups are left untouched.Fictitious JIDs/numbers only.