fix: invalidate sender key cache on device changes and handle identity notifications - #489
Conversation
…y notifications Root cause of "Waiting for messages" in group chats: when a participant added/removed a device, the sender key device cache was not invalidated, so SKDM was never sent to the new device on next group message. Device notification fixes (verified against WAWeb/Identity/UpdateDeviceTableApi): - patch_device_add: detect genuinely new devices and invalidate sender key device cache so SKDM is sent on next group message - patch_device_remove: delete Signal sessions for removed device (matching WA Web's deleteRemoteInfo), then invalidate sender key cache - Extract shared delete_sessions_for_devices() helper (DRY with clear_device_record) Identity change handler (verified against WAWeb/Handle/IdentityChange): - Handle type="encrypt" notifications with <identity/> child (was silently ignored before) - Clear device record (sessions + sender keys) and invalidate device cache - Ignore companion devices (device != 0), matching WA Web - Dispatch new IdentityChange event for application layer
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds identity-change notification handling and an IdentityChange event; refactors device registry to centralize session deletion and consistently invalidate the sender-key device cache on device add/remove/clear; updates tests to cover cache invalidation and identity-change dispatch. Changes
Sequence DiagramsequenceDiagram
participant Client
participant NotificationHandler
participant DeviceRegistry
participant Cache
participant EventBus
Client->>NotificationHandler: receive <notification type="encrypt"> with <identity/>
NotificationHandler->>NotificationHandler: parse <identity/>, extract from JID and optional lid
NotificationHandler->>DeviceRegistry: load device record for user
DeviceRegistry->>DeviceRegistry: clear_device_record(user) -> delete_sessions_for_devices(device_ids)
DeviceRegistry->>Cache: flush signal_cache / invalidate sender_key_device_cache
NotificationHandler->>Cache: invalidate device_registry_cache(user)
NotificationHandler->>EventBus: dispatch Event::IdentityChange{ user, lid_user }
EventBus->>Client: deliver IdentityChange event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Bencher Report![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: 80a5f5c9d2
ℹ️ 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".
| } | ||
|
|
||
| // Invalidate device cache so next send triggers fresh usync | ||
| client.invalidate_device_cache(&from_jid.user).await; |
There was a problem hiding this comment.
handle_identity_change invalidates caches by from_jid.user only, while the optional lid attribute is merely forwarded in the event and never used to refresh lookup aliases first. When PN↔LID mapping is missing or expired, this means load_device_record/invalidate_device_cache only touch the PN key and can leave LID-keyed device records/sessions behind, so the identity-change cleanup is incomplete and stale state can continue to affect decryption.
Useful? React with 👍 / 👎.
| let servers = [ | ||
| wacore_binary::jid::HIDDEN_USER_SERVER, | ||
| wacore_binary::jid::DEFAULT_USER_SERVER, | ||
| ]; |
There was a problem hiding this comment.
delete_sessions_for_devices now hard-codes only HIDDEN_USER_SERVER and DEFAULT_USER_SERVER, so clear_device_record/patch_device_remove no longer honor other server types when cleaning sessions. For contacts/devices on supported non-PN/LID user domains (for example hosted variants), session deletion is skipped, leaving stale Signal sessions after device removals or identity-change cleanup.
Useful? React with 👍 / 👎.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 53s —— View job Code Review: Sender Key Cache Invalidation & Identity Change Handling
Overall this is solid work — the cache invalidation fixes address a real correctness issue, the DRY refactor is clean, and the tests are well-structured. A few observations: Things that look good
Issues & suggestions1. The 2. Behavior change for The old 3.
4. Identity change doesn't apply cleanup to LID alias ( The Codex reviewer's P1 point has some merit: In practice, // After the existing invalidate_device_cache call:
if let Some(lid_jid) = node.attrs().optional_jid("lid") {
client.invalidate_device_cache(&lid_jid.user).await;
}This is a minor robustness improvement, not a blocker. 5. All three paths ( Benchmark alertThe Bencher alert on VerdictThe PR correctly addresses the "Waiting for messages" bug from stale sender key caches. The implementation is well-aligned with WA Web's behavior, the DRY refactor improves maintainability, and test coverage is good. The issues noted above are minor and non-blocking. |
WA Web's isMePrimary check (IdentityChange.js:58) prevents processing identity change notifications for our own primary device. Without this, we would clear our own sessions and device record.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262167e142
ℹ️ 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".
| ); | ||
|
|
||
| // Load existing record to pass to clear_device_record | ||
| if let Some(record) = client.load_device_record(&from_jid.user).await { |
There was a problem hiding this comment.
Identity-change cleanup is currently gated on load_device_record succeeding, so when the registry was already invalidated (for example the hash-only device-update path calls invalidate_device_cache), this branch skips clear_device_record entirely. In that case stale sender-key tracking is left intact (no clear_all_sender_key_devices() / sender_key_device_cache.invalidate_all()), and later group sends can still treat old devices as has_key=true and skip SKDM redistribution, reproducing undecryptable “waiting for messages” behavior.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes "Waiting for messages" in group chats caused by stale sender key device caches.
Device notification fixes (
src/client/device_registry.rs)patch_device_add: Detects genuinely new devices and callssender_key_device_cache.invalidate_all()so SKDM is sent on next group message. Verified againstWAWeb/Identity/UpdateDeviceTableApiwhere new devices enter the participant store withhas_key=false.patch_device_remove: Deletes Signal sessions for the removed device under both LID and PN addresses (matching WA Web'sdeleteRemoteInfo), then invalidates sender key cache.delete_sessions_for_devices()shared helper, used by bothclear_device_recordandpatch_device_remove. Previouslyclear_device_recordhad an inline loop that was duplicated.Identity change handler (
src/handlers/notification.rs)type="encrypt"notifications with<identity/>child — previously silently ignored because we only checkedfrom == SERVER_JIDfor encrypt notifications.WAWebHandleIdentityChangewhich callsclearDeviceRecordForIdentityChange+deleteRemoteInfo.device != 0), matching WA Web.IdentityChangeevent for application layer (wacore/src/types/events.rs).Test plan
test_patch_device_add_invalidates_sender_key_cache— new device triggers cache invalidationtest_patch_device_add_no_invalidation_when_device_exists— re-adding existing device is no-optest_patch_device_remove_invalidates_sender_key_cache— removed device triggers cache invalidation + session cleanuptest_identity_change_dispatches_event_and_invalidates_cache— identity change clears record and dispatches eventtest_identity_change_ignores_companion_device— companion device identity change is ignoredSummary by CodeRabbit
New Features
Bug Fixes
Tests