fix(prekeys): act on the device a rejected fetch names - #1153
Conversation
A prekey fetch is one IQ over up to 50 devices, and a batch-wide 406 does not say which device is gone. But the server also names a rejected device individually, answering with an <error> in place of its key material, and that arm was landing in the generic "failed to parse bundle" path: the device was skipped, its code was lost, and nothing refreshed the list that still named it. Rejected devices come back alongside the bundles now, and the preflight refreshes exactly those lists rather than the whole batch. The send continues, because every device that did return a bundle is unaffected by a rejection that only concerns the named ones. WA Web reads the same arm (FetchKeyBundlesUserError) and keeps its per-item errors beside the bundles. Refs #1143
📝 WalkthroughWalkthroughPre-key fetch results now contain successful bundles and named per-device rejections. Parsing, IQ contracts, client adapters, session setup, cache invalidation, benchmarks, and fan-out tests were updated to propagate and handle this structured outcome. ChangesPre-key outcome propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant PreKeyUtils
participant SessionSetup
participant DeviceCache
Server->>PreKeyUtils: Return bundles and per-device errors
PreKeyUtils->>SessionSetup: Provide PreKeyFetchOutcome
SessionSetup->>DeviceCache: Invalidate devices rejected with code 406
SessionSetup->>SessionSetup: Establish sessions for returned bundles
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| wacore/src/prekeys.rs | Parses valid per-device errors separately from successful prekey bundles and rejects unusable JIDs or out-of-range codes. |
| wacore/src/send/encrypt.rs | Preserves named unregistered-device information across session planning and encryption results. |
| wacore/src/send/group.rs | Uses named rejections for targeted stale-user recovery while retaining the existing batch-wide fallback. |
| src/client/sessions.rs | Refreshes device lists named by per-device 406 responses while continuing to install healthy bundles. |
| wacore/src/send/tests.rs | Adds focused regression coverage for rejection propagation and stale-user selection. |
Sequence Diagram
sequenceDiagram
participant WA as WhatsApp Server
participant Parser as Prekey Parser
participant Fanout as Send Fan-out
participant Cache as Device Cache
WA-->>Parser: Bundles plus per-device error 406
Parser-->>Fanout: PreKeyFetchOutcome
Fanout->>Fanout: Encrypt for devices with bundles
Fanout-->>Cache: Invalidate users named by 406
Note over Fanout,Cache: Batch-wide 406 retains unencrypted-remainder fallback
Reviews (4): Last reviewed commit: "fix(send): refresh only the devices the ..." | Re-trigger Greptile
![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: 9b0430fbbe
ℹ️ 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.fetch_pre_keys(jids, Some(PreKeyFetchReason::Identity)) | ||
| .await | ||
| .map(|o| o.bundles) |
There was a problem hiding this comment.
When a cold group or status SKDM fetch returns bundles plus a per-user 406, this mapping discards the rejected JID before ensure_sessions_for_devices receives it (wacore/src/send/encrypt.rs:572-589). That path only sets had_unregistered_device for an Err(406), so prepare_group_stanza produces no stale_device_users and the post-ACK invalidation in src/send/mod.rs:1867-1872 never runs; it can even mark the full target set, including the rejected device, as having the sender key. Consequently the group/status path retains the stale device and phash despite this change correctly invalidating it on the DM preflight path. The outcome or equivalent rejected-device signal needs to survive this resolver boundary.
Useful? React with 👍 / 👎.
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
The resolver flattened the outcome to its bundles, so a device the server named was indistinguishable from one that simply returned nothing by the time the group fan-out saw it. Only a batch-wide error set had_unregistered_device, so no stale_device_users reached the post-ACK invalidation and the group path kept the absent device. The trait carries the outcome now, and a named rejection raises the same flag a batch failure raises.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wacore/src/prekeys.rs`:
- Around line 228-240: Update the rejected-device handling around
get_optional_child("error") to validate metadata before pushing RejectedDevice:
parse the user identity with optional_jid and skip entries whose JID is missing
or invalid, and convert the raw error code with u16::try_from instead of an
unchecked cast, skipping or otherwise rejecting out-of-range values without
recording them. Preserve logging and continue processing the remaining batch
entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6414ebba-c5b9-46c3-b97b-ec39e2eebd70
📒 Files selected for processing (9)
src/client/context_impl.rssrc/client/sessions.rssrc/prekeys.rswacore/benches/send_receive_benchmark.rswacore/src/client/context.rswacore/src/iq/prekeys.rswacore/src/prekeys.rswacore/src/send/encrypt.rswacore/src/send/tests.rs
`65942 as u16` is exactly 406, so a truncating cast let an out-of-range code arrive as the one value that makes the caller refresh a device list. The code is narrowed with try_from now, and a code that does not fit is dropped rather than invented. A `<user>` whose jid is missing or unparseable named no device either: the defaulting accessor turned it into device 0 of an empty user, and the rejection would have been recorded against whatever that resolved to. It is skipped. Also covers the group recovery loop with tests: a device that never got a bundle is reported stale, a fully delivered distribution reports nothing, and a non-406 rejection leaves the lists alone.
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: f304fb717c
ℹ️ 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".
The plan carried the rejection as a bare flag, so the group path rebuilt the identities from every target that went unencrypted. A device whose bundle was merely absent or malformed, or whose session setup failed, was then treated as evidence of a stale list, and its user's device registry was deleted for a failure that says nothing about the list. The named JIDs travel with the flag now. A batch-wide failure still falls back to the unencrypted remainder, which is sound there because it names nobody and none of those targets got a bundle either.
1 participant
Closes #1143. Answers its first question — does the server name the offending device? — with yes, which makes the narrowing retry that issue also considered unnecessary.
What the investigation found
#1143 framed the problem as a batch-wide
406that cannot say which of up to 50 devices is gone, and listed three ways out: parse the device out of the error, narrow the batch by retrying, or mark the batch to avoid repeating it.The server already answers per device, in the success response. A device it no longer knows comes back as its own
<user>carrying an<error code="406">where the key material would be. WA Web reads exactly that arm —FetchKeyBundlesUserError/FetchKeyBundlesUserErrorFallbackinWAWeb/Fetch/PrekeysJob— and returns{ prekeyBundles, errors }, keeping the per-item errors beside the bundles.We were dropping it.
parse_prekeys_responsefed that<user>to the bundle parser, which failed on the missing fields, loggedFailed to parse prekey bundle, and moved on. The device was skipped (so the send did continue), but the code was lost, the log blamed a malformed bundle rather than a removed device, and nothing refreshed the device list that still named it — so the next send resolved the same absent device again.What changes
The rejection is parsed and kept.
parse_prekeys_responsereturns aPreKeyFetchOutcome: the bundles it did get, plus the devices the server rejected and the code it rejected them with. Bundles and rejections are kept apart deliberately — a missing bundle is ambiguous and the device is simply skipped this round, while a rejected one is the server telling us which cached device is gone.The DM preflight acts on it, refreshing exactly those device lists rather than the whole batch, and the send continues because every device that did return a bundle is unaffected.
The signal survives the resolver boundary. The resolver used to flatten the outcome to its bundles, so by the time the group fan-out saw it, a named rejection was indistinguishable from a device that simply returned nothing. Only a batch-wide error raised
had_unregistered_device, so nostale_device_usersreached the post-ACK invalidation and the group path kept the absent device — the DM path would have been fixed while the group path stayed broken.Only the named devices are refreshed. The plan first carried the rejection as a bare flag, which meant the group path rebuilt the identities from every target that went unencrypted — including devices whose bundle was merely absent or malformed, or whose session setup failed. Those users' device registries would have been deleted over failures that say nothing about the list being stale, which is the opposite of what this PR sets out to do. The named JIDs travel with the flag now. A batch-wide failure still falls back to the unencrypted remainder, which is sound there precisely because it names nobody and none of those targets got a bundle either.
Only a real
406counts.65942 as u16is exactly 406, so the truncating cast could turn an out-of-range code into the one value that triggers a refresh; the code is narrowed withtry_fromand dropped if it does not fit. A<user>whose jid is missing or unparseable names no device either — the defaulting accessor turned it into device 0 of an empty user — so it is skipped rather than recorded against whatever that resolved to.What this does not change, and why
The sender-key marking still covers the whole target set. A reviewer flagged that a rejected device is marked as having the sender key, which sounded like it would strand that device. It is deliberate and matches WA Web:
GroupSkmsgJobcallsmarkHasSenderKey(x, M)withM = a.skDistribList, the distribution list rather than the encrypted subset. The recovery is not that marking — it iscollect_stale_device_users, which compares the target list against the devices actually encrypted for and reports the difference. That path now runs for a named rejection, which is what closes the loop, and there are tests for both halves of it.The batch-wide
406is untouched. It still refreshes every device in the batch and still fails the send, exactly as #1149 left it. Whether that path is even reachable for "one device is gone" — as opposed to something global like auth or rate limiting — is not answerable from here; it needs a capture from a live account. The per-device path is the one the server actually uses, and it is handled now.No narrowing retry. It was #1143's fallback if the error could not identify the device, at ~6 extra round trips on a path that already failed. The server identifies it, so that cost is not worth paying.
Tests
a_rejected_user_is_named_without_costing_the_rest_of_the_batcha_named_rejection_reaches_the_fan_out_like_a_batch_failurea_rejection_that_is_not_a_406_leaves_the_device_list_alonea_clean_fetch_reports_no_unregistered_devicean_out_of_range_error_code_is_not_narrowed_into_a_40665942 as u16 == 406a_user_without_a_usable_jid_is_skipped_rather_than_defaulteda_rejection_keeps_the_code_the_server_senta_device_that_was_never_encrypted_for_is_reported_stalea_fully_delivered_distribution_reports_nothing_staleonly_the_named_device_is_refreshed_when_the_server_named_ita_batch_wide_failure_falls_back_to_the_unencrypted_remaindernothing_is_refreshed_without_an_unregistered_deviceEach was mutation-checked. Restoring the truncating cast, the defaulting jid accessor, the flattened resolver, the inferred-identity fallback, or the old parser arm fails its test.
Verification
cargo fmt --all,cargo clippy --workspace --all-targetswith zero warnings, and green suites:wacore1283,whatsapp-rust1238.