perf(send): memoize the per-group device list behind a topology generation - #824
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a DeviceTopology tracker and topology-aware DeviceRegistryCache, memoizes fully resolved group device lists validated by topology generation, records topology touches from registry and LID↔PN mutations, refactors SKDM target resolution to use per-group cached sender-key maps and filtered recipient subsets, and shares resolved device lists via Arc for phash. ChangesDevice topology memoization and SKDM caching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
You wanted this to work right — review the topology + memoization layers and SKDM filtering paths first; they carry the most behavioral risk. 🚥 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 |
![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: 891ea38a96
ℹ️ 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".
Benchmark Results67 unchanged benchmark(s)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/send.rs (1)
1420-1425:⚠️ Potential issue | 🟠 MajorFix SKDM warm-path memo misses when
selfis synthesized (pointer-equality breaks the cache)
Insrc/send.rs,group_info = ensure_self_in_group(...)is passed intoresolve_skdm_targets_memoized(...). When the server omits our sender JID,ensure_self_in_groupreturns a freshArc<GroupInfo>;resolve_group_devices_memoized(...)only reuses its memo whenArc::ptr_eq(&memo.group_info, &group_info)(and generation matches). So the “warm repeat send” memo will miss every time we synthesize self, forcing the expensive device-resolution path and killing the optimization right when you need it.🤖 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/send.rs` around lines 1420 - 1425, The memo lookup misses when ensure_self_in_group returns a fresh Arc<GroupInfo> because resolve_group_devices_memoized only accepts memo hits via Arc::ptr_eq(&memo.group_info, &group_info) and generation match; change the memo key/comparison to use a stable identity rather than pointer equality — e.g., have resolve_skdm_targets_memoized / resolve_group_devices_memoized compare memo.group_info.generation and a stable group identifier (or group_info.id or computed structural key) against the incoming group_info, or normalize group_info to a canonical Arc before lookup so ptr_eq succeeds; update the memo struct and lookup logic (references: ensure_self_in_group, resolve_skdm_targets_memoized, resolve_group_devices_memoized, memo.group_info, Arc::ptr_eq, generation) accordingly.
🤖 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/device_registry.rs`:
- Around line 83-146: The resolve_group_devices_memoized function reads
device_topology_generation before calling get_user_devices which intentionally
allows a racing write to bump generation and cause wasted work but prevents
serving stale memos; update the doc comment above resolve_group_devices_memoized
(or add an inline comment next to the device_topology_generation.load) to
explicitly state this race semantics, explain why the generation must be loaded
before get_user_devices and that storing a memo stamped with the older
generation is deliberate (optimistic cache tradeoff), and explicitly instruct
future maintainers not to move the load-after-resolve since that would break
invalidation; reference resolve_group_devices_memoized,
device_topology_generation, get_user_devices, group_devices_memo, and generation
in the comment so the intent is unambiguous.
In `@src/client/lifecycle.rs`:
- Line 223: The hard-coded 64 in the Cache::builder().max_capacity(64) call (for
group_devices_memo) should be extracted to a named constant to make the
cache-size decision explicit and easy to tune; define a descriptive constant
(e.g. GROUP_DEVICES_CACHE_CAPACITY) near the top of this file or in a cache
constants module, replace the literal 64 in the group_devices_memo
initialization with that constant, and ensure any tests or docs referencing the
value are updated to use the constant.
In `@src/send.rs`:
- Around line 784-790: The tracing span attribute #[cfg_attr(feature =
"tracing", tracing::instrument(name = "wa.send.resolve_skdm_targets", ...)) was
placed on skdm_device_map but needs to annotate the actual resolver
resolve_skdm_targets so spans reflect real work; move the cfg_attr
tracing::instrument from the skdm_device_map function declaration to the
resolve_skdm_targets function declaration (keeping the same name, level,
skip_all and fields configuration) and remove it from skdm_device_map so traces
correctly attribute resolution time to resolve_skdm_targets.
---
Outside diff comments:
In `@src/send.rs`:
- Around line 1420-1425: The memo lookup misses when ensure_self_in_group
returns a fresh Arc<GroupInfo> because resolve_group_devices_memoized only
accepts memo hits via Arc::ptr_eq(&memo.group_info, &group_info) and generation
match; change the memo key/comparison to use a stable identity rather than
pointer equality — e.g., have resolve_skdm_targets_memoized /
resolve_group_devices_memoized compare memo.group_info.generation and a stable
group identifier (or group_info.id or computed structural key) against the
incoming group_info, or normalize group_info to a canonical Arc before lookup so
ptr_eq succeeds; update the memo struct and lookup logic (references:
ensure_self_in_group, resolve_skdm_targets_memoized,
resolve_group_devices_memoized, memo.group_info, Arc::ptr_eq, generation)
accordingly.
🪄 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: a99807bd-35c6-47e4-814c-550759913ff3
📒 Files selected for processing (5)
src/client.rssrc/client/device_registry.rssrc/client/lid_pn.rssrc/client/lifecycle.rssrc/send.rs
… invalidation Addresses every cost of the memo design plus the review findings: - The generation now lives inside DeviceRegistryCache (cache + tracker fused) and LidPnCache::add, so a write path cannot forget to record a change; manual bumps are gone. This also covers the Codex P2: the LID-PN warm-up records through add() now. - The memo stores a Weak<GroupInfo>, keeping pointer-identity validation (ABA-safe: the Weak holds the allocation) while releasing the old snapshot's data as soon as the metadata cache refreshes. - Changes log WHICH users they touched (both namespaces); a stale memo re-stamps itself when every change since its stamp hit other groups' users, so unrelated write storms no longer flush every group. Doubt (log overflow, global events) recomputes. - Memo hits share the snapshot Arc instead of cloning the device Vec, and the tracing instrument moved back onto the resolvers. Measured (release, 800 members x 2 devices): hit 161ns; re-stamp under a storm of unrelated writes 1.0us (vs the 514us recompute the coarse design paid); raw hit before this change was 8us (Vec clone).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac657c15a
ℹ️ 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".
A LID-keyed update (canonical == original == LID) only recorded the LID, so a PN-addressed group's memo, whose member set knows the member by PN, re-stamped its stale snapshot instead of recomputing. Updates now record all resolved lookup keys. Regression test reproduces the exact scenario and fails without the alias rule (verified by reverting the fix).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3abd7ebd6
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/device_registry.rs`:
- Around line 932-943: The code currently calls self.device_topology.record([pn,
lid]) unconditionally before checking backend.update_device_list result, causing
duplicate topology records on success; change flow so the manual record is only
done in the backend error branch: await
backend.update_device_list(record.clone()) into backend_write, then if let
Err(e) = backend_write { call self.device_topology.record([pn, lid]); warn!(...)
and return; } and remove the prior unconditional call so successful writes rely
on DeviceRegistryCache::insert (device_registry_cache.insert) to record the
change once.
- Around line 164-172: The loop uses a nested if/if-let pattern; replace it with
a let-chain to satisfy the repo style and Clippy's collapsible_if rule: after
inserting participant.user into members, combine the is_lid check with the phone
lookup using a let-chain (e.g., if participant.is_lid() && let Some(pn) =
group_info.phone_jid_for_lid_user(&participant.user) {
members.insert(pn.user.clone()); } else if let Some(lid) =
group_info.lid_user_for_phone_user(&participant.user) {
members.insert(lid.clone()); } so keep the same logic but collapse the nested
if-let into a single conditional using participant.is_lid(),
group_info.phone_jid_for_lid_user, and group_info.lid_user_for_phone_user.
- Around line 95-105: The current code reads generation via
device_topology.current() before fetching group_devices_memo, which can let a
concurrent write make memo stale while still passing memo.generation ==
generation; to fix, remove the early generation read and instead: call
self.group_devices_memo.get(group).await first, then immediately read let
generation = self.device_topology.current() and compare memo.generation ==
generation and pointer equality (memo.group_info vs Arc::as_ptr(group_info)) for
a valid hit; if it's a miss, when you go to resolve/get_user_devices, take a
separate pre-resolve stamp by calling device_topology.current() right before
resolving so the memo you store uses that fresh stamp. Ensure the code
references the existing symbols group_devices_memo.get,
device_topology.current(), memo.generation, and group_info as shown.
In `@src/send.rs`:
- Around line 1425-1427: The memo cache misses because
resolve_skdm_targets_memoized/resolve_group_devices_memoized uses Arc::ptr_eq on
the GroupInfo snapshot but ensure_self_in_group may clone a new GroupInfo (when
self was missing) before the memo lookup; normalize the GroupInfo prior to the
memo identity check by ensuring self inclusion before calling
resolve_skdm_targets_memoized (or alternatively pass the original Arc<GroupInfo>
as the cache key instead of the possibly-cloned one). Update the call site
around resolve_skdm_targets_memoized/resolve_group_devices_memoized so the same
Arc<GroupInfo> instance (or a stable key derived from GroupInfo id/version) is
used for memoization, and reference the functions resolve_skdm_targets_memoized,
resolve_group_devices_memoized, ensure_self_in_group, query_info, GroupInfo, and
Arc::ptr_eq when making the change.
🪄 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: cc77ae35-fac6-41f7-b79b-1c70de9a73f6
📒 Files selected for processing (9)
src/client.rssrc/client/device_registry.rssrc/client/device_topology.rssrc/client/lifecycle.rssrc/handlers/notification/mod.rssrc/lid_pn_cache.rssrc/send.rswacore/benches/send_receive_benchmark.rswacore/src/send/group.rs
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/client/device_registry.rs (3)
100-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate memo hits against a fresh topology generation.
Lines 100-123 use the pre-resolve stamp for hit validation too. If a topology write lands after Line 100 but before the memo read, this call can still return the old memo once, so a live group send can miss a new device or keep targeting a removed one. Split the reads: use a fresh
current_generationfor the hit/re-stamp path, then take the pre-resolvegenerationonly right beforeget_user_devices.🔧 Minimal fix
- let generation = self.device_topology.current(); - if let Some(memo) = self.group_devices_memo.get(group).await && std::ptr::eq(memo.group_info.as_ptr(), Arc::as_ptr(group_info)) { - if memo.generation == generation { + let current_generation = self.device_topology.current(); + if memo.generation == current_generation { // Refcount bump: the snapshot is immutable, so a hit shares // it instead of cloning the device Vec. return Ok(Arc::clone(&memo.devices)); } @@ group.clone(), Arc::new(GroupDevicesMemo { group_info: memo.group_info.clone(), - generation, + generation: current_generation, members: Arc::clone(&memo.members), devices: Arc::clone(&memo.devices), }), ) .await; return Ok(Arc::clone(&memo.devices)); } } + + // Load the generation BEFORE resolving (do NOT move this after + // get_user_devices): a write racing the resolve bumps it afterwards, + // so the memo we store is already stale by its own stamp and the next + // read revalidates. Loading after would stamp racing writes as seen + // and serve their effects stale. + let generation = self.device_topology.current();🤖 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/device_registry.rs` around lines 100 - 123, The memo hit logic uses the same pre-resolve generation for both checking cache validity and later resolving devices, which can return a stale memo if topology advanced between those reads; change the ordering in the group device lookup (the code paths around device_topology.current(), group_devices_memo.get(group).await, the memo.generation check, unchanged_for(...), group_devices_memo.insert(...), and the later get_user_devices call) so you first read a fresh current_generation and use that for the memo hit/unchanged_for validation/hit-return paths, and only after deciding to compute devices read the pre-resolve generation used immediately before calling get_user_devices and storing a new GroupDevicesMemo; ensure you compare memo.generation against the fresh current_generation for hits and reserve the original pre-resolve generation only for the subsequent compute/insert step.
942-953:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecord this migration once on the success path.
Lines 942-953 double-record the same topology change on success.
DeviceRegistryCache::insertalready owns the recording write path, so the unconditionalrecord([pn, lid])burns two generations for one migration and shortens the scoped re-stamp window for no gain.🔧 Minimal fix
- let backend_write = backend.update_device_list(record.clone()).await; - // The backend row may have changed even when the write errors, - // so the change is recorded before the early return. - self.device_topology.record([pn, lid]); - if let Err(e) = backend_write { + if let Err(e) = backend.update_device_list(record.clone()).await { + // The backend row may have changed even when the write errors, + // so record the touched aliases before returning. + self.device_topology.record([pn, lid]); warn!("Failed to migrate device registry to LID: {}", e); return; }🤖 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/device_registry.rs` around lines 942 - 953, The code currently calls self.device_topology.record([pn, lid]) before checking backend.update_device_list's result, causing the same topology change to be recorded twice because DeviceRegistryCache::insert also records on success; fix this by removing the unconditional self.device_topology.record call and only record once on the successful path (i.e., after backend.update_device_list returns Ok or rely on device_registry_cache.insert's recording), updating the block around backend.update_device_list, the error branch that logs the warn, and the subsequent self.device_registry_cache.insert call to ensure a single record occurs.
166-170: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCollapse this nested
if letinto a let-chain.This is still the one changed Rust branch that falls back to the nested form. Keep it consistent with the repo rule and Clippy here.
♻️ Minimal rewrite
- if participant.is_lid() { - if let Some(pn) = group_info.phone_jid_for_lid_user(&participant.user) { - members.insert(pn.user.clone()); - } + if participant.is_lid() + && let Some(pn) = group_info.phone_jid_for_lid_user(&participant.user) + { + members.insert(pn.user.clone()); } else if let Some(lid) = group_info.lid_user_for_phone_user(&participant.user) { members.insert(lid.clone()); }As per coding guidelines,
**/*.rs: “Always use let-chains (if let Some(x) = foo && let Some(y) = x.bar { ... }) instead of nestedif letblocks to maintain consistency with Clippy'scollapsible_iflint”.🤖 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/device_registry.rs` around lines 166 - 170, The nested if-let should be collapsed into a let-chain: replace the inner nested form starting with if participant.is_lid() { if let Some(pn) = group_info.phone_jid_for_lid_user(&participant.user) { members.insert(pn.user.clone()); } } with an if using a let-chain (if participant.is_lid() && let Some(pn) = group_info.phone_jid_for_lid_user(&participant.user) { members.insert(pn.user.clone()); }), keeping the existing else-if branch that uses group_info.lid_user_for_phone_user(&participant.user); reference symbols: participant.is_lid(), group_info.phone_jid_for_lid_user, group_info.lid_user_for_phone_user, and members.insert.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@src/client/device_registry.rs`:
- Around line 100-123: The memo hit logic uses the same pre-resolve generation
for both checking cache validity and later resolving devices, which can return a
stale memo if topology advanced between those reads; change the ordering in the
group device lookup (the code paths around device_topology.current(),
group_devices_memo.get(group).await, the memo.generation check,
unchanged_for(...), group_devices_memo.insert(...), and the later
get_user_devices call) so you first read a fresh current_generation and use that
for the memo hit/unchanged_for validation/hit-return paths, and only after
deciding to compute devices read the pre-resolve generation used immediately
before calling get_user_devices and storing a new GroupDevicesMemo; ensure you
compare memo.generation against the fresh current_generation for hits and
reserve the original pre-resolve generation only for the subsequent
compute/insert step.
- Around line 942-953: The code currently calls self.device_topology.record([pn,
lid]) before checking backend.update_device_list's result, causing the same
topology change to be recorded twice because DeviceRegistryCache::insert also
records on success; fix this by removing the unconditional
self.device_topology.record call and only record once on the successful path
(i.e., after backend.update_device_list returns Ok or rely on
device_registry_cache.insert's recording), updating the block around
backend.update_device_list, the error branch that logs the warn, and the
subsequent self.device_registry_cache.insert call to ensure a single record
occurs.
- Around line 166-170: The nested if-let should be collapsed into a let-chain:
replace the inner nested form starting with if participant.is_lid() { if let
Some(pn) = group_info.phone_jid_for_lid_user(&participant.user) {
members.insert(pn.user.clone()); } } with an if using a let-chain (if
participant.is_lid() && let Some(pn) =
group_info.phone_jid_for_lid_user(&participant.user) {
members.insert(pn.user.clone()); }), keeping the existing else-if branch that
uses group_info.lid_user_for_phone_user(&participant.user); reference symbols:
participant.is_lid(), group_info.phone_jid_for_lid_user,
group_info.lid_user_for_phone_user, and members.insert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c7c334d-d4ae-44e1-a642-61313cb3d727
📒 Files selected for processing (1)
src/client/device_registry.rs
…ings - Key the memo off the pre-ensure GroupInfo Arc and append self inside the derivation: groups whose server snapshot omits self used to get a fresh Arc per send from ensure_self_in_group, making the memo permanently miss (CodeRabbit + internal adversarial review). - Disable the memo when the device registry or LID-PN caches are store-backed: a shared external store can be written by other processes the in-process topology tracker cannot observe, so the freshness contract does not hold there (internal adversarial review). - Second invalidate in invalidate_device_cache after the DB delete, mirroring the canonical-flip guard: a concurrent reader could promote() the doomed DB row back between the invalidate and the delete commit. - Record the migration only once on success (the fused insert already records); the manual record stays on the backend-error early return (CodeRabbit). - Fix the member-set comment (the alias coverage comes from writes recording lookup.all_keys(), not from device users) and a let-chain.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/device_registry.rs (1)
384-392:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop double-recording the original lookup key.
lookup.all_keys()already includes the incoming identifier, so chainingoriginal_userrecords the same user twice on every write.DeviceTopology::record()stores one log entry per touched user, so this shrinks the bounded log horizon and pushesunchanged_for()into full recomputes earlier than necessary under write bursts.♻️ Minimal fix
self.device_registry_cache .insert( canonical_key.clone(), Arc::new(record_for_cache), lookup .all_keys() - .into_iter() - .chain(std::iter::once(original_user.as_str())), + .into_iter(), ) .await;Apply the same change in
update_device_lists(...).Also applies to: 451-459
🤖 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/device_registry.rs` around lines 384 - 392, The insert call into device_registry_cache is double-recording the incoming identifier because lookup.all_keys() already contains original_user; remove the chained std::iter::once(original_user.as_str()) from the iterator passed to device_registry_cache.insert so only lookup.all_keys() (plus any truly extra keys) are recorded; apply the same removal in the corresponding update_device_lists(...) insertion site to avoid duplicating entries that cause DeviceTopology::record() to log the same user twice.
🤖 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.
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 384-392: The insert call into device_registry_cache is
double-recording the incoming identifier because lookup.all_keys() already
contains original_user; remove the chained
std::iter::once(original_user.as_str()) from the iterator passed to
device_registry_cache.insert so only lookup.all_keys() (plus any truly extra
keys) are recorded; apply the same removal in the corresponding
update_device_lists(...) insertion site to avoid duplicating entries that cause
DeviceTopology::record() to log the same user twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 782b2917-5ef3-4128-86a9-934f481e5479
📒 Files selected for processing (4)
src/client.rssrc/client/device_registry.rssrc/client/lifecycle.rssrc/send.rs
This was referenced Jun 10, 2026
This was referenced Jul 10, 2026
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
1 participant
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.
Problem
Every group send resolves the full participant device set (it feeds the phash and the SKDM filter), paying the per-member registry fan-out each time: with the directed probe from #823 that is still 2 cache lookups per participant, ~514us per warm send to an 800-member group, repeated on every message to that group.
Change
The resolved (and LID-converted) device list is memoized per group, with the correctness machinery designed so its classic failure modes cannot happen:
DeviceRegistryCache(the registry cache newtype whose only write entry points record the change; the DB-promote path is a separate, documentedpromotesince it copies what the fallback already answered) andLidPnCache::add(mapping changes alter which canonical record either key resolves to; this also covers the startup warm-up, which loopsadd). A future write path cannot forget a bump because there is no unrecorded write API.Weak. The memo validates by pointer identity (any metadata refresh or membership change produces a new Arc, invalidating for free), ABA-safe because the Weak keeps the allocation alive, while the old snapshot's heavy data (participants, maps) is released as soon as the metadata cache refreshes. The status path builds a freshGroupInfoper send (no stable identity), so it keeps the unmemoized resolution.Arc<Vec<Jid>>end to end (the phash consumer only reads), so a hit is a refcount bump, not a 1600-JID Vec clone.resolve_skdm_targetswas split into shared helpers with two thin variants (status unmemoized, cached-group memoized), and the tracing spans sit on the resolvers.Benchmark
Release, warm caches, 800 members x 2 devices, back-to-back on the same machine:
Tests
group_devices_memo_hits_and_invalidates: a raw cache change without a recorded write is served stale (proving hits are hits), a member change recomputes, and a refreshedGroupInfoArc recomputes by identity.group_devices_memo_scoped_invalidation: unrelated-user changes re-stamp without recomputing; a member change, a global event, and a log overflow each recompute.group_devices_memo_invalidated_by_member_mapping_change: a LID mapping learned for a member invalidates even though the group only knows the PN namespace (writes log both keys).topology_mutators_bump_the_generation: every mutator (including the migration with a real PN-keyed row) records a change.cargo fmt --allcargo clippy --all-targets -- -D warningscargo test -p whatsapp-rust --lib(763 passing)cargo test -p wacore(995 passing)wasm32 lib build with the CI invocation
Breaking
None. Public API unchanged; the status send path behavior is untouched.