perf(group): keep warm sends warm under the own-device SKDM steady state - #1021
Merged
Conversation
Since own devices are never memoized warm (WA Web's !isMeDevice guard on markHasSenderKey), every warm group send resolves a non-empty needs_skdm set — and that tripped the cold-send machinery on every send: take the group distribution lock, invalidate the device map cache, re-read it from SQLite and resolve targets a second time. WA Web has no per-send counterpart to any of this: getGroupSenderKeyList reads the in-memory participant map and markHasSenderKey mutates it after the ack. Three changes, wire behavior unchanged (own devices still re-receive their SKDM on every send): - send_group_branch treats an own-devices-only needs set as the warm steady state: no distribution guard, no invalidation, no re-resolve. - update_sender_key_devices drops its unconditional cache invalidation; set_sender_key_status_for_devices already invalidates when it actually writes a new warm mark, and an own-only set writes nothing. - skdm_warm_memo stores the memoized needs set (empty or own-only) so the warm fast path works again — it had been dead since the needs set stopped ever being empty.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe client warm memo now stores memoized SKDM needs. Sender logic recognizes own-device-only needs, preserves warm memoization, limits sender-key device cache invalidation to failures, and tests classification and cache retention. ChangesSKDM warm-path refinement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant send_group_branch
participant resolve_skdm_targets_memoized
participant Client
participant SenderKeyDeviceMap
send_group_branch->>resolve_skdm_targets_memoized: resolve SKDM targets
resolve_skdm_targets_memoized->>Client: read skdm_warm_memo
Client-->>resolve_skdm_targets_memoized: memoized needs
resolve_skdm_targets_memoized->>SenderKeyDeviceMap: classify own-device-only needs
SenderKeyDeviceMap-->>send_group_branch: own-only needs
send_group_branch-->>send_group_branch: retain warm send path
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 |
|
| Filename | Overview |
|---|---|
| src/client.rs | Adds a typed SKDM warm memo entry that includes the device/map pair, generation, sending identity, and memoized targets. |
| src/send/mod.rs | Updates SKDM warm-path classification, memo reuse, distribution lock handling, and sender-key cache invalidation behavior. |
Reviews (3): Last reviewed commit: "fix(group): read the map generation afte..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client.rs`:
- Around line 150-159: The SkdmWarmMemoEntry key omits the exact sending
identity, allowing stale memoized targets when own_sending_jid changes. Add a
Jid field to SkdmWarmMemoEntry, store own_sending_jid.clone() when creating
entries, and require exact sender equality on every memo hit in the warm-memo
lookup logic. Add a regression test covering a sender-device change while the
resolved-device Arc remains reusable.
In `@src/send/mod.rs`:
- Around line 1748-1756: Update the post-re-resolution handling in the send flow
near the cold-branch logic so that when the newly resolved needs satisfy
skdm_needs_only_own_devices(&needs, Some(own_jid), Some(own_lid)), the
distribution_guard is released before the network send. Apply the same own-only
warm classification used in the initial match, avoiding cache invalidation,
distribution guarding, and re-resolution for this case.
🪄 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: 18aab5a8-c0ea-4d27-876c-aba7281e4904
📒 Files selected for processing (2)
src/client.rssrc/send/mod.rs
📦 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.
All reported issues were addressed across 2 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- release the distribution guard when the cold-path re-resolve lands on the own-only steady state, mirroring the first-resolution arm - key the warm memo on the exact sending identity, so a mid-session identity change (e.g. LID discovery) can't reuse needs computed for the old sender - invalidate the device-map cache when the warm-mark write errors: a failed write may have partially landed and backends are not required to be atomic
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
A cold flip during the awaited device resolve advanced the map's generation, but both sides of the memo comparison still held the pre-flip value, so the stale memoized needs could be reused for one send. Loading the generation after the resolve (still before the filter, so a flip racing the filter stamps the inserted memo stale) shrinks the window to the same bounded one the unmemoized filter has.
greptile-apps
Bot
dismissed
their stale review
July 11, 2026 01:22
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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 (2)
src/send/mod.rs (2)
3187-3203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify that the external warm mark actually succeeded.
Both success and failure invalidate this cache, so the test passes even if persistence always fails. Assert the member was stored as warm before checking cache rebuilding.
Proposed test hardening
client - .update_sender_key_devices(group, &[own_primary, member]) + .update_sender_key_devices(group, &[own_primary, member.clone()]) .await; + let rows = client + .persistence_manager + .get_sender_key_devices(group) + .await + .unwrap(); + assert!( + rows.iter() + .any(|(jid, has_key)| jid == &member.to_string() && *has_key), + "the external warm mark must persist successfully" + );🤖 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/mod.rs` around lines 3187 - 3203, Strengthen the test around the external warm mark by verifying persistence succeeded before testing cache invalidation. After update_sender_key_devices, query the stored sender-key device state for member and assert it is marked warm, then retain the existing get_or_init and rebuilt assertion.
1155-1180: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRead the generation after the awaited memo lookup.
skdm_warm_memo.get(group).awaitcan suspend aftercached_map_genis captured. A cold flip during that await increments the map generation, but the stale local value still permits returning an outdated memo and omitting the newly cold target for this send.Proposed fix
- let cached_map_gen = cached_map.generation(); + let warm_memo = if self.group_devices_memo_enabled { + self.skdm_warm_memo.get(group).await + } else { + None + }; + let cached_map_gen = cached_map.generation(); ... - if self.group_devices_memo_enabled - && let Some((dw, cw, memo_gen, memo_sender, memo_needs)) = - self.skdm_warm_memo.get(group).await + if let Some((dw, cw, memo_gen, memo_sender, memo_needs)) = warm_memo && std::ptr::eq(dw.as_ptr(), std::sync::Arc::as_ptr(&all_devices))🤖 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/mod.rs` around lines 1155 - 1180, Refresh the cached map generation after the awaited skdm_warm_memo.get(group) lookup and use that refreshed value for the memo_gen comparison in the fast path. Update the logic around skdm_warm_memo.get and cached_map_gen so a generation change during the await prevents returning stale memoized targets.
🤖 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/send/mod.rs`:
- Around line 3187-3203: Strengthen the test around the external warm mark by
verifying persistence succeeded before testing cache invalidation. After
update_sender_key_devices, query the stored sender-key device state for member
and assert it is marked warm, then retain the existing get_or_init and rebuilt
assertion.
- Around line 1155-1180: Refresh the cached map generation after the awaited
skdm_warm_memo.get(group) lookup and use that refreshed value for the memo_gen
comparison in the fast path. Update the logic around skdm_warm_memo.get and
cached_map_gen so a generation change during the await prevents returning stale
memoized targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 90feaf5f-ba77-494a-82c2-cdfb5056a43a
📒 Files selected for processing (1)
src/send/mod.rs
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Optimizes group send cache; modifies core sending logic.
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
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.
Summary
Since #999, own devices are never memoized warm (WA Web's
!isMeDeviceguard onmarkHasSenderKey), so every warm group send resolves a non-emptyneeds_skdmset — and that tripped the cold-send machinery on every send: take the per-group distribution lock, invalidate the device-map cache, re-read it from SQLite (from_db_rows) and resolve targets a second time. The symbolic DHAT profile showed ~430 of 500 warm group sends going through that path.Verified against the captured WA Web bundles that none of this has a counterpart there:
ParticipantStore.getGroupSenderKeyListcomputesskDistribList/skListfrom the in-memory participant map with no storage re-read (own devices sit atfalseforever and re-enter the distrib list each send), andGroupSkmsgJobonly callsmarkHasSenderKeyafter the ack — whose mutation skips own devices. Wire behavior is unchanged by this PR: own devices still re-receive their SKDM on every send, external members still take the cold flow.Three changes:
send_group_branchtreats an own-devices-only needs set as the warm steady state: no distribution guard, no cache invalidation, no re-resolve.update_sender_key_devicesdrops its unconditional cache invalidation —set_sender_key_status_for_devicesalready invalidates when it actually writes a new warm mark, and an own-only set writes nothing.skdm_warm_memostores the memoized needs set (empty or own-only), reviving the warm fast path: it had been dead since needs stopped ever being empty, so the O(devices) filter ran on every send.Besides allocations, this removes one SQLite read from every warm group send and stops the per-group distribution lock from serializing warm sends.
Impact
DHAT group-send A/B (16 members, 500 cycles, 2 runs per side, baseline = main @ 2591b24): -12.1% bytes and -23.2% blocks per cycle (45,584 → 40,068 B; 319.5 → 245.3 blocks). DM ping-pong sanity run shows no regression. 500/500 pongs with 0 lost on both sides — own-device SKDM distribution still flows.
Validation
cargo test -p whatsapp-rust --lib(978 passed; new tests: an own-only SKDM mark keeps the device map cached while an external warm mark still invalidates it, plus the own-only classification gate incl. the empty/external cases)cargo clippy -p whatsapp-rust -p wacore --lib --tests -- -D warningsRUSTFLAGS='--cfg getrandom_backend="wasm_js"' cargo check -p whatsapp-rust --lib --target wasm32-unknown-unknown --no-default-featuresdocs/captured-js:WAWeb/Api/ParticipantStore.js(isMeDeviceguard in the mark mutation; in-memorygetGroupSenderKeyList) andWAWeb/Send/GroupSkmsgJob.js(markHasSenderKeyafter ack only)