fix(device-list): always keep the primary device after a raw_id mismatch patch - #797
Merged
Conversation
…tch patch On a raw_id mismatch (identity rotation/relink), patch_device_add clears the device list and rebuilds it from the notification. filter_devices_by_key_index only keeps device 0 when it is already in the input, so the primary was dropped and the record was persisted without it (or empty when the notified device's key index is rejected). Such a record sticks because get_user_devices treats any present record as authoritative and never re-fetches. Mirror WA Web's WAWebHandleAdvDeviceNotificationApi, which re-adds the primary unconditionally at the end of the rebuild, so the device list always contains device 0.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR ensures patch_device_add re-inserts the primary device (device 0) when missing after ADVSignedKeyIndexList decode/filter rebuilds, and updates/adds tests to validate deduplication and raw-id mismatch regression scenarios. ChangesPrimary Device Preservation in Device Registry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
Collaborator
Author
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 32s —— View job Code Review
The fix is small, surgical, and correct. The bug is real: Strengths
Observations / follow-ups (not blocking)
VerdictLGTM. Ship it; the two notes above are good candidates for a small follow-up PR. |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Benchmark Results67 unchanged benchmark(s)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/device_registry.rs`:
- Around line 1248-1283: Add an assertion after retrieving updated (from
client.device_registry_cache.get) in the test async fn
test_patch_device_add_raw_id_mismatch_rejected_device_keeps_primary to verify
the primary device's key_index is set to Some(0); specifically, after the
existing asserts on updated.devices.len() and updated.devices[0].device_id,
assert that updated.devices[0].key_index == Some(0) to document the expected
rebuilt state of device 0 after calling client.patch_device_add.
- Around line 1206-1242: In
test_patch_device_add_raw_id_mismatch_preserves_primary, after retrieving
updated from device_registry_cache, locate the device with device_id == 0 (e.g.
via updated.devices.iter().find(|d| d.device_id == 0).unwrap()) and add an
assertion that its key_index equals Some(0) (assert_eq!(dev0.key_index, Some(0),
...)) so the test documents the expected key_index for the primary after the
raw_id mismatch rebuild.
- Around line 398-403: The code adds a primary device with device_id == 0 but
sets key_index to Some(0), creating inconsistent state; change the DeviceInfo
construction so device_id 0 uses key_index: None instead of Some(0) (update the
record.devices.push of wacore::store::traits::DeviceInfo for device_id 0 to use
key_index: None) so it matches filter_devices_by_key_index and the rest of the
code/tests.
🪄 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: 105e7447-a3f0-475a-b65c-f5512b834643
📒 Files selected for processing (1)
src/client/device_registry.rs
Comment on lines
+1206
to
+1242
| async fn test_patch_device_add_raw_id_mismatch_preserves_primary() { | ||
| let client = create_test_client().await; | ||
|
|
||
| client | ||
| .device_registry_cache | ||
| .insert( | ||
| "15551234567".to_string(), | ||
| Arc::new(record_with_raw_id("15551234567", &[0, 5], 1)), | ||
| ) | ||
| .await; | ||
|
|
||
| // New raw_id (2) != stored (1) → clear + rebuild. Notified device 19 has a | ||
| // valid key index, so the rebuilt list is the companion plus the primary. | ||
| let signed = make_signed_key_index_bytes(2, 0, vec![7]); | ||
| let key_index_info = wacore::stanza::devices::KeyIndexInfo { | ||
| timestamp: 100, | ||
| signed_bytes: Some(signed), | ||
| }; | ||
| let elem = make_device_element(19, Some(7)); | ||
| client | ||
| .patch_device_add("15551234567", &elem, Some(&key_index_info)) | ||
| .await; | ||
|
|
||
| let updated = client | ||
| .device_registry_cache | ||
| .get("15551234567") | ||
| .await | ||
| .unwrap(); | ||
| assert!( | ||
| updated.devices.iter().any(|d| d.device_id == 0), | ||
| "primary (device 0) must survive a raw_id mismatch clear, got {:?}", | ||
| updated.devices | ||
| ); | ||
| assert!(updated.devices.iter().any(|d| d.device_id == 19)); | ||
| // Stale companion from the old identity is dropped by the clear. | ||
| assert!(!updated.devices.iter().any(|d| d.device_id == 5)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider asserting device 0's key_index value in this test.
The test verifies device 0 is present after the raw_id mismatch rebuild, but it doesn't check what key_index value device 0 has. Given the inconsistency I flagged earlier (Some(0) in production code vs None in test helpers), we should explicitly verify the expected key_index here. That way if we need to change it later, the test will document the correct behavior.
At WhatsApp's scale, we can't afford ambiguity about device state. Add an assertion like:
let dev0 = updated.devices.iter().find(|d| d.device_id == 0).unwrap();
assert_eq!(dev0.key_index, Some(0), "device 0 should have key_index 0 after rebuild");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/device_registry.rs` around lines 1206 - 1242, In
test_patch_device_add_raw_id_mismatch_preserves_primary, after retrieving
updated from device_registry_cache, locate the device with device_id == 0 (e.g.
via updated.devices.iter().find(|d| d.device_id == 0).unwrap()) and add an
assertion that its key_index equals Some(0) (assert_eq!(dev0.key_index, Some(0),
...)) so the test documents the expected key_index for the primary after the
raw_id mismatch rebuild.
Address review feedback: device 0's key_index is never read (filter_devices_by_key_index keeps the primary regardless and is_key_index_valid is not applied to it), so store None to match how device 0 is recorded everywhere else instead of a one-off Some(0). Tests now assert the rebuilt primary's key_index.
This was referenced Jun 9, 2026
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a device-add notification carries a
key-index-listwhoseraw_iddiffers from the stored one (an identity rotation or relink),patch_device_addclears the device list and rebuilds it from the notification.filter_devices_by_key_indexonly keeps the primary (device 0) when it is already present in the input, so after the clear the primary is dropped: the record is persisted as just the notified companion (for example[{device_id: 19}]), or as an empty list when the notified device's key index is rejected.A device record that exists but is missing the primary is a broken state.
get_user_devicestreats any present record as authoritative and only goes to the network when there is no record at all, so the bad list sticks and the self-healing usync re-fetch never fires. Downstream this leaves the user with a device set that does not cover the primary.Fix
Mirror WA Web's
WAWebHandleAdvDeviceNotificationApi. InhandleDeviceAddNotification, after filtering the existing devices and adding the notified ones, it unconditionally re-adds the primary withC.push({ id: DEFAULT_DEVICE_ID, keyIndex: 0 })(and the remove path does the same). The invariant is that the rebuilt device list always contains device 0.patch_device_addnow ensures device 0 is present after the rebuild, so a raw_id mismatch can never drop the primary or persist an empty list. whatsmeow keeps the same invariant on its device-list rebuild.Tests
cargo test -p whatsapp-rust --lib client::device_registry::tests(37 passing), fullcargo test -p whatsapp-rust --lib(739 passing),cargo test -p wacore, andcargo clippy --all-targets -- -D warnings.New tests:
test_patch_device_add_raw_id_mismatch_preserves_primary: a raw_id mismatch clears the stale companion but keeps device 0 and adds the new companion. Guards the[{device_id: N}](no primary) record.test_patch_device_add_raw_id_mismatch_rejected_device_keeps_primary: a mismatch where the notified key index is rejected rebuilds to just the primary instead of an empty list.test_patch_device_add_deduplicatesto the realistic seed (a record that includes the primary) and to assert the dedupe plus the primary invariant.Breaking
None.