fix!: decode key-index-list to filter stale devices from device registry - #469
Conversation
Closes #468. Device add notifications include ADVSignedKeyIndexList protobuf in key-index-list that encodes which device key indices are still valid. We stored these bytes but never decoded them, so stale devices accumulated in the registry causing persistent 406 errors on group sends. Changes: - wacore/adv: shared utility to decode ADVKeyIndexList and filter devices by valid_indexes (matching WA Web AdvDeviceNotificationApi) - patch_device_add: decode key-index-list, filter stale devices, detect raw_id mismatch for identity change (clearDeviceRecord) - usync response: parse key-index-list, apply same filtering, reject companion devices without signedKeyIndexBytes (AdvForUsyncApi) - encrypt_for_devices: catch 406 IQ errors (device unregistered) via typed downcast instead of propagating fatal error - SKDM distribution: wrap in match so failures don't kill group send (matching WA Web GroupSkmsgJob try/catch pattern) - DeviceListRecord: add raw_id field with DB migration
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughImplements ADV key-index decoding/filtering for device add notifications and usync responses, persists nullable Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Notification Handler
participant Registry as Device Registry
participant Decoder as Key-Index Decoder
participant Filter as Device Filter
participant Store as Storage
participant Cache as Session & SenderKey Cache
Handler->>Registry: patch_device_add(user, device, key_index_info)
Registry->>Store: load_device_record(user)
Store-->>Registry: existing_record?
alt signed_bytes present
Registry->>Decoder: decode_key_index_list(signed_bytes)
Decoder-->>Registry: DecodedKeyIndex
alt raw_id mismatch
Registry->>Cache: clear_device_record(user) — delete sessions, invalidate sender-key cache
Cache-->>Registry: cleared
Registry->>Registry: record.devices = []
end
Registry->>Filter: filter_devices_by_key_index(existing_devices, decoded)
Filter-->>Registry: filtered_devices
Registry->>Registry: append_device_if_new(device, filtered_devices)
else fallback (no signed_bytes or decode fails)
Registry->>Registry: append_device_if_new(device, existing_devices)
end
Registry->>Store: update_device_list(user, updated_record with raw_id)
Store-->>Registry: persist result
sequenceDiagram
participant Usync as USYNC parser
participant Validator as Companion Validator
participant Decoder as Key-Index Decoder
participant Filter as Device Filter
participant Store as Storage
Usync->>Usync: parse_get_user_devices_response_with_phash()
Usync->>Validator: detect companion devices (device != 0)
alt companion && key_index_bytes missing
Validator-->>Usync: log warning, skip user
else
Usync->>Decoder: decode_key_index_list(key_index_bytes)
alt decode succeeds
Decoder-->>Usync: DecodedKeyIndex
Usync->>Filter: filter_devices_by_key_index(devices, decoded)
Filter-->>Usync: filtered_devices
else decode fails
Usync->>Usync: use unfiltered devices
end
Usync->>Store: update_device_list(user, DeviceListRecord with raw_id)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: d4cfd34dbd
ℹ️ 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".
| log::warn!( | ||
| "Prekey fetch returned 406 (device unregistered) for {} devices, skipping all: {e}", | ||
| jids_needing_prekeys.len() | ||
| ); | ||
| std::collections::HashMap::new() |
There was a problem hiding this comment.
When fetch_prekeys_for_identity_check returns a top-level 406, this branch replaces the entire bundle map with an empty map, so every device in jids_needing_prekeys is skipped, not just the stale one. In prepare_dm_stanza, encrypt_for_devices is used for recipient encryption; if all recipient sessions are missing, this can produce no encrypted participant nodes and still continue building the stanza, causing messages to be sent without decryptable payloads for intended recipients instead of retrying with valid devices.
Useful? React with 👍 / 👎.
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/usync.rs (1)
140-146:⚠️ Potential issue | 🟠 MajorReturn the filtered devices on the cache-miss path.
The registry is updated with the filtered
devices, but the value returned to the caller still comes from the rawresponse.device_lists. The first send after a cache miss will therefore target the stale companions this PR just removed from storage.💡 Suggested direction
- for user_list in &response.device_lists { + let mut fetched_devices = Vec::new(); + for user_list in &response.device_lists { // Update device registry (single source of truth for device lists). // Preserve key_index values from existing records (set via account_sync) let existing_record = self @@ // Apply valid_indexes filtering if key-index-list was decoded if let Some(ref decoded) = decoded_key_index { devices = wacore::adv::filter_devices_by_key_index(&devices, decoded); } + + fetched_devices.extend(devices.iter().filter_map(|d| { + u16::try_from(d.device_id).ok().map(|device_id| { + let mut jid = user_list.user.clone(); + jid.device = device_id; + jid + }) + })); let device_list = wacore::store::traits::DeviceListRecord { user: user_list.user.user.clone(), @@ - let fetched_devices: Vec<Jid> = response - .device_lists - .into_iter() - .flat_map(|u| u.devices) - .collect(); all_devices.extend(fetched_devices);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/usync.rs` around lines 140 - 146, The code extends all_devices from the raw response.device_lists (fetched_devices) instead of the filtered list that was written to the registry, causing the first post-cache-miss send to include removed companions; change the extension to use the filtered variable (the `devices` you already computed and stored in the registry) — e.g., stop collecting from `response.device_lists` into `fetched_devices` and instead extend `all_devices` with the filtered `devices` (or replace `fetched_devices` with a collection sourced from that filtered `devices`) so the returned list matches what was saved to the registry.src/handlers/notification.rs (1)
494-505:⚠️ Potential issue | 🟠 MajorPreserve the existing
raw_idwhenaccount_syncdoesn't carry one.
update_device_list()upsertsraw_id, so writingNonehere erases any value previously learned from usync/device notifications. After that, a real identity change on our own device list will no longer trip the raw_id-mismatch invalidation path. Carry the storedraw_idforward whenaccount_synchas no replacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 494 - 505, When constructing the DeviceListRecord in the notification handler, do not always set raw_id to None; instead preserve the existing stored raw_id when account_sync provides no replacement so update_device_list() doesn't erase previously learned raw_id from usync/device notifications. Locate the DeviceListRecord creation (symbol: DeviceListRecord) around the notification path that uses from_jid and devices, check for an incoming account_sync raw_id (or absence thereof) and set DeviceListRecord.raw_id to the existing stored value when account_sync has no raw_id; ensure update_device_list() receives that preserved raw_id so raw_id-mismatch invalidation still triggers on real identity changes.
🤖 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 273-286: clear_device_record currently only invalidates the
in-memory cache via sender_key_device_cache.invalidate_all(), but must also
purge persisted sender-key tracking, stored sender keys, and Signal sessions for
the user so stale identity state cannot be resurrected; update
clear_device_record to, after invalidating the in-memory cache, call the
persistence-layer clear methods (e.g.
sender_key_tracking_store.remove_user(user), sender_key_store.remove_user(user),
and session_store.delete_sessions_for_user(user) or whatever concrete APIs exist
in your codebase) to delete persisted sender-key tracking, sender keys, and
Signal sessions for the given user/record.
- Around line 227-237: After calling
wacore::adv::filter_devices_by_key_index(&record.devices, &decoded), avoid
blindly re-adding the notified device; change the logic in the block using
record.devices, device_id, device.key_index and decoded so the push only happens
when the notified device's key_index is accepted by the decoded ADV list (i.e.,
present in decoded's valid indexes and not stale) — check the
decoded/valid-index condition before record.devices.push to prevent
reintroducing devices that filter_devices_by_key_index just removed.
In `@src/usync.rs`:
- Around line 87-123: The code uses existing_key_indices (built before the
raw_id mismatch check) after calling clear_device_record(), which reuses old
identity key indexes and can drop valid devices; update the logic so that when
decoded_key_index and existing_record exist and stored_raw_id != decoded.raw_id
(the same condition where you call clear_device_record), you also invalidate any
preserved identity state: clear or replace existing_key_indices (or set it to an
empty map) and avoid preserving existing.raw_id into raw_id so subsequent device
mapping/filtering (the devices vector and the call to
wacore::adv::filter_devices_by_key_index) uses only the fresh usync data; touch
the mismatch branch that contains decoded_key_index, existing_record,
stored_raw_id != decoded.raw_id and modify handling of existing_key_indices and
raw_id there to ensure old indexes aren’t reused.
- Around line 62-79: existing_record is fetched using the literal key
user_list.user.user which misses LID aliases; instead resolve the canonical user
key via the same alias-aware path used by the device-registry helpers (the path
used by update_device_list) before calling get_devices. Change the lookup to
first resolve the alias (e.g. call the registry/alias resolver used by the
device-registry helpers or a resolve_alias/resolve_user_key helper) to obtain
the canonical ID, then pass that canonical ID into
persistence_manager.backend().get_devices(...) so existing_record reflects
entries stored under LID and preserves key_index/raw_id for ADV filtering.
---
Outside diff comments:
In `@src/handlers/notification.rs`:
- Around line 494-505: When constructing the DeviceListRecord in the
notification handler, do not always set raw_id to None; instead preserve the
existing stored raw_id when account_sync provides no replacement so
update_device_list() doesn't erase previously learned raw_id from usync/device
notifications. Locate the DeviceListRecord creation (symbol: DeviceListRecord)
around the notification path that uses from_jid and devices, check for an
incoming account_sync raw_id (or absence thereof) and set
DeviceListRecord.raw_id to the existing stored value when account_sync has no
raw_id; ensure update_device_list() receives that preserved raw_id so
raw_id-mismatch invalidation still triggers on real identity changes.
In `@src/usync.rs`:
- Around line 140-146: The code extends all_devices from the raw
response.device_lists (fetched_devices) instead of the filtered list that was
written to the registry, causing the first post-cache-miss send to include
removed companions; change the extension to use the filtered variable (the
`devices` you already computed and stored in the registry) — e.g., stop
collecting from `response.device_lists` into `fetched_devices` and instead
extend `all_devices` with the filtered `devices` (or replace `fetched_devices`
with a collection sourced from that filtered `devices`) so the returned list
matches what was saved to the registry.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21a53bc2-0fa2-4c59-89a6-919988d947ce
📒 Files selected for processing (14)
src/client/device_registry.rssrc/handlers/notification.rssrc/sender_key_device_cache.rssrc/usync.rsstorages/sqlite-storage/migrations/2026-03-31-000000_add_device_registry_raw_id/down.sqlstorages/sqlite-storage/migrations/2026-03-31-000000_add_device_registry_raw_id/up.sqlstorages/sqlite-storage/src/schema.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/adv.rswacore/src/iq/usync.rswacore/src/lib.rswacore/src/send.rswacore/src/store/traits.rswacore/src/usync.rs
- clear_device_record: delete Signal sessions for non-primary devices from cache + DB (not just in-memory cache invalidation) - patch_device_add: validate new device key_index against valid_indexes before pushing (prevents reintroducing stale devices) - usync: clear existing_key_indices after raw_id mismatch so old identity key indices aren't reused for fresh devices - usync: use alias-aware load_device_record instead of raw get_devices to find records stored under LID when usync returns PN - usync: return filtered devices (matching registry) instead of raw unfiltered response devices - account_sync: preserve existing raw_id instead of always setting None
There was a problem hiding this comment.
Actionable comments posted: 2
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)
736-771: 🧹 Nitpick | 🔵 TrivialConsider adding tests for
patch_device_addwith actualkey_index_info.The existing tests pass
Noneforkey_index_info, exercising only the lenient fallback path. Consider adding tests that:
- Provide valid
KeyIndexInfowithsigned_bytes(mock protobuf)- Verify stale devices are filtered out
- Verify devices with invalid
key_indexare rejected- Verify
raw_idmismatch triggersclear_device_recordThis would provide coverage for the new ADV filtering logic in
patch_device_add.🤖 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 736 - 771, Tests for patch_device_add only exercise the None key_index_info path; add unit tests that pass real KeyIndexInfo (including signed_bytes mock protobuf) to exercise the ADV filtering and key-index validation paths. Create tests that: call patch_device_add with make_device_element providing KeyIndexInfo containing signed_bytes for a valid key_index and assert the device is accepted and stored; supply a KeyIndexInfo whose signed_bytes indicate a stale timestamp and assert stale devices are filtered out; supply elements with invalid/out-of-range key_index and assert they are rejected; and supply an element whose raw_id does not match the cached DeviceListRecord to assert clear_device_record is invoked and the record cleared. Reference patch_device_add, make_device_element, KeyIndexInfo, device_registry_cache, and clear_device_record when adding these cases.
🤖 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 287-300: The code currently only invalidates the in-memory
sender_key_device_cache (self.sender_key_device_cache.invalidate_all()) after
deleting sessions and flushing the signal cache, but does not clear persisted
SKDM tracking; call
self.persistence_manager.clear_sender_key_devices(group_id).await (or the async
equivalent) for each group that the user belongs to when the identity/raw_id
changes—mirroring the pattern used in retry.rs (where sender key rotation clears
persisted tracking) so persisted sender_key_devices rows are removed and
replacement devices will receive redistributions; insert this call alongside the
flush_signal_cache() and cache invalidation steps in the same function (e.g.,
clear_device_record) to ensure both in-memory and persisted SKDM state are
cleared.
In `@wacore/src/adv.rs`:
- Around line 74-82: The function is_key_index_valid currently does O(n) lookups
via DecodedKeyIndex.valid_indexes.contains(&ki); change it to use a HashSet like
filter_devices_by_key_index to keep complexity consistent: either (A) accept a
&HashSet<u32> (or &DecodedKeyIndex::valid_index_set) instead of
&DecodedKeyIndex, or (B) add a method on DecodedKeyIndex (e.g., fn
contains_index(&self, idx: u32) -> bool) that checks a cached HashSet
representation and call that from is_key_index_valid; update the call sites
accordingly so is_key_index_valid uses HashSet::contains for O(1) lookups and
retains the same behavior with Some(ki) and None case.
---
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 736-771: Tests for patch_device_add only exercise the None
key_index_info path; add unit tests that pass real KeyIndexInfo (including
signed_bytes mock protobuf) to exercise the ADV filtering and key-index
validation paths. Create tests that: call patch_device_add with
make_device_element providing KeyIndexInfo containing signed_bytes for a valid
key_index and assert the device is accepted and stored; supply a KeyIndexInfo
whose signed_bytes indicate a stale timestamp and assert stale devices are
filtered out; supply elements with invalid/out-of-range key_index and assert
they are rejected; and supply an element whose raw_id does not match the cached
DeviceListRecord to assert clear_device_record is invoked and the record
cleared. Reference patch_device_add, make_device_element, KeyIndexInfo,
device_registry_cache, and clear_device_record when adding these cases.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e241ee9c-5785-417c-9805-01f468740c08
📒 Files selected for processing (4)
src/client/device_registry.rssrc/handlers/notification.rssrc/usync.rswacore/src/adv.rs
| /// Check if a key_index is accepted by the decoded ADV list. | ||
| /// Used to validate a newly-notified device before adding it to the registry. | ||
| pub fn is_key_index_valid(key_index: Option<u32>, decoded: &DecodedKeyIndex) -> bool { | ||
| match key_index { | ||
| Some(ki) => decoded.valid_indexes.contains(&ki) || ki > decoded.current_index, | ||
| // No key_index — can't validate, accept to be lenient | ||
| None => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using HashSet for consistency with filter_devices_by_key_index.
is_key_index_valid uses Vec::contains() (O(n)) while filter_devices_by_key_index builds a HashSet for the same lookup. For small valid_indexes lists this is fine, but for consistency and if this is called in a loop, consider accepting a pre-built HashSet or using the same pattern.
♻️ Optional optimization
-pub fn is_key_index_valid(key_index: Option<u32>, decoded: &DecodedKeyIndex) -> bool {
- match key_index {
- Some(ki) => decoded.valid_indexes.contains(&ki) || ki > decoded.current_index,
- // No key_index — can't validate, accept to be lenient
- None => true,
- }
+pub fn is_key_index_valid(
+ key_index: Option<u32>,
+ decoded: &DecodedKeyIndex,
+ valid_set: Option<&std::collections::HashSet<u32>>,
+) -> bool {
+ match key_index {
+ Some(ki) => {
+ let is_valid = valid_set
+ .map(|s| s.contains(&ki))
+ .unwrap_or_else(|| decoded.valid_indexes.contains(&ki));
+ is_valid || ki > decoded.current_index
+ }
+ None => true,
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/adv.rs` around lines 74 - 82, The function is_key_index_valid
currently does O(n) lookups via DecodedKeyIndex.valid_indexes.contains(&ki);
change it to use a HashSet like filter_devices_by_key_index to keep complexity
consistent: either (A) accept a &HashSet<u32> (or
&DecodedKeyIndex::valid_index_set) instead of &DecodedKeyIndex, or (B) add a
method on DecodedKeyIndex (e.g., fn contains_index(&self, idx: u32) -> bool)
that checks a cached HashSet representation and call that from
is_key_index_valid; update the call sites accordingly so is_key_index_valid uses
HashSet::contains for O(1) lookups and retains the same behavior with Some(ki)
and None case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11a244a8b8
ℹ️ 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".
| ); | ||
| self.clear_device_record(user, &device.jid.server, &record) | ||
| .await; | ||
| record.devices.clear(); |
There was a problem hiding this comment.
Clearing record.devices here drops device 0 before the new list is rebuilt, but add notifications usually carry only the newly added companion. In that case the persisted registry can end up containing only the companion device, and later get_user_devices treats the registry as authoritative (cache hit path) and never refetches from usync, so group fanout can skip the user’s primary phone entirely. Keeping/rehydrating primary 0 after a raw-id reset avoids silently under-targeting recipients.
Useful? React with 👍 / 👎.
| // Invalidate sender_key_device_cache so stale SKDM tracking is discarded. | ||
| // Global invalidation because we don't track which groups a user is in. | ||
| self.sender_key_device_cache.invalidate_all(); |
There was a problem hiding this comment.
This only invalidates the in-memory cache, but SKDM targeting is repopulated from persisted rows (resolve_skdm_targets reloads via get_sender_key_devices). After a raw_id identity change, stale has_key=true DB entries remain and are reloaded, so the sender still believes the recipient already has sender keys and may skip redistribution, causing ongoing group decrypt failures until another path clears DB state.
Useful? React with 👍 / 👎.
When SKDM distribution fails with 406 (device unregistered), the stale device was never marked as "sent" in SKDM tracking, causing a failing network round-trip (~2s) on every subsequent group message. Now: - Mark distribution list as "sent" when all devices had missing prekeys or the IQ returned 406 — prevents retry loops - Invalidate device registry for affected users so the next send gets a fresh device list from the server (without the stale device) - Propagate skdm_had_unregistered_devices flag through PreparedGroupStanza so both send paths (group + status) handle it
…es as sent Removes the hack that marked failed SKDM devices as "sent" in tracking. WA Web never does this — failed devices stay in the distribution list for retry (matching GroupSkmsgJob behavior). Instead of preventing retries by lying about success: - Only mark actually encrypted devices as "sent" (WA Web compliant) - Invalidate device registry for specific users whose devices returned 406 — the next send re-fetches from server, stale device is pruned - Compute stale_device_users by diffing distribution_list vs successfully encrypted devices (zero unnecessary clones) - First send: one 406 delay, second send: clean (stale device pruned)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8af6e6835
ℹ️ 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".
| if actually_encrypted.is_empty() && !distribution_list.is_empty() { | ||
| skdm_encrypted_devices = distribution_list.clone(); | ||
| had_unregistered_devices = true; |
There was a problem hiding this comment.
This branch treats actually_encrypted.is_empty() as if all targets were just unregistered and copies distribution_list into skdm_encrypted_devices, but an empty result can also happen for non-406 paths (e.g., no bundles returned or per-device encryption failures). The caller later persists skdm_encrypted_devices via update_sender_key_devices, so devices that never received SKDM get recorded as has_key=true, and future resolve_skdm_targets runs can skip redistribution and leave recipients unable to decrypt group messages.
Useful? React with 👍 / 👎.
| if update.had_unregistered { | ||
| for device in &update.devices { | ||
| self.invalidate_device_cache(&device.user).await; |
There was a problem hiding this comment.
Invalidating only the in-memory device cache here does not force a server refresh: get_user_devices falls back to the persisted device_registry row through get_devices_from_registry, treats it as a cache hit, and skips usync. That means stale/unregistered devices remain authoritative after this branch runs, so repeated sends can keep reusing the same bad device set and re-triggering 406 behavior.
Useful? React with 👍 / 👎.
| self.clear_device_record( | ||
| &user_list.user.user, | ||
| &user_list.user.server, | ||
| existing, |
There was a problem hiding this comment.
This cleanup call passes the usync user/server directly, but clear_device_record deletes sessions by reconstructing addresses from those exact values; for mapped contacts that commonly means PN addresses. Since encryption/session establishment is LID-first for mapped users, stale LID sessions can survive a raw_id mismatch and continue being used after an identity reset, causing ongoing identity/session inconsistencies.
Useful? React with 👍 / 👎.
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/send.rs`:
- Around line 1122-1128: The loop invalidates the same user's cache multiple
times when update.devices contains several devices for one user; before calling
self.invalidate_device_cache(&device.user).await, collect and deduplicate the
users from update.devices (e.g., into a HashSet or by iterating and skipping
duplicates) and then call invalidate_device_cache once per unique user when
update.had_unregistered is true (references: update.had_unregistered,
update.devices, self.invalidate_device_cache).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f81925a-5130-4625-9ef5-6c32e5fa15ea
📒 Files selected for processing (2)
src/send.rswacore/src/send.rs
WA Web's AdvDeviceNotificationApi.js line 59: h.has(e.keyIndex) || e.keyIndex > y When keyIndex is null: h.has(null)→false, null>y→false → device REMOVED. Our code incorrectly returned true for None key_index, keeping devices that WA Web would remove. Fixed both filter_devices_by_key_index() and is_key_index_valid() to return false for None.
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)
wacore/src/send.rs (1)
1073-1128:⚠️ Potential issue | 🟠 MajorDon't infer 406s from
actually_encrypted.is_empty().That condition is neither necessary nor sufficient evidence of an unregistered device. Mixed outcomes leave
stale_device_usersempty even though some devices were skipped as stale, while all-skipped non-406 misses can mark users stale incorrectly. Also,resolved_devices_for_phashis never narrowed/cleared here, so the stanza can still emit aphashfor devices that never got<participants>entries. Return explicit unregistered-device info fromencrypt_for_devices, and derivephashfrom the successfully encrypted set.Also applies to: 1215-1226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/send.rs` around lines 1073 - 1128, The current logic infers unregistered devices from actually_encrypted.is_empty() and leaves resolved_devices_for_phash unfiltered, which causes incorrect 406 detection and phash emission; update encrypt_for_devices (call sites in this block and the similar one at 1215-1226) to return explicit skipped/unregistered device info (e.g., a list/flag of unregistered devices and the set of successfully encrypted devices), then use that explicit unregistered list to set had_unregistered_devices and populate stale_device_users, and when building resolved_devices_for_phash / skdm_encrypted_devices derive the phash only from the successfully encrypted device set (actually_encrypted) rather than the original distribution_list so phash/stanza emission matches actual encrypted recipients.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/send.rs`:
- Around line 411-431: The current batch prekey fetch replaces all bundles with
an empty HashMap when fetch_prekeys_for_identity_check returns a
device-unregistered 406; instead, on Err(e) where
is_device_unregistered_error(&e) is true, retry/fallback per JID: iterate
jids_needing_prekeys and call the resolver per-JID (e.g., reusing
fetch_prekeys_for_identity_check for single-JID or a single-fetch API), collect
successful bundles into prekey_bundles, skip only those JIDs that return a 406,
and only return Err(e) for non-406 errors; update the code around
fetch_prekeys_for_identity_check, is_device_unregistered_error, prekey_bundles,
and jids_needing_prekeys to implement this per-JID retry/skip behavior so one
stale device does not blank the whole batch.
---
Outside diff comments:
In `@wacore/src/send.rs`:
- Around line 1073-1128: The current logic infers unregistered devices from
actually_encrypted.is_empty() and leaves resolved_devices_for_phash unfiltered,
which causes incorrect 406 detection and phash emission; update
encrypt_for_devices (call sites in this block and the similar one at 1215-1226)
to return explicit skipped/unregistered device info (e.g., a list/flag of
unregistered devices and the set of successfully encrypted devices), then use
that explicit unregistered list to set had_unregistered_devices and populate
stale_device_users, and when building resolved_devices_for_phash /
skdm_encrypted_devices derive the phash only from the successfully encrypted
device set (actually_encrypted) rather than the original distribution_list so
phash/stanza emission matches actual encrypted recipients.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c1a8c4b-6cc5-4572-a623-6195f9be14aa
📒 Files selected for processing (2)
src/send.rswacore/src/send.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7473ae799e
ℹ️ 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".
| // Apply valid_indexes filtering if key-index-list was decoded | ||
| if let Some(ref decoded) = decoded_key_index { | ||
| devices = wacore::adv::filter_devices_by_key_index(&devices, decoded); |
There was a problem hiding this comment.
When get_user_devices fetches a user not already present in the registry, each fetched companion is created with key_index = None, and this new filter call removes all non-primary devices with unknown key indexes. In that first-fetch path the persisted/returned device list collapses to device 0, so outbound fanout under-targets linked devices until some later notification happens to backfill key indexes (which is not guaranteed). This should not filter out companions solely because key index metadata is missing from prior cache state.
Useful? React with 👍 / 👎.
| user: user_jid.to_non_ad(), | ||
| devices, | ||
| phash, | ||
| key_index_bytes, |
There was a problem hiding this comment.
DeviceListSpec::parse_response now extracts key-index-list bytes but still unconditionally pushes users with companion devices even when those bytes are absent. The production Client::get_user_devices path consumes this parser, so unverified companion entries are still persisted and reused, allowing stale/unregistered devices to remain authoritative and repeatedly trigger 406 errors on send. The same guard already exists in wacore/src/usync.rs; this parser should enforce it before pushing UserDeviceList.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/adv.rs`:
- Around line 74-80: The comment in is_key_index_valid is inconsistent with the
logic (it currently allows Some(ki) when ki > decoded.current_index or ki in
decoded.valid_indexes). Update the comment for the function is_key_index_valid
(and the inline WA Web note) to state that a non-null key_index is accepted if
it is in decoded.valid_indexes OR if it is greater than decoded.current_index
(explain briefly why, e.g., to allow new rotations/forward indices), so the
comment matches the actual checks against decoded.valid_indexes and
decoded.current_index.
- Around line 95-178: Add focused unit tests for the helper is_key_index_valid
to lock in its new semantics: assert that None returns false, that a device
key_index > current_index returns true, that a key_index present in
valid_indexes returns true, and that other numeric key_index values return
false; place these new tests alongside the existing tests (e.g., near
filter_devices_by_key_index tests) and reference the is_key_index_valid function
directly so future changes to filter logic won’t regress the helper behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db0d50d5-ff42-4973-89a4-35925d4d7e39
📒 Files selected for processing (1)
wacore/src/adv.rs
The 406 downcast was silently failing because the anyhow error wraps whatsapp_rust::IqError (high-level crate) but wacore tried to downcast to wacore::IqError — different types, always returns false. Fix: introduce wacore::request::ServerErrorCode as a shared lightweight error type that any crate can embed in anyhow. The SendContextResolver impl in the high-level crate wraps server errors in this type before returning. wacore's is_device_unregistered_error() downcasts to it. This is scalable: future server error checks (409, 503, etc.) use the same ServerErrorCode::from_anyhow() pattern without trait changes. Added 4 tests validating the downcast logic.
The invalidation only cleared the moka in-memory cache but left the stale device record in SQLite. On the next get_user_devices call, the cache miss fell back to the DB and re-loaded the stale device, causing the 406 SKDM retry on every single group send. Fix: invalidate_device_cache now calls delete_devices() on the DB backend for all alias keys, forcing a network re-fetch (usync) on the next query. Added delete_devices to ProtocolStore trait with implementations in SqliteStore and InMemoryBackend.
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 50s —— View job Code Review: PR #469 — Stale Device Registry Fix
Overall this is a well-structured, multi-phase fix that correctly addresses the root cause of stale device accumulation. The WA Web cross-references are valuable. Here are my findings: Issues1. Both CodeRabbit and Codex flagged this, and it's real. The project already has
I'd recommend at minimum adding a comment documenting this limitation, or ideally clearing persisted SKDM state for all groups (identity changes are rare enough that the cost is acceptable). 2. Batch 406 blanks all prekey bundles — When The comment says "WA Web's fetchPrekeys() collects per-device errors separately" — but the implementation doesn't match that description. WA Web does per-device retry; this does batch-level fallback. In practice this may be mitigated because the batch is usually per-user (single device list), but for multi-device users with one stale companion this would drop all companions. 3. Missing companion guard in
4. Minor but the comment on line 79 is misleading: // WA Web: keyIndex must be non-null and in valid_indexes to be added
None => false,The Things that look good / previously addressed
Minor suggestions
| |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c171325f7e
ℹ️ 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".
| if actually_encrypted.is_empty() && !distribution_list.is_empty() { | ||
| had_unregistered_devices = true; |
There was a problem hiding this comment.
This sets had_unregistered_devices only when actually_encrypted.is_empty(), so a mixed case is missed: if some devices already have sessions (and encrypt succeeds) while others hit 406 during prekey fetch, stale_device_users stays empty. In that scenario the caller never invalidates those users’ device registry entries, so stale devices remain cached and every later group send keeps re-hitting the same 406 path instead of triggering a refresh.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,2 @@ | |||
| -- SQLite doesn't support DROP COLUMN in older versions, but newer SQLite (3.35+) does. | |||
| ALTER TABLE device_registry DROP COLUMN raw_id; | |||
There was a problem hiding this comment.
The rollback migration uses ALTER TABLE ... DROP COLUMN, which fails on SQLite versions prior to 3.35.0; this repo’s other down migrations use table-rebuild patterns specifically for that compatibility. As written, diesel migration revert can fail in older runtime environments, blocking downgrade/reset workflows.
Useful? React with 👍 / 👎.
…s, tests 1. clear_device_record: now calls clear_all_sender_key_devices() on DB backend so stale has_key=true rows don't survive restart. Added clear_all_sender_key_devices to ProtocolStore trait. 2. Batch 406 prekey fetch: on 406, retry per-device to salvage valid companions. Explicit had_406 flag propagated through EncryptResult instead of inferring from actually_encrypted.is_empty(). 3. DeviceListSpec::parse_response: added companion-without-signedKeyIndexBytes guard (defense-in-depth, matching the guard in wacore/src/usync.rs). 4. is_key_index_valid: fixed misleading comment, added 4 unit tests covering valid set, not-in-set, newer-than-current, and None rejection.
|
@claude review |
|
Claude finished @jlucaso1's task in 4m 29s —— View job Code Review: PR #469 — Stale Device Registry Fix
This is a well-structured, multi-phase fix. The WA Web cross-references are valuable and the phased approach (graceful error handling → root cause fix → security hardening → type safety) is sound. Most prior review findings have been addressed. Here's what I found on the current state: Previously reported issues — now fixed
Remaining issues1.
Consider deriving 2. The 3. if raw_id.is_none() && !existing_key_indices.is_empty() {
raw_id = existing_record.as_ref().and_then(|r| r.raw_id);
}The condition 4. Device When building Things that look good
Minor nits
Overall: solid PR with good WA Web compliance. The phash issue (finding #1) is the most actionable remaining item. |
jlucaso1
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e4b130d41
ℹ️ 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".
| self.clear_device_record(user, &device.jid.server, &record) | ||
| .await; |
There was a problem hiding this comment.
On raw_id mismatch in patch_device_add, this call uses the notification alias (user + device.jid.server) instead of the canonical identity stored in the record, so clear_device_record can delete only PN-addressed sessions for mapped users while LID sessions remain. Because send encryption is LID-first, those stale LID sessions can continue to be selected after an identity reset, causing ongoing identity/session inconsistencies instead of a clean re-establishment.
Useful? React with 👍 / 👎.
clear_device_record used only the notification's server address, missing sessions stored under the alternate address (e.g., LID sessions when notification arrived via PN). Now resolves both LID and PN aliases via resolve_lookup_keys and deletes sessions for all combinations.
…ment phash was computed from the full distribution_list before encryption, including devices that might fail. Now derived from skdm_encrypted_devices so phash matches the actual <participants> nodes in the stanza. Also clarified the raw_id preservation heuristic comment in usync.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f1aafb3a1
ℹ️ 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".
| CREATE TABLE device_registry ( | ||
| user_id TEXT NOT NULL, | ||
| devices_json TEXT NOT NULL, | ||
| timestamp INTEGER NOT NULL, | ||
| phash TEXT, |
There was a problem hiding this comment.
The down migration rebuilds device_registry with a different schema than the one it is supposed to restore: it changes updated_at to DEFAULT 0 and never recreates the idx_device_registry_timestamp, idx_device_registry_device, and idx_device_registry_updated_at indexes that were introduced in 2025-12-24-000000_add_whatsapp_web_alignment/up.sql. After diesel migration revert, this leaves the database in a non-equivalent state, which can change default-row behavior and degrade lookup performance in downgrade/reset workflows.
Useful? React with 👍 / 👎.
WA Web computes phash from the target distribution list at send time, not the actual encryption outcome. Reverted the phash change that used skdm_encrypted_devices — this caused phash mismatch vs server expectation. Also added TODO for advAccountType mismatch check (WA Web BizCoex feature gated behind bizHostedDevicesEnabled — not yet implemented).
Down migration was missing the strftime default for updated_at and the three indexes from the original 2025-12-24 migration.
1 participant
Summary
Closes #468.
Stale devices accumulated in the device registry because we never decoded the
key-index-listsigned protobuf bytes from device add notifications. This caused persistent 406 errors and ~2-4s delays on every group send.Before: 4.08s per ping-pong (406 retry on every send)
After: 273ms first send (usync re-fetch), 2.69ms subsequent (cached)
Breaking Changes
ProtocolStoretrait: addeddelete_devices(&self, user: &str)andclear_all_sender_key_devices(&self)— implementors must add these methodsDeviceListRecord: addedraw_id: Option<u32>field — constructors must include it (uses#[serde(default)]so deserialization is backward-compatible)raw_idcolumn todevice_registrytable (auto-applied by Diesel)Changes
Phase 1: Graceful SKDM error handling
wacore/src/send.rs— SKDM distribution wrapped inmatchso failures don't kill the group send (matching WA WebGroupSkmsgJobtry/catch)wacore/src/send.rs— On batch 406, retry per-device to salvage valid companions. Explicithad_unregistered_deviceflag inEncryptResult(no heuristic inference)wacore/src/send.rs—stale_device_userscomputed by diffing distribution_list vs encrypted setsrc/send.rs— Caller invalidates device registry (cache + DB) for stale users so next send re-fetches from serverPhase 2: Fix root cause (key-index-list filtering)
wacore/src/adv.rs— New module:decode_key_index_list(),filter_devices_by_key_index(),is_key_index_valid()matching WA WebAdvDeviceNotificationApi+AdvKeyIndexResultApisrc/client/device_registry.rs—patch_device_adddecodeskey-index-list, filters stale devices byvalid_indexes, validates new devicekey_indexbefore adding, detectsraw_idmismatchsrc/usync.rs— Usync response applies samevalid_indexesfiltering, rejects companion devices withoutsignedKeyIndexBytes, uses alias-awareload_device_record, returns filtered deviceswacore/src/usync.rs+wacore/src/iq/usync.rs— Parse<key-index-list>from<devices>response, companion guard in both parsersPhase 3: Security hardening
wacore/src/store/traits.rs—DeviceListRecordgainsraw_id: Option<u32>fieldraw_idcolumn todevice_registrytable (table-rebuild down migration for SQLite < 3.35)raw_idmismatch triggersclear_device_record: deletes Signal sessions for non-primary devices, clears ALL persisted sender key device tracking, invalidates in-memory sender key cachesrc/handlers/notification.rs— Account sync preserves existingraw_idsrc/usync.rs— Clearsexisting_key_indicesafterraw_idmismatch to avoid reusing old identity statePhase 4: Cross-crate type safety
wacore/src/request.rs— NewServerErrorCodeshared error type for typed cross-crate server error detection (scalable: future 409/503 checks use same pattern)src/client/context_impl.rs—SendContextResolverwraps server errors inServerErrorCodewacore/src/send.rs—is_device_unregistered_error()usesServerErrorCode::from_anyhow()for zero-cost typed downcastWA Web compliance
Nonekey_index → device removed (matchingh.has(null)→falseinAdvDeviceNotificationApi)GroupSkmsgJob)markHasSenderKey(matching WA WebParticipantStore)AdvForUsyncApi)handleDeviceRemoveNotificationdoesn't usevalid_indexesPersistence & caching
invalidate_device_cachenow deletes from both moka cache AND SQLite DB (was the root cause of the infinite retry loop — DB fallback reloaded stale devices)clear_device_recordclears persistedsender_key_devicestable (was only clearing in-memory cache — stalehas_key=truerows survived restart)Key files
wacore/src/adv.rswacore/src/request.rsServerErrorCodeshared error typewacore/src/send.rsEncryptResultstruct, per-device 406 retry, SKDM try/catch, stale user detectionsrc/client/device_registry.rspatch_device_addrewrite,clear_device_record,invalidate_device_cacheDB deletionsrc/client/context_impl.rssrc/usync.rssrc/send.rssrc/handlers/notification.rsKeyIndexInfotopatch_device_add, preserveraw_idwacore/src/store/traits.rsdelete_devices,clear_all_sender_key_devicesonProtocolStoreTest plan
cargo fmt --all— cleancargo clippy --all --tests— no warningscargo test --all --exclude e2e-tests— all 1099 tests passis_device_unregistered_errortests (4): 406 detected, non-406 rejected, unrelated rejected, bare wacore IqError rejectedis_key_index_validtests (4): valid set, not-in-set, newer-than-current, None rejectionSummary by CodeRabbit
New Features
Bug Fixes
Chores