fix(send): close SKDM flow gaps for forward secrecy and cache hygiene - #604
Merged
Conversation
Five WA Web alignment issues found while auditing the post-PR-#603 SKDM flow. Each had an empirical regression test before being fixed. R1 — Forward secrecy on participant remove `handle_group_notification` Remove and `Groups::remove_participants` only patched `group_cache`. WA Web's `removeParticipantInfo` (`GroupParticipantHelpers.js`) sets `rotateKey=true` whenever any removed participant had `has_key=true`, then `GroupSkmsgJob.js:238` calls `deleteGroupSenderKeyInfo` to drop the bot's own sender key before the next send. Fix: new `Client::rotate_sender_key_on_participant_remove` helper wired into both paths. Reads `sender_key_devices`, checks if any removed user had `has_key=true`, and if so deletes the bot's own sender key + clears the group's tracker so the next send takes `force_skdm=true` (`!key_exists`) and redistributes to remaining participants. R2 — `patch_device_add` / `patch_device_remove` `invalidate_all()` The previous code dropped every group's sender-key cache when any user's device set changed. WA Web mutates only the affected groups' `senderKey` Map (`UpdateParticipantApi.js`). Fix: drop the global invalidation. Unknown new devices are picked up automatically because `device_has_key()` returns `None` for entries not yet in the map → falls into `needs_skdm` on the next send. R3 — Phash mismatch left persisted state stale `spawn_phash_validation` only invalidated the in-memory cache; reload from DB surfaced the same stale rows. Group sends also skipped the group_cache invalidation that status sends performed. Fix: clear `sender_key_devices` for the chat on mismatch and invalidate `group_cache` for groups too. R4 — Orphan `sender_key_devices` rows after `patch_device_remove` The removed device's row was never deleted, accumulating dead state. Fix: new `delete_sender_key_device_rows(device_jids)` backend method (Sqlite + InMemory + Persistence wrapper). `patch_device_remove` builds candidate JIDs under both LID and PN aliases via `resolve_lookup_keys` and deletes them. R5 — No periodic sender-key rotation WA Web has `EXPIRY_REASON.PERIODIC_ROTATION` in the WAM enum; captured-js doesn't surface the threshold. We never rotated. Fix: when a group send finds the existing sender key with `chain_key.iteration() ≥ 1000`, delete it and clear the tracker so the next send regenerates and full-distributes. Conservative default; can be adjusted if a WA Web reference surfaces.
📝 WalkthroughSummary by CodeRabbit
WalkthroughTargeted per-(user,device) sender-key-device invalidation replaces global wipes, adds a persistence API to delete sender_key_devices rows by device JID, and triggers sender-key rotation when accepted group participants are removed. Storage backends and tests updated to support batched deletion. Changes
Sequence Diagram(s)sequenceDiagram
participant GroupHandler as Group Handler
participant Client as Client (sender_keys)
participant Store as Persistence Store
participant SignalCache as Signal Cache
participant DeviceCache as Device Cache
GroupHandler->>Client: rotate_sender_key_on_participant_remove(group_jid, removed_user_ids)
Client->>Store: read sender_key_devices for group_jid
Store-->>Client: sender_key_devices rows
alt any removed had has_key = true
Client->>SignalCache: delete sender key for group_jid
Client->>SignalCache: flush cached keys
Client->>Store: delete_sender_key_device_rows(device_jids_for_group)
Store-->>Client: deletion result
Client->>DeviceCache: invalidate_entries_for_device(user, device_id) for affected devices
else none had has_key (read succeeded)
Client-->>GroupHandler: no rotation required
end
Client-->>GroupHandler: rotation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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: 3
🤖 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/device_registry.rs`:
- Around line 378-402: delete_sender_key_rows_for_device currently deletes DB
rows but does not evict warmed in-memory sender-key state for groups that
indexed those JIDs; after calling
self.persistence_manager.delete_sender_key_device_rows(&refs).await you must
invalidate the in-memory cache for those JIDs (use the JID strings in
candidates/refs). If there is an existing cache helper, call it (e.g.
self.invalidate_groups_indexing_jids(&refs) or
self.sender_key_cache.invalidate_by_jids(&refs)); otherwise add a small helper
method on the same impl (e.g. invalidate_groups_for_jids(&[&str])) that clears
per-group entries referencing those JIDs and call it after the DB delete; also
add a regression test that warms the group cache for a specific group, calls
delete_sender_key_rows_for_device(user, device_id) and then asserts that
subsequent sender-key lookups for that group do not return has_key for the
removed JID.
In `@src/client/sender_keys.rs`:
- Around line 85-97: The error handling in
rotate_sender_key_on_participant_remove currently returns early on
persistence_manager.get_sender_key_devices failure, leaving the old sender key
in place; change the Err(e) branch to log the error but not return — instead
proceed with rotation (e.g., treat rows as empty or otherwise continue into the
rotation path) so the function always performs key rotation after a participant
removal; update the Err branch that references
persistence_manager.get_sender_key_devices and
rotate_sender_key_on_participant_remove to log the error and continue rather
than returning.
In `@src/send.rs`:
- Around line 699-709: The current code logs and continues when
client.persistence_manager.clear_sender_key_devices(&jid_str).await fails, which
leaves persistent state intact and allows reload of stale rows; instead, when
clear_sender_key_devices returns Err(e) you must prevent fallback to stale
persisted state by propagating the error (return Err) or otherwise forcing a
full-distribution path (e.g., set a force_full_distribution flag) rather than
only invalidating client.sender_key_device_cache; update the branch around
clear_sender_key_devices / sender_key_device_cache.invalidate to either return
the error from this function or set a concrete indicator on client so subsequent
sends cannot reload the stale DB rows.
🪄 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: c2245f0d-6493-4482-a326-3a7b14aa9c5d
📒 Files selected for processing (12)
src/appstate_sync.rssrc/client/device_registry.rssrc/client/sender_keys.rssrc/features/groups.rssrc/handlers/notification.rssrc/send.rssrc/sender_key_device_cache.rssrc/store/persistence_manager.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/store/in_memory.rswacore/src/store/persistence.rswacore/src/store/traits.rs
💤 Files with no reviewable changes (1)
- src/sender_key_device_cache.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: 79ce0b8ce9
ℹ️ 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".
Comment on lines
+387
to
+391
| for server in servers { | ||
| for key in lookup.all_keys() { | ||
| let mut jid = Jid::new(key, server); | ||
| jid.device = device_id as u16; | ||
| candidates.push(jid.to_string()); |
There was a problem hiding this comment.
delete_sender_key_rows_for_device builds candidate JIDs as a Cartesian product of lookup.all_keys() and both servers, which generates impossible identities like pn@lid and lid@s.whatsapp.net. Because delete_sender_key_device_rows deletes across all groups, a numeric collision with another account can remove unrelated sender-key tracking rows and force unnecessary redistributions. Deletion should be limited to valid namespace pairs for mapped users (and only broadened when user type is truly unknown).
Useful? React with 👍 / 👎.
Comment on lines
+703
to
+706
| if (jid.is_group() || jid.is_status_broadcast()) | ||
| && let Err(e) = | ||
| client.persistence_manager.clear_sender_key_devices(&jid_str).await | ||
| { |
There was a problem hiding this comment.
This mismatch handler clears sender_key_devices in a spawned task, but status sends still call update_sender_key_devices immediately after spawning validation. If the ACK is already available and this task runs first, that later update can repopulate the stale rows that were just cleared, so the next send may skip the intended full redistribution after a mismatch. The mismatch clear needs deterministic ordering against post-send row updates.
Useful? React with 👍 / 👎.
- patch_device_remove now also evicts cached has_key entries for the removed (user, device_id). A re-add of the same device_id would otherwise hit the stale `has_key=true` entry and skip SKDM redistribution. New helper `SenderKeyDeviceCache::invalidate_entries_for_device`. - rotate_sender_key_on_participant_remove rotates conservatively when the `sender_key_devices` read fails, instead of returning early. Better to pay the redistribute cost than leave the sender key in place after a remove we couldn't audit. - phash mismatch handler now falls back to deleting the bot's own sender key when `clear_sender_key_devices` fails. With the persisted state potentially stale, the next send is forced through `force_skdm=true` via `!key_exists` instead of trusting the in-memory invalidation alone. - New regression `patch_device_remove_evicts_cached_has_key_for_removed_device`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/device_registry.rs`:
- Around line 396-402: The delete path currently swallows errors from
persistence_manager.delete_sender_key_device_rows (logged with warn!) which can
leave stale has_key=true rows; change this to fail closed by propagating the
error instead of only logging: replace the warn! branch in the
delete_sender_key_rows_for_device flow so that on Err(e) you return Err(e) (or
map it into the surrounding function's error type) so callers know deletion
failed and can retry/abort redistribution; ensure you update the surrounding
function's signature/propagation (the call site of
persistence_manager.delete_sender_key_device_rows and the enclosing
delete_sender_key_rows_for_device logic) to propagate the error instead of
continuing.
🪄 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: bf9fc505-9a79-45ff-82a5-177665a24a57
📒 Files selected for processing (4)
src/client/device_registry.rssrc/client/sender_keys.rssrc/send.rssrc/sender_key_device_cache.rs
Comment on lines
+396
to
+402
| if let Err(e) = self | ||
| .persistence_manager | ||
| .delete_sender_key_device_rows(&refs) | ||
| .await | ||
| { | ||
| warn!("delete_sender_key_rows_for_device: {e}"); | ||
| } |
There was a problem hiding this comment.
Fail closed when sender-key row deletion fails.
At Line 396-402, we only log on delete failure. That leaves stale persisted has_key=true rows; a future re-add of the same device ID can reload stale state and skip redistribution.
Suggested conservative fallback
if let Err(e) = self
.persistence_manager
.delete_sender_key_device_rows(&refs)
.await
{
warn!("delete_sender_key_rows_for_device: {e}");
+ // Conservative fallback: avoid retaining stale SKDM tracker rows.
+ if let Err(e2) = self.persistence_manager.clear_all_sender_key_devices().await {
+ warn!(
+ "delete_sender_key_rows_for_device: fallback clear_all_sender_key_devices failed: {e2}"
+ );
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/device_registry.rs` around lines 396 - 402, The delete path
currently swallows errors from persistence_manager.delete_sender_key_device_rows
(logged with warn!) which can leave stale has_key=true rows; change this to fail
closed by propagating the error instead of only logging: replace the warn!
branch in the delete_sender_key_rows_for_device flow so that on Err(e) you
return Err(e) (or map it into the surrounding function's error type) so callers
know deletion failed and can retry/abort redistribution; ensure you update the
surrounding function's signature/propagation (the call site of
persistence_manager.delete_sender_key_device_rows and the enclosing
delete_sender_key_rows_for_device logic) to propagate the error instead of
continuing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4fdad27fe
ℹ️ 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".
Comment on lines
+103
to
+107
| *has_key | ||
| && jid_str | ||
| .parse::<Jid>() | ||
| .ok() | ||
| .is_some_and(|jid| removed_user_ids.iter().any(|u| *u == jid.user.as_str())) |
There was a problem hiding this comment.
rotate_sender_key_on_participant_remove only treats a removed member as having the key when the removed ID exactly matches jid.user from persisted sender_key_devices rows. Since callers provide a single user form (participant.jid.user), this misses cases where the row is stored under the opposite PN/LID alias (which can happen as mappings are learned/updated). In that case any_had_key is false and rotation is skipped, so an actually removed participant who had previously received SKDM can still decrypt subsequent group messages.
Useful? React with 👍 / 👎.
`delete_sender_key_rows_for_device` now returns `Result<(), StoreError>` instead of swallowing the persistence error with `warn!`. Cache eviction runs only on success. `patch_device_remove` aborts the registry update when the sender-key cleanup fails — a half-applied state where `resolve_devices` reports "gone" but the tracker still vouches `has_key=true` would silently skip SKDM redistribution and leave forward-secrecy gaps the rotation paths were designed to close.
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/device_registry.rs (1)
358-365:⚠️ Potential issue | 🟠 MajorGuard the
u32 -> u16downcast before cleanup.
device_idis au32, but both session deletion and sender-key cleanup silently truncate it tou16. If a bad notification or corrupted record ever carries> u16::MAX, this path cleans up the wrong device and can wipe another device's session/SKDM state.Proposed fix
pub(crate) async fn patch_device_remove(&self, user: &str, device_id: u32) { + let Ok(device_id_u16) = u16::try_from(device_id) else { + warn!("patch_device_remove: device_id {device_id} exceeds u16::MAX; aborting"); + return; + }; + if let Some(mut record) = self.load_device_record(user).await { let before = record.devices.len(); record.devices.retain(|d| d.device_id != device_id); if record.devices.len() != before { if device_id != 0 { - self.delete_sessions_for_devices(user, &[device_id as u16]) + self.delete_sessions_for_devices(user, &[device_id_u16]) .await; } @@ if let Err(e) = self - .delete_sender_key_rows_for_device(user, device_id) + .delete_sender_key_rows_for_device(user, device_id_u16) .await {async fn delete_sender_key_rows_for_device( &self, user: &str, - device_id: u32, + device_id: u16, ) -> Result<(), wacore::store::error::StoreError> { @@ let mut jid = Jid::new(key, server); - jid.device = device_id as u16; + jid.device = device_id; candidates.push(jid.to_string()); @@ for key in lookup.all_keys() { self.sender_key_device_cache - .invalidate_entries_for_device(key, device_id as u16) + .invalidate_entries_for_device(key, device_id) .await; }Also applies to: 399-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/device_registry.rs` around lines 358 - 365, The removal path in patch_device_remove performs a silent downcast from u32 device_id to u16 when calling delete_sessions_for_devices (and similarly where sender-key cleanup is done), which can truncate ids > u16::MAX and delete the wrong device data; update patch_device_remove to check that device_id <= u16::MAX before converting, and if it exceeds u16::MAX skip session/SKDM cleanup (or return an error/log and still persist the device removal) so you never cast blindly; apply the same guarded conversion logic to the other cleanup site referenced (the similar block around lines 399-423) so all u32->u16 downcasts use a checked guard or explicit handling instead of unchecked truncation.
🤖 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/device_registry.rs`:
- Around line 278-280: Update the method docs for patch_device_add to reflect
current behavior: state that new devices are not causing global sender-key
device cache invalidation because resolve_skdm_targets will automatically pick
up unknown devices (device_has_key() returns None and falls into needs_skdm).
Edit the doc comment above the patch_device_add function to remove or replace
any text claiming it invalidates the sender-key device cache and instead
describe the new automatic resolution flow (reference resolve_skdm_targets,
device_has_key, needs_skdm and the sender-key device cache to make the intent
explicit).
---
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 358-365: The removal path in patch_device_remove performs a silent
downcast from u32 device_id to u16 when calling delete_sessions_for_devices (and
similarly where sender-key cleanup is done), which can truncate ids > u16::MAX
and delete the wrong device data; update patch_device_remove to check that
device_id <= u16::MAX before converting, and if it exceeds u16::MAX skip
session/SKDM cleanup (or return an error/log and still persist the device
removal) so you never cast blindly; apply the same guarded conversion logic to
the other cleanup site referenced (the similar block around lines 399-423) so
all u32->u16 downcasts use a checked guard or explicit handling instead of
unchecked truncation.
🪄 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: 9cc7b2cd-f4ce-498e-9d88-3f9ed14838c2
📒 Files selected for processing (1)
src/client/device_registry.rs
…_add doc `patch_device_remove` now uses `u16::try_from` before passing the device id to JID-keyed cleanup paths. Out-of-range ids are logged and the registry update is still applied so `resolve_devices` stops returning the device — blind `as u16` would otherwise truncate to a different id and clean up the wrong device's sessions/sender-key rows. The inner helper takes `u16` directly so the bounds check happens once at the entry point. `patch_device_add`'s doc comment now describes the post-PR-#604 flow (automatic resolution via `resolve_skdm_targets` / `device_has_key`) instead of claiming a global cache invalidation the body no longer does.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/device_registry.rs (1)
388-397:⚠️ Potential issue | 🟠 MajorDon’t silently drop remove events on cleanup failure — bubble an error to the caller.
Right now this just logs + returns. If cleanup fails transiently, the device removal can be lost with no retry signal. We need deterministic failure propagation on this path.
Suggested fix direction
-pub(crate) async fn patch_device_remove(&self, user: &str, device_id: u32) { +pub(crate) async fn patch_device_remove(&self, user: &str, device_id: u32) -> Result<()> { @@ - if let Err(e) = self - .delete_sender_key_rows_for_device(user, device_id_u16) - .await - { - warn!( - "patch_device_remove: sender-key cleanup failed for {user}:{device_id}: {e} \ - — aborting registry update" - ); - return; - } + self + .delete_sender_key_rows_for_device(user, device_id_u16) + .await + .map_err(|e| anyhow::anyhow!( + "patch_device_remove: sender-key cleanup failed for {user}:{device_id}: {e}" + ))?; @@ - return; + return Ok(()); }; @@ -} + Ok(()) +}Also update call sites (e.g., notification handler) to handle
Resultexplicitly and apply retry/backoff policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/device_registry.rs` around lines 388 - 397, The patch_device_remove path currently swallows errors from delete_sender_key_rows_for_device by logging and returning; change patch_device_remove to return a Result (propagate the error) instead of unit, and replace the bare return with Err(e) so failures bubble to callers; update delete_sender_key_rows_for_device call sites (e.g., the notification handler) to handle the Result explicitly and implement retry/backoff or propagate further so transient cleanup failures are retried or surfaced to the caller; ensure function signatures (patch_device_remove and any callers) and error types align to compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/device_registry.rs`:
- Around line 388-397: The patch_device_remove path currently swallows errors
from delete_sender_key_rows_for_device by logging and returning; change
patch_device_remove to return a Result (propagate the error) instead of unit,
and replace the bare return with Err(e) so failures bubble to callers; update
delete_sender_key_rows_for_device call sites (e.g., the notification handler) to
handle the Result explicitly and implement retry/backoff or propagate further so
transient cleanup failures are retried or surfaced to the caller; ensure
function signatures (patch_device_remove and any callers) and error types align
to compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d25b90d-bc45-4810-bd1b-e141448132f8
📒 Files selected for processing (1)
src/client/device_registry.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae0ab110e4
ℹ️ 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".
Comment on lines
+109
to
+110
| if !read_failed && !any_had_key { | ||
| return; |
There was a problem hiding this comment.
This early return treats an empty sender_key_devices read as proof that no removed participant had the key, but the new phash-mismatch path now calls clear_sender_key_devices without deleting the current sender key first. In that state, a later participant removal will skip rotation here, keep using the old sender key, and a previously keyed removed member can still decrypt future group messages. Empty tracker state is "unknown," so this path should rotate (or otherwise force key replacement) instead of returning.
Useful? React with 👍 / 👎.
3 tasks
jlucaso1
added a commit
that referenced
this pull request
Apr 29, 2026
The default Cache type alias falls through to PortableCache when the moka-cache feature is disabled. PR #604 added invalidate_entries_for_device to SenderKeyDeviceCache, which calls self.inner.iter() — moka has it, PortableCache didn't. Builds with --no-default-features broke at src/sender_key_device_cache.rs:93 with "no method named iter". Snapshot the live entries under a brief read lock (same try_read + spin pattern used by entry_count and invalidate_all) and return an owned Vec::IntoIter of (Arc<K>, V), matching moka's iter signature for the call site. The eager snapshot frees the lock before the caller starts iterating, so callers can mutate the cache mid-iteration without deadlock.
jlucaso1
added a commit
that referenced
this pull request
Apr 29, 2026
The default Cache type alias falls through to PortableCache when the moka-cache feature is disabled. PR #604 added invalidate_entries_for_device to SenderKeyDeviceCache, which calls self.inner.iter() — moka has it, PortableCache didn't. Builds with --no-default-features broke at src/sender_key_device_cache.rs:93 with "no method named iter". Snapshot the live entries under a brief read lock (same try_read + spin pattern used by entry_count and invalidate_all) and return an owned Vec::IntoIter of (Arc<K>, V), matching moka's iter signature for the call site. The eager snapshot frees the lock before the caller starts iterating, so callers can mutate the cache mid-iteration without deadlock.
jlucaso1
added a commit
that referenced
this pull request
Apr 29, 2026
The default Cache type alias falls through to PortableCache when the moka-cache feature is disabled. PR #604 added invalidate_entries_for_device to SenderKeyDeviceCache, which calls self.inner.iter() — moka has it, PortableCache didn't. Builds with --no-default-features broke at src/sender_key_device_cache.rs:93 with "no method named iter". Snapshot the live entries under a brief read lock (same try_read + spin pattern used by entry_count and invalidate_all) and return an owned Vec::IntoIter of (Arc<K>, V), matching moka's iter signature for the call site. The eager snapshot frees the lock before the caller starts iterating, so callers can mutate the cache mid-iteration without deadlock.
jlucaso1
added a commit
that referenced
this pull request
Apr 29, 2026
The default Cache type alias falls through to PortableCache when the moka-cache feature is disabled. PR #604 added invalidate_entries_for_device to SenderKeyDeviceCache, which calls self.inner.iter() — moka has it, PortableCache didn't. Builds with --no-default-features broke at src/sender_key_device_cache.rs:93 with "no method named iter". Snapshot the live entries under a brief read lock (same try_read + spin pattern used by entry_count and invalidate_all) and return an owned Vec::IntoIter of (Arc<K>, V), matching moka's iter signature for the call site. The eager snapshot frees the lock before the caller starts iterating, so callers can mutate the cache mid-iteration without deadlock.
This was referenced Jun 4, 2026
JeanCapixaba
pushed a commit
to JeanCapixaba/whatsapp-rust
that referenced
this pull request
Jun 18, 2026
…pos-oxidezap#604) Emite Event::ServerAck { id } em handle_ack_response pra TODO <ack> do servidor com id, em adicao (nao no lugar) a resolucao do waiter interno. Permite medir recebido->aceite-do-servidor sem tocar no fluxo de envio/phash. EventKind no fim (nao desloca discriminantes); on_event do consumidor usa EventInterest::ALL.
JeanCapixaba
pushed a commit
to JeanCapixaba/whatsapp-rust
that referenced
this pull request
Jun 30, 2026
…pos-oxidezap#604) Emite Event::ServerAck { id } em handle_ack_response pra TODO <ack> do servidor com id, em adicao (nao no lugar) a resolucao do waiter interno. Permite medir recebido->aceite-do-servidor sem tocar no fluxo de envio/phash. EventKind no fim (nao desloca discriminantes); on_event do consumidor usa EventInterest::ALL.
JeanCapixaba
pushed a commit
to JeanCapixaba/whatsapp-rust
that referenced
this pull request
Jul 1, 2026
…pos-oxidezap#604) Emite Event::ServerAck { id } em handle_ack_response pra TODO <ack> do servidor com id, em adicao (nao no lugar) a resolucao do waiter interno. Permite medir recebido->aceite-do-servidor sem tocar no fluxo de envio/phash. EventKind no fim (nao desloca discriminantes); on_event do consumidor usa EventInterest::ALL.
JeanCapixaba
pushed a commit
to JeanCapixaba/whatsapp-rust
that referenced
this pull request
Jul 6, 2026
…pos-oxidezap#604) Emite Event::ServerAck { id } em handle_ack_response pra TODO <ack> do servidor com id, em adicao (nao no lugar) a resolucao do waiter interno. Permite medir recebido->aceite-do-servidor sem tocar no fluxo de envio/phash. EventKind no fim (nao desloca discriminantes); on_event do consumidor usa EventInterest::ALL.
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.
Audit of the SKDM flow post-#603 surfaced five gaps versus WA Web. Each is fixed and covered by a regression test.
R1 — Forward secrecy on participant remove
handle_group_notificationRemove andGroups::remove_participantsonly patchedgroup_cache. WA Web's `removeParticipantInfo` (`GroupParticipantHelpers.js`) sets `rotateKey=true` whenever any removed participant had `has_key=true`, then `GroupSkmsgJob.js:238` calls `deleteGroupSenderKeyInfo` to drop the bot's own sender key before the next send. Without this rotation a kicked participant who already had the SKDM keeps decrypting future skmsg payloads using the cached sender key.Fix: new `Client::rotate_sender_key_on_participant_remove` wired into both paths. Reads `sender_key_devices`, checks if any removed user had `has_key=true`, and if so deletes the bot's own sender key + clears the tracker so the next send takes `force_skdm=true` (`!key_exists`) and redistributes to remaining participants.
R2 — Sledgehammer
invalidate_all()on device patches`patch_device_add` and `patch_device_remove` were dropping every group's sender-key cache when any user's device set changed. WA Web's `updateGroupParticipantsInTransaction` mutates only the affected groups' `senderKey` Map.
Fix: drop both global invalidations. New devices are picked up automatically because `device_has_key()` returns `None` for entries not yet in the map → falls into `needs_skdm` on the next send.
R3 — Phash mismatch left persisted state stale
`spawn_phash_validation` only invalidated the in-memory cache; the next send reloaded the same stale rows from DB. Group sends also skipped the group_cache invalidation that status sends performed.
Fix: clear `sender_key_devices` for the chat on mismatch and invalidate `group_cache` for groups too. Next send takes the full-distribution path.
R4 — Orphan rows after
patch_device_removeThe removed device's row in `sender_key_devices` was never deleted, accumulating dead state. WA Web (`UpdateParticipantApi.js:42`) does `senderKey.delete(deviceJid)` per affected group.
Fix: new `delete_sender_key_device_rows(device_jids)` backend method (Sqlite + InMemory + Persistence wrapper). `patch_device_remove` builds candidate JIDs under both LID and PN aliases via `resolve_lookup_keys` and deletes them.
R5 — No periodic sender-key rotation
WA Web has `EXPIRY_REASON.PERIODIC_ROTATION` in `WAWebWamEnumExpiryReason`; the threshold itself isn't visible in captured-js (likely server-pushed config). We never rotated, so long-running bots used the same sender key for months.
Fix: when a group send finds the existing sender key with `chain_key.iteration() ≥ 1000`, delete it and clear the tracker so the next send regenerates and full-distributes. Conservative default; can be adjusted if a real reference surfaces.
Tests
R3 is implicitly covered by `resolve_skdm_targets_distributes_when_cache_empty_but_devices_known` from #603 — the phash fix calls `clear_sender_key_devices` whose post-condition is the same empty-cache state.
R5 has no unit test because the threshold check is inline in `send_message_impl` and exercising it requires a connected client; manually validated by code review.
Test plan